Re: [PATCH] serial: exar: Fix stuck MSIs

[Andy Shevchenko]

On Mon, 2017-04-24 at 11:06 +0200, Jan Kiszka wrote:
> On 2017-04-24 10:59, Andy Shevchenko wrote:
> > On Sat, 2017-04-22 at 11:36 +0200, Jan Kiszka wrote:
> > > From: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
> > > 
> > > After migrating 8250_exar to MSI in 172c33cb61da, we can get stuck
> > > without further interrupts because of the special wake-up event
> > > these
> > > chips send. They are only cleared by reading INT0. As we fail to
> > > do so
> > > during startup and shutdown, we can leave the interrupt line
> > > asserted,
> > > which is fatal with edge-triggered MSIs.
> > > 
> > > Add the required reading of INT0 to startup and shutdown. Also
> > > account
> > > for the fact that a pending wake-up interrupt means we have to
> > > return
> > > 1
> > > from exar_handle_irq.
> > > 
> > > An alternative approach would have been disabling the wake-up
> > > interrupt.
> > > Unfortunately, this feature (REGB[17] = 1) is not available on the
> > > XR17D15X.
> > > 
> > > Fixes: 172c33cb61da ("serial: exar: Enable MSI support")
> > > Signed-off-by: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
> > > ---
> > > 
> > > Regression of upcoming 4.11.
> > > 
> > > Âdrivers/tty/serial/8250/8250_port.c | 19 ++++++++++---------
> > > Â1 file changed, 10 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/tty/serial/8250/8250_port.c
> > > b/drivers/tty/serial/8250/8250_port.c
> > > index 6119516ef5fc..3a3667880fcf 100644
> > > --- a/drivers/tty/serial/8250/8250_port.c
> > > +++ b/drivers/tty/serial/8250/8250_port.c
> > > @@ -47,6 +47,7 @@
> > > Â/*
> > > Â * These are definitions for the Exar XR17V35X and XR17(C|D)15X
> > > Â */
> > > +#define UART_EXAR_INT0		0x80
> > > Â#define UART_EXAR_SLEEP		0x8b	/* Sleep mode
> > > */
> > > Â#define UART_EXAR_DVID		0x8d	/* Device
> > > identification */
> > > Â
> > > @@ -1869,17 +1870,13 @@ static int
> > > serial8250_default_handle_irq(struct uart_port *port)
> > > Âstatic int exar_handle_irq(struct uart_port *port)
> > > Â{
> > > Â	unsigned int iir = serial_port_in(port, UART_IIR);
> > > -	int ret;
> > > +	int ret = 0;
> > > Â
> > > -	ret = serial8250_handle_irq(port, iir);
> > > +	if (((port->type == PORT_XR17V35X) || (port->type ==
> > > PORT_XR17D15X)) &&
> > > +	ÂÂÂÂserial_port_in(port, UART_EXAR_INT0) != 0)
> > > +		ret = 1;
> > > Â
> > > -	if ((port->type == PORT_XR17V35X) ||
> > > -	ÂÂÂ(port->type == PORT_XR17D15X)) {
> > > -		serial_port_in(port, 0x80);
> > > -		serial_port_in(port, 0x81);
> > > -		serial_port_in(port, 0x82);
> > > -		serial_port_in(port, 0x83);
> > 
> > You replaced 4 reads by one. I'm suspecting that on multi-port cards
> > you
> > still need to read all of them (I assume they are called INT0, INT1,
> > ...). Perhaps you need a helper where you do that and call it from
> > all
> > necessary places.
> 
> Nope, we never had to read them all: "Wake-up Indicator is cleared by
> reading the INT0 register." (Exar manual) INT0 contains the interrupt
> status for all channels.

Perhaps it needs to be mentioned in commit message as well.

-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy