linux-kernel - Re: [PATCH] serial: amba-pl011: add 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: <98c29d64-3155-4769-8336-413325de4057@gmx.de>
Date:   Thu, 17 Jun 2021 18:12:00 +0200
From:   Lino Sanfilippo <LinoSanfilippo@....de>
To:     Jiri Slaby <jirislaby@...nel.org>, gregkh@...uxfoundation.org
Cc:     linux@...linux.org.uk, linux-serial@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] serial: amba-pl011: add RS485 support



Hi,

On 16.06.21 at 08:18, Jiri Slaby wrote:

>> +    if (port->rs485.delay_rts_before_send)
>> +        mdelay(port->rs485.delay_rts_before_send);
>
> This is up to 1 second delay with interrupts disabled. Definitely not nice. 8250 clamps this to 100 ms at least, why did you choose 1000 ms?
>

I got this value from rs485.yaml where the max number for both rts delay before and after send
is specified as 1000ms. I thought to keep it up to the user to choose a sane setting. But yes,
its a very long time with interrupts disabled. I will send an updated version of this patch that
limits both values to 100ms.


os(struct uart_port *port, struct ktermios *termios,
>>       unsigned int lcr_h, old_cr;
>>       unsigned long flags;
>>       unsigned int baud, quot, clkdiv;
>> +    unsigned int bits;
>>         if (uap->vendor->oversampling)
>>           clkdiv = 8;
>> @@ -1968,25 +2049,32 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
>>       switch (termios->c_cflag & CSIZE) {
>>       case CS5:
>>           lcr_h = UART01x_LCRH_WLEN_5;
>> +        bits = 7;
>>           break;
>>       case CS6:
>>           lcr_h = UART01x_LCRH_WLEN_6;
>> +        bits = 8;
>>           break;
>>       case CS7:
>>           lcr_h = UART01x_LCRH_WLEN_7;
>> +        bits = 9;
>>           break;
>>       default: // CS8
>>           lcr_h = UART01x_LCRH_WLEN_8;
>> +        bits = 10;
>>           break;
>>       }
>> -    if (termios->c_cflag & CSTOPB)
>> +    if (termios->c_cflag & CSTOPB) {
>>           lcr_h |= UART01x_LCRH_STP2;
>> +        bits++;
>> +    }
>>       if (termios->c_cflag & PARENB) {
>>           lcr_h |= UART01x_LCRH_PEN;
>>           if (!(termios->c_cflag & PARODD))
>>               lcr_h |= UART01x_LCRH_EPS;
>>           if (termios->c_cflag & CMSPAR)
>>               lcr_h |= UART011_LCRH_SPS;
>> +        bits++;
>>       }
>
> You can do simply:
>   bits = tty_get_frame_size(termios->c_cflag);
> now:
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?h=tty-testing&id=3ec2ff37230e1c961d4b0d0118dd23c46b5bcdbb

Ah thats a nice function, thanks for pointing at it. I will adjust the code accordingly.


>
> thanks,

Thanks for the review!

Regards,
Lino