[0/6] Fixes and improvements for RS485

  From: Lino Sanfilippo <l.sanfilippo@kunbus.com>

Patch 1: Do not hold the port lock when setting rx-during-tx GPIO
Patch 2: Get rid of useless wrapper pl011_get_rs485_mode()
Patch 3: set missing supported flag for RX during TX GPIO
Patch 4: fix sanitizing check for RTS settings
Patch 5: make sure RS485 is cannot be enabled when it is not supported
Patch 6: IMX: do not set RS485 enabled if it is not supported


Lino Sanfilippo (6):
  serial: Do not hold the port lock when setting rx-during-tx GPIO
  serial: amba-pl011: get rid of useless wrapper pl011_get_rs485_mode()
  serial: core: set missing supported flag for RX during TX GPIO
  serial: core: fix sanitizing check for RTS settings
  serial: core: make sure RS485 is cannot be enabled when it is not
    supported
  serial: imx: do not set RS485 enabled if it is not supported

 drivers/tty/serial/amba-pl011.c  | 14 +---------
 drivers/tty/serial/imx.c         | 18 +++++--------
 drivers/tty/serial/serial_core.c | 45 +++++++++++++++++++++-----------
 drivers/tty/serial/stm32-usart.c |  5 +---
 4 files changed, 38 insertions(+), 44 deletions(-)


base-commit: 9ed22ae6be817d7a3f5c15ca22cbc9d3963b481d
  

On Fri, Sep 29, 2023 at 12:12:41AM +0200, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> 
> Both the imx and stm32 driver set the rx-during-tx GPIO in the
> rs485_config() function by means of gpiod_set_value(). Since rs485_config()
> is called with the port lock held, this can be an problem in case that
> setting the GPIO line can sleep (e.g. if a GPIO expander is used which is
> connected via SPI or I2C).
> 
> Avoid this issue by setting the GPIO outside of the port lock in the serial
> core and by using gpiod_set_value_cansleep() instead of gpiod_set_value().
> 
> Since now both the term and the rx-during-tx GPIO are set within the serial
> core use a common function uart_set_rs485_gpios() to set both.
> 
> With moving it into the serial core setting the rx-during-tx GPIO is now
> automatically done for all drivers that support such a GPIO.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>

You cc: stable on many of these, but do not provide a "Fixes:" tag
showing just how far back they should go.  Can you do that so that we
have a hint as to what to do here?

thanks,

greg k-h
  

On Fri, Sep 29, 2023 at 12:12:46AM +0200, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> 
> If the imx driver cannot support RS485 it sets the UARTS rs485_supported
> structure to NULL. But it still calls uart_get_rs485_mode() which may set
> the RS485_ENABLED flag.
> The flag however is evaluated by the serial core in uart_configure_port()
> at port startup and thus may lead to an attempt to configure RS485 even if
> it is not supported.
> 
> Avoid this by calling uart_get_rs485_mode() only if RS485 is actually
> supported by the driver. Remove also a check for an error condition
> that is not possible any more now.
> 
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> ---
>  drivers/tty/serial/imx.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)

Why is this patch not marked for stable?

thanks,

greg k-h
  

On Fri, Sep 29, 2023 at 12:12:46AM +0200, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> 
> If the imx driver cannot support RS485 it sets the UARTS rs485_supported
> structure to NULL. But it still calls uart_get_rs485_mode() which may set
> the RS485_ENABLED flag.
> The flag however is evaluated by the serial core in uart_configure_port()

I wonder if this is the code location where this problem should be
addressed. Or alternatively don't let uart_get_rs485_mode() set
RS485_ENABLED (and other flags) if rs485_supported doesn't suggest that
this works?

> [...]
> 
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>

I don't know how picky Greg is here, but formally you missed to add an
S-o-b line for the sender of this patch (i.e. you with your gmx
address).

Best regards
Uwe
  

Hi,

