1. Patchew
  2. linux
  3. [PATCH next v2 0/4] convert 8250 to nbcon
  4. View patch

[PATCH next v2 2/4] serial: 8250: Split out IER from rs485_stop_tx()

John Ogness posted 4 patches 2 months, 2 weeks ago
There is a newer version of this series
[PATCH next v2 2/4] serial: 8250: Split out IER from rs485_stop_tx()
Posted by John Ogness 2 months, 2 weeks ago 
Move IER handling out of rs485_stop_tx() callback and into a new
wrapper serial8250_rs485_stop_tx(). Replace all callback call sites
with wrapper, except for the console write() callback, where it is
inappropriate to modify IER.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/8250/8250.h      |  1 +
 drivers/tty/serial/8250/8250_omap.c |  2 +-
 drivers/tty/serial/8250/8250_port.c | 30 ++++++++++++++++++++---------
 3 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 6e90223ba1d6..b1e4b5fef8cc 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -237,6 +237,7 @@ void serial8250_em485_destroy(struct uart_8250_port *p);
 extern struct serial_rs485 serial8250_em485_supported;
 
 void serial8250_rs485_start_tx(struct uart_8250_port *up);
+void serial8250_rs485_stop_tx(struct uart_8250_port *p);
 
 /* MCR <-> TIOCM conversion */
 static inline int serial8250_TIOCM_to_MCR(int tiocm)
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index afef1dd4ddf4..2b62d49a935d 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -366,7 +366,7 @@ static void omap8250_restore_regs(struct uart_8250_port *up)
 
 	if (up->port.rs485.flags & SER_RS485_ENABLED &&
 	    up->port.rs485_config == serial8250_em485_config)
-		serial8250_em485_stop_tx(up);
+		serial8250_rs485_stop_tx(up);
 }
 
 /*
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index ba8d9cefc451..7ee74ec944d2 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -558,7 +558,7 @@ static int serial8250_em485_init(struct uart_8250_port *p)
 
 deassert_rts:
 	if (p->em485->tx_stopped)
-		p->rs485_stop_tx(p);
+		serial8250_rs485_stop_tx(p);
 
 	return 0;
 }
@@ -1380,14 +1380,13 @@ static void serial8250_stop_rx(struct uart_port *port)
  * @p: uart 8250 port
  *
  * Generic callback usable by 8250 uart drivers to stop rs485 transmission.
+ * It does not restore RX interrupts. Use the wrapper function
+ * serial8250_rs485_stop_tx() if that is also needed.
  */
 void serial8250_em485_stop_tx(struct uart_8250_port *p)
 {
 	unsigned char mcr = serial8250_in_MCR(p);
 
-	/* Port locked to synchronize UART_IER access against the console. */
-	lockdep_assert_held_once(&p->port.lock);
-
 	if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
 		mcr |= UART_MCR_RTS;
 	else
@@ -1397,16 +1396,29 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p)
 	/*
 	 * Empty the RX FIFO, we are not interested in anything
 	 * received during the half-duplex transmission.
-	 * Enable previously disabled RX interrupts.
 	 */
-	if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
+	if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
 		serial8250_clear_and_reinit_fifos(p);
+}
+EXPORT_SYMBOL_GPL(serial8250_em485_stop_tx);
+
+/**
+ * serial8250_rs485_stop_tx() - stop rs485 transmission, restore RX interrupts
+ * @p: uart 8250 port
+ */
+void serial8250_rs485_stop_tx(struct uart_8250_port *p)
+{
+	/* Port locked to synchronize UART_IER access against the console. */
+	lockdep_assert_held_once(&p->port.lock);
+
+	p->rs485_stop_tx(p);
 
+	/* Enable previously disabled RX interrupts. */
+	if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
 		p->ier |= UART_IER_RLSI | UART_IER_RDI;
 		serial_port_out(&p->port, UART_IER, p->ier);
 	}
 }
-EXPORT_SYMBOL_GPL(serial8250_em485_stop_tx);
 
 static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t)
 {
@@ -1418,7 +1430,7 @@ static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t)
 	serial8250_rpm_get(p);
 	uart_port_lock_irqsave(&p->port, &flags);
 	if (em485->active_timer == &em485->stop_tx_timer) {
-		p->rs485_stop_tx(p);
+		serial8250_rs485_stop_tx(p);
 		em485->active_timer = NULL;
 		em485->tx_stopped = true;
 	}
@@ -1450,7 +1462,7 @@ static void __stop_tx_rs485(struct uart_8250_port *p, u64 stop_delay)
 		em485->active_timer = &em485->stop_tx_timer;
 		hrtimer_start(&em485->stop_tx_timer, ns_to_ktime(stop_delay), HRTIMER_MODE_REL);
 	} else {
-		p->rs485_stop_tx(p);
+		serial8250_rs485_stop_tx(p);
 		em485->active_timer = NULL;
 		em485->tx_stopped = true;
 	}
-- 
2.39.2
Re: [PATCH next v2 2/4] serial: 8250: Split out IER from rs485_stop_tx()
Posted by Petr Mladek 2 months, 1 week ago 
On Fri 2024-09-13 16:11:36, John Ogness wrote:
> Move IER handling out of rs485_stop_tx() callback and into a new
> wrapper serial8250_rs485_stop_tx(). Replace all callback call sites
> with wrapper, except for the console write() callback, where it is
> inappropriate to modify IER.

