linux-kernel - Re: [PATCH v7 6/7] serial: omap: do not override settings for RS485 support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c88034d4-cb5-b64e-7a76-194ef35f28@linux.intel.com>
Date: Wed, 3 Jan 2024 13:36:23 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Lino Sanfilippo <l.sanfilippo@...bus.com>
cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
    Jiri Slaby <jirislaby@...nel.org>, u.kleine-koenig@...gutronix.de, 
    shawnguo@...nel.org, s.hauer@...gutronix.de, mcoquelin.stm32@...il.com, 
    alexandre.torgue@...s.st.com, cniedermaier@...electronics.com, 
    hugo@...ovil.com, m.brock@...mierlo.com, 
    LKML <linux-kernel@...r.kernel.org>, 
    linux-serial <linux-serial@...r.kernel.org>, LinoSanfilippo@....de, 
    Lukas Wunner <lukas@...ner.de>, p.rosenberger@...bus.com, 
    stable@...r.kernel.org
Subject: Re: [PATCH v7 6/7] serial: omap: do not override settings for RS485
 support

On Wed, 3 Jan 2024, Lino Sanfilippo wrote:

> The drivers RS485 support is deactivated if there is no RTS GPIO available.
> This is done by nullifying the ports rs485_supported struct. After that
> however the settings in serial_omap_rs485_supported are assigned to the
> same structure unconditionally, which results in an unintended reactivation
> of RS485 support.
> 
> Fix this by moving the assignment to the beginning of
> serial_omap_probe_rs485() and thus before uart_get_rs485_mode() gets
> called.

This doesn't seem to accurately reflect what the problem is (which you 
correctly described in the paragraph above this). The problem doesn't seem 
to have anything to do with the placement of uart_get_rs485_mode() call 
but the if (IS_ERR(up->rts_gpiod)) block that clears rs485_supported?

A future work item that came to my mind while reviewing this: I suppose 
uart_disable_rs485_support() could be added into core which memsets  
rs485_supported and rs485 to zero so this driver could just call it.

> Also replace the assignment of rs485_config() to have the complete RS485
> setup in one function.
> 
> Fixes: e2752ae3cfc9 ("serial: omap: Disallow RS-485 if rts-gpio is not specified")
> Cc: stable@...r.kernel.org
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@...bus.com>
> ---
>  drivers/tty/serial/omap-serial.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index ad4c1c5d0a7f..f4c6ff806465 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -1483,6 +1483,13 @@ static struct omap_uart_port_info *of_get_uart_port_info(struct device *dev)
>  	return omap_up_info;
>  }
>  
> +static const struct serial_rs485 serial_omap_rs485_supported = {
> +	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | SER_RS485_RTS_AFTER_SEND |
> +		 SER_RS485_RX_DURING_TX,
> +	.delay_rts_before_send = 1,
> +	.delay_rts_after_send = 1,
> +};
> +
>  static int serial_omap_probe_rs485(struct uart_omap_port *up,
>  				   struct device *dev)
>  {
> @@ -1497,6 +1504,9 @@ static int serial_omap_probe_rs485(struct uart_omap_port *up,
>  	if (!np)
>  		return 0;
>  
> +	up->port.rs485_config = serial_omap_config_rs485;
> +	up->port.rs485_supported = serial_omap_rs485_supported;
> +
>  	ret = uart_get_rs485_mode(&up->port);
>  	if (ret)
>  		return ret;
> @@ -1531,13 +1541,6 @@ static int serial_omap_probe_rs485(struct uart_omap_port *up,
>  	return 0;
>  }
>  
> -static const struct serial_rs485 serial_omap_rs485_supported = {
> -	.flags = SER_RS485_ENABLED | SER_RS485_RTS_ON_SEND | SER_RS485_RTS_AFTER_SEND |
> -		 SER_RS485_RX_DURING_TX,
> -	.delay_rts_before_send = 1,
> -	.delay_rts_after_send = 1,
> -};
> -
>  static int serial_omap_probe(struct platform_device *pdev)
>  {
>  	struct omap_uart_port_info *omap_up_info = dev_get_platdata(&pdev->dev);
> @@ -1604,17 +1607,11 @@ static int serial_omap_probe(struct platform_device *pdev)
>  		dev_info(up->port.dev, "no wakeirq for uart%d\n",
>  			 up->port.line);
>  
> -	ret = serial_omap_probe_rs485(up, &pdev->dev);
> -	if (ret < 0)
> -		goto err_rs485;
> -
>  	sprintf(up->name, "OMAP UART%d", up->port.line);
>  	up->port.mapbase = mem->start;
>  	up->port.membase = base;
>  	up->port.flags = omap_up_info->flags;
>  	up->port.uartclk = omap_up_info->uartclk;
> -	up->port.rs485_config = serial_omap_config_rs485;
> -	up->port.rs485_supported = serial_omap_rs485_supported;
>  	if (!up->port.uartclk) {
>  		up->port.uartclk = DEFAULT_CLK_SPEED;
>  		dev_warn(&pdev->dev,
> @@ -1622,6 +1619,10 @@ static int serial_omap_probe(struct platform_device *pdev)
>  			 DEFAULT_CLK_SPEED);
>  	}
>  
> +	ret = serial_omap_probe_rs485(up, &pdev->dev);
> +	if (ret < 0)
> +		goto err_rs485;
> +
>  	up->latency = PM_QOS_CPU_LATENCY_DEFAULT_VALUE;
>  	up->calc_latency = PM_QOS_CPU_LATENCY_DEFAULT_VALUE;
>  	cpu_latency_qos_add_request(&up->pm_qos_request, up->latency);

Moving of serial_omap_probe_rs485() later in serial_omap_probe() doesn't 
seem anymore required but I guess it doesn't hurt either.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>

-- 
 i.