[RFC PATCH] serial: Don't assume uart_ops .throttle is always set

[RFC PATCH] serial: Don't assume uart_ops .throttle is always set

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]

uart_throttle() assumes that a driver provides a throttle function in
uart_ops. But not all drivers do and there seems to nothing in
serial_core that would set it either. Thus, check it before calling.

Reported-by: Nuno Gonçalves <nunojpg@gmail.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

---
 drivers/tty/serial/serial_core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 6a8963caf954..18c9d46e0b85 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -697,7 +697,8 @@ static void uart_throttle(struct tty_struct *tty)
 		mask |= UPSTAT_AUTORTS;
 
 	if (port->status & mask) {
-		port->ops->throttle(port);
+		if (port->ops->throttle)
+			port->ops->throttle(port);
 		mask &= ~port->status;
 	}
 

-- 
tg: (19317433057d..) serial/fix-uart_throttle (depends on: tty-linus)

Re: [RFC PATCH] serial: Don't assume uart_ops .throttle is always set

[Greg KH]

On Thu, Apr 28, 2022 at 08:11:56PM +0300, Ilpo Järvinen wrote:
> uart_throttle() assumes that a driver provides a throttle function in
> uart_ops. But not all drivers do and there seems to nothing in
> serial_core that would set it either. Thus, check it before calling.
> 
> Reported-by: Nuno Gonçalves <nunojpg@gmail.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> ---
>  drivers/tty/serial/serial_core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 6a8963caf954..18c9d46e0b85 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -697,7 +697,8 @@ static void uart_throttle(struct tty_struct *tty)
>  		mask |= UPSTAT_AUTORTS;
>  
>  	if (port->status & mask) {
> -		port->ops->throttle(port);
> +		if (port->ops->throttle)
> +			port->ops->throttle(port);
>  		mask &= ~port->status;
>  	}
>  
> 

What commit id does this "fix"?

thanks,

greg k-h

Re: [RFC PATCH] serial: Don't assume uart_ops .throttle is always set

[Tomasz Mon]

On Thu, 2022-04-28 at 20:11 +0300, Ilpo Järvinen wrote:
> uart_throttle() assumes that a driver provides a throttle function in
> uart_ops. But not all drivers do and there seems to nothing in
> serial_core that would set it either. Thus, check it before calling.

If the driver does not provide throttle function, it should not
set UPSTAT_AUTORTS nor UPSTAT_AUTOXOFF in port->status.

> diff --git a/drivers/tty/serial/serial_core.c
> b/drivers/tty/serial/serial_core.c
> index 6a8963caf954..18c9d46e0b85 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -697,7 +697,8 @@ static void uart_throttle(struct tty_struct *tty)
>  		mask |= UPSTAT_AUTORTS;
>  
>  	if (port->status & mask) {

This if condition is always false if driver did not set UPSTAT_AUTORTS
nor UPSTAT_AUTOXOFF in port->status.

> -		port->ops->throttle(port);
> +		if (port->ops->throttle)
> +			port->ops->throttle(port);
>  		mask &= ~port->status;
>  	}
> 

I think this patch is incorrect. The assumption is not ".throttle is
always set" but rather ".throttle is set when UPSTAT_AUTORTS or
UPSTAT_AUTOXOFF is set".

Best Regards,
Tomasz Mon

Re: [RFC PATCH] serial: Don't assume uart_ops .throttle is always set

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1447 bytes --]

On Fri, 29 Apr 2022, Tomasz Mon wrote:

> On Thu, 2022-04-28 at 20:11 +0300, Ilpo Järvinen wrote:
> > uart_throttle() assumes that a driver provides a throttle function in
> > uart_ops. But not all drivers do and there seems to nothing in
> > serial_core that would set it either. Thus, check it before calling.
> 
> If the driver does not provide throttle function, it should not
> set UPSTAT_AUTORTS nor UPSTAT_AUTOXOFF in port->status.
> 
> > diff --git a/drivers/tty/serial/serial_core.c
> > b/drivers/tty/serial/serial_core.c
> > index 6a8963caf954..18c9d46e0b85 100644
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -697,7 +697,8 @@ static void uart_throttle(struct tty_struct *tty)
> >  		mask |= UPSTAT_AUTORTS;
> >  
> >  	if (port->status & mask) {
> 
> This if condition is always false if driver did not set UPSTAT_AUTORTS
> nor UPSTAT_AUTOXOFF in port->status.
> 
> > -		port->ops->throttle(port);
> > +		if (port->ops->throttle)
> > +			port->ops->throttle(port);
> >  		mask &= ~port->status;
> >  	}
> > 
> 
> I think this patch is incorrect. The assumption is not ".throttle is
> always set" but rather ".throttle is set when UPSTAT_AUTORTS or
> UPSTAT_AUTOXOFF is set".

Right, I kind of ended up concluding that's likely the case after sending 
the patch when looking into 9aba8d5b0111 (SERIAL: core: add 
throttle/unthrottle callbacks for hardware assisted flow control).


-- 
 i.