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
