Re: [PATCH v3] USB: serial: xr: Add TIOCGRS485 and TIOCSRS485 ioctls

Jarkko Sonninen
2023-04-16T08:54:20Z

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 <kasper@iki.fi>
>> ---
>>
>> 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