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

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

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

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

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/202304241303.WkdqPPbt-lkp@intel.com/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 <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304241303.WkdqPPbt-lkp@intel.com/

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