Re: [cxl:pending 13/21] drivers/cxl/pci.c:511:8-33: WARNING: Threaded IRQ with no primary handler requested without IRQF_ONESHOT (unless it is nested IRQ)

Re: [cxl:pending 13/21] drivers/cxl/pci.c:511:8-33: WARNING: Threaded IRQ with no primary handler requested without IRQF_ONESHOT (unless it is nested IRQ)

[Davidlohr Bueso]

On Sun, 29 Jan 2023, kernel test robot wrote:

>BCC: lkp@intel.com
>CC: oe-kbuild-all@lists.linux.dev
>CC: Alison Schofield <alison.schofield@intel.com>
>CC: Vishal Verma <vishal.l.verma@intel.com>
>CC: Ira Weiny <ira.weiny@intel.com>
>CC: Ben Widawsky <ben.widawsky@intel.com>
>CC: Dan Williams <dan.j.williams@intel.com>
>CC: linux-cxl@vger.kernel.org
>TO: Davidlohr Bueso <dave@stgolabs.net>
>CC: Dan Williams <dan.j.williams@intel.com>
>CC: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>CC: Ira Weiny <ira.weiny@intel.com>
>
>tree:   https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git pending
>head:   ace4b76d4ab70335cffb5a1c8450d3b34c16771a
>commit: a49aa8141b657a7fd33d86f7bed3ca07567ab37a [13/21] cxl/mem: Wire up event interrupts
>:::::: branch date: 4 hours ago
>:::::: commit date: 2 days ago
>config: microblaze-randconfig-c043-20230129 (https://download.01.org/0day-ci/archive/20230129/202301291620.DxnDWLoW-lkp@intel.com/config)
>compiler: microblaze-linux-gcc (GCC) 12.1.0
>
>If you fix the issue, kindly add following tag where applicable
>| Reported-by: kernel test robot <lkp@intel.com>
>| Reported-by: Julia Lawall <julia.lawall@lip6.fr>

Thanks for reporting, and I'm adding more folks explicitly to the Cc.

>
>cocci warnings: (new ones prefixed by >>)
>>> drivers/cxl/pci.c:511:8-33: WARNING: Threaded IRQ with no primary handler requested without IRQF_ONESHOT (unless it is nested IRQ)

Considering MSIs are edge triggered interrupts, this should be benign.

Thanks,
Davidlohr

Re: [cxl:pending 13/21] drivers/cxl/pci.c:511:8-33: WARNING: Threaded IRQ with no primary handler requested without IRQF_ONESHOT (unless it is nested IRQ)

[Ira Weiny]

Davidlohr Bueso wrote:
> On Sun, 29 Jan 2023, kernel test robot wrote:
> 
> >BCC: lkp@intel.com
> >CC: oe-kbuild-all@lists.linux.dev
> >CC: Alison Schofield <alison.schofield@intel.com>
> >CC: Vishal Verma <vishal.l.verma@intel.com>
> >CC: Ira Weiny <ira.weiny@intel.com>
> >CC: Ben Widawsky <ben.widawsky@intel.com>
> >CC: Dan Williams <dan.j.williams@intel.com>
> >CC: linux-cxl@vger.kernel.org
> >TO: Davidlohr Bueso <dave@stgolabs.net>
> >CC: Dan Williams <dan.j.williams@intel.com>
> >CC: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >CC: Ira Weiny <ira.weiny@intel.com>
> >
> >tree:   https://git.kernel.org/pub/scm/linux/kernel/git/cxl/cxl.git pending
> >head:   ace4b76d4ab70335cffb5a1c8450d3b34c16771a
> >commit: a49aa8141b657a7fd33d86f7bed3ca07567ab37a [13/21] cxl/mem: Wire up event interrupts
> >:::::: branch date: 4 hours ago
> >:::::: commit date: 2 days ago
> >config: microblaze-randconfig-c043-20230129 (https://download.01.org/0day-ci/archive/20230129/202301291620.DxnDWLoW-lkp@intel.com/config)
> >compiler: microblaze-linux-gcc (GCC) 12.1.0
> >
> >If you fix the issue, kindly add following tag where applicable
> >| Reported-by: kernel test robot <lkp@intel.com>
> >| Reported-by: Julia Lawall <julia.lawall@lip6.fr>
> 
> Thanks for reporting, and I'm adding more folks explicitly to the Cc.

Thanks for the Cc.

> 
> >
> >cocci warnings: (new ones prefixed by >>)
> >>> drivers/cxl/pci.c:511:8-33: WARNING: Threaded IRQ with no primary handler requested without IRQF_ONESHOT (unless it is nested IRQ)
> 
> Considering MSIs are edge triggered interrupts, this should be benign.

I'm trying to understand this a bit more.  So I looked in more detail.  I
believe Dan suggested that I could use irq_default_primary_handler() in
this case by passing NULL.  And this would handle threading the irq
processing for me.

It looks from a software standpoint this should work.  But the kdocs
really imply that not masking the irq in the handler may be an issue.
Thus the 'requirement' of using IRQF_ONESHOT.

Is this somehow different with MSI?  Is this a case of the kdocs being a
bit out of date for MSI?

Ira

Re: [cxl:pending 13/21] drivers/cxl/pci.c:511:8-33: WARNING: Threaded IRQ with no primary handler requested without IRQF_ONESHOT (unless it is nested IRQ)

[Dan Williams]

Ira Weiny wrote:
[..]
> > Considering MSIs are edge triggered interrupts, this should be benign.
> 
> I'm trying to understand this a bit more.  So I looked in more detail.  I
> believe Dan suggested that I could use irq_default_primary_handler() in
> this case by passing NULL.  And this would handle threading the irq
> processing for me.
> 
> It looks from a software standpoint this should work.  But the kdocs
> really imply that not masking the irq in the handler may be an issue.
> Thus the 'requirement' of using IRQF_ONESHOT.
> 
> Is this somehow different with MSI?  Is this a case of the kdocs being a
> bit out of date for MSI?

My reading, may be from the changelog and not the kdocs, was that at
request_irq() time it is difficult to discern level triggered vs
edge-triggered. So for the default primary handler, that does nothing to
quiet a screaming level-triggered interrupt, just require oneshot mode.

Re: [cxl:pending 13/21] drivers/cxl/pci.c:511:8-33: WARNING: Threaded IRQ with no primary handler requested without IRQF_ONESHOT (unless it is nested IRQ)

[Davidlohr Bueso]

On Tue, 31 Jan 2023, Dan Williams wrote:

>Ira Weiny wrote:
>[..]
>> > Considering MSIs are edge triggered interrupts, this should be benign.
>>
>> I'm trying to understand this a bit more.  So I looked in more detail.  I
>> believe Dan suggested that I could use irq_default_primary_handler() in
>> this case by passing NULL.  And this would handle threading the irq
>> processing for me.
>>
>> It looks from a software standpoint this should work.  But the kdocs
>> really imply that not masking the irq in the handler may be an issue.
>> Thus the 'requirement' of using IRQF_ONESHOT.
>>
>> Is this somehow different with MSI?  Is this a case of the kdocs being a
>> bit out of date for MSI?
>
>My reading, may be from the changelog and not the kdocs, was that at
>request_irq() time it is difficult to discern level triggered vs
>edge-triggered. So for the default primary handler, that does nothing to
>quiet a screaming level-triggered interrupt, just require oneshot mode.

Right and it's only level-triggered that are the problem after done with
the hardirq handler. So in the msi/cxl case we already know that all this
is edge (sent once and forgotten by the hardware), which is why I was saying
it's harmless. It's still probably a good idea to have it explicitly oneshot
semantics as Dan's patch.

Thanks,
Davidlohr

Re: [cxl:pending 13/21] drivers/cxl/pci.c:511:8-33: WARNING: Threaded IRQ with no primary handler requested without IRQF_ONESHOT (unless it is nested IRQ)

[Ira Weiny]

Davidlohr Bueso wrote:
> On Tue, 31 Jan 2023, Dan Williams wrote:
> 
> >Ira Weiny wrote:
> >[..]
> >> > Considering MSIs are edge triggered interrupts, this should be benign.
> >>
> >> I'm trying to understand this a bit more.  So I looked in more detail.  I
> >> believe Dan suggested that I could use irq_default_primary_handler() in
> >> this case by passing NULL.  And this would handle threading the irq
> >> processing for me.
> >>
> >> It looks from a software standpoint this should work.  But the kdocs
> >> really imply that not masking the irq in the handler may be an issue.
> >> Thus the 'requirement' of using IRQF_ONESHOT.
> >>
> >> Is this somehow different with MSI?  Is this a case of the kdocs being a
> >> bit out of date for MSI?
> >
> >My reading, may be from the changelog and not the kdocs, was that at
> >request_irq() time it is difficult to discern level triggered vs
> >edge-triggered. So for the default primary handler, that does nothing to
> >quiet a screaming level-triggered interrupt, just require oneshot mode.
> 
> Right and it's only level-triggered that are the problem after done with
> the hardirq handler. So in the msi/cxl case we already know that all this
> is edge (sent once and forgotten by the hardware), which is why I was saying
> it's harmless. It's still probably a good idea to have it explicitly oneshot
> semantics as Dan's patch.

Ok that makes sense.

Thanks to you both for clarifying and to Dan for the patch.

Ira