Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <57fef05a-e885-f83b-1536-7a9f9e8a4adf@iki.fi>
Date:   Sun, 16 Apr 2023 11:40:00 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH v3] USB: serial: xr: Add TIOCGRS485 and TIOCSRS485 ioctls
Content-Language: en-US
To:     Johan Hovold <johan@kernel.org>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230313010416.845252-1-kasper@iki.fi>
 <20230314070002.1008959-1-kasper@iki.fi>
 <ZDfC9xkDMAJn60jc@hovoldconsulting.com>
From:   Jarkko Sonninen <kasper@iki.fi>
In-Reply-To: <ZDfC9xkDMAJn60jc@hovoldconsulting.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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