[v2] USB: serial: xr: Add TIOCGRS485 and TIOCSRS485 ioctls

Add support for RS-485 in Exar USB adapters.
RS-485 mode is controlled by TIOCGRS485 and TIOCSRS485 ioctls.
Gpio mode register is set to enable RS-485.

Signed-off-by: Jarkko Sonninen <kasper@iki.fi>
---
 drivers/usb/serial/xr_serial.c | 65 +++++++++++++++++++++++++++++++++-
 1 file changed, 64 insertions(+), 1 deletion(-)

On Mon, Mar 13, 2023 at 10:27:34AM +0200, Jarkko Sonninen wrote:
> Add support for RS-485 in Exar USB adapters.
> RS-485 mode is controlled by TIOCGRS485 and TIOCSRS485 ioctls.
> Gpio mode register is set to enable RS-485.
> 
> Signed-off-by: Jarkko Sonninen <kasper@iki.fi>
> ---
>  drivers/usb/serial/xr_serial.c | 65 +++++++++++++++++++++++++++++++++-
>  1 file changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
> index fdb0aae546c3..480cda0daafc 100644
> --- a/drivers/usb/serial/xr_serial.c
> +++ b/drivers/usb/serial/xr_serial.c
> @@ -93,6 +93,7 @@ struct xr_txrx_clk_mask {
>  #define XR_GPIO_MODE_SEL_DTR_DSR	0x2
>  #define XR_GPIO_MODE_SEL_RS485		0x3
>  #define XR_GPIO_MODE_SEL_RS485_ADDR	0x4
> +#define XR_GPIO_MODE_RS485_TX_H		0x8
>  #define XR_GPIO_MODE_TX_TOGGLE		0x100
>  #define XR_GPIO_MODE_RX_TOGGLE		0x200
>  
> @@ -237,6 +238,8 @@ static const struct xr_type xr_types[] = {
>  struct xr_data {
>  	const struct xr_type *type;
>  	u8 channel;			/* zero-based index or interface number */
> +	struct serial_rs485 rs485;
> +	spinlock_t lock;
>  };
>  
>  static int xr_set_reg(struct usb_serial_port *port, u8 channel, u16 reg, u16 val)
> @@ -629,6 +632,7 @@ static void xr_set_flow_mode(struct tty_struct *tty,
>  	struct xr_data *data = usb_get_serial_port_data(port);
>  	const struct xr_type *type = data->type;
>  	u16 flow, gpio_mode;
> +	unsigned long flags, rs485_flags;
>  	int ret;
>  
>  	ret = xr_get_reg_uart(port, type->gpio_mode, &gpio_mode);
> @@ -645,9 +649,16 @@ static void xr_set_flow_mode(struct tty_struct *tty,
>  	/* Set GPIO mode for controlling the pins manually by default. */
>  	gpio_mode &= ~XR_GPIO_MODE_SEL_MASK;
>  
> +	spin_lock_irqsave(&data->lock, flags);
> +	rs485_flags = data->rs485.flags;
> +	spin_unlock_irqrestore(&data->lock, flags);
> +	if (rs485_flags & SER_RS485_ENABLED)
> +		gpio_mode |= XR_GPIO_MODE_SEL_RS485 | XR_GPIO_MODE_RS485_TX_H;
> +	else if (C_CRTSCTS(tty) && C_BAUD(tty) != B0)
> +		gpio_mode |= XR_GPIO_MODE_SEL_RTS_CTS;
> +
>  	if (C_CRTSCTS(tty) && C_BAUD(tty) != B0) {
>  		dev_dbg(&port->dev, "Enabling hardware flow ctrl\n");
> -		gpio_mode |= XR_GPIO_MODE_SEL_RTS_CTS;
>  		flow = XR_UART_FLOW_MODE_HW;
>  	} else if (I_IXON(tty)) {
>  		u8 start_char = START_CHAR(tty);
> @@ -827,6 +838,57 @@ static void xr_set_termios(struct tty_struct *tty,
>  	xr_set_flow_mode(tty, port, old_termios);
>  }
>  
> +static int xr_get_rs485_config(struct tty_struct *tty,
> +			 unsigned int __user *argp)
> +{
> +	struct usb_serial_port *port = tty->driver_data;
> +	struct xr_data *data = usb_get_serial_port_data(port);
> +
> +	dev_dbg(tty->dev, "Flags %02x\n", data->rs485.flags);
> +	if (copy_to_user(argp, &data->rs485, sizeof(data->rs485)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +static int xr_set_rs485_config(struct tty_struct *tty,
> +			 unsigned long __user *argp)
> +{
> +	struct usb_serial_port *port = tty->driver_data;
> +	struct xr_data *data = usb_get_serial_port_data(port);
> +	struct serial_rs485 rs485;
> +	unsigned long flags;
> +
> +	if (copy_from_user(&rs485, argp, sizeof(rs485)))
> +		return -EFAULT;
> +
> +	dev_dbg(tty->dev, "Flags %02x\n", rs485.flags);
> +	rs485.flags &= SER_RS485_ENABLED;
> +	spin_lock_irqsave(&data->lock, flags);
> +	memcpy(&data->rs485, &rs485, sizeof(rs485));
> +	spin_unlock_irqrestore(&data->lock, flags);
> +	xr_set_flow_mode(tty, port, 0);
> +
> +	if (copy_to_user(argp, &data->rs485, sizeof(data->rs485)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> +
> +
> +static int xr_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
> +{
> +	void __user *argp = (void __user *)arg;
> +
> +	switch (cmd) {
> +	case TIOCGRS485:
> +		return xr_get_rs485_config(tty, argp);
> +	case TIOCSRS485:
> +		return xr_set_rs485_config(tty, argp);
> +	}
> +	return -ENOTTY;
> +}
> +
>  static int xr_open(struct tty_struct *tty, struct usb_serial_port *port)
>  {
>  	int ret;
> @@ -1010,6 +1072,7 @@ static struct usb_serial_driver xr_device = {
>  	.set_termios		= xr_set_termios,
>  	.tiocmget		= xr_tiocmget,
>  	.tiocmset		= xr_tiocmset,
> +	.ioctl			= xr_ioctl,
>  	.dtr_rts		= xr_dtr_rts
>  };
>  
> -- 
> 2.34.1
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/process/submitting-patches.rst for what
  needs to be done here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

On 13.03.23 11:47, Jarkko Sonninen wrote:
>      Hello,
> 
> It uses only one flag from the struct from user. Would it be better to store only that to state ?
> 
> Do I need locking at all in that case ?
> 
> The whole struct is stored just in case, if someone would implement other functionality later.

Well,

1. would you be happy if you were the one to implement additional
features and found that you'd have to reinvent locking?

2. That would mean discarding the values given for delay_rts_before_send
and delay_rts_after_send. That wouldn't be nice.

It seems to me that all our algorithmic complexity goes away
if you just turn "lock" into a mutex and just take it.

	Regards
		Oliver

From: Jarkko Sonninen
> Sent: 13 March 2023 08:28
> 
> Add support for RS-485 in Exar USB adapters.
> RS-485 mode is controlled by TIOCGRS485 and TIOCSRS485 ioctls.
> Gpio mode register is set to enable RS-485.

The locking is entirely dubious.
Summary:

Taking the lock to read the flags is pretty pointless.
You are only looking at one bit and nothing else is tied
to the lock.
Even a READ_ONCE() isn't needed.
> +	spin_lock_irqsave(&data->lock, flags);
> +	rs485_flags = data->rs485.flags;
> +	spin_unlock_irqrestore(&data->lock, flags);
> +	if (rs485_flags & SER_RS485_ENABLED)
> +		gpio_mode |= XR_GPIO_MODE_SEL_RS485 | XR_GPIO_MODE_RS485_TX_H;
> +	else if (C_CRTSCTS(tty) && C_BAUD(tty) != B0)
> +		gpio_mode |= XR_GPIO_MODE_SEL_RTS_CTS;
> +

The ioctl read code reads the data unlocked.
> +	if (copy_to_user(argp, &data->rs485, sizeof(data->rs485)))
> +		return -EFAULT;
So could return old and new data if the ioctl write code
runs concurrently (and you get a hardware interrupt or page
fault mid-buffer).

The ioctl write code acquires the lock across a structure copy.
(which should be a structure copy, not a memcpy).
The only way the lock will have any effect is if multiple
threads are doing updates at the same time.
Code doing that won't work anyway.
> +	if (copy_from_user(&rs485, argp, sizeof(rs485)))
> +		return -EFAULT;
> +
> +	dev_dbg(tty->dev, "Flags %02x\n", rs485.flags);
> +	rs485.flags &= SER_RS485_ENABLED;
> +	spin_lock_irqsave(&data->lock, flags);
> +	memcpy(&data->rs485, &rs485, sizeof(rs485));
> +	spin_unlock_irqrestore(&data->lock, flags);

In any case you one seem to be implementing one bit of
the flags - so the rest of the data can be ignored.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Hi Jarkko,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on johan-usb-serial/usb-next]
[also build test WARNING on johan-usb-serial/usb-linus linus/master v6.3-rc2 next-20230314]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jarkko-Sonninen/USB-serial-xr-Add-TIOCGRS485-and-TIOCSRS485-ioctls/20230313-163032
base:   https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git usb-next
patch link:    https://lore.kernel.org/r/20230313082734.886890-1-kasper%40iki.fi
patch subject: [PATCH v2] USB: serial: xr: Add TIOCGRS485 and TIOCSRS485 ioctls
config: sparc64-randconfig-s031-20230312 (https://download.01.org/0day-ci/archive/20230314/202303141402.sfS74rp6-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/3470fe1d77e0edf25fc417f516491abbd812dc41
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jarkko-Sonninen/USB-serial-xr-Add-TIOCGRS485-and-TIOCSRS485-ioctls/20230313-163032
        git checkout 3470fe1d77e0edf25fc417f516491abbd812dc41
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sparc64 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sparc64 SHELL=/bin/bash drivers/usb/serial/ kernel/trace/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303141402.sfS74rp6-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/usb/serial/xr_serial.c:870:37: sparse: sparse: Using plain integer as NULL pointer

vim +870 drivers/usb/serial/xr_serial.c

   853	
   854	static int xr_set_rs485_config(struct tty_struct *tty,
   855				 unsigned long __user *argp)
   856	{
   857		struct usb_serial_port *port = tty->driver_data;
   858		struct xr_data *data = usb_get_serial_port_data(port);
   859		struct serial_rs485 rs485;
   860		unsigned long flags;
   861	
   862		if (copy_from_user(&rs485, argp, sizeof(rs485)))
   863			return -EFAULT;
   864	
   865		dev_dbg(tty->dev, "Flags %02x\n", rs485.flags);
   866		rs485.flags &= SER_RS485_ENABLED;
   867		spin_lock_irqsave(&data->lock, flags);
   868		memcpy(&data->rs485, &rs485, sizeof(rs485));
   869		spin_unlock_irqrestore(&data->lock, flags);
 > 870		xr_set_flow_mode(tty, port, 0);
   871	
   872		if (copy_to_user(argp, &data->rs485, sizeof(data->rs485)))
   873			return -EFAULT;
   874	
   875		return 0;
   876	}
   877