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 <[email protected]>
---
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));
+ 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
};
--
2.34.1
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 <[email protected]>
> ---
> 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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
>
>
> 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
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
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/[email protected]/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 <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
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 <[email protected]>
---
In this version only rs485.flags are stored to state.
There is no locking as only one bit of the flags is used.
ioctl returns -ENOIOCTLCMD as the actual error handling is in tty code.
drivers/usb/serial/xr_serial.c | 62 +++++++++++++++++++++++++++++++++-
1 file changed, 61 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
index fdb0aae546c3..7b542ccb6596 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,7 @@ static const struct xr_type xr_types[] = {
struct xr_data {
const struct xr_type *type;
u8 channel; /* zero-based index or interface number */
+ u32 rs485_flags;
};
static int xr_set_reg(struct usb_serial_port *port, u8 channel, u16 reg, u16 val)
@@ -645,9 +647,13 @@ 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;
+ if (data->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 +833,59 @@ 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);
+ struct serial_rs485 rs485;
+
+ dev_dbg(tty->dev, "Flags %02x\n", data->rs485_flags);
+ memset(&rs485, 0, sizeof(rs485));
+ rs485.flags = data->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;
+
+ if (copy_from_user(&rs485, argp, sizeof(rs485)))
+ return -EFAULT;
+
+ dev_dbg(tty->dev, "Flags %02x\n", rs485.flags);
+ data->rs485_flags = rs485.flags & SER_RS485_ENABLED;
+ xr_set_flow_mode(tty, port, (const struct ktermios *)0);
+
+ // Only the enable flag is implemented
+ memset(&rs485, 0, sizeof(rs485));
+ rs485.flags = data->rs485_flags;
+ if (copy_to_user(argp, &rs485, sizeof(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 +1069,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
On Tue, Mar 14, 2023 at 09:00:01AM +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 <[email protected]>
> ---
>
> In this version only rs485.flags are stored to state.
> There is no locking as only one bit of the flags is used.
> ioctl returns -ENOIOCTLCMD as the actual error handling is in tty code.
And the difference between previous versions? Take a look at the
documentation for how to better describe version differences please.
>
> drivers/usb/serial/xr_serial.c | 62 +++++++++++++++++++++++++++++++++-
> 1 file changed, 61 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
> index fdb0aae546c3..7b542ccb6596 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,7 @@ static const struct xr_type xr_types[] = {
> struct xr_data {
> const struct xr_type *type;
> u8 channel; /* zero-based index or interface number */
> + u32 rs485_flags;
Nit, you might want to move this up above channel as you now have a hole
in this structure. Not like it's that big of a deal so if you don't
have to respin this no need to change.
> };
>
> static int xr_set_reg(struct usb_serial_port *port, u8 channel, u16 reg, u16 val)
> @@ -645,9 +647,13 @@ 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;
>
> + if (data->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 +833,59 @@ 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);
> + struct serial_rs485 rs485;
> +
> + dev_dbg(tty->dev, "Flags %02x\n", data->rs485_flags);
> + memset(&rs485, 0, sizeof(rs485));
> + rs485.flags = data->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;
> +
> + if (copy_from_user(&rs485, argp, sizeof(rs485)))
> + return -EFAULT;
> +
> + dev_dbg(tty->dev, "Flags %02x\n", rs485.flags);
> + data->rs485_flags = rs485.flags & SER_RS485_ENABLED;
> + xr_set_flow_mode(tty, port, (const struct ktermios *)0);
> +
> + // Only the enable flag is implemented
All the other comments in this file use /* */ so you should be
consistent.
> + memset(&rs485, 0, sizeof(rs485));
But you just copied this from userspace, why zero it out again? Is that
to be expected (I really don't remember, sorry).
Anyway, just minor comments, I'll let others review it as well.
thanks,
greg k-h
On 3/14/23 09:37, Greg Kroah-Hartman wrote:
> On Tue, Mar 14, 2023 at 09:00:01AM +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 <[email protected]>
>> ---
>>
>> In this version only rs485.flags are stored to state.
>> There is no locking as only one bit of the flags is used.
>> ioctl returns -ENOIOCTLCMD as the actual error handling is in tty code.
> And the difference between previous versions? Take a look at the
> documentation for how to better describe version differences please.
I'll try to be more precise in the future.
>> + memset(&rs485, 0, sizeof(rs485));
> But you just copied this from userspace, why zero it out again? Is that
> to be expected (I really don't remember, sorry).
serial.h says
/*
* Serial interface for controlling RS485 settings on chips with suitable
* support. Set with TIOCSRS485 and get with TIOCGRS485 if supported by
your
* platform. The set function returns the new state, with any
unsupported bits
* reverted appropriately.
*/
This may mean only the flags, but I decided to zero the other
unsupported fields as well.
> Anyway, just minor comments, I'll let others review it as well.
>
> thanks,
>
> greg k-h
- Jarkko
On Tue, Mar 14, 2023 at 09:00:01AM +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.
Which register you use is an implementation details which is not really
needed in the commit message.
Please say something about how the hardware works and try to describe
what you are implementing here and perhaps something about what is left
unsupported (e.g. the fixed rts polarity).
> Signed-off-by: Jarkko Sonninen <[email protected]>
> ---
>
> In this version only rs485.flags are stored to state.
> There is no locking as only one bit of the flags is used.
> ioctl returns -ENOIOCTLCMD as the actual error handling is in tty code.
>
> drivers/usb/serial/xr_serial.c | 62 +++++++++++++++++++++++++++++++++-
> 1 file changed, 61 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
> index fdb0aae546c3..7b542ccb6596 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,7 @@ static const struct xr_type xr_types[] = {
> struct xr_data {
> const struct xr_type *type;
> u8 channel; /* zero-based index or interface number */
> + u32 rs485_flags;
> };
>
> static int xr_set_reg(struct usb_serial_port *port, u8 channel, u16 reg, u16 val)
> @@ -645,9 +647,13 @@ 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;
>
> + if (data->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;
The logic here is unnecessarily convoluted here and you also should not
set hardware flow control mode if rs485 mode is enabled.
Perhaps you can add a local boolean flag to hold the rs485 state and
test it before the current if-else construct. Then you only enable
hw-flow when rs485 mode is disabled while stile allowing sw-flow to be
set (hopefully that's a legal combination, please do try to verify
that).
It also looks like you have inverted the RS485 polarity by using
XR_GPIO_MODE_RS485_TX_H (more on that below).
> } else if (I_IXON(tty)) {
> u8 start_char = START_CHAR(tty);
> @@ -827,6 +833,59 @@ 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)
argp points to struct serial_rs485 to use that as the type rather than
pointer to unsigned int.
> +{
> + struct usb_serial_port *port = tty->driver_data;
> + struct xr_data *data = usb_get_serial_port_data(port);
> + struct serial_rs485 rs485;
> +
> + dev_dbg(tty->dev, "Flags %02x\n", data->rs485_flags);
This is not a very informative message. Please add back the function
prefix so that is also distinguishable from the dev_dbg() in the other
rs485 helper and use the following format:
"%s - flags = 0x%02x\n", __func__, ...
And add a new line here.
> + memset(&rs485, 0, sizeof(rs485));
> + rs485.flags = data->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)
Use a pointer to the struct here too.
> +{
> + struct usb_serial_port *port = tty->driver_data;
> + struct xr_data *data = usb_get_serial_port_data(port);
> + struct serial_rs485 rs485;
> +
> + if (copy_from_user(&rs485, argp, sizeof(rs485)))
> + return -EFAULT;
> +
> + dev_dbg(tty->dev, "Flags %02x\n", rs485.flags);
Please update the format string as mentioned above.
Add a newline here.
> + data->rs485_flags = rs485.flags & SER_RS485_ENABLED;
> + xr_set_flow_mode(tty, port, (const struct ktermios *)0);
This function accesses tty->termios so you can not call it here without
any locking as it can change underneath you and nothing currently
prevents set_termios() from calling the same function in parallel.
If you take a write lock on the termios rw sempahore you can use it also
to protect the rs485 data instead of relying on implicit atomicity
rules.
And perhaps you should just copy the entire rs485 struct from the start
as these devices supports further features which someone may want to
implement support for later (e.g. delay after send and 9th bit
addressing).
You should just use NULL for the third (old_termios) argument.
> +
> + // Only the enable flag is implemented
No c99 comments, please.
> + memset(&rs485, 0, sizeof(rs485));
> + rs485.flags = data->rs485_flags;
This does not look correct given that you set the RS485 TX polarity so
that RTS is high (logic disable) during TX above.
You need to at least make sure that both the SER_RS485_RTS_ON_SEND and
SER_RS485_RTS_AFTER_SEND bits match the polarity setting. But perhaps
you could consider implementing support for configuring the polarity
from the start.
> + if (copy_to_user(argp, &rs485, sizeof(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 +1069,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
> };
Johan
On Tue, Mar 14, 2023 at 08:37:30AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Mar 14, 2023 at 09:00:01AM +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 <[email protected]>
> > ---
> > @@ -237,6 +238,7 @@ static const struct xr_type xr_types[] = {
> > struct xr_data {
> > const struct xr_type *type;
> > u8 channel; /* zero-based index or interface number */
> > + u32 rs485_flags;
>
> Nit, you might want to move this up above channel as you now have a hole
> in this structure. Not like it's that big of a deal so if you don't
> have to respin this no need to change.
Generally, it's better to keep related fields together than worry about
holes so the above is just fine.
Johan
On 4/13/23 11:53, Johan Hovold wrote:
> On Tue, Mar 14, 2023 at 09:00:01AM +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.
> Which register you use is an implementation details which is not really
> needed in the commit message.
>
> Please say something about how the hardware works and try to describe
> what you are implementing here and perhaps something about what is left
> unsupported (e.g. the fixed rts polarity).
Add support for RS-485 in Exar USB adapters.
RS-485 mode is controlled by TIOCGRS485 and TIOCSRS485 ioctls.
SER_RS485_ENABLED and SER_RS485_RTS_ON_SEND flags are implemented.
There is only one polarity control in Exar for both RTS_ON_SEND and
RTS_AFTER_SEND.
RS-485 delays and addressing modes are not implemented.
ok ?
>> Signed-off-by: Jarkko Sonninen <[email protected]>
>> ---
>>
>> In this version only rs485.flags are stored to state.
>> There is no locking as only one bit of the flags is used.
>> ioctl returns -ENOIOCTLCMD as the actual error handling is in tty code.
>>
>> drivers/usb/serial/xr_serial.c | 62 +++++++++++++++++++++++++++++++++-
>> 1 file changed, 61 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
>> index fdb0aae546c3..7b542ccb6596 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,7 @@ static const struct xr_type xr_types[] = {
>> struct xr_data {
>> const struct xr_type *type;
>> u8 channel; /* zero-based index or interface number */
>> + u32 rs485_flags;
>> };
>>
>> static int xr_set_reg(struct usb_serial_port *port, u8 channel, u16 reg, u16 val)
>> @@ -645,9 +647,13 @@ 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;
>>
>> + if (data->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;
> The logic here is unnecessarily convoluted here and you also should not
> set hardware flow control mode if rs485 mode is enabled.
>
> Perhaps you can add a local boolean flag to hold the rs485 state and
> test it before the current if-else construct. Then you only enable
> hw-flow when rs485 mode is disabled while stile allowing sw-flow to be
> set (hopefully that's a legal combination, please do try to verify
> that).
I'll implement SER_RS485_RTS_ON_SEND and set XR_GPIO_MODE_RS485_TX_H
according to it.
I tested sw flow and it works.
> It also looks like you have inverted the RS485 polarity by using
> XR_GPIO_MODE_RS485_TX_H (more on that below).
>
>> } else if (I_IXON(tty)) {
>> u8 start_char = START_CHAR(tty);
>> @@ -827,6 +833,59 @@ 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)
> argp points to struct serial_rs485 to use that as the type rather than
> pointer to unsigned int.
>
>> +{
>> + struct usb_serial_port *port = tty->driver_data;
>> + struct xr_data *data = usb_get_serial_port_data(port);
>> + struct serial_rs485 rs485;
>> +
>> + dev_dbg(tty->dev, "Flags %02x\n", data->rs485_flags);
> This is not a very informative message. Please add back the function
> prefix so that is also distinguishable from the dev_dbg() in the other
> rs485 helper and use the following format:
>
> "%s - flags = 0x%02x\n", __func__, ...
>
> And add a new line here.
I'll remove this debug and add one to xr_set_flow_mode
dev_dbg(&port->dev, "Enabling RS-485\n");
>> + memset(&rs485, 0, sizeof(rs485));
>> + rs485.flags = data->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)
> Use a pointer to the struct here too.
>
>> +{
>> + struct usb_serial_port *port = tty->driver_data;
>> + struct xr_data *data = usb_get_serial_port_data(port);
>> + struct serial_rs485 rs485;
>> +
>> + if (copy_from_user(&rs485, argp, sizeof(rs485)))
>> + return -EFAULT;
>> +
>> + dev_dbg(tty->dev, "Flags %02x\n", rs485.flags);
> Please update the format string as mentioned above.
>
> Add a newline here.
I'll remove this debug.
>> + data->rs485_flags = rs485.flags & SER_RS485_ENABLED;
>> + xr_set_flow_mode(tty, port, (const struct ktermios *)0);
> This function accesses tty->termios so you can not call it here without
> any locking as it can change underneath you and nothing currently
> prevents set_termios() from calling the same function in parallel.
>
> If you take a write lock on the termios rw sempahore you can use it also
> to protect the rs485 data instead of relying on implicit atomicity
> rules.
>
> And perhaps you should just copy the entire rs485 struct from the start
> as these devices supports further features which someone may want to
> implement support for later (e.g. delay after send and 9th bit
> addressing).
>
> You should just use NULL for the third (old_termios) argument.
>
>> +
>> + // Only the enable flag is implemented
> No c99 comments, please.
>
>> + memset(&rs485, 0, sizeof(rs485));
>> + rs485.flags = data->rs485_flags;
> This does not look correct given that you set the RS485 TX polarity so
> that RTS is high (logic disable) during TX above.
>
> You need to at least make sure that both the SER_RS485_RTS_ON_SEND and
> SER_RS485_RTS_AFTER_SEND bits match the polarity setting. But perhaps
> you could consider implementing support for configuring the polarity
> from the start.
What should happen if user sets SER_RS485_RTS_ON_SEND=1 and
SER_RS485_RTS_AFTER_SEND=0 or vice versa ?
There is only one bit for polarity control in the Exar register.
I am thinking of using only SER_RS485_RTS_ON_SEND to control the
polarity and setting _RTS_AFTER_SEND to the same value.
>> + if (copy_to_user(argp, &rs485, sizeof(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 +1069,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
>> };
> Johan
- Jarkko
On Sun, Apr 16, 2023 at 11:40:00AM +0300, Jarkko Sonninen wrote:
> On 4/13/23 11:53, Johan Hovold wrote:
> > On Tue, Mar 14, 2023 at 09:00:01AM +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.
> > Which register you use is an implementation details which is not really
> > needed in the commit message.
> >
> > Please say something about how the hardware works and try to describe
> > what you are implementing here and perhaps something about what is left
> > unsupported (e.g. the fixed rts polarity).
>
> Add support for RS-485 in Exar USB adapters.
> RS-485 mode is controlled by TIOCGRS485 and TIOCSRS485 ioctls.
> SER_RS485_ENABLED and SER_RS485_RTS_ON_SEND flags are implemented.
> There is only one polarity control in Exar for both RTS_ON_SEND and
> RTS_AFTER_SEND.
> RS-485 delays and addressing modes are not implemented.
>
> ok ?
Sure, something like that, but perhaps you can amend the bit about
RTS_AFTER_SEND (see below).
It would be even better if you could try to rephrase this so that it
reads a little easier. Something along the lines of
Exar devices like <model> can control an RS485 tranceiver by
automatically asserting the RTS#/RS485 pin before sending data
and deasserting it when the last stop bit has been transmitted.
The polarity of the RST#/RS485 signal is configurable and the
hardware also supports features <XYZ>...
Add support for enabling and disabling RS-485 mode and
configuring the signal polarity using the TIOCGRS485 and
TIOCSRS485 ioctls. Support for <XYZ> is left unimplemented for
now.
> >> Signed-off-by: Jarkko Sonninen <[email protected]>
> >> ---
> >>
> >> In this version only rs485.flags are stored to state.
> >> There is no locking as only one bit of the flags is used.
> >> ioctl returns -ENOIOCTLCMD as the actual error handling is in tty code.
> >>
> >> drivers/usb/serial/xr_serial.c | 62 +++++++++++++++++++++++++++++++++-
> >> 1 file changed, 61 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
> >> index fdb0aae546c3..7b542ccb6596 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,7 @@ static const struct xr_type xr_types[] = {
> >> struct xr_data {
> >> const struct xr_type *type;
> >> u8 channel; /* zero-based index or interface number */
> >> + u32 rs485_flags;
> >> };
> >>
> >> static int xr_set_reg(struct usb_serial_port *port, u8 channel, u16 reg, u16 val)
> >> @@ -645,9 +647,13 @@ 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;
> >>
> >> + if (data->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;
> > The logic here is unnecessarily convoluted here and you also should not
> > set hardware flow control mode if rs485 mode is enabled.
> >
> > Perhaps you can add a local boolean flag to hold the rs485 state and
> > test it before the current if-else construct. Then you only enable
> > hw-flow when rs485 mode is disabled while stile allowing sw-flow to be
> > set (hopefully that's a legal combination, please do try to verify
> > that).
>
> I'll implement SER_RS485_RTS_ON_SEND and set XR_GPIO_MODE_RS485_TX_H
> according to it.
>
> I tested sw flow and it works.
Thanks for checking.
> > It also looks like you have inverted the RS485 polarity by using
> > XR_GPIO_MODE_RS485_TX_H (more on that below).
> >
> >> } else if (I_IXON(tty)) {
> >> u8 start_char = START_CHAR(tty);
> >> +static int xr_set_rs485_config(struct tty_struct *tty,
> >> + unsigned long __user *argp)
> > Use a pointer to the struct here too.
> >
> >> +{
> >> + struct usb_serial_port *port = tty->driver_data;
> >> + struct xr_data *data = usb_get_serial_port_data(port);
> >> + struct serial_rs485 rs485;
> >> +
> >> + if (copy_from_user(&rs485, argp, sizeof(rs485)))
> >> + return -EFAULT;
> >> +
> >> + dev_dbg(tty->dev, "Flags %02x\n", rs485.flags);
> > Please update the format string as mentioned above.
> >
> > Add a newline here.
>
> I'll remove this debug.
>
> >> + data->rs485_flags = rs485.flags & SER_RS485_ENABLED;
> >> + xr_set_flow_mode(tty, port, (const struct ktermios *)0);
> > This function accesses tty->termios so you can not call it here without
> > any locking as it can change underneath you and nothing currently
> > prevents set_termios() from calling the same function in parallel.
> >
> > If you take a write lock on the termios rw sempahore you can use it also
> > to protect the rs485 data instead of relying on implicit atomicity
> > rules.
> >
> > And perhaps you should just copy the entire rs485 struct from the start
> > as these devices supports further features which someone may want to
> > implement support for later (e.g. delay after send and 9th bit
> > addressing).
> >
> > You should just use NULL for the third (old_termios) argument.
> >
> >> +
> >> + // Only the enable flag is implemented
> > No c99 comments, please.
> >
> >> + memset(&rs485, 0, sizeof(rs485));
> >> + rs485.flags = data->rs485_flags;
> > This does not look correct given that you set the RS485 TX polarity so
> > that RTS is high (logic disable) during TX above.
> >
> > You need to at least make sure that both the SER_RS485_RTS_ON_SEND and
> > SER_RS485_RTS_AFTER_SEND bits match the polarity setting. But perhaps
> > you could consider implementing support for configuring the polarity
> > from the start.
>
> What should happen if user sets SER_RS485_RTS_ON_SEND=1 and
> SER_RS485_RTS_AFTER_SEND=0 or vice versa ?
>
> There is only one bit for polarity control in the Exar register.
>
> I am thinking of using only SER_RS485_RTS_ON_SEND to control the
> polarity and setting _RTS_AFTER_SEND to the same value.
Yeah, this is perhaps not the best designed interface we have...
The way I interpret those flag, they should generally be each others
negation as presumably most hardware controlled rs485 implementations
works as the Exar devices do (i.e. they have one polarity flag).
Settings both to the same value doesn't seem to make much sense so not
sure why it was designed this way.
But here you should just set SER_RS485_RTS_AFTER_SEND to
!SER_RS485_RTS_ON_SEND.
> >> + if (copy_to_user(argp, &rs485, sizeof(rs485)))
> >> + return -EFAULT;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +
You can drop the second newline here (and elsewhere if you added more of
these).
> >> +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;
> >> +}
Johan
Exar devices like XR21B1411 can control an RS485 transceiver by
automatically asserting the RTS#/RS485 pin before sending data
and deasserting it when the last stop bit has been transmitted.
The polarity of the RST#/RS485 signal is configurable and the
hardware also supports half-duplex turn-around delay and
address matching mode.
Add support for enabling and disabling RS-485 mode and
configuring the RST#/RS485 signal polarity using the TIOCGRS485
and TIOCSRS485 ioctls. Support for half-duplex turn-around delay
and address matching mode are left unimplemented for now.
User enables RS-485 mode by setting SER_RS485_ENABLED flag in
struct serial_rs485 flags. User should also set either
SER_RS485_RTS_ON_SEND or SER_RS485_RTS_AFTER_SEND to select the
behaviour of the RTS#/RS485 pin. Setting SER_RS485_RTS_ON_SEND
will drive RTS#/RS485 high during transmission. As this is the
typical application described by Exar, it is selected when
user sets neither or both flags.
Signed-off-by: Jarkko Sonninen <[email protected]>
---
Changes in v3:
- In this version only rs485.flags are stored to state.
- There is no locking as only one bit of the flags is used.
- ioctl returns -ENOIOCTLCMD as the actual error handling is in tty code.
Changes in v4:
- Store struct rs485 to data
- Add mutex to protect data->rs485.
- Implement SER_RS485_RTS_ON_SEND or SER_RS485_RTS_AFTER_SEND flags
- SER_RS485_RTS_ON_SEND is the default like in serial_core.c
drivers/usb/serial/xr_serial.c | 93 +++++++++++++++++++++++++++++++++-
1 file changed, 92 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
index fdb0aae546c3..a2157619b63c 100644
--- a/drivers/usb/serial/xr_serial.c
+++ b/drivers/usb/serial/xr_serial.c
@@ -19,6 +19,7 @@
#include <linux/usb.h>
#include <linux/usb/cdc.h>
#include <linux/usb/serial.h>
+#include <linux/mutex.h>
struct xr_txrx_clk_mask {
u16 tx;
@@ -93,6 +94,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 +239,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;
+ struct mutex lock;
};
static int xr_set_reg(struct usb_serial_port *port, u8 channel, u16 reg, u16 val)
@@ -630,6 +634,7 @@ static void xr_set_flow_mode(struct tty_struct *tty,
const struct xr_type *type = data->type;
u16 flow, gpio_mode;
int ret;
+ bool rs485_enabled;
ret = xr_get_reg_uart(port, type->gpio_mode, &gpio_mode);
if (ret)
@@ -645,7 +650,17 @@ 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;
- if (C_CRTSCTS(tty) && C_BAUD(tty) != B0) {
+ rs485_enabled = !!(data->rs485.flags & SER_RS485_ENABLED);
+ if (rs485_enabled) {
+ dev_dbg(&port->dev, "Enabling RS-485\n");
+ gpio_mode |= XR_GPIO_MODE_SEL_RS485;
+ if (data->rs485.flags & SER_RS485_RTS_ON_SEND)
+ gpio_mode |= XR_GPIO_MODE_RS485_TX_H;
+ else
+ gpio_mode &= ~XR_GPIO_MODE_RS485_TX_H;
+ }
+
+ if (C_CRTSCTS(tty) && C_BAUD(tty) != B0 && !rs485_enabled) {
dev_dbg(&port->dev, "Enabling hardware flow ctrl\n");
gpio_mode |= XR_GPIO_MODE_SEL_RTS_CTS;
flow = XR_UART_FLOW_MODE_HW;
@@ -809,6 +824,80 @@ static void xr_cdc_set_line_coding(struct tty_struct *tty,
kfree(lc);
}
+static void xr_sanitize_serial_rs485(struct serial_rs485 *rs485)
+{
+ if (!(rs485->flags & SER_RS485_ENABLED)) {
+ memset(rs485, 0, sizeof(*rs485));
+ return;
+ }
+
+ /* Select RTS on send if the user hasn't selected the mode properly */
+ if (!!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
+ !!(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
+ rs485->flags |= SER_RS485_RTS_ON_SEND;
+ rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
+ }
+
+ /* Only the flags are implemented at the moment */
+ rs485->flags &= SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
+ SER_RS485_RTS_AFTER_SEND;
+ rs485->delay_rts_before_send = 0;
+ rs485->delay_rts_after_send = 0;
+ memset(rs485->padding, 0, sizeof(rs485->padding));
+}
+
+static int xr_get_rs485_config(struct tty_struct *tty,
+ struct serial_rs485 *argp)
+{
+ struct usb_serial_port *port = tty->driver_data;
+ struct xr_data *data = usb_get_serial_port_data(port);
+
+ mutex_lock(&data->lock);
+ if (copy_to_user(argp, &data->rs485, sizeof(data->rs485))) {
+ mutex_unlock(&data->lock);
+ return -EFAULT;
+ }
+ mutex_unlock(&data->lock);
+
+ return 0;
+}
+
+static int xr_set_rs485_config(struct tty_struct *tty,
+ struct serial_rs485 *argp)
+{
+ struct usb_serial_port *port = tty->driver_data;
+ struct xr_data *data = usb_get_serial_port_data(port);
+ struct serial_rs485 rs485;
+
+ if (copy_from_user(&rs485, argp, sizeof(rs485)))
+ return -EFAULT;
+ xr_sanitize_serial_rs485(&rs485);
+
+ mutex_lock(&data->lock);
+ data->rs485 = rs485;
+ xr_set_flow_mode(tty, port, NULL);
+ mutex_unlock(&data->lock);
+
+ if (copy_to_user(argp, &rs485, sizeof(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 void xr_set_termios(struct tty_struct *tty,
struct usb_serial_port *port,
const struct ktermios *old_termios)
@@ -939,6 +1028,7 @@ static int xr_port_probe(struct usb_serial_port *port)
return -ENOMEM;
data->type = type;
+ mutex_init(&data->lock);
desc = &port->serial->interface->cur_altsetting->desc;
if (type_id == XR21V141X)
@@ -1010,6 +1100,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 Jarkko,
kernel test robot noticed the following build warnings:
[auto build test WARNING on johan-usb-serial/usb-next]
[also build test WARNING on johan-usb-serial/usb-linus linus/master v6.3 next-20230421]
[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/20230424-030038
base: https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git usb-next
patch link: https://lore.kernel.org/r/20230423185929.1595056-1-kasper%40iki.fi
patch subject: [PATCH v4] USB: serial: xr: Add TIOCGRS485 and TIOCSRS485 ioctls
config: ia64-randconfig-s053-20230423 (https://download.01.org/0day-ci/archive/20230424/[email protected]/config)
compiler: ia64-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/a80fa27b4fe1974bad2427d7f3260012a04b721a
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/20230424-030038
git checkout a80fa27b4fe1974bad2427d7f3260012a04b721a
# 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=ia64 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=ia64 SHELL=/bin/bash drivers/usb/serial/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
sparse warnings: (new ones prefixed by >>)
>> drivers/usb/serial/xr_serial.c:856:26: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void [noderef] __user *to @@ got struct serial_rs485 *argp @@
drivers/usb/serial/xr_serial.c:856:26: sparse: expected void [noderef] __user *to
drivers/usb/serial/xr_serial.c:856:26: sparse: got struct serial_rs485 *argp
>> drivers/usb/serial/xr_serial.c:872:36: sparse: sparse: incorrect type in argument 2 (different address spaces) @@ expected void const [noderef] __user *from @@ got struct serial_rs485 *argp @@
drivers/usb/serial/xr_serial.c:872:36: sparse: expected void const [noderef] __user *from
drivers/usb/serial/xr_serial.c:872:36: sparse: got struct serial_rs485 *argp
drivers/usb/serial/xr_serial.c:881:26: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void [noderef] __user *to @@ got struct serial_rs485 *argp @@
drivers/usb/serial/xr_serial.c:881:26: sparse: expected void [noderef] __user *to
drivers/usb/serial/xr_serial.c:881:26: sparse: got struct serial_rs485 *argp
>> drivers/usb/serial/xr_serial.c:893:49: sparse: sparse: incorrect type in argument 2 (different address spaces) @@ expected struct serial_rs485 *argp @@ got void [noderef] __user *argp @@
drivers/usb/serial/xr_serial.c:893:49: sparse: expected struct serial_rs485 *argp
drivers/usb/serial/xr_serial.c:893:49: sparse: got void [noderef] __user *argp
drivers/usb/serial/xr_serial.c:895:49: sparse: sparse: incorrect type in argument 2 (different address spaces) @@ expected struct serial_rs485 *argp @@ got void [noderef] __user *argp @@
drivers/usb/serial/xr_serial.c:895:49: sparse: expected struct serial_rs485 *argp
drivers/usb/serial/xr_serial.c:895:49: sparse: got void [noderef] __user *argp
vim +856 drivers/usb/serial/xr_serial.c
848
849 static int xr_get_rs485_config(struct tty_struct *tty,
850 struct serial_rs485 *argp)
851 {
852 struct usb_serial_port *port = tty->driver_data;
853 struct xr_data *data = usb_get_serial_port_data(port);
854
855 mutex_lock(&data->lock);
> 856 if (copy_to_user(argp, &data->rs485, sizeof(data->rs485))) {
857 mutex_unlock(&data->lock);
858 return -EFAULT;
859 }
860 mutex_unlock(&data->lock);
861
862 return 0;
863 }
864
865 static int xr_set_rs485_config(struct tty_struct *tty,
866 struct serial_rs485 *argp)
867 {
868 struct usb_serial_port *port = tty->driver_data;
869 struct xr_data *data = usb_get_serial_port_data(port);
870 struct serial_rs485 rs485;
871
> 872 if (copy_from_user(&rs485, argp, sizeof(rs485)))
873 return -EFAULT;
874 xr_sanitize_serial_rs485(&rs485);
875
876 mutex_lock(&data->lock);
877 data->rs485 = rs485;
878 xr_set_flow_mode(tty, port, NULL);
879 mutex_unlock(&data->lock);
880
881 if (copy_to_user(argp, &rs485, sizeof(rs485)))
882 return -EFAULT;
883
884 return 0;
885 }
886
887 static int xr_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
888 {
889 void __user *argp = (void __user *)arg;
890
891 switch (cmd) {
892 case TIOCGRS485:
> 893 return xr_get_rs485_config(tty, argp);
894 case TIOCSRS485:
895 return xr_set_rs485_config(tty, argp);
896 }
897
898 return -ENOIOCTLCMD;
899 }
900
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Jarkko,
I've been waiting for you to send a v5 that addresses the issue that the
kernel test robot found, but I guess I can review the other changes in
v4 first.
On Sun, Apr 23, 2023 at 09:59:28PM +0300, Jarkko Sonninen wrote:
> Exar devices like XR21B1411 can control an RS485 transceiver by
> automatically asserting the RTS#/RS485 pin before sending data
> and deasserting it when the last stop bit has been transmitted.
> The polarity of the RST#/RS485 signal is configurable and the
> hardware also supports half-duplex turn-around delay and
> address matching mode.
>
> Add support for enabling and disabling RS-485 mode and
> configuring the RST#/RS485 signal polarity using the TIOCGRS485
> and TIOCSRS485 ioctls. Support for half-duplex turn-around delay
> and address matching mode are left unimplemented for now.
>
> User enables RS-485 mode by setting SER_RS485_ENABLED flag in
> struct serial_rs485 flags. User should also set either
> SER_RS485_RTS_ON_SEND or SER_RS485_RTS_AFTER_SEND to select the
> behaviour of the RTS#/RS485 pin. Setting SER_RS485_RTS_ON_SEND
> will drive RTS#/RS485 high during transmission. As this is the
> typical application described by Exar, it is selected when
> user sets neither or both flags.
Thanks for updating the commit message here.
Since RTS# is active low, shouldn't SER_RS485_RTS_ON_SEND drive RTS# low
rather than high during transmission as I also pointed out earlier?
> Signed-off-by: Jarkko Sonninen <[email protected]>
> ---
> Changes in v3:
> - In this version only rs485.flags are stored to state.
> - There is no locking as only one bit of the flags is used.
> - ioctl returns -ENOIOCTLCMD as the actual error handling is in tty code.
> Changes in v4:
> - Store struct rs485 to data
> - Add mutex to protect data->rs485.
> - Implement SER_RS485_RTS_ON_SEND or SER_RS485_RTS_AFTER_SEND flags
> - SER_RS485_RTS_ON_SEND is the default like in serial_core.c
>
>
> drivers/usb/serial/xr_serial.c | 93 +++++++++++++++++++++++++++++++++-
> 1 file changed, 92 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
> index fdb0aae546c3..a2157619b63c 100644
> --- a/drivers/usb/serial/xr_serial.c
> +++ b/drivers/usb/serial/xr_serial.c
> @@ -19,6 +19,7 @@
> #include <linux/usb.h>
> #include <linux/usb/cdc.h>
> #include <linux/usb/serial.h>
> +#include <linux/mutex.h>
Nit: keep the includes sorted alphabetically.
> struct xr_txrx_clk_mask {
> u16 tx;
> @@ -93,6 +94,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 +239,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;
> + struct mutex lock;
> };
>
> static int xr_set_reg(struct usb_serial_port *port, u8 channel, u16 reg, u16 val)
> @@ -630,6 +634,7 @@ static void xr_set_flow_mode(struct tty_struct *tty,
> const struct xr_type *type = data->type;
> u16 flow, gpio_mode;
> int ret;
> + bool rs485_enabled;
Nit: move above ret to maintain some approximation of reverse xmas style
declaration.
>
> ret = xr_get_reg_uart(port, type->gpio_mode, &gpio_mode);
> if (ret)
> @@ -645,7 +650,17 @@ 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;
>
> - if (C_CRTSCTS(tty) && C_BAUD(tty) != B0) {
> + rs485_enabled = !!(data->rs485.flags & SER_RS485_ENABLED);
> + if (rs485_enabled) {
> + dev_dbg(&port->dev, "Enabling RS-485\n");
> + gpio_mode |= XR_GPIO_MODE_SEL_RS485;
> + if (data->rs485.flags & SER_RS485_RTS_ON_SEND)
> + gpio_mode |= XR_GPIO_MODE_RS485_TX_H;
> + else
> + gpio_mode &= ~XR_GPIO_MODE_RS485_TX_H;
As mentioned above, should this not be inverted?
> + }
> +
> + if (C_CRTSCTS(tty) && C_BAUD(tty) != B0 && !rs485_enabled) {
> dev_dbg(&port->dev, "Enabling hardware flow ctrl\n");
> gpio_mode |= XR_GPIO_MODE_SEL_RTS_CTS;
> flow = XR_UART_FLOW_MODE_HW;
> @@ -809,6 +824,80 @@ static void xr_cdc_set_line_coding(struct tty_struct *tty,
> kfree(lc);
> }
>
> +static void xr_sanitize_serial_rs485(struct serial_rs485 *rs485)
> +{
> + if (!(rs485->flags & SER_RS485_ENABLED)) {
> + memset(rs485, 0, sizeof(*rs485));
> + return;
> + }
This looks odd to me, but I see that this is what serial core is
currently doing...
> +
> + /* Select RTS on send if the user hasn't selected the mode properly */
> + if (!!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
> + !!(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
> + rs485->flags |= SER_RS485_RTS_ON_SEND;
> + rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
> + }
> +
> + /* Only the flags are implemented at the moment */
> + rs485->flags &= SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
> + SER_RS485_RTS_AFTER_SEND;
> + rs485->delay_rts_before_send = 0;
> + rs485->delay_rts_after_send = 0;
> + memset(rs485->padding, 0, sizeof(rs485->padding));
> +}
> +
> +static int xr_get_rs485_config(struct tty_struct *tty,
> + struct serial_rs485 *argp)
You still need __user annotation here as the build robot reported.
> +{
> + struct usb_serial_port *port = tty->driver_data;
> + struct xr_data *data = usb_get_serial_port_data(port);
> +
> + mutex_lock(&data->lock);
This does not prevent the termios structure from changing underneath
you. You need to take a write lock on the termios rw sem here instead.
> + if (copy_to_user(argp, &data->rs485, sizeof(data->rs485))) {
> + mutex_unlock(&data->lock);
> + return -EFAULT;
> + }
> + mutex_unlock(&data->lock);
> +
> + return 0;
> +}
> +
> +static int xr_set_rs485_config(struct tty_struct *tty,
> + struct serial_rs485 *argp)
missing __user
> +{
> + struct usb_serial_port *port = tty->driver_data;
> + struct xr_data *data = usb_get_serial_port_data(port);
> + struct serial_rs485 rs485;
> +
> + if (copy_from_user(&rs485, argp, sizeof(rs485)))
> + return -EFAULT;
> + xr_sanitize_serial_rs485(&rs485);
> +
> + mutex_lock(&data->lock);
Same here as set_termios() (and xr_set_flow_mode()) can otherwise be
called in parallel.
> + data->rs485 = rs485;
> + xr_set_flow_mode(tty, port, NULL);
> + mutex_unlock(&data->lock);
> +
> + if (copy_to_user(argp, &rs485, sizeof(rs485)))
> + return -EFAULT;
> +
> + return 0;
> +}
Looks good otherwise.
Johan
On 6/20/23 15:38, Johan Hovold wrote:
> Hi Jarkko,
>
> I've been waiting for you to send a v5 that addresses the issue that the
> kernel test robot found, but I guess I can review the other changes in
> v4 first.
Thank you. Sorry for slow responses. My mind has been elsewhere.
> On Sun, Apr 23, 2023 at 09:59:28PM +0300, Jarkko Sonninen wrote:
>> Exar devices like XR21B1411 can control an RS485 transceiver by
>> automatically asserting the RTS#/RS485 pin before sending data
>> and deasserting it when the last stop bit has been transmitted.
>> The polarity of the RST#/RS485 signal is configurable and the
>> hardware also supports half-duplex turn-around delay and
>> address matching mode.
>>
>> Add support for enabling and disabling RS-485 mode and
>> configuring the RST#/RS485 signal polarity using the TIOCGRS485
>> and TIOCSRS485 ioctls. Support for half-duplex turn-around delay
>> and address matching mode are left unimplemented for now.
>>
>> User enables RS-485 mode by setting SER_RS485_ENABLED flag in
>> struct serial_rs485 flags. User should also set either
>> SER_RS485_RTS_ON_SEND or SER_RS485_RTS_AFTER_SEND to select the
>> behaviour of the RTS#/RS485 pin. Setting SER_RS485_RTS_ON_SEND
>> will drive RTS#/RS485 high during transmission. As this is the
>> typical application described by Exar, it is selected when
>> user sets neither or both flags.
> Thanks for updating the commit message here.
>
> Since RTS# is active low, shouldn't SER_RS485_RTS_ON_SEND drive RTS# low
> rather than high during transmission as I also pointed out earlier?
I guess you are right. I'll change that.
I use an exar usb adapter to control a solar charging controller. I
haven't found any other type of exar adapters in ebay.
This adapter uses high level (RTS off) on TX. So I really would like it
to work with the default configuration.
I hope it is ok to use SER_RS485_RTS_AFTER_SEND as the default
>> Signed-off-by: Jarkko Sonninen <[email protected]>
>> ---
>> Changes in v3:
>> - In this version only rs485.flags are stored to state.
>> - There is no locking as only one bit of the flags is used.
>> - ioctl returns -ENOIOCTLCMD as the actual error handling is in tty code.
>> Changes in v4:
>> - Store struct rs485 to data
>> - Add mutex to protect data->rs485.
>> - Implement SER_RS485_RTS_ON_SEND or SER_RS485_RTS_AFTER_SEND flags
>> - SER_RS485_RTS_ON_SEND is the default like in serial_core.c
>>
>>
>> drivers/usb/serial/xr_serial.c | 93 +++++++++++++++++++++++++++++++++-
>> 1 file changed, 92 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
>> index fdb0aae546c3..a2157619b63c 100644
>> --- a/drivers/usb/serial/xr_serial.c
>> +++ b/drivers/usb/serial/xr_serial.c
>> @@ -19,6 +19,7 @@
>> #include <linux/usb.h>
>> #include <linux/usb/cdc.h>
>> #include <linux/usb/serial.h>
>> +#include <linux/mutex.h>
> Nit: keep the includes sorted alphabetically.
>
>> struct xr_txrx_clk_mask {
>> u16 tx;
>> @@ -93,6 +94,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 +239,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;
>> + struct mutex lock;
>> };
>>
>> static int xr_set_reg(struct usb_serial_port *port, u8 channel, u16 reg, u16 val)
>> @@ -630,6 +634,7 @@ static void xr_set_flow_mode(struct tty_struct *tty,
>> const struct xr_type *type = data->type;
>> u16 flow, gpio_mode;
>> int ret;
>> + bool rs485_enabled;
> Nit: move above ret to maintain some approximation of reverse xmas style
> declaration.
>
>>
>> ret = xr_get_reg_uart(port, type->gpio_mode, &gpio_mode);
>> if (ret)
>> @@ -645,7 +650,17 @@ 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;
>>
>> - if (C_CRTSCTS(tty) && C_BAUD(tty) != B0) {
>> + rs485_enabled = !!(data->rs485.flags & SER_RS485_ENABLED);
>> + if (rs485_enabled) {
>> + dev_dbg(&port->dev, "Enabling RS-485\n");
>> + gpio_mode |= XR_GPIO_MODE_SEL_RS485;
>> + if (data->rs485.flags & SER_RS485_RTS_ON_SEND)
>> + gpio_mode |= XR_GPIO_MODE_RS485_TX_H;
>> + else
>> + gpio_mode &= ~XR_GPIO_MODE_RS485_TX_H;
> As mentioned above, should this not be inverted?
>
>> + }
>> +
>> + if (C_CRTSCTS(tty) && C_BAUD(tty) != B0 && !rs485_enabled) {
>> dev_dbg(&port->dev, "Enabling hardware flow ctrl\n");
>> gpio_mode |= XR_GPIO_MODE_SEL_RTS_CTS;
>> flow = XR_UART_FLOW_MODE_HW;
>> @@ -809,6 +824,80 @@ static void xr_cdc_set_line_coding(struct tty_struct *tty,
>> kfree(lc);
>> }
>>
>> +static void xr_sanitize_serial_rs485(struct serial_rs485 *rs485)
>> +{
>> + if (!(rs485->flags & SER_RS485_ENABLED)) {
>> + memset(rs485, 0, sizeof(*rs485));
>> + return;
>> + }
> This looks odd to me, but I see that this is what serial core is
> currently doing...
>
>> +
>> + /* Select RTS on send if the user hasn't selected the mode properly */
>> + if (!!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
>> + !!(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
>> + rs485->flags |= SER_RS485_RTS_ON_SEND;
>> + rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
>> + }
>> +
>> + /* Only the flags are implemented at the moment */
>> + rs485->flags &= SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
>> + SER_RS485_RTS_AFTER_SEND;
>> + rs485->delay_rts_before_send = 0;
>> + rs485->delay_rts_after_send = 0;
>> + memset(rs485->padding, 0, sizeof(rs485->padding));
>> +}
>> +
>> +static int xr_get_rs485_config(struct tty_struct *tty,
>> + struct serial_rs485 *argp)
> You still need __user annotation here as the build robot reported.
>
>> +{
>> + struct usb_serial_port *port = tty->driver_data;
>> + struct xr_data *data = usb_get_serial_port_data(port);
>> +
>> + mutex_lock(&data->lock);
> This does not prevent the termios structure from changing underneath
> you. You need to take a write lock on the termios rw sem here instead.
I'll change these to "down_read(&tty->termios_rwsem);" and
"up_read(&tty->termios_rwsem);"
I would feel safer by using a local variable "struct serial_rs485" to
minimize the holding of the semaphore.
>
>> + if (copy_to_user(argp, &data->rs485, sizeof(data->rs485))) {
>> + mutex_unlock(&data->lock);
>> + return -EFAULT;
>> + }
>> + mutex_unlock(&data->lock);
>> +
>> + return 0;
>> +}
>> +
>> +static int xr_set_rs485_config(struct tty_struct *tty,
>> + struct serial_rs485 *argp)
> missing __user
>
>> +{
>> + struct usb_serial_port *port = tty->driver_data;
>> + struct xr_data *data = usb_get_serial_port_data(port);
>> + struct serial_rs485 rs485;
>> +
>> + if (copy_from_user(&rs485, argp, sizeof(rs485)))
>> + return -EFAULT;
>> + xr_sanitize_serial_rs485(&rs485);
>> +
>> + mutex_lock(&data->lock);
> Same here as set_termios() (and xr_set_flow_mode()) can otherwise be
> called in parallel.
I'll change these to "down_write(&tty->termios_rwsem);" and
"up_write(&tty->termios_rwsem);"
>> + data->rs485 = rs485;
>> + xr_set_flow_mode(tty, port, NULL);
>> + mutex_unlock(&data->lock);
>> +
>> + if (copy_to_user(argp, &rs485, sizeof(rs485)))
>> + return -EFAULT;
>> +
>> + return 0;
>> +}
> Looks good otherwise.
>
> Johan
- Jarkko
Exar devices like XR21B1411 can control an RS485 transceiver by
automatically asserting the RTS#/RS485 pin before sending data
and deasserting it when the last stop bit has been transmitted.
The polarity of the RST#/RS485 signal is configurable and the
hardware also supports half-duplex turn-around delay and
address matching mode.
Add support for enabling and disabling RS-485 mode and
configuring the RST#/RS485 signal polarity using the TIOCGRS485
and TIOCSRS485 ioctls. Support for half-duplex turn-around delay
and address matching mode are left unimplemented for now.
User enables RS-485 mode by setting SER_RS485_ENABLED flag in
struct serial_rs485 flags. User should also set either
SER_RS485_RTS_ON_SEND or SER_RS485_RTS_AFTER_SEND to select the
behaviour of the RTS#/RS485 pin. Setting SER_RS485_RTS_AFTER_SEND
will drive RTS#/RS485 high during transmission. As this is the
typical application described by Exar, it is selected when
user sets neither or both flags.
Signed-off-by: Jarkko Sonninen <[email protected]>
---
Changes in v3:
- In this version only rs485.flags are stored to state.
- There is no locking as only one bit of the flags is used.
- ioctl returns -ENOIOCTLCMD as the actual error handling is in tty code.
Changes in v4:
- Store struct rs485 to data
- Add mutex to protect data->rs485.
- Implement SER_RS485_RTS_ON_SEND or SER_RS485_RTS_AFTER_SEND flags
- SER_RS485_RTS_ON_SEND is the default like in serial_core.c
Change in v5:
- Use tty->termios_rwsem semaphore instead of own mutex
- Set SER_RS485_RTS_AFTER_SEND as the default
- Fix XR_GPIO_MODE_RS485_TX_H setting with SER_RS485_RTS_ON_SEND
- Add missing __user directives
drivers/usb/serial/xr_serial.c | 90 +++++++++++++++++++++++++++++++++-
1 file changed, 89 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
index 4ec7c5892b84..6a5a70e77969 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,7 @@ 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;
};
static int xr_set_reg(struct usb_serial_port *port, u8 channel, u16 reg, u16 val)
@@ -629,6 +631,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;
+ bool rs485_enabled;
int ret;
ret = xr_get_reg_uart(port, type->gpio_mode, &gpio_mode);
@@ -645,7 +648,17 @@ 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;
- if (C_CRTSCTS(tty) && C_BAUD(tty) != B0) {
+ rs485_enabled = !!(data->rs485.flags & SER_RS485_ENABLED);
+ if (rs485_enabled) {
+ dev_dbg(&port->dev, "Enabling RS-485\n");
+ gpio_mode |= XR_GPIO_MODE_SEL_RS485;
+ if (data->rs485.flags & SER_RS485_RTS_ON_SEND)
+ gpio_mode &= ~XR_GPIO_MODE_RS485_TX_H;
+ else
+ gpio_mode |= XR_GPIO_MODE_RS485_TX_H;
+ }
+
+ if (C_CRTSCTS(tty) && C_BAUD(tty) != B0 && !rs485_enabled) {
dev_dbg(&port->dev, "Enabling hardware flow ctrl\n");
gpio_mode |= XR_GPIO_MODE_SEL_RTS_CTS;
flow = XR_UART_FLOW_MODE_HW;
@@ -809,6 +822,80 @@ static void xr_cdc_set_line_coding(struct tty_struct *tty,
kfree(lc);
}
+static void xr_sanitize_serial_rs485(struct serial_rs485 *rs485)
+{
+ if (!(rs485->flags & SER_RS485_ENABLED)) {
+ memset(rs485, 0, sizeof(*rs485));
+ return;
+ }
+
+ /* Select RTS after send if the user hasn't selected the mode properly */
+ if (!!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
+ !!(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
+ rs485->flags |= SER_RS485_RTS_AFTER_SEND;
+ rs485->flags &= ~SER_RS485_RTS_ON_SEND;
+ }
+
+ /* Only the flags are implemented at the moment */
+ rs485->flags &= SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND |
+ SER_RS485_RTS_AFTER_SEND;
+ rs485->delay_rts_before_send = 0;
+ rs485->delay_rts_after_send = 0;
+ memset(rs485->padding, 0, sizeof(rs485->padding));
+}
+
+static int xr_get_rs485_config(struct tty_struct *tty,
+ struct serial_rs485 __user *argp)
+{
+ struct usb_serial_port *port = tty->driver_data;
+ struct xr_data *data = usb_get_serial_port_data(port);
+
+ down_read(&tty->termios_rwsem);
+ if (copy_to_user(argp, &data->rs485, sizeof(data->rs485))) {
+ up_read(&tty->termios_rwsem);
+ return -EFAULT;
+ }
+ up_read(&tty->termios_rwsem);
+
+ return 0;
+}
+
+static int xr_set_rs485_config(struct tty_struct *tty,
+ struct serial_rs485 __user *argp)
+{
+ struct usb_serial_port *port = tty->driver_data;
+ struct xr_data *data = usb_get_serial_port_data(port);
+ struct serial_rs485 rs485;
+
+ if (copy_from_user(&rs485, argp, sizeof(rs485)))
+ return -EFAULT;
+ xr_sanitize_serial_rs485(&rs485);
+
+ down_write(&tty->termios_rwsem);
+ data->rs485 = rs485;
+ xr_set_flow_mode(tty, port, NULL);
+ up_write(&tty->termios_rwsem);
+
+ if (copy_to_user(argp, &rs485, sizeof(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 void xr_set_termios(struct tty_struct *tty,
struct usb_serial_port *port,
const struct ktermios *old_termios)
@@ -1010,6 +1097,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
On Sat, Jul 08, 2023 at 05:56:50PM +0300, Jarkko Sonninen wrote:
> Exar devices like XR21B1411 can control an RS485 transceiver by
> automatically asserting the RTS#/RS485 pin before sending data
> and deasserting it when the last stop bit has been transmitted.
> The polarity of the RST#/RS485 signal is configurable and the
> hardware also supports half-duplex turn-around delay and
> address matching mode.
>
> Add support for enabling and disabling RS-485 mode and
> configuring the RST#/RS485 signal polarity using the TIOCGRS485
> and TIOCSRS485 ioctls. Support for half-duplex turn-around delay
> and address matching mode are left unimplemented for now.
>
> User enables RS-485 mode by setting SER_RS485_ENABLED flag in
> struct serial_rs485 flags. User should also set either
> SER_RS485_RTS_ON_SEND or SER_RS485_RTS_AFTER_SEND to select the
> behaviour of the RTS#/RS485 pin. Setting SER_RS485_RTS_AFTER_SEND
> will drive RTS#/RS485 high during transmission. As this is the
> typical application described by Exar, it is selected when
> user sets neither or both flags.
>
> Signed-off-by: Jarkko Sonninen <[email protected]>
> ---
> Changes in v3:
> - In this version only rs485.flags are stored to state.
> - There is no locking as only one bit of the flags is used.
> - ioctl returns -ENOIOCTLCMD as the actual error handling is in tty code.
> Changes in v4:
> - Store struct rs485 to data
> - Add mutex to protect data->rs485.
> - Implement SER_RS485_RTS_ON_SEND or SER_RS485_RTS_AFTER_SEND flags
> - SER_RS485_RTS_ON_SEND is the default like in serial_core.c
> Change in v5:
> - Use tty->termios_rwsem semaphore instead of own mutex
> - Set SER_RS485_RTS_AFTER_SEND as the default
> - Fix XR_GPIO_MODE_RS485_TX_H setting with SER_RS485_RTS_ON_SEND
> - Add missing __user directives
I've applied this one now but with the small change I just mentioned
that makes SER_RS485_RTS_ON_SEND determine SER_RS485_RTS_AFTER_SEND.
Since this sets SER_RS485_RTS_AFTER_SEND when neither flag is set, I
believe you get the behaviour you preferred (even if
SER_RS485_RTS_AFTER_SEND is now cleared when both flags are set).
Let me know otherwise and we'll discuss it. Here's the result:
https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/commit/?h=usb-next&id=974e2f6a0554685493cc44406bc7d8ba0a3b0e33
Thanks for sticking with. I think the end result looks really good.
Johan
On Thu, Jul 06, 2023 at 10:37:51PM +0300, Jarkko Sonninen wrote:
> On 6/20/23 15:38, Johan Hovold wrote:
> Thank you. Sorry for slow responses. My mind has been elsewhere.
No worries at all.
> > On Sun, Apr 23, 2023 at 09:59:28PM +0300, Jarkko Sonninen wrote:
> >> User enables RS-485 mode by setting SER_RS485_ENABLED flag in
> >> struct serial_rs485 flags. User should also set either
> >> SER_RS485_RTS_ON_SEND or SER_RS485_RTS_AFTER_SEND to select the
> >> behaviour of the RTS#/RS485 pin. Setting SER_RS485_RTS_ON_SEND
> >> will drive RTS#/RS485 high during transmission. As this is the
> >> typical application described by Exar, it is selected when
> >> user sets neither or both flags.
> > Since RTS# is active low, shouldn't SER_RS485_RTS_ON_SEND drive RTS# low
> > rather than high during transmission as I also pointed out earlier?
>
> I guess you are right. I'll change that.
>
>
> I use an exar usb adapter to control a solar charging controller. I
> haven't found any other type of exar adapters in ebay.
>
> This adapter uses high level (RTS off) on TX. So I really would like it
> to work with the default configuration.
>
> I hope it is ok to use SER_RS485_RTS_AFTER_SEND as the default
I was first going to argue against with this as serial core defaults to
SER_RS485_RTS_ON_SEND when neither is set, but I changed my mind as I
believe this is more in line with how these flags were intended to be
used.
Having both flags set to the same value clearly makes no sense, but if
left that way I think SER_RS485_RTS_ON_SEND should take precedence and
SER_RS485_RTS_AFTER_SEND simply be set not its negation (when the
hardware does not support the nonsensical RTS always asserted
combination...). That is:
/* RTS always toggles after TX */
if (rs485->flags & SER_RS485_RTS_ON_SEND)
rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
else
rs485->flags |= SER_RS485_RTS_AFTER_SEND;
Since you still need to use the new ioctl() to enable RS485 mode, there
shouldn't really be any reason not to simultaneously set the polarity
your application expects anyway.
Johan