On 29.09.23 07:50, Greg KH wrote:
> On Fri, Sep 29, 2023 at 12:12:41AM +0200, Lino Sanfilippo wrote:
>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>
>> Both the imx and stm32 driver set the rx-during-tx GPIO in the
>> rs485_config() function by means of gpiod_set_value(). Since rs485_config()
>> is called with the port lock held, this can be an problem in case that
>> setting the GPIO line can sleep (e.g. if a GPIO expander is used which is
>> connected via SPI or I2C).
>>
>> Avoid this issue by setting the GPIO outside of the port lock in the serial
>> core and by using gpiod_set_value_cansleep() instead of gpiod_set_value().
>>
>> Since now both the term and the rx-during-tx GPIO are set within the serial
>> core use a common function uart_set_rs485_gpios() to set both.
>>
>> With moving it into the serial core setting the rx-during-tx GPIO is now
>> automatically done for all drivers that support such a GPIO.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>
> You cc: stable on many of these, but do not provide a "Fixes:" tag
> showing just how far back they should go.  Can you do that so that we
> have a hint as to what to do here?
>

Yes, will do so in the next version.

BR,
Lino

> thanks,
>
> greg k-h
  

On 29.09.23 07:51, Greg KH wrote:
> On Fri, Sep 29, 2023 at 12:12:46AM +0200, Lino Sanfilippo wrote:
>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>
>> If the imx driver cannot support RS485 it sets the UARTS rs485_supported
>> structure to NULL. But it still calls uart_get_rs485_mode() which may set
>> the RS485_ENABLED flag.
>> The flag however is evaluated by the serial core in uart_configure_port()
>> at port startup and thus may lead to an attempt to configure RS485 even if
>> it is not supported.
>>
>> Avoid this by calling uart_get_rs485_mode() only if RS485 is actually
>> supported by the driver. Remove also a check for an error condition
>> that is not possible any more now.
>>
>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>> ---
>>  drivers/tty/serial/imx.c | 14 ++++++--------
>>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> Why is this patch not marked for stable?
>

Right, it should be, I will correct this, thanks!
  

Hi,

On 29.09.23 08:39, Uwe Kleine-König wrote:
> On Fri, Sep 29, 2023 at 12:12:46AM +0200, Lino Sanfilippo wrote:
>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>
>> If the imx driver cannot support RS485 it sets the UARTS rs485_supported
>> structure to NULL. But it still calls uart_get_rs485_mode() which may set
>> the RS485_ENABLED flag.
>> The flag however is evaluated by the serial core in uart_configure_port()
>
> I wonder if this is the code location where this problem should be
> addressed. Or alternatively don't let uart_get_rs485_mode() set
> RS485_ENABLED (and other flags) if rs485_supported doesn't suggest that
> this works?
>

This is indeed the better solution. Especially since the check is then in the serial
core and all RS485 supporting drivers benefit from it. I will change this for v2, thanks!


>> [...]
>>
>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>
> I don't know how picky Greg is here, but formally you missed to add an
> S-o-b line for the sender of this patch (i.e. you with your gmx
> address).
>

Hm, until now there have never been complaints about this. Is this really an issue? Of
course I can also S-o-b with my gmx address but adding two S-o-b's for the same person seems
a bit odd to me...

BR,
Lino

> Best regards
> Uwe
>
  

Hello Lino,

On Fri, Sep 29, 2023 at 03:46:52PM +0200, Lino Sanfilippo wrote:
> On 29.09.23 08:39, Uwe Kleine-König wrote:
> > On Fri, Sep 29, 2023 at 12:12:46AM +0200, Lino Sanfilippo wrote:
> >> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> >
> > I don't know how picky Greg is here, but formally you missed to add an
> > S-o-b line for the sender of this patch (i.e. you with your gmx
> > address).
> >
> 
> Hm, until now there have never been complaints about this. Is this really an issue? Of
> course I can also S-o-b with my gmx address but adding two S-o-b's for the same person seems
> a bit odd to me...

The obvious and easy fix is of course to use the author address to send
the patches. :-)

Best regards
Uwe