Re: [PATCH] cxl: disable the lazy approach for irqs in POWERVM environment.

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Fri, 2018-03-23 at 17:17 +0100, christophe lombard wrote:
> Le 23/03/2018 à 03:14, Benjamin Herrenschmidt a écrit :
> > On Thu, 2018-03-22 at 17:37 +0100, Christophe Lombard wrote:
> > > The cxl driver cannot disable the interrupt at the device level and has
> > > to use disable_irq[_nosync] instead.
> > > To avoid the implementation of the lazy optimisation (the interrupt is
> > > marked disabled, but the hardware is left unmasked), we can disable it,
> > > for a particular irq line, by calling
> > > 'irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY)'.
> > 
> > Why do you need that ? What's wrong with the lazy approach ? It makes
> > disable_irq/enable_irq faster...
> > 
> > You shouldn't need that unless your device is generating a *LOT* of
> > irqs while disabled.
> > 
> 
> An issue on POWERVM (CAPI) has been introduced with the following patch
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bf22ff45bed664aefb5c4e43029057a199b7070c 
> 
> The PSL or AFU interrupts are never received by the cxl driver because 
> the interrupts are never unmasked.
> 
> Without this patch (genirq: Avoid unnecessary low level irq function 
> calls), the callback desc->irq_data.chip->irq_unmask(&desc->irq_data); 
> (= ics_rtas_unmask_irq()) is called by default through irq_enable().

I don't see why this would change with the patch... 

> The cxl driver disables the interrupts before attaching the process 
> element and enables the interrupts after that.

How ? Using disable_irq or something else ?

> In the current code, irq_enable() unmasks the irq only if the irq state 
> is IRQD_IRQ_MASKED but it does not.

Sorry I don't really parse your sentence. You mean there's a disconnect
between the "HW" (or pHyp) state of the interrupt and the
IRQD_IRQ_MASKED flag ? That shouldn't happen... if that's the case the
bug is elsewhere, what is causing the disconnect in the first place ?

> Call irq_set_status_flags(irq, IRQ_DISABLE_UNLAZY) allows forcing 
> irq_disable() to update the irq state to IRQD_IRQ_MASKED and by default
> irq_enable() will unmask the irq through ics_rtas_unmask_irq().

Hrm. I don't quite understand. You shouldn't need that, I suspect you
are papering over a different bug but I'm not 100% certain as I don't
completely understand what's happening.

Cheers,
Ben.
> 
> 
> 
> > > Signed-off-by: Christophe Lombard <clombard@linux.vnet.ibm.com>
> > > ---
> > >   drivers/misc/cxl/guest.c | 1 +
> > >   1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/misc/cxl/guest.c b/drivers/misc/cxl/guest.c
> > > index f58b4b6c..dc476e1 100644
> > > --- a/drivers/misc/cxl/guest.c
> > > +++ b/drivers/misc/cxl/guest.c
> > > @@ -389,6 +389,7 @@ static void disable_afu_irqs(struct cxl_context *ctx)
> > >   		hwirq = ctx->irqs.offset[r];
> > >   		for (i = 0; i < ctx->irqs.range[r]; hwirq++, i++) {
> > >   			virq = irq_find_mapping(NULL, hwirq);
> > > +			irq_set_status_flags(virq, IRQ_DISABLE_UNLAZY);
> > >   			disable_irq(virq);
> > >   		}
> > >   	}