Re: [PATCH] serial: fix TIOCSRS485 locking

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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

On Wed, 12 Apr 2023, Johan Hovold wrote:

> The RS485 multipoint addressing support for some reason added a new
> ADDRB termios cflag which is (only!) updated from one of the RS485
> ioctls.
> 
> Make sure to take the termios rw semaphore for the right ioctl (i.e.
> set, not get).
> 
> Fixes: ae50bb275283 ("serial: take termios_rwsem for ->rs485_config() & pass termios as param")
> Cc: stable@vger.kernel.org	# 6.0
> Cc: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
> 
> I did not have time to review the multipoint addressing patches at the
> time and only skimmed the archives now, but I can't seem to find any
> motivation for why a precious termios bit was seemingly wasted on ADDRB
> when it is only updated from the RS485 ioctls.
> 
> I hope it wasn't done just to simplify the implementation of
> tty_get_frame_size()? Or was it a left-over from the RFC which
> apparently actually used termios to enable this feature?

No. I made it intentionally. It felt natural place for storing it because 
ADDRB does impact the wire format and cflag is where other wire-format 
impacting bits are also stored.

> Should we consider dropping the Linux-specific ADDRB bit again?
> 
> Johan
> 
> 
>  drivers/tty/serial/serial_core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 2bd32c8ece39..728cb72be066 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1552,7 +1552,7 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
>  		goto out;
>  
>  	/* rs485_config requires more locking than others */
> -	if (cmd == TIOCGRS485)
> +	if (cmd == TIOCSRS485)
>  		down_write(&tty->termios_rwsem);
>  
>  	mutex_lock(&port->mutex);
> @@ -1595,7 +1595,7 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
>  	}
>  out_up:
>  	mutex_unlock(&port->mutex);
> -	if (cmd == TIOCGRS485)
> +	if (cmd == TIOCSRS485)
>  		up_write(&tty->termios_rwsem);
>  out:
>  	return ret;
> 

Indeed, the caps are so blinding.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.