| 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: Fri, 29 Jan 2021 08:23:31 +0100
From: Jiri Slaby <jirislaby@...nel.org>
To: Eric Tremblay <etremblay@...tech-controls.com>,
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 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.
> 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?
> 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.
> + 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?
> +
> + 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.
> + 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 */
> };
>
--
js
suse labs
Powered by blists - more mailing lists