| Message ID | 20230313082734.886890-1-kasper@iki.fi (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] USB: serial: xr: Add TIOCGRS485 and TIOCSRS485 ioctls | expand |
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 09:27, 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> Hi, I am sorry, but locking is really broken here. All these contexts can sleep. There is no need for a spinlock. As far as you need locking, just use a mutex. Secondly, if xr_set_rs485_config() needs locking, so will xr_get_rs485_config() or you can get the case that you return half a new and half an old state to user space. Regards Oliver
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. - Jarkko Oliver Neukum kirjoitti 13/03/2023 klo 11.54: > > > On 13.03.23 09:27, 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> > > > Hi, > > I am sorry, but locking is really broken here. All these contexts can > sleep. > There is no need for a spinlock. As far as you need locking, just use > a mutex. > > Secondly, if xr_set_rs485_config() needs locking, so will > xr_get_rs485_config() > or you can get the case that you return half a new and half an old > state to user > space. > > Regards > Oliver
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
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 };
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(-)