| 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
| ||
|
Message-ID: <89b44cd-6550-32b2-9bd-ae8dde6b48cd@linux.intel.com>
Date: Fri, 2 Sep 2022 17:21:14 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Jiri Slaby <jslaby@...e.cz>
cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-serial <linux-serial@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Tobias Klauser <tklauser@...tanz.ch>,
Richard Genoud <richard.genoud@...il.com>,
Nicolas Ferre <nicolas.ferre@...rochip.com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Claudiu Beznea <claudiu.beznea@...rochip.com>,
Vladimir Zapolskiy <vz@...ia.com>,
Liviu Dudau <liviu.dudau@....com>,
Sudeep Holla <sudeep.holla@....com>,
Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Shawn Guo <shawnguo@...nel.org>,
Sascha Hauer <s.hauer@...gutronix.de>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Fabio Estevam <festevam@...il.com>,
NXP Linux Team <linux-imx@....com>,
Andreas Färber <afaerber@...e.de>,
Manivannan Sadhasivam <mani@...nel.org>
Subject: Re: [PATCH v2 2/3] tty: serial: use DEFINE_UART_PORT_TX_HELPER()
On Thu, 1 Sep 2022, Jiri Slaby wrote:
> DEFINE_UART_PORT_TX_HELPER() is a new helper to send characters to the
> device. Use it in these drivers.
>
> Cc: Tobias Klauser <tklauser@...tanz.ch>
> Cc: Richard Genoud <richard.genoud@...il.com>
> Cc: Nicolas Ferre <nicolas.ferre@...rochip.com>
> Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>
> Cc: Claudiu Beznea <claudiu.beznea@...rochip.com>
> Cc: Vladimir Zapolskiy <vz@...ia.com>
> Cc: Liviu Dudau <liviu.dudau@....com>
> Cc: Sudeep Holla <sudeep.holla@....com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@....com>
> Cc: Shawn Guo <shawnguo@...nel.org>
> Cc: Sascha Hauer <s.hauer@...gutronix.de>
> Cc: Pengutronix Kernel Team <kernel@...gutronix.de>
> Cc: Fabio Estevam <festevam@...il.com>
> Cc: NXP Linux Team <linux-imx@....com>
> Cc: "Andreas Färber" <afaerber@...e.de>
> Cc: Manivannan Sadhasivam <mani@...nel.org>
> Signed-off-by: Jiri Slaby <jslaby@...e.cz>
> ---
> diff --git a/drivers/tty/serial/mpc52xx_uart.c b/drivers/tty/serial/mpc52xx_uart.c
> index 3f1986c89694..944686a0d00d 100644
> --- a/drivers/tty/serial/mpc52xx_uart.c
> +++ b/drivers/tty/serial/mpc52xx_uart.c
> @@ -1338,7 +1338,6 @@ mpc52xx_uart_verify_port(struct uart_port *port, struct serial_struct *ser)
> return 0;
> }
>
> -
> static const struct uart_ops mpc52xx_uart_ops = {
> .tx_empty = mpc52xx_uart_tx_empty,
> .set_mctrl = mpc52xx_uart_set_mctrl,
Stray change.
> diff --git a/drivers/tty/serial/sa1100.c b/drivers/tty/serial/sa1100.c
> index e64e42a19d1a..738f4b20cb8e 100644
> --- a/drivers/tty/serial/sa1100.c
> +++ b/drivers/tty/serial/sa1100.c
> @@ -226,45 +226,34 @@ sa1100_rx_chars(struct sa1100_port *sport)
> tty_flip_buffer_push(&sport->port.state->port);
> }
>
> -static void sa1100_tx_chars(struct sa1100_port *sport)
> +static bool sa1100_tx_ready(struct uart_port *port)
> {
> - struct circ_buf *xmit = &sport->port.state->xmit;
> + struct sa1100_port *sport =
> + container_of(port, struct sa1100_port, port);
>
> - if (sport->port.x_char) {
> - UART_PUT_CHAR(sport, sport->port.x_char);
> - sport->port.icount.tx++;
> - sport->port.x_char = 0;
> - return;
> - }
> + return UART_GET_UTSR1(sport) & UTSR1_TNF;
> +}
> +
> +static void sa1100_put_char(struct uart_port *port, unsigned char ch)
> +{
> + struct sa1100_port *sport =
> + container_of(port, struct sa1100_port, port);
> +
> + UART_PUT_CHAR(sport, ch);
> +}
Perhaps not to add into this change itself, but I just point out these
would be useful in addition to what is changed:
- Get rid of UART_PUT_CHAR()
- sa1100_console_putchar() could use sa1100_tx_ready()
> diff --git a/drivers/tty/serial/vt8500_serial.c b/drivers/tty/serial/vt8500_serial.c
> index 6f08136ce78a..ff430a1cc3a2 100644
> --- a/drivers/tty/serial/vt8500_serial.c
> +++ b/drivers/tty/serial/vt8500_serial.c
> @@ -187,37 +187,17 @@ static void handle_rx(struct uart_port *port)
> tty_flip_buffer_push(tport);
> }
>
> -static void handle_tx(struct uart_port *port)
> +static unsigned int vt8500_tx_empty(struct uart_port *port)
> {
> - struct circ_buf *xmit = &port->state->xmit;
> -
> - if (port->x_char) {
> - writeb(port->x_char, port->membase + VT8500_TXFIFO);
> - port->icount.tx++;
> - port->x_char = 0;
> - }
> - if (uart_circ_empty(xmit) || uart_tx_stopped(port)) {
> - vt8500_stop_tx(port);
> - return;
> - }
> + unsigned int idx = vt8500_read(port, VT8500_URFIDX) & 0x1f;
>
> - while ((vt8500_read(port, VT8500_URFIDX) & 0x1f) < 16) {
> - if (uart_circ_empty(xmit))
> - break;
> -
> - writeb(xmit->buf[xmit->tail], port->membase + VT8500_TXFIFO);
> -
> - xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> - port->icount.tx++;
> - }
> -
> - if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> - uart_write_wakeup(port);
> -
> - if (uart_circ_empty(xmit))
> - vt8500_stop_tx(port);
> + return idx < 16 ? TIOCSER_TEMT : 0;
> }
So there's just plain move and re-layouting of existing vt8500_tx_empty()
embedded here. Why not do it in a separate change beforehand?
I discovered this the hard way, that is, I started to look why tx on
earth "FIFO index" < 16 should be called "tx empty", if it would be a
separate change it would have been immediately obvious that it was a
pre-existing thing.
> +static DEFINE_UART_PORT_TX_HELPER(handle_tx, port, ch,
> + vt8500_tx_empty(port),
> + writeb(ch, port->membase + VT8500_TXFIFO));
> +
> static void vt8500_start_tx(struct uart_port *port)
> {
> struct vt8500_port *vt8500_port = container_of(port,
> @@ -260,12 +240,6 @@ static irqreturn_t vt8500_irq(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> -static unsigned int vt8500_tx_empty(struct uart_port *port)
> -{
> - return (vt8500_read(port, VT8500_URFIDX) & 0x1f) < 16 ?
> - TIOCSER_TEMT : 0;
> -}
> -
> static unsigned int vt8500_get_mctrl(struct uart_port *port)
> {
> unsigned int usr;
>
--
i.
Powered by blists - more mailing lists