It would be great to provide more details:

  + why it is done (IER modification requires port lock?)

  + why it is suddenly safe to call serial8250_em485_handle_stop_tx()
    without holding &p->port.lock


> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -558,7 +558,7 @@ static int serial8250_em485_init(struct uart_8250_port *p)
>  
>  deassert_rts:
>  	if (p->em485->tx_stopped)
> -		p->rs485_stop_tx(p);
> +		serial8250_rs485_stop_tx(p);

This would keep the same functionality only when

	p->rs485_stop_tx == serial8250_em485_stop_tx

Is it always the case?
Is it OK when it is not the case?

For example, serial8250_em485_init() is involved in bcm2835aux driver
probe which uses another rs485_stop_tx() callback, see below.

>  
>  	return 0;
>  }
> @@ -1397,16 +1396,29 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p)
>  	/*
>  	 * Empty the RX FIFO, we are not interested in anything
>  	 * received during the half-duplex transmission.
> -	 * Enable previously disabled RX interrupts.
>  	 */
> -	if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
> +	if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
>  		serial8250_clear_and_reinit_fifos(p);
> +}
> +EXPORT_SYMBOL_GPL(serial8250_em485_stop_tx);
> +
> +/**
> + * serial8250_rs485_stop_tx() - stop rs485 transmission, restore RX interrupts
> + * @p: uart 8250 port
> + */
> +void serial8250_rs485_stop_tx(struct uart_8250_port *p)
> +{
> +	/* Port locked to synchronize UART_IER access against the console. */
> +	lockdep_assert_held_once(&p->port.lock);
> +
> +	p->rs485_stop_tx(p);
>  
> +	/* Enable previously disabled RX interrupts. */
> +	if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
>  		p->ier |= UART_IER_RLSI | UART_IER_RDI;
>  		serial_port_out(&p->port, UART_IER, p->ier);
>  	}
>  }
> -EXPORT_SYMBOL_GPL(serial8250_em485_stop_tx);
>  
>  static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t)
>  {
> @@ -1418,7 +1430,7 @@ static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t)
>  	serial8250_rpm_get(p);
>  	uart_port_lock_irqsave(&p->port, &flags);
>  	if (em485->active_timer == &em485->stop_tx_timer) {
> -		p->rs485_stop_tx(p);
> +		serial8250_rs485_stop_tx(p);

This causes that UART_IER is manipulated for all p->rs485_stop_tx()
callbacks. Is that correct, please?

For example, it seems serial8250_em485_handle_stop_tx() might be used
also by bcm2835aux driver. It set by:

static int serial8250_em485_init(struct uart_8250_port *p)
{
	[...]
	p->em485->stop_tx_timer.function = &serial8250_em485_handle_stop_tx;
	[...]
}

which is called via

int serial8250_em485_config(struct uart_port *port, struct ktermios *termios,
			    struct serial_rs485 *rs485)
{
	[...]
	if (rs485->flags & SER_RS485_ENABLED)
		return serial8250_em485_init(up);
	[...]
}

which is set by:

static int bcm2835aux_serial_probe(struct platform_device *pdev)
{
	[...]
	up.port.rs485_config = serial8250_em485_config;		<--------
	[...]
	up.rs485_stop_tx = bcm2835aux_rs485_stop_tx;
	[...]
}

But this same _probe() call sets

	up.rs485_stop_tx = bcm2835aux_rs485_stop_tx;

which does not manipulate UART_IER.

>  		em485->active_timer = NULL;
>  		em485->tx_stopped = true;
>  	}
> @@ -1450,7 +1462,7 @@ static void __stop_tx_rs485(struct uart_8250_port *p, u64 stop_delay)
>  		em485->active_timer = &em485->stop_tx_timer;
>  		hrtimer_start(&em485->stop_tx_timer, ns_to_ktime(stop_delay), HRTIMER_MODE_REL);
>  	} else {
> -		p->rs485_stop_tx(p);
> +		serial8250_rs485_stop_tx(p);

I can't find easily whether serial8250_em485_stop_tx() is always set
as p->rs485_stop_tx callback here. I would expect that it might be
another callback. It is a callback after all.

Is it always safe?

>  		em485->active_timer = NULL;
>  		em485->tx_stopped = true;
>  	}

Best Regards,
Petr
Re: [PATCH next v2 2/4] serial: 8250: Split out IER from rs485_stop_tx()
Posted by kernel test robot 2 months, 2 weeks ago 
Hi John,

kernel test robot noticed the following build errors:

[auto build test ERROR on b794563ea12fb46d9499da9e30c33d9607e33697]

url:    https://github.com/intel-lab-lkp/linux/commits/John-Ogness/serial-8250-Split-out-IER-from-rs485_start_tx/20240913-220810
base:   b794563ea12fb46d9499da9e30c33d9607e33697
patch link:    https://lore.kernel.org/r/20240913140538.221708-3-john.ogness%40linutronix.de
patch subject: [PATCH next v2 2/4] serial: 8250: Split out IER from rs485_stop_tx()
config: parisc-randconfig-r064-20240914 (https://download.01.org/0day-ci/archive/20240914/202409141849.iyZNNZgc-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409141849.iyZNNZgc-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409141849.iyZNNZgc-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in kernel/locking/test-ww_mutex.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/devfreq/governor_performance.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/devfreq/governor_userspace.o
>> ERROR: modpost: "serial8250_rs485_stop_tx" [drivers/tty/serial/8250/8250_omap.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.