| Message ID | 20240110102102.61587-11-tudor.ambarus@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | serial: samsung: gs101 updates and winter cleanup | expand |
On Wed, Jan 10, 2024 at 4:23 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > ``max_count`` negative values are not used. Since ``port->fifosize`` > is an unsigned int, make ``max_count`` the same. > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > drivers/tty/serial/samsung_tty.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c > index 90c49197efc7..dbbe6b8e3ceb 100644 > --- a/drivers/tty/serial/samsung_tty.c > +++ b/drivers/tty/serial/samsung_tty.c > @@ -760,8 +760,8 @@ static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id) > static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport) > { > struct uart_port *port = &ourport->port; > + unsigned int max_count = port->fifosize; What if port->fifosize is 0? Then this code below: while (max_count-- > 0) { would cause int overflow, if max_count is unsigned? > unsigned int fifocnt = 0; > - int max_count = port->fifosize; > u32 ufcon, ufstat, uerstat; > u8 ch, flag; > > -- > 2.43.0.472.g3155946c3a-goog > >
On 1/16/24 18:21, Sam Protsenko wrote: > On Wed, Jan 10, 2024 at 4:23 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: >> >> ``max_count`` negative values are not used. Since ``port->fifosize`` >> is an unsigned int, make ``max_count`` the same. >> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> >> --- >> drivers/tty/serial/samsung_tty.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c >> index 90c49197efc7..dbbe6b8e3ceb 100644 >> --- a/drivers/tty/serial/samsung_tty.c >> +++ b/drivers/tty/serial/samsung_tty.c >> @@ -760,8 +760,8 @@ static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id) >> static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport) >> { >> struct uart_port *port = &ourport->port; >> + unsigned int max_count = port->fifosize; > > What if port->fifosize is 0? Then this code below: > > while (max_count-- > 0) { > > would cause int overflow, if max_count is unsigned? > good catch, Sam! I'm thinking of amending this and add at the beginning of the method: if (!max_count) return tty_flip_buffer_push(&port->state->port); Thanks! ta
Hi, On Wed, 2024-01-17 at 15:21 +0000, Tudor Ambarus wrote: > > > On 1/16/24 18:21, Sam Protsenko wrote: > > On Wed, Jan 10, 2024 at 4:23 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > > > > > ``max_count`` negative values are not used. Since ``port->fifosize`` > > > is an unsigned int, make ``max_count`` the same. > > > > > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > > > --- > > > drivers/tty/serial/samsung_tty.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c > > > index 90c49197efc7..dbbe6b8e3ceb 100644 > > > --- a/drivers/tty/serial/samsung_tty.c > > > +++ b/drivers/tty/serial/samsung_tty.c > > > @@ -760,8 +760,8 @@ static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id) > > > static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport) > > > { > > > struct uart_port *port = &ourport->port; > > > + unsigned int max_count = port->fifosize; > > > > What if port->fifosize is 0? Then this code below: > > > > while (max_count-- > 0) { > > > > would cause int overflow, if max_count is unsigned? > > > > good catch, Sam! Does it matter, though? As this is a post-decrement, the test is done first, and the decrement after. Therefore, it'll still bail out as expected. > I'm thinking of amending this and add at the beginning of the method: > > if (!max_count) > return tty_flip_buffer_push(&port->state->port); This will not help with overflow. It'll still have wrapped around after completing the while() (always, no matter what start-value max_count had) Cheers, Andre'
On 1/17/24 15:38, André Draszik wrote: >>>> + unsigned int max_count = port->fifosize; >>> What if port->fifosize is 0? Then this code below: >>> >>> while (max_count-- > 0) { >>> >>> would cause int overflow, if max_count is unsigned? >>> >> good catch, Sam! > Does it matter, though? As this is a post-decrement, the test is done first, and the > decrement after. Therefore, it'll still bail out as expected. Indeed, it doesn't. This reminds me of stop replying to emails at the end of the day :) Cheers Andre'! ta
On Wed, Jan 17, 2024 at 9:38 AM André Draszik <andre.draszik@linaro.org> wrote: > > Hi, > > On Wed, 2024-01-17 at 15:21 +0000, Tudor Ambarus wrote: > > > > > > On 1/16/24 18:21, Sam Protsenko wrote: > > > On Wed, Jan 10, 2024 at 4:23 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > > > > > > > ``max_count`` negative values are not used. Since ``port->fifosize`` > > > > is an unsigned int, make ``max_count`` the same. > > > > > > > > Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> > > > > --- > > > > drivers/tty/serial/samsung_tty.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c > > > > index 90c49197efc7..dbbe6b8e3ceb 100644 > > > > --- a/drivers/tty/serial/samsung_tty.c > > > > +++ b/drivers/tty/serial/samsung_tty.c > > > > @@ -760,8 +760,8 @@ static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id) > > > > static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport) > > > > { > > > > struct uart_port *port = &ourport->port; > > > > + unsigned int max_count = port->fifosize; > > > > > > What if port->fifosize is 0? Then this code below: > > > > > > while (max_count-- > 0) { > > > > > > would cause int overflow, if max_count is unsigned? > > > > > > > good catch, Sam! > > Does it matter, though? As this is a post-decrement, the test is done first, and the > decrement after. Therefore, it'll still bail out as expected. > Good catch on my good catch :) > > I'm thinking of amending this and add at the beginning of the method: > > > > if (!max_count) > > return tty_flip_buffer_push(&port->state->port); > > This will not help with overflow. It'll still have wrapped around after completing the > while() (always, no matter what start-value max_count had) > > Cheers, > Andre' >
On Wed, Jan 17, 2024 at 9:54 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > > > On 1/17/24 15:38, André Draszik wrote: > >>>> + unsigned int max_count = port->fifosize; > >>> What if port->fifosize is 0? Then this code below: > >>> > >>> while (max_count-- > 0) { > >>> > >>> would cause int overflow, if max_count is unsigned? > >>> > >> good catch, Sam! > > Does it matter, though? As this is a post-decrement, the test is done first, and the > > decrement after. Therefore, it'll still bail out as expected. > > Indeed, it doesn't. This reminds me of stop replying to emails at the > end of the day :) > And it reminds me to drink some coffee in the morning before doing any reviews :) With above condition sorted, feel free to add: Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org> > Cheers Andre'! > ta
diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c index 90c49197efc7..dbbe6b8e3ceb 100644 --- a/drivers/tty/serial/samsung_tty.c +++ b/drivers/tty/serial/samsung_tty.c @@ -760,8 +760,8 @@ static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id) static void s3c24xx_serial_rx_drain_fifo(struct s3c24xx_uart_port *ourport) { struct uart_port *port = &ourport->port; + unsigned int max_count = port->fifosize; unsigned int fifocnt = 0; - int max_count = port->fifosize; u32 ufcon, ufstat, uerstat; u8 ch, flag;
``max_count`` negative values are not used. Since ``port->fifosize`` is an unsigned int, make ``max_count`` the same. Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org> --- drivers/tty/serial/samsung_tty.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)