Message ID | 1521736674-13128-1-git-send-email-clombard@linux.vnet.ibm.com (mailing list archive) |
---|---|
State | Rejected, archived |
Headers | show |
Series | cxl: disable the lazy approach for irqs in POWERVM environment. | expand |
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. > 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); > } > }
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(). The cxl driver disables the interrupts before attaching the process element and enables the interrupts after that. In the current code, irq_enable() unmasks the irq only if the irq state is IRQD_IRQ_MASKED but it does not. 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(). >> 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); >> } >> }
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); > > > } > > > }
Le 24/03/2018 à 09:14, Benjamin Herrenschmidt a écrit : > 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. Hi Ben, To answer to your questions, here is the timeline in the cxl driver 1. call disable_irq() 2. call plpar_hcall9() to attach a process element During this phase, phyp (as described in CAPI PAPR document) disables the virtual interrupts provided in the process-element- struct. Then the partition must use ibm,set-xive to enable the virtual interrupt source after H_ATTACH_CA_PROCESS completes successfully. 3. call enable_irq() Before the following patch (genirq: Avoid unnecessary low level irq function calls) be pushed in mainline, the unmask api (= ics_rtas_unmask_irq()) was called by default and everything was ok. With this patch, the unmask api is now called only if IRQD_IRQ_MASKED is set and in our case, it's never done and the irqs remain masked. So, there is a disconnect between the 'HW' (phyp) state of the interrupt and the IRQD_IRQ_MASKED flag. In the timeline of the cxl driver, the IRQD_IRQ_MASKED flag is never set. This flag was not used in enable_irq(), before that the patch "genirq: Avoid ..." was pushed. So ics_rtas_unmask_irq() was called and everything was ok. Thanks >> >> >> >>>> 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); >>>> } >>>> } >
On Mon, 2018-05-07 at 18:02 +0200, christophe lombard wrote: > > To answer to your questions, here is the timeline in the cxl driver > > 1. call disable_irq() > > 2. call plpar_hcall9() to attach a process element > During this phase, phyp (as described in CAPI PAPR document) > disables the virtual interrupts provided in the process-element- > struct. > Then the partition must use ibm,set-xive to enable the virtual > interrupt source after H_ATTACH_CA_PROCESS completes > successfully. Ok, this is the breakage. This PAPR API is badly defined, nothing other than the existing interrupt management PAPR calls should affect the enable/disable state of the interrupt The fact that attaching a process element does is a broken design. > 3. call enable_irq() > Before the following patch (genirq: Avoid unnecessary low level > irq function calls) be pushed in mainline, the unmask api > (= ics_rtas_unmask_irq()) was called by default and everything > was ok. > > With this patch, the unmask api is now called only if > IRQD_IRQ_MASKED is set and in our case, it's never done and the > irqs remain masked. > > > So, there is a disconnect between the 'HW' (phyp) state of the > interrupt and the IRQD_IRQ_MASKED flag. > > In the timeline of the cxl driver, the IRQD_IRQ_MASKED flag is never > set. > This flag was not used in enable_irq(), before that the patch "genirq: > Avoid ..." was pushed. So ics_rtas_unmask_irq() was called and > everything was ok. > > Thanks > > > > > > > > > > > > > > > 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); > > > > > } > > > > > }
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); } }
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)'. Signed-off-by: Christophe Lombard <clombard@linux.vnet.ibm.com> --- drivers/misc/cxl/guest.c | 1 + 1 file changed, 1 insertion(+)