* [PATCH] cxl: disable the lazy approach for irqs in POWERVM environment.
@ 2018-03-22 16:37 Christophe Lombard
2018-03-23 2:14 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 6+ messages in thread
From: Christophe Lombard @ 2018-03-22 16:37 UTC (permalink / raw)
To: linuxppc-dev, fbarrat, vaibhav, andrew.donnellan
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(+)
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);
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] cxl: disable the lazy approach for irqs in POWERVM environment.
2018-03-22 16:37 [PATCH] cxl: disable the lazy approach for irqs in POWERVM environment Christophe Lombard
@ 2018-03-23 2:14 ` Benjamin Herrenschmidt
2018-03-23 16:17 ` christophe lombard
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2018-03-23 2:14 UTC (permalink / raw)
To: Christophe Lombard, linuxppc-dev, fbarrat, vaibhav, andrew.donnellan
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);
> }
> }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cxl: disable the lazy approach for irqs in POWERVM environment.
2018-03-23 2:14 ` Benjamin Herrenschmidt
@ 2018-03-23 16:17 ` christophe lombard
2018-03-24 8:14 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 6+ messages in thread
From: christophe lombard @ 2018-03-23 16:17 UTC (permalink / raw)
To: benh, linuxppc-dev, fbarrat, vaibhav, andrew.donnellan
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);
>> }
>> }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cxl: disable the lazy approach for irqs in POWERVM environment.
2018-03-23 16:17 ` christophe lombard
@ 2018-03-24 8:14 ` Benjamin Herrenschmidt
2018-05-07 16:02 ` christophe lombard
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2018-03-24 8:14 UTC (permalink / raw)
To: christophe lombard, linuxppc-dev, fbarrat, vaibhav, andrew.donnellan
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);
> > > }
> > > }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cxl: disable the lazy approach for irqs in POWERVM environment.
2018-03-24 8:14 ` Benjamin Herrenschmidt
@ 2018-05-07 16:02 ` christophe lombard
2018-05-07 21:01 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 6+ messages in thread
From: christophe lombard @ 2018-05-07 16:02 UTC (permalink / raw)
To: Benjamin Herrenschmidt, linuxppc-dev, vaibhav, andrew.donnellan
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);
>>>> }
>>>> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cxl: disable the lazy approach for irqs in POWERVM environment.
2018-05-07 16:02 ` christophe lombard
@ 2018-05-07 21:01 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2018-05-07 21:01 UTC (permalink / raw)
To: christophe lombard, linuxppc-dev, vaibhav, andrew.donnellan
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);
> > > > > }
> > > > > }
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-05-07 21:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-22 16:37 [PATCH] cxl: disable the lazy approach for irqs in POWERVM environment Christophe Lombard
2018-03-23 2:14 ` Benjamin Herrenschmidt
2018-03-23 16:17 ` christophe lombard
2018-03-24 8:14 ` Benjamin Herrenschmidt
2018-05-07 16:02 ` christophe lombard
2018-05-07 21:01 ` Benjamin Herrenschmidt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.