| Message ID | 20240913140538.221708-3-john.ogness@linutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | convert 8250 to nbcon | expand |
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!
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
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; }
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(-)