diff mbox series

[v3,3/6] serial: core: fix sanitizing check for RTS settings

Message ID 20231011181544.7893-4-l.sanfilippo@kunbus.com
State New
Headers
Series Fixes and improvements for RS485 |

Commit Message

Lino Sanfilippo Oct. 11, 2023, 6:15 p.m. UTC 
  Among other things uart_sanitize_serial_rs485() tests the sanity of the RTS
settings in a RS485 configuration that has been passed by userspace.
If RTS-on-send and RTS-after-send are both set or unset the configuration
is adjusted and RTS-after-send is disabled and RTS-on-send enabled.

This however makes only sense if both RTS modes are actually supported by
the driver.

With commit be2e2cb1d281 ("serial: Sanitize rs485_struct") the code does
take the driver support into account but only checks if one of both RTS
modes are supported. This may lead to the errorneous result of RTS-on-send
being set even if only RTS-after-send is supported.

Fix this by changing the implemented logic: First clear all unsupported
flags in the RS485 configuration, then adjust an invalid RTS setting by
taking into account which RTS mode is supported.

Cc: stable@vger.kernel.org
Fixes: be2e2cb1d281 ("serial: Sanitize rs485_struct")
Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
---
 drivers/tty/serial/serial_core.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

Comments

Ilpo Järvinen Oct. 12, 2023, 1:10 p.m. UTC | #1 
On Wed, 11 Oct 2023, Lino Sanfilippo wrote:

