| Message ID | 20230313010416.845252-1-kasper@iki.fi (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | USB: serial: xr: Add TIOCGRS485 and TIOCSRS485 ioctls | expand |
Nit, your "To:" line was corrupted :( On Mon, Mar 13, 2023 at 03:04:16AM +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 | 72 +++++++++++++++++++++++++++++++++- > 1 file changed, 71 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c > index fdb0aae546c3..edbb6add087c 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,64 @@ 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); > + unsigned long flags; > + struct serial_rs485 rs485; > + > + spin_lock_irqsave(&data->lock, flags); > + memcpy(&rs485, &data->rs485, sizeof(rs485)); Why are you using the stack for this? Why not directly access the real structure instead? And are you sure you need a lock? If so, why? > + spin_unlock_irqrestore(&data->lock, flags); > + dev_dbg(tty->dev, "%s flags %02x\n", __func__, rs485.flags); dev_dbg() provides __func__ for free, so you never need to provide it again. Just use the +f flag when enabling it from userspace. > + > + if (copy_to_user(argp, &rs485, sizeof(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, "%s flags %02x\n", __func__, rs485.flags); Again, no need for __func__. > + 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) Very odd indentation, please fix up. > +{ > + 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 -ENOIOCTLCMD; Wrong ioctl return value :( thanks, greg k-h
On 3/13/23 09:00, Greg Kroah-Hartman wrote: > Nit, your "To:" line was corrupted :( > > On Mon, Mar 13, 2023 at 03:04:16AM +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 | 72 +++++++++++++++++++++++++++++++++- >> 1 file changed, 71 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c >> index fdb0aae546c3..edbb6add087c 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,64 @@ 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); >> + unsigned long flags; >> + struct serial_rs485 rs485; >> + >> + spin_lock_irqsave(&data->lock, flags); >> + memcpy(&rs485, &data->rs485, sizeof(rs485)); > Why are you using the stack for this? Why not directly access the real > structure instead? And are you sure you need a lock? If so, why? I suppose the lock is not really needed and I'll change this. I was unsure about it. >> + spin_unlock_irqrestore(&data->lock, flags); >> + dev_dbg(tty->dev, "%s flags %02x\n", __func__, rs485.flags); > dev_dbg() provides __func__ for free, so you never need to provide it > again. Just use the +f flag when enabling it from userspace. > >> + >> + if (copy_to_user(argp, &rs485, sizeof(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, "%s flags %02x\n", __func__, rs485.flags); > Again, no need for __func__. > >> + 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) > Very odd indentation, please fix up. > >> +{ >> + 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 -ENOIOCTLCMD; > Wrong ioctl return value :( What is the correct ioctl error return value ? ENOIOCTLCMD was used in most places in usb serial as an error return. > thanks, > > greg k-h - Jarkko
On Mon, Mar 13, 2023 at 09:49:26AM +0200, Jarkko Sonninen wrote: > > > +{ > > > + 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 -ENOIOCTLCMD; > > Wrong ioctl return value :( > > What is the correct ioctl error return value ? > ENOIOCTLCMD was used in most places in usb serial as an error return. ENOTTY is the correct one for when an ioctl is not handled by the ioctl call. thanks, greg k-h
On 3/13/23 09:53, Greg Kroah-Hartman wrote: > On Mon, Mar 13, 2023 at 09:49:26AM +0200, Jarkko Sonninen wrote: >>>> +{ >>>> + 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 -ENOIOCTLCMD; >>> Wrong ioctl return value :( >> What is the correct ioctl error return value ? >> ENOIOCTLCMD was used in most places in usb serial as an error return. > ENOTTY is the correct one for when an ioctl is not handled by the ioctl > call. > > thanks, > > greg k-h Using ENOTTY breaks all other tty ioctls. - Jarkko
On Mon, Mar 13, 2023 at 05:07:59PM +0200, Jarkko Sonninen wrote: > On 3/13/23 09:53, Greg Kroah-Hartman wrote: > > On Mon, Mar 13, 2023 at 09:49:26AM +0200, Jarkko Sonninen wrote: > > > > > +{ > > > > > + 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 -ENOIOCTLCMD; > > > > Wrong ioctl return value :( > > > What is the correct ioctl error return value ? > > > ENOIOCTLCMD was used in most places in usb serial as an error return. > > ENOTTY is the correct one for when an ioctl is not handled by the ioctl > > call. > > > > thanks, > > > > greg k-h > > Using ENOTTY breaks all other tty ioctls. What other tty ioctls? confused, greg k-h
On 3/13/23 17:50, Greg Kroah-Hartman wrote: > On Mon, Mar 13, 2023 at 05:07:59PM +0200, Jarkko Sonninen wrote: >> On 3/13/23 09:53, Greg Kroah-Hartman wrote: >>> On Mon, Mar 13, 2023 at 09:49:26AM +0200, Jarkko Sonninen wrote: >>>>>> +{ >>>>>> + 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 -ENOIOCTLCMD; >>>>> Wrong ioctl return value :( >>>> What is the correct ioctl error return value ? >>>> ENOIOCTLCMD was used in most places in usb serial as an error return. >>> ENOTTY is the correct one for when an ioctl is not handled by the ioctl >>> call. >>> >>> thanks, >>> >>> greg k-h >> Using ENOTTY breaks all other tty ioctls. > What other tty ioctls? > > confused, > > greg k-h For example TCGETS and TCFLSH - Jarkko
On 13.03.23 16:50, Greg Kroah-Hartman wrote: > On Mon, Mar 13, 2023 at 05:07:59PM +0200, Jarkko Sonninen wrote: >> Using ENOTTY breaks all other tty ioctls. > > What other tty ioctls? > > confused, > This is one of those handlers which simulate OO inheritance in C. You can implement any IOCTL in the driver. If you do not, you give back -ENOICTLCOMMAND. Any other error means that you did implement it and an error occured in the method. HTH Oliver
diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c index fdb0aae546c3..edbb6add087c 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,64 @@ 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); + unsigned long flags; + struct serial_rs485 rs485; + + spin_lock_irqsave(&data->lock, flags); + memcpy(&rs485, &data->rs485, sizeof(rs485)); + spin_unlock_irqrestore(&data->lock, flags); + dev_dbg(tty->dev, "%s flags %02x\n", __func__, rs485.flags); + + if (copy_to_user(argp, &rs485, sizeof(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, "%s flags %02x\n", __func__, 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 -ENOIOCTLCMD; +} + static int xr_open(struct tty_struct *tty, struct usb_serial_port *port) { int ret; @@ -1010,6 +1079,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 | 72 +++++++++++++++++++++++++++++++++- 1 file changed, 71 insertions(+), 1 deletion(-)