Re: [PATCH 6/6] serial: 8250_dw: Ensure BUSY is deasserted

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4266 bytes --]

On Tue, 27 Jan 2026, Andy Shevchenko wrote:

> On Tue, Jan 27, 2026 at 03:35:27PM +0200, Ilpo Järvinen wrote:
> > On Sat, 24 Jan 2026, Andy Shevchenko wrote:
> > > On Fri, Jan 23, 2026 at 07:27:39PM +0200, Ilpo Järvinen wrote:
> 
> +Cc: printk people to check on printing from a serial driver routines.
> 
> ...
> 
> > > > +	/* Prevent triggering interrupt from RBR filling */
> > > > +	p->serial_out(p, UART_IER, 0);
> > > 
> > > Do we specifically use callbacks directly and not wrappers all over the change?
> > 
> > I guess it's just a habit, I suppose you meant using serial_port_in/out 
> > instead. I can try to change those.
> 
> Not (only) me. Jiri updated this driver (and many others) to use callbacks.
> That's why I added comments here and there about possible recursions.

Fair, this patch originated from a time way older than Jiri's conversion
(not an excuse, just stating how it came to be and I've not realized 
using an old way until you mentioned).

> > > > +	serial8250_fifo_wait_for_lsr_thre(up, p->fifosize);
> > > > +	ndelay(p->frame_time);
> > > 
> > > Wouldn't be a problem on lowest baud rates (exempli gratia 110)?
> > 
> > Perhaps, but until somebody comes with an issue report related to 110, I'm 
> > wondering if this really is worth trying to address. Any suggestion how is 
> > welcome as well?
> 
> Polling work? Timer?

And how do I prevent others messing with the UART during that time? While 
IER is zeroed here (and I could make up->ier zero as well, I think), I 
can't hold port's lock if I do either of those.

And I can't take the tty_port's mutex here either because the caller 
is already holding port's lock (and it wouldn't prevent console writes 
anyway as that, I think, only takes port's lock).

Sadly THRE/TEMT are not trustworthy as they are set before all those 
non-data bits have been fully blasted on to the wire (we learned this with 
rs485 half-duplex scenarios).


Normal behavioral exceptation what I have here is that userspace is sane 
and won't do LCR write and tx at the same time but I don't know how to 
ensure that. Perhaps using now > last xmit timestamp + frame_time could 
avoid this unconditional delay.

> > > > +	retries = 4;	/* Arbitrary limit, 2 was always enough in tests */
> > > > +	do {
> > > > +		serial8250_clear_fifos(up);
> > > > +		if (!(p->serial_in(p, usr_reg) & DW_UART_USR_BUSY))
> > > > +			break;
> > > > +		ndelay(p->frame_time);
> > > > +	} while (--retries);
> > > 
> > > read_poll_timeout_atomic() ? I assume it can't be used due to small frame time?
> > 
> > Frame time is in nanoseconds yes. I did consider 
> > read_poll_timeout_atomic() but it would have required nsec -> usec 
> > conversion so I left this as it is.
> 
> Yeah with the same issue on low baud rates. So far I think we need to consider
> 9600 as commonly used by the old HW (which may be connected to a modern PC with
> this new kernel running), so the frame time sounds like close to a millisecond.
> And this can be met in real life.
> 
> Maybe put TODO/FIXME around these ndelay() calls?

Seems reasonable, I'll add that.

I'm under impression that all LCR writes occur from contexts that are 
non-atomic by nature (except they are holding the port's lock, of course) 
so this should never delay an interrupt handler.

> > > > +	if (d->in_idle) {
> > > 
> > > > +		/*
> > > > +		 * FIXME: this deadlocks if port->lock is already held
> > > > +		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
> > > > +		 */
> > > 
> > > Hmm... That FIXME should gone since we have non-blocking consoles, no?
> > 
> > No, lockdep still gets angry if printing is used while holding port's 
> > lock.
> 
> Hmm... Let's ask PRINTK people about this. John, do we still have a gap
> with nbcon? Or did I misunderstand the scope of its use?
> 
> > What would be possible though, is to mark the port's lock critical section 
> > for print deferral (but it's outside the scope of this series). In case of 
> > serial, it would be justified to use deferred printing (which is only 
> > meant for special cases) because serial console and printing are related.
> > 
> > > > +		return;
> > > > +	}
> 
> 

-- 
 i.