| 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
| ||
|
Date: Mon, 1 Feb 2021 19:15:48 -0500
From: Eric Tremblay <etremblay@...tech-controls.com>
To: Jiri Slaby <jirislaby@...nel.org>, gregkh@...uxfoundation.org
Cc: andriy.shevchenko@...ux.intel.com, matwey.kornilov@...il.com,
giulio.benetti@...ronovasrl.com, lukas@...ner.de,
linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
christoph.muellner@...obroma-systems.com, heiko@...ech.de,
heiko.stuebner@...obroma-systems.com
Subject: Re: [PATCH 1/3] serial: 8250: Handle UART without interrupt on TEMT
using em485
On 2021-01-29 2:23 a.m., Jiri Slaby wrote:
> On 29. 01. 21, 0:36, Eric Tremblay wrote:
>> The patch introduce the UART_CAP_TEMT capability which is by default
>> assigned to all 8250 UART since the code assume that device has the
>> interrupt on TEMT
>>
>> In the case where the device does not support it, we calculate the
>> maximum of time it could take for the transmitter to empty the
>> shift register. When we get in the situation where we get the
>> THRE interrupt but the TEMT bit is not set we start the timer
>> and recall __stop_tx after the delay
>>
>> Signed-off-by: Eric Tremblay <etremblay@...tech-controls.com>
>> ---
>> drivers/tty/serial/8250/8250.h | 1 +
>> drivers/tty/serial/8250/8250_bcm2835aux.c | 2 +-
>> drivers/tty/serial/8250/8250_omap.c | 2 +-
>> drivers/tty/serial/8250/8250_port.c | 89 ++++++++++++++++++++++-
>> include/linux/serial_8250.h | 2 +
>> 5 files changed, 93 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
>> index 52bb21205bb6..5361b761eed7 100644
>> --- a/drivers/tty/serial/8250/8250.h
>> +++ b/drivers/tty/serial/8250/8250.h
>> @@ -82,6 +82,7 @@ struct serial8250_config {
>> #define UART_CAP_MINI (1 << 17) /* Mini UART on BCM283X family lacks:
>> * STOP PARITY EPAR SPAR WLEN5 WLEN6
>> */
>> +#define UART_CAP_TEMT (1 << 18) /* UART have interrupt on TEMT */
>
> What about the inversion _NOTEMT? You then set it only on uarts without TEMT and don't need to update every single driver.
That's a good Idea, I will use that in the next version.
>
>> diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
>> index fd95860cd661..354faebce885 100644
>> --- a/drivers/tty/serial/8250/8250_bcm2835aux.c
>> +++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
>> @@ -91,7 +91,7 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev)
>> return -ENOMEM;
>> /* initialize data */
>> - up.capabilities = UART_CAP_FIFO | UART_CAP_MINI;
>> + data->uart.capabilities = UART_CAP_FIFO | UART_CAP_MINI | UART_CAP_TEMT;
>
> This change looks weird and undocumented. Why do you set data->uart suddenly?
>
> Actually, does this build?
This is a silly merge mistake, sorry about that. However, it will not be there
in the next version with the _NOTEMT capability.
>
>> up.port.dev = &pdev->dev;
>> up.port.regshift = 2;
>> up.port.type = PORT_16550;
>> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
>> index 23e0decde33e..1c21ac68ff37 100644
>> --- a/drivers/tty/serial/8250/8250_omap.c
>> +++ b/drivers/tty/serial/8250/8250_omap.c
>> @@ -1294,7 +1294,7 @@ static int omap8250_probe(struct platform_device *pdev)
>> up.port.regshift = 2;
>> up.port.fifosize = 64;
>> up.tx_loadsz = 64;
>> - up.capabilities = UART_CAP_FIFO;
>> + up.capabilities = UART_CAP_FIFO | UART_CAP_TEMT;
>> #ifdef CONFIG_PM
>> /*
>> * Runtime PM is mostly transparent. However to do it right we need to a
>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>> index b0af13074cd3..44a54406e4b4 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -558,8 +558,41 @@ static void serial8250_clear_fifos(struct uart_8250_port *p)
>> }
>> }
>> +static inline void serial8250_em485_update_temt_delay(struct uart_8250_port *p,
>> + unsigned int cflag, unsigned int baud)
>> +{
>> + unsigned int bits;
>> +
>> + if (!p->em485)
>> + return;
>> +
>> + /* byte size and parity */
>> + switch (cflag & CSIZE) {
>> + case CS5:
>> + bits = 7;
>> + break;
>> + case CS6:
>> + bits = 8;
>> + break;
>> + case CS7:
>> + bits = 9;
>> + break;
>> + default:
>> + bits = 10;
>> + break; /* CS8 */
>> + }
>> +
>> + if (cflag & CSTOPB)
>> + bits++;
>> + if (cflag & PARENB)
>> + bits++;
>> +
>> + p->em485->no_temt_delay = bits*1000000/baud;
>> +}
>> +
>> static enum hrtimer_restart serial8250_em485_handle_start_tx(struct hrtimer *t);
>> static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t);
>> +static enum hrtimer_restart serial8250_em485_handle_no_temt(struct hrtimer *t);
>> void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p)
>> {
>> @@ -618,6 +651,18 @@ static int serial8250_em485_init(struct uart_8250_port *p)
>> HRTIMER_MODE_REL);
>> hrtimer_init(&p->em485->start_tx_timer, CLOCK_MONOTONIC,
>> HRTIMER_MODE_REL);
>> +
>> + if (!(p->capabilities & UART_CAP_TEMT)) {
>> + struct tty_struct *tty = p->port.state->port.tty;
>
> Is this safe? Don't you need a tty reference? Or maybe you need to pass the tty from the TIOCSRS485 ioctl to here.
I saw that there was some place in the code where the tty_struct or the termios struct
was accessed that way, I may have done a wrong assumption. To get the TTY struct from the IOCTL,
we will need to modify the rs485_config() callback, I'm not sure I want to go there.
Is using tty_port_initialized() before and returning an error on failure would be a sufficient
safety check ?
>
>> + serial8250_em485_update_temt_delay(p, tty->termios.c_cflag,
>> + tty_get_baud_rate(tty));
>> + hrtimer_init(&p->em485->no_temt_timer, CLOCK_MONOTONIC,
>> + HRTIMER_MODE_REL);
>> + p->em485->no_temt_timer.function =
>> + &serial8250_em485_handle_no_temt;
>> + }
>> +
>> p->em485->stop_tx_timer.function = &serial8250_em485_handle_stop_tx;
>> p->em485->start_tx_timer.function = &serial8250_em485_handle_start_tx;
>> p->em485->port = p;
>> @@ -649,6 +694,7 @@ void serial8250_em485_destroy(struct uart_8250_port *p)
>> hrtimer_cancel(&p->em485->start_tx_timer);
>> hrtimer_cancel(&p->em485->stop_tx_timer);
>> + hrtimer_cancel(&p->em485->no_temt_timer);
>> kfree(p->em485);
>> p->em485 = NULL;
>> @@ -1494,6 +1540,15 @@ static void start_hrtimer_ms(struct hrtimer *hrt, unsigned long msec)
>> hrtimer_start(hrt, t, HRTIMER_MODE_REL);
>> }
>> +static void start_hrtimer_us(struct hrtimer *hrt, unsigned long usec)
>> +{
>> + long sec = usec / 1000000;
>> + long nsec = (usec % 1000000) * 1000;
>> + ktime_t t = ktime_set(sec, nsec);
>
> Why not ns_to_ktime without all those divisions?
I will change the timer to _ns and use that function, thanks.
>
>> +
>> + hrtimer_start(hrt, t, HRTIMER_MODE_REL);
>> +}
>> +
>> static void __stop_tx_rs485(struct uart_8250_port *p)
>> {
>> struct uart_8250_em485 *em485 = p->em485;
>> @@ -1531,8 +1586,18 @@ static inline void __stop_tx(struct uart_8250_port *p)
>> * shift register are empty. It is for device driver to enable
>> * interrupt on TEMT.
>> */
>> - if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
>> + if ((lsr & BOTH_EMPTY) != BOTH_EMPTY) {
>> + /*
>> + * On devices with no interrupt on TEMT available
>> + * start a timer for a byte time, the timer will recall
>> + * __stop_tx
>> + */
>> + if (!(p->capabilities & UART_CAP_TEMT) && (lsr & UART_LSR_THRE)) {
>> + em485->active_timer = &em485->no_temt_timer;
>
> How does this interfere with the current handling of active_timer? You should explain the changed functionality in the commit log. And you don't reset it to NULL in the timer.
I will add that information in the commit message thanks.
>
>> + start_hrtimer_us(&em485->no_temt_timer, em485->no_temt_delay);
>> + }
>> return;
>> + }
>> __stop_tx_rs485(p);
>> }
>> @@ -1631,6 +1696,25 @@ static inline void start_tx_rs485(struct uart_port *port)
>> __start_tx(port);
>> }
>> +static enum hrtimer_restart serial8250_em485_handle_no_temt(struct hrtimer *t)
>> +{
>> + struct uart_8250_em485 *em485;
>> + struct uart_8250_port *p;
>> + unsigned long flags;
>> +
>> + em485 = container_of(t, struct uart_8250_em485, no_temt_timer);
>> + p = em485->port;
>> +
>> + serial8250_rpm_get(p);
>> + spin_lock_irqsave(&p->port.lock, flags);
>> + if (em485->active_timer == &em485->no_temt_timer)
>> + __stop_tx(p);
>> +
>> + spin_unlock_irqrestore(&p->port.lock, flags);
>> + serial8250_rpm_put(p);
>> + return HRTIMER_NORESTART;
>> +}
>> +
>> static enum hrtimer_restart serial8250_em485_handle_start_tx(struct hrtimer *t)
>> {
>> struct uart_8250_em485 *em485;
>> @@ -2792,6 +2876,9 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
>> serial8250_set_divisor(port, baud, quot, frac);
>> + if (!(up->capabilities & UART_CAP_TEMT))
>> + serial8250_em485_update_temt_delay(up, termios->c_cflag, baud);
>> +
>> /*
>> * LCR DLAB must be set to enable 64-byte FIFO mode. If the FCR
>> * is written without DLAB set, this mode will be disabled.
>> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
>> index 9e655055112d..d2c66faff0dd 100644
>> --- a/include/linux/serial_8250.h
>> +++ b/include/linux/serial_8250.h
>> @@ -79,7 +79,9 @@ struct uart_8250_ops {
>> struct uart_8250_em485 {
>> struct hrtimer start_tx_timer; /* "rs485 start tx" timer */
>> struct hrtimer stop_tx_timer; /* "rs485 stop tx" timer */
>> + struct hrtimer no_temt_timer; /* "rs485 no tempt interrupt" timer */
>> struct hrtimer *active_timer; /* pointer to active timer */
>> + unsigned int no_temt_delay; /* Value of delay for no TEMT UART */
>> struct uart_8250_port *port; /* for hrtimer callbacks */
>> unsigned int tx_stopped:1; /* tx is currently stopped */
>> };
>>
>
>
Powered by blists - more mailing lists