On Thu, Jul 02, 2020 at 02:40:33PM +0900, Sergey Senozhatsky wrote: > On (20/07/02 14:12), Sergey Senozhatsky wrote: > > From: Sergey Senozhatsky <sergey.senozhat...@gmail.com> > > On (20/06/30 11:02), Tony Lindgren wrote:
... > > I think we can do it. serial8250_do_startup() and irq handler take > > port->lock, so they should be synchronized. > > Hmm, hold on. Why does it disable IRQ in the first place? IRQ handlers > should grab the port->lock. So if there is already running IRQ, then > serial8250_do_startup() will wait until IRQ handler unlocks the port->lock. > If serial8250_do_startup() grabs the port->lock first, then IRQ will wait > for serial8250_do_startup() to unlock it. serial8250_do_startup() does > not release the port->unlock until its done: > > spin_lock_irqsave(&port->lock, flags); > > wait_for_xmitr(up, UART_LSR_THRE); > serial_port_out_sync(port, UART_IER, UART_IER_THRI); > udelay(1); /* allow THRE to set */ > iir1 = serial_port_in(port, UART_IIR); > serial_port_out(port, UART_IER, 0); > serial_port_out_sync(port, UART_IER, UART_IER_THRI); > udelay(1); /* allow a working UART time to re-assert THRE */ > iir = serial_port_in(port, UART_IIR); > serial_port_out(port, UART_IER, 0); > > spin_unlock_irqrestore(&port->lock, flags); > > so IRQ will not see the inconsistent device state. > > What exactly is the purpose of disable_irq_nosync()? Can we just remove > disable_irq_nosync()/enable_irq() instead? Are there any IRQ handlers > that don't acquire the port->lock? I didn't look into this deeply, but my understanding that this is something for special case when you have several UART ports sharing the IRQ (multi-port card) and IRQ even maybe undesirable b/c it will confuse real IRQ handler. I don't remember details, but AFAIR IRQ handler does a busyloop to service as much as possible and in between it may release the lock (again, multi-port UART cards), that's why we better avoid IRQ event in the first place. But it's my pure speculation. -- With Best Regards, Andy Shevchenko
