Re: [PATCH v6 2/5] serial: serialize asynchronous write wakeups with the port lock

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Sat, Oct 24, 2026 at 02:52:27PM -0600, James Hilliard wrote:
> uart_write_wakeup() expects the UART port lock to be held. Several worker,
> DMA completion and interrupt paths call it without that lock, including
> indirect calls from uart_handle_cts_change() and the transmit helpers.
> 
> Take the port lock around these notifications or their transmit helper.
> Keep sleeping SPI and regmap accesses outside the locked region, and keep
> the FSL LPUART completion lock held through its wakeup. Acquire the port
> lock after releasing private channel locks to preserve the ordering used
> by UART callbacks. In the MUX timer, publish available FIFO space under
> the port lock and keep the hardware FIFO-drain wait outside it.
> 
> Use IRQ-safe locking in the Atmel RX and TX tasklets and SB1250 termios
> path, since their interrupt handlers now acquire the same port lock.
> Preserve the caller's interrupt state when releasing those locks.
> 
> Serialize CLPS711x TX servicing and its interrupt-enable state with the
> port lock. Disable TX interrupts without waiting in stop_tx(), since a
> threaded handler may need that same lock. Have an already queued handler
> leave stopped output untouched, and drain it outside the lock during
> shutdown before the transmit FIFO can be released.
> 
> SCCNXP handles interrupts under a controller lock which its UART callbacks
> acquire with the port lock already held. Collect the ports needing wakeups
> while servicing the controller, then release that lock before taking each
> port lock and rechecking the wakeup threshold. Removal drains the IRQ or
> timer before unregistering the ports.
> 
> This serializes write notifications with port-locked serial-core state
> changes, allowing readiness and transmit-storage reservations to gate the
> notification without a check-versus-shutdown race.

...

> -static void sccnxp_handle_events(struct sccnxp_port *s)
> +static unsigned int sccnxp_handle_events(struct sccnxp_port *s)
>  {
> +	unsigned int wakeup = 0;

unsigned long

>  	int i;
>  	u8 isr;

> @@ -491,20 +491,43 @@ static void sccnxp_handle_events(struct sccnxp_port *s)
>  		for (i = 0; i < s->uart.nr; i++) {
>  			if (s->opened[i] && (isr & ISR_RXRDY(i)))
>  				sccnxp_handle_rx(&s->port[i]);
> -			if (s->opened[i] && (isr & ISR_TXRDY(i)))
> -				sccnxp_handle_tx(&s->port[i]);
> +			if (s->opened[i] && (isr & ISR_TXRDY(i)) &&
> +			    sccnxp_handle_tx(&s->port[i]))
> +				wakeup |= BIT(i);
>  		}
>  	} while (1);
> +
> +	return wakeup;
> +}
> +
> +static void sccnxp_write_wakeup(struct sccnxp_port *s, unsigned int wakeup)

unsigned long

> +{

> +	int i;

Why is 'i' signed?

> +	/* UART callbacks take s->lock with the port lock already held. */

> +	for (i = 0; i < s->uart.nr; i++) {
> +		struct uart_port *port = &s->port[i];
> +
> +		if (!(wakeup & BIT(i)))
> +			continue;

NIH [1] for_each_set_bit()

> +		scoped_guard(uart_port_lock_irqsave, port) {
> +			if (kfifo_len(&port->state->port.xmit_fifo) < WAKEUP_CHARS)
> +				uart_write_wakeup(port);
> +		}
> +	}
>  }

NIH syndrome is one of the weakest points of AI, please, read and study
the code you are changing.

[1]: https://en.wikipedia.org/wiki/Not_invented_here

-- 
With Best Regards,
Andy Shevchenko