Message-ID: <a1ba59be-30aa-08e9-65e7-2c458cc164f9@iki.fi>
Date:   Mon, 13 Mar 2023 09:49:26 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH] USB: serial: xr: Add TIOCGRS485 and TIOCSRS485 ioctls
Content-Language: en-US
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Johan Hovold <johan@kernel.org>, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230313010416.845252-1-kasper@iki.fi>
 <ZA7KIs2jA/acpN9n@kroah.com>
From:   Jarkko Sonninen <kasper@iki.fi>
In-Reply-To: <ZA7KIs2jA/acpN9n@kroah.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 3/13/23 09:00, Greg Kroah-Hartman wrote:
> Nit, your "To:" line was corrupted :(
>
> On Mon, Mar 13, 2023 at 03:04:16AM +0200, Jarkko Sonninen wrote:
>> Add support for RS-485 in Exar USB adapters.
>> RS-485 mode is controlled by TIOCGRS485 and TIOCSRS485 ioctls.
>> Gpio mode register is set to enable RS-485.
>>
>> Signed-off-by: Jarkko Sonninen <kasper@iki.fi>
>> ---
>>   drivers/usb/serial/xr_serial.c | 72 +++++++++++++++++++++++++++++++++-
>>   1 file changed, 71 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c
>> index fdb0aae546c3..edbb6add087c 100644
>> --- a/drivers/usb/serial/xr_serial.c
>> +++ b/drivers/usb/serial/xr_serial.c
>> @@ -93,6 +93,7 @@ struct xr_txrx_clk_mask {
>>   #define XR_GPIO_MODE_SEL_DTR_DSR	0x2
>>   #define XR_GPIO_MODE_SEL_RS485		0x3
>>   #define XR_GPIO_MODE_SEL_RS485_ADDR	0x4
>> +#define XR_GPIO_MODE_RS485_TX_H		0x8
>>   #define XR_GPIO_MODE_TX_TOGGLE		0x100
>>   #define XR_GPIO_MODE_RX_TOGGLE		0x200
>>   
>> @@ -237,6 +238,8 @@ static const struct xr_type xr_types[] = {
>>   struct xr_data {
>>   	const struct xr_type *type;
>>   	u8 channel;			/* zero-based index or interface number */
>> +	struct serial_rs485 rs485;
>> +	spinlock_t lock;
>>   };
>>   
>>   static int xr_set_reg(struct usb_serial_port *port, u8 channel, u16 reg, u16 val)
>> @@ -629,6 +632,7 @@ static void xr_set_flow_mode(struct tty_struct *tty,
>>   	struct xr_data *data = usb_get_serial_port_data(port);
>>   	const struct xr_type *type = data->type;
>>   	u16 flow, gpio_mode;
>> +	unsigned long flags, rs485_flags;
>>   	int ret;
>>   
>>   	ret = xr_get_reg_uart(port, type->gpio_mode, &gpio_mode);
>> @@ -645,9 +649,16 @@ static void xr_set_flow_mode(struct tty_struct *tty,
>>   	/* Set GPIO mode for controlling the pins manually by default. */
>>   	gpio_mode &= ~XR_GPIO_MODE_SEL_MASK;
>>   
>> +	spin_lock_irqsave(&data->lock, flags);
>> +	rs485_flags = data->rs485.flags;
>> +	spin_unlock_irqrestore(&data->lock, flags);
>> +	if (rs485_flags & SER_RS485_ENABLED)
>> +		gpio_mode |= XR_GPIO_MODE_SEL_RS485 | XR_GPIO_MODE_RS485_TX_H;
>> +	else if (C_CRTSCTS(tty) && C_BAUD(tty) != B0)
>> +		gpio_mode |= XR_GPIO_MODE_SEL_RTS_CTS;
>> +
>>   	if (C_CRTSCTS(tty) && C_BAUD(tty) != B0) {
>>   		dev_dbg(&port->dev, "Enabling hardware flow ctrl\n");
>> -		gpio_mode |= XR_GPIO_MODE_SEL_RTS_CTS;
>>   		flow = XR_UART_FLOW_MODE_HW;
>>   	} else if (I_IXON(tty)) {
>>   		u8 start_char = START_CHAR(tty);
>> @@ -827,6 +838,64 @@ static void xr_set_termios(struct tty_struct *tty,
>>   	xr_set_flow_mode(tty, port, old_termios);
>>   }
>>   
>> +static int xr_get_rs485_config(struct tty_struct *tty,
>> +			 unsigned int __user *argp)
>> +{
>> +	struct usb_serial_port *port = tty->driver_data;
>> +	struct xr_data *data = usb_get_serial_port_data(port);
>> +	unsigned long flags;
>> +	struct serial_rs485 rs485;
>> +
>> +	spin_lock_irqsave(&data->lock, flags);
>> +	memcpy(&rs485, &data->rs485, sizeof(rs485));
> Why are you using the stack for this?  Why not directly access the real
> structure instead?  And are you sure you need a lock?  If so, why?

I suppose the lock is not really needed and I'll change this. I was 
unsure about it.

>> +	spin_unlock_irqrestore(&data->lock, flags);
>> +	dev_dbg(tty->dev, "%s flags %02x\n", __func__, rs485.flags);
> dev_dbg() provides __func__ for free, so you never need to provide it
> again.  Just use the +f flag when enabling it from userspace.
>
>> +
>> +	if (copy_to_user(argp, &rs485, sizeof(rs485)))
>> +		return -EFAULT;
>> +
>> +	return 0;
>> +}
>> +
>> +static int xr_set_rs485_config(struct tty_struct *tty,
>> +			 unsigned long __user *argp)
>> +{
>> +	struct usb_serial_port *port = tty->driver_data;
>> +	struct xr_data *data = usb_get_serial_port_data(port);
>> +	struct serial_rs485 rs485;
>> +	unsigned long flags;
>> +
>> +	if (copy_from_user(&rs485, argp, sizeof(rs485)))
>> +		return -EFAULT;
>> +
>> +	dev_dbg(tty->dev, "%s flags %02x\n", __func__, rs485.flags);
> Again, no need for __func__.
>
>> +	rs485.flags &= SER_RS485_ENABLED;
>> +	spin_lock_irqsave(&data->lock, flags);
>> +	memcpy(&data->rs485, &rs485, sizeof(rs485));
>> +	spin_unlock_irqrestore(&data->lock, flags);
>> +	xr_set_flow_mode(tty, port, 0);
>> +
>> +	if (copy_to_user(argp, &data->rs485, sizeof(data->rs485)))
>> +		return -EFAULT;
>> +
>> +	return 0;
>> +}
>> +
>> +
>> +static int xr_ioctl(struct tty_struct *tty,
>> +					unsigned int cmd, unsigned long arg)
> Very odd indentation, please fix up.
>
>> +{
>> +	void __user *argp = (void __user *)arg;
>> +
>> +	switch (cmd) {
>> +	case TIOCGRS485:
>> +		return xr_get_rs485_config(tty, argp);
>> +	case TIOCSRS485:
>> +		return xr_set_rs485_config(tty, argp);
>> +	}
>> +	return -ENOIOCTLCMD;
> Wrong ioctl return value :(

What is the correct ioctl error return value ?
ENOIOCTLCMD was used in most places in usb serial as an error return.


> thanks,
>
> greg k-h

     - Jarkko