> Among other things uart_sanitize_serial_rs485() tests the sanity of the RTS
> settings in a RS485 configuration that has been passed by userspace.
> If RTS-on-send and RTS-after-send are both set or unset the configuration
> is adjusted and RTS-after-send is disabled and RTS-on-send enabled.
> 
> This however makes only sense if both RTS modes are actually supported by
> the driver.
> 
> With commit be2e2cb1d281 ("serial: Sanitize rs485_struct") the code does
> take the driver support into account but only checks if one of both RTS
> modes are supported. This may lead to the errorneous result of RTS-on-send
> being set even if only RTS-after-send is supported.
> 
> Fix this by changing the implemented logic: First clear all unsupported
> flags in the RS485 configuration, then adjust an invalid RTS setting by
> taking into account which RTS mode is supported.
> 
> Cc: stable@vger.kernel.org
> Fixes: be2e2cb1d281 ("serial: Sanitize rs485_struct")
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> ---
>  drivers/tty/serial/serial_core.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 697c36dc7ec8..f4feebf8200f 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1370,19 +1370,27 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4
>  		return;
>  	}
>  
> +	rs485->flags &= supported_flags;
> +
>  	/* Pick sane settings if the user hasn't */
> -	if ((supported_flags & (SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND)) &&
> -	    !(rs485->flags & SER_RS485_RTS_ON_SEND) ==
> +	if (!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
>  	    !(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
> -		dev_warn_ratelimited(port->dev,
> -			"%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
> -			port->name, port->line);
> -		rs485->flags |= SER_RS485_RTS_ON_SEND;
> -		rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
> -		supported_flags |= SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND;
> -	}
> +		if (supported_flags & SER_RS485_RTS_ON_SEND) {
> +			rs485->flags |= SER_RS485_RTS_ON_SEND;
> +			rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
>  
> -	rs485->flags &= supported_flags;
> +			dev_warn_ratelimited(port->dev,
> +				"%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
> +				port->name, port->line);
> +		} else {
> +			rs485->flags |= SER_RS485_RTS_AFTER_SEND;
> +			rs485->flags &= ~SER_RS485_RTS_ON_SEND;

So if neither of the flags is supported, what will happen? You might want 
add if after that else?
Lino Sanfilippo Oct. 12, 2023, 9:01 p.m. UTC | #2 
Hi,

On 12.10.23 15:10, Ilpo Järvinen wrote:

> 
> 
> On Wed, 11 Oct 2023, Lino Sanfilippo wrote:
> 
>> Among other things uart_sanitize_serial_rs485() tests the sanity of the RTS
>> settings in a RS485 configuration that has been passed by userspace.
>> If RTS-on-send and RTS-after-send are both set or unset the configuration
>> is adjusted and RTS-after-send is disabled and RTS-on-send enabled.
>>
>> This however makes only sense if both RTS modes are actually supported by
>> the driver.
>>
>> With commit be2e2cb1d281 ("serial: Sanitize rs485_struct") the code does
>> take the driver support into account but only checks if one of both RTS
>> modes are supported. This may lead to the errorneous result of RTS-on-send
>> being set even if only RTS-after-send is supported.
>>
>> Fix this by changing the implemented logic: First clear all unsupported
>> flags in the RS485 configuration, then adjust an invalid RTS setting by
>> taking into account which RTS mode is supported.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: be2e2cb1d281 ("serial: Sanitize rs485_struct")
>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>> ---
>>  drivers/tty/serial/serial_core.c | 28 ++++++++++++++++++----------
>>  1 file changed, 18 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index 697c36dc7ec8..f4feebf8200f 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -1370,19 +1370,27 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4
>>               return;
>>       }
>>
>> +     rs485->flags &= supported_flags;
>> +
>>       /* Pick sane settings if the user hasn't */
>> -     if ((supported_flags & (SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND)) &&
>> -         !(rs485->flags & SER_RS485_RTS_ON_SEND) ==
>> +     if (!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
>>           !(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
>> -             dev_warn_ratelimited(port->dev,
>> -                     "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
>> -                     port->name, port->line);
>> -             rs485->flags |= SER_RS485_RTS_ON_SEND;
>> -             rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
>> -             supported_flags |= SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND;
>> -     }
>> +             if (supported_flags & SER_RS485_RTS_ON_SEND) {
>> +                     rs485->flags |= SER_RS485_RTS_ON_SEND;
>> +                     rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
>>
>> -     rs485->flags &= supported_flags;
>> +                     dev_warn_ratelimited(port->dev,
>> +                             "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
>> +                             port->name, port->line);
>> +             } else {
>> +                     rs485->flags |= SER_RS485_RTS_AFTER_SEND;
>> +                     rs485->flags &= ~SER_RS485_RTS_ON_SEND;
> 
> So if neither of the flags is supported, what will happen? You might want
> add if after that else?
>

I would consider this a bug in the driver, as at least one of both modes 
has to be supported. If the driver does not have at least one of both flags
set in rs485_supported.flags we could print a warning though. Would you prefer that?

Regards,
Lino
Ilpo Järvinen Oct. 13, 2023, 10:24 a.m. UTC | #3 
On Thu, 12 Oct 2023, Lino Sanfilippo wrote:
> On 12.10.23 15:10, Ilpo Järvinen wrote:
> > On Wed, 11 Oct 2023, Lino Sanfilippo wrote:
> > 
> >> Among other things uart_sanitize_serial_rs485() tests the sanity of the RTS
> >> settings in a RS485 configuration that has been passed by userspace.
> >> If RTS-on-send and RTS-after-send are both set or unset the configuration
> >> is adjusted and RTS-after-send is disabled and RTS-on-send enabled.
> >>
> >> This however makes only sense if both RTS modes are actually supported by
> >> the driver.
> >>
> >> With commit be2e2cb1d281 ("serial: Sanitize rs485_struct") the code does
> >> take the driver support into account but only checks if one of both RTS
> >> modes are supported. This may lead to the errorneous result of RTS-on-send
> >> being set even if only RTS-after-send is supported.
> >>
> >> Fix this by changing the implemented logic: First clear all unsupported
> >> flags in the RS485 configuration, then adjust an invalid RTS setting by
> >> taking into account which RTS mode is supported.
> >>
> >> Cc: stable@vger.kernel.org
> >> Fixes: be2e2cb1d281 ("serial: Sanitize rs485_struct")
> >> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> >> ---
> >>  drivers/tty/serial/serial_core.c | 28 ++++++++++++++++++----------
> >>  1 file changed, 18 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> >> index 697c36dc7ec8..f4feebf8200f 100644
> >> --- a/drivers/tty/serial/serial_core.c
> >> +++ b/drivers/tty/serial/serial_core.c
> >> @@ -1370,19 +1370,27 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4
> >>               return;
> >>       }
> >>
> >> +     rs485->flags &= supported_flags;
> >> +
> >>       /* Pick sane settings if the user hasn't */
> >> -     if ((supported_flags & (SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND)) &&
> >> -         !(rs485->flags & SER_RS485_RTS_ON_SEND) ==
> >> +     if (!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
> >>           !(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
> >> -             dev_warn_ratelimited(port->dev,
> >> -                     "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
> >> -                     port->name, port->line);
> >> -             rs485->flags |= SER_RS485_RTS_ON_SEND;
> >> -             rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
> >> -             supported_flags |= SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND;
> >> -     }
> >> +             if (supported_flags & SER_RS485_RTS_ON_SEND) {
> >> +                     rs485->flags |= SER_RS485_RTS_ON_SEND;
> >> +                     rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
> >>
> >> -     rs485->flags &= supported_flags;
> >> +                     dev_warn_ratelimited(port->dev,
> >> +                             "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
> >> +                             port->name, port->line);
> >> +             } else {
> >> +                     rs485->flags |= SER_RS485_RTS_AFTER_SEND;
> >> +                     rs485->flags &= ~SER_RS485_RTS_ON_SEND;
> > 
> > So if neither of the flags is supported, what will happen? You might want
> > add if after that else?
> >
> 
> I would consider this a bug in the driver, as at least one of both modes 
> has to be supported. If the driver does not have at least one of both flags
> set in rs485_supported.flags we could print a warning though. Would you prefer that?

8250_exar.c needs to fixed then? I was taking these as things one can 
"configure" even if when there's support only for a one of them there's 
not that much to configure. As there was neither in 8250_exar's code, I 
didn't add either flag.

But I suppose your interpretation of those flag makes more sense.
Lino Sanfilippo Oct. 14, 2023, 12:23 p.m. UTC | #4 
Hi Ilpo,

On 13.10.23 12:24, Ilpo Järvinen wrote:
> On Thu, 12 Oct 2023, Lino Sanfilippo wrote:
>> On 12.10.23 15:10, Ilpo Järvinen wrote:
>>> On Wed, 11 Oct 2023, Lino Sanfilippo wrote:
>>>
>>>> Among other things uart_sanitize_serial_rs485() tests the sanity of the RTS
>>>> settings in a RS485 configuration that has been passed by userspace.
>>>> If RTS-on-send and RTS-after-send are both set or unset the configuration
>>>> is adjusted and RTS-after-send is disabled and RTS-on-send enabled.
>>>>
>>>> This however makes only sense if both RTS modes are actually supported by
>>>> the driver.
>>>>
>>>> With commit be2e2cb1d281 ("serial: Sanitize rs485_struct") the code does
>>>> take the driver support into account but only checks if one of both RTS
>>>> modes are supported. This may lead to the errorneous result of RTS-on-send
>>>> being set even if only RTS-after-send is supported.
>>>>
>>>> Fix this by changing the implemented logic: First clear all unsupported
>>>> flags in the RS485 configuration, then adjust an invalid RTS setting by
>>>> taking into account which RTS mode is supported.
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> Fixes: be2e2cb1d281 ("serial: Sanitize rs485_struct")
>>>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>>> ---
>>>>  drivers/tty/serial/serial_core.c | 28 ++++++++++++++++++----------
>>>>  1 file changed, 18 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>>>> index 697c36dc7ec8..f4feebf8200f 100644
>>>> --- a/drivers/tty/serial/serial_core.c
>>>> +++ b/drivers/tty/serial/serial_core.c
>>>> @@ -1370,19 +1370,27 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4
>>>>               return;
>>>>       }
>>>>
>>>> +     rs485->flags &= supported_flags;
>>>> +
>>>>       /* Pick sane settings if the user hasn't */
>>>> -     if ((supported_flags & (SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND)) &&
>>>> -         !(rs485->flags & SER_RS485_RTS_ON_SEND) ==
>>>> +     if (!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
>>>>           !(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
>>>> -             dev_warn_ratelimited(port->dev,
>>>> -                     "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
>>>> -                     port->name, port->line);
>>>> -             rs485->flags |= SER_RS485_RTS_ON_SEND;
>>>> -             rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
>>>> -             supported_flags |= SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND;
>>>> -     }
>>>> +             if (supported_flags & SER_RS485_RTS_ON_SEND) {
>>>> +                     rs485->flags |= SER_RS485_RTS_ON_SEND;
>>>> +                     rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
>>>>
>>>> -     rs485->flags &= supported_flags;
>>>> +                     dev_warn_ratelimited(port->dev,
>>>> +                             "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
>>>> +                             port->name, port->line);
>>>> +             } else {
>>>> +                     rs485->flags |= SER_RS485_RTS_AFTER_SEND;
>>>> +                     rs485->flags &= ~SER_RS485_RTS_ON_SEND;
>>>
>>> So if neither of the flags is supported, what will happen? You might want
>>> add if after that else?
>>>
>>
>> I would consider this a bug in the driver, as at least one of both modes
>> has to be supported. If the driver does not have at least one of both flags
>> set in rs485_supported.flags we could print a warning though. Would you prefer that?
>
> 8250_exar.c needs to fixed then?
I was taking these as things one can
> "configure" even if when there's support only for a one of them there's
> not that much to configure. As there was neither in 8250_exar's code, I
> didn't add either flag.

> But I suppose your interpretation of those flag makes more sense.
>

IMHO this is consistent with what we have in uart_get_rs485_mode(). This function
ensures that we have at least one RTS mode set (with default to RTS_ON_SEND). So
concerning 8250_exar.c, I think it should be fixed (havent noticed the missing
RTS mode though until you mentioned it). Would you like to provide a fix for this
or shall I include one into the next version of this series?


BR,
Lino
Ilpo Järvinen Oct. 16, 2023, 10:05 a.m. UTC | #5 
On Sat, 14 Oct 2023, Lino Sanfilippo wrote:
> On 13.10.23 12:24, Ilpo Järvinen wrote:
> > On Thu, 12 Oct 2023, Lino Sanfilippo wrote:
> >> On 12.10.23 15:10, Ilpo Järvinen wrote:
> >>> On Wed, 11 Oct 2023, Lino Sanfilippo wrote:
> >>>
> >>>> Among other things uart_sanitize_serial_rs485() tests the sanity of the RTS
> >>>> settings in a RS485 configuration that has been passed by userspace.
> >>>> If RTS-on-send and RTS-after-send are both set or unset the configuration
> >>>> is adjusted and RTS-after-send is disabled and RTS-on-send enabled.
> >>>>
> >>>> This however makes only sense if both RTS modes are actually supported by
> >>>> the driver.
> >>>>
> >>>> With commit be2e2cb1d281 ("serial: Sanitize rs485_struct") the code does
> >>>> take the driver support into account but only checks if one of both RTS
> >>>> modes are supported. This may lead to the errorneous result of RTS-on-send
> >>>> being set even if only RTS-after-send is supported.
> >>>>
> >>>> Fix this by changing the implemented logic: First clear all unsupported
> >>>> flags in the RS485 configuration, then adjust an invalid RTS setting by
> >>>> taking into account which RTS mode is supported.
> >>>>
> >>>> Cc: stable@vger.kernel.org
> >>>> Fixes: be2e2cb1d281 ("serial: Sanitize rs485_struct")
> >>>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> >>>> ---
> >>>>  drivers/tty/serial/serial_core.c | 28 ++++++++++++++++++----------
> >>>>  1 file changed, 18 insertions(+), 10 deletions(-)
> >>>>
> >>>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> >>>> index 697c36dc7ec8..f4feebf8200f 100644
> >>>> --- a/drivers/tty/serial/serial_core.c
> >>>> +++ b/drivers/tty/serial/serial_core.c
> >>>> @@ -1370,19 +1370,27 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4
> >>>>               return;
> >>>>       }
> >>>>
> >>>> +     rs485->flags &= supported_flags;
> >>>> +
> >>>>       /* Pick sane settings if the user hasn't */
> >>>> -     if ((supported_flags & (SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND)) &&
> >>>> -         !(rs485->flags & SER_RS485_RTS_ON_SEND) ==
> >>>> +     if (!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
> >>>>           !(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
> >>>> -             dev_warn_ratelimited(port->dev,
> >>>> -                     "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
> >>>> -                     port->name, port->line);
> >>>> -             rs485->flags |= SER_RS485_RTS_ON_SEND;
> >>>> -             rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
> >>>> -             supported_flags |= SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND;
> >>>> -     }
> >>>> +             if (supported_flags & SER_RS485_RTS_ON_SEND) {
> >>>> +                     rs485->flags |= SER_RS485_RTS_ON_SEND;
> >>>> +                     rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
> >>>>
> >>>> -     rs485->flags &= supported_flags;
> >>>> +                     dev_warn_ratelimited(port->dev,
> >>>> +                             "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
> >>>> +                             port->name, port->line);
> >>>> +             } else {
> >>>> +                     rs485->flags |= SER_RS485_RTS_AFTER_SEND;
> >>>> +                     rs485->flags &= ~SER_RS485_RTS_ON_SEND;
> >>>
> >>> So if neither of the flags is supported, what will happen? You might want
> >>> add if after that else?
> >>>
> >>
> >> I would consider this a bug in the driver, as at least one of both modes
> >> has to be supported. If the driver does not have at least one of both flags
> >> set in rs485_supported.flags we could print a warning though. Would you prefer that?
> >
> > 8250_exar.c needs to fixed then?
> I was taking these as things one can
> > "configure" even if when there's support only for a one of them there's
> > not that much to configure. As there was neither in 8250_exar's code, I
> > didn't add either flag.
> 
> > But I suppose your interpretation of those flag makes more sense.
> 
> IMHO this is consistent with what we have in uart_get_rs485_mode(). This function
> ensures that we have at least one RTS mode set (with default to RTS_ON_SEND). So
> concerning 8250_exar.c, I think it should be fixed (havent noticed the missing
> RTS mode though until you mentioned it). Would you like to provide a fix for this
> or shall I include one into the next version of this series?

Just create that fix yourself thank you and include it into your series, 
I'm busy with other stuff currently.
Lino Sanfilippo Oct. 16, 2023, 10:10 a.m. UTC | #6 
On 16.10.23 12:05, Ilpo Järvinen wrote:
> On Sat, 14 Oct 2023, Lino Sanfilippo wrote:
>> On 13.10.23 12:24, Ilpo Järvinen wrote:
>>> On Thu, 12 Oct 2023, Lino Sanfilippo wrote:
>>>> On 12.10.23 15:10, Ilpo Järvinen wrote:
>>>>> On Wed, 11 Oct 2023, Lino Sanfilippo wrote:
>>>>>
>>>>>> Among other things uart_sanitize_serial_rs485() tests the sanity of the RTS
>>>>>> settings in a RS485 configuration that has been passed by userspace.
>>>>>> If RTS-on-send and RTS-after-send are both set or unset the configuration
>>>>>> is adjusted and RTS-after-send is disabled and RTS-on-send enabled.
>>>>>>
>>>>>> This however makes only sense if both RTS modes are actually supported by
>>>>>> the driver.
>>>>>>
>>>>>> With commit be2e2cb1d281 ("serial: Sanitize rs485_struct") the code does
>>>>>> take the driver support into account but only checks if one of both RTS
>>>>>> modes are supported. This may lead to the errorneous result of RTS-on-send
>>>>>> being set even if only RTS-after-send is supported.
>>>>>>
>>>>>> Fix this by changing the implemented logic: First clear all unsupported
>>>>>> flags in the RS485 configuration, then adjust an invalid RTS setting by
>>>>>> taking into account which RTS mode is supported.
>>>>>>
>>>>>> Cc: stable@vger.kernel.org
>>>>>> Fixes: be2e2cb1d281 ("serial: Sanitize rs485_struct")
>>>>>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>>>>> ---
>>>>>>  drivers/tty/serial/serial_core.c | 28 ++++++++++++++++++----------
>>>>>>  1 file changed, 18 insertions(+), 10 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>>>>>> index 697c36dc7ec8..f4feebf8200f 100644
>>>>>> --- a/drivers/tty/serial/serial_core.c
>>>>>> +++ b/drivers/tty/serial/serial_core.c
>>>>>> @@ -1370,19 +1370,27 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4
>>>>>>               return;
>>>>>>       }
>>>>>>
>>>>>> +     rs485->flags &= supported_flags;
>>>>>> +
>>>>>>       /* Pick sane settings if the user hasn't */
>>>>>> -     if ((supported_flags & (SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND)) &&
>>>>>> -         !(rs485->flags & SER_RS485_RTS_ON_SEND) ==
>>>>>> +     if (!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
>>>>>>           !(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
>>>>>> -             dev_warn_ratelimited(port->dev,
>>>>>> -                     "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
>>>>>> -                     port->name, port->line);
>>>>>> -             rs485->flags |= SER_RS485_RTS_ON_SEND;
>>>>>> -             rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
>>>>>> -             supported_flags |= SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND;
>>>>>> -     }
>>>>>> +             if (supported_flags & SER_RS485_RTS_ON_SEND) {
>>>>>> +                     rs485->flags |= SER_RS485_RTS_ON_SEND;
>>>>>> +                     rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
>>>>>>
>>>>>> -     rs485->flags &= supported_flags;
>>>>>> +                     dev_warn_ratelimited(port->dev,
>>>>>> +                             "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
>>>>>> +                             port->name, port->line);
>>>>>> +             } else {
>>>>>> +                     rs485->flags |= SER_RS485_RTS_AFTER_SEND;
>>>>>> +                     rs485->flags &= ~SER_RS485_RTS_ON_SEND;
>>>>>
>>>>> So if neither of the flags is supported, what will happen? You might want
>>>>> add if after that else?
>>>>>
>>>>
>>>> I would consider this a bug in the driver, as at least one of both modes
>>>> has to be supported. If the driver does not have at least one of both flags
>>>> set in rs485_supported.flags we could print a warning though. Would you prefer that?
>>>
>>> 8250_exar.c needs to fixed then?
>> I was taking these as things one can
>>> "configure" even if when there's support only for a one of them there's
>>> not that much to configure. As there was neither in 8250_exar's code, I
>>> didn't add either flag.
>>
>>> But I suppose your interpretation of those flag makes more sense.
>>
>> IMHO this is consistent with what we have in uart_get_rs485_mode(). This function
>> ensures that we have at least one RTS mode set (with default to RTS_ON_SEND). So
>> concerning 8250_exar.c, I think it should be fixed (havent noticed the missing
>> RTS mode though until you mentioned it). Would you like to provide a fix for this
>> or shall I include one into the next version of this series?
>
> Just create that fix yourself thank you and include it into your series,
> I'm busy with other stuff currently.
>
>

Sure, will do.

BR,
Lino
diff mbox series

Patch

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 697c36dc7ec8..f4feebf8200f 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1370,19 +1370,27 @@  static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4
 		return;
 	}
 
+	rs485->flags &= supported_flags;
+
 	/* Pick sane settings if the user hasn't */
-	if ((supported_flags & (SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND)) &&
-	    !(rs485->flags & SER_RS485_RTS_ON_SEND) ==
+	if (!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
 	    !(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
-		dev_warn_ratelimited(port->dev,
-			"%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
-			port->name, port->line);
-		rs485->flags |= SER_RS485_RTS_ON_SEND;
-		rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
-		supported_flags |= SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND;
-	}
+		if (supported_flags & SER_RS485_RTS_ON_SEND) {
+			rs485->flags |= SER_RS485_RTS_ON_SEND;
+			rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
 
-	rs485->flags &= supported_flags;
+			dev_warn_ratelimited(port->dev,
+				"%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
+				port->name, port->line);
+		} else {
+			rs485->flags |= SER_RS485_RTS_AFTER_SEND;
+			rs485->flags &= ~SER_RS485_RTS_ON_SEND;
+
+			dev_warn_ratelimited(port->dev,
+				"%s (%d): invalid RTS setting, using RTS_AFTER_SEND instead\n",
+				port->name, port->line);
+		}
+	}
 
 	uart_sanitize_serial_rs485_delays(port, rs485);