Re: [PATCH] PCI: let pci_request_irq properly deal with threaded interrupts

Re: [PATCH] PCI: let pci_request_irq properly deal with threaded interrupts

[Bjorn Helgaas]

[+cc Thomas, Christoph, LKML]

On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote:
> If we have a threaded interrupt with the handler being NULL, then
> request_threaded_irq() -> __setup_irq() will complain and bail out
> if the IRQF_ONESHOT flag isn't set. Therefore check for the handler
> being NULL and set IRQF_ONESHOT in this case.
> 
> This change is needed to migrate the mei_me driver to
> pci_alloc_irq_vectors() and pci_request_irq().
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

I'd like an ack from Thomas because this requirement about IRQF_ONESHOT
usage isn't mentioned in the request_threaded_irq() function doc or
Documentation/

> ---
>  drivers/pci/irq.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/irq.c b/drivers/pci/irq.c
> index 2a808e10..a1de501a 100644
> --- a/drivers/pci/irq.c
> +++ b/drivers/pci/irq.c
> @@ -86,13 +86,17 @@ int pci_request_irq(struct pci_dev *dev, unsigned int nr, irq_handler_t handler,
>  	va_list ap;
>  	int ret;
>  	char *devname;
> +	unsigned long irqflags = IRQF_SHARED;
> +
> +	if (!handler)
> +		irqflags |= IRQF_ONESHOT;
>  
>  	va_start(ap, fmt);
>  	devname = kvasprintf(GFP_KERNEL, fmt, ap);
>  	va_end(ap);
>  
>  	ret = request_threaded_irq(pci_irq_vector(dev, nr), handler, thread_fn,
> -			IRQF_SHARED, devname, dev_id);
> +				   irqflags, devname, dev_id);
>  	if (ret)
>  		kfree(devname);
>  	return ret;
> -- 
> 2.18.0
> 

Re: [PATCH] PCI: let pci_request_irq properly deal with threaded interrupts

[Heiner Kallweit]

On 30.07.2018 23:30, Bjorn Helgaas wrote:
> [+cc Thomas, Christoph, LKML]
> 
> On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote:
>> If we have a threaded interrupt with the handler being NULL, then
>> request_threaded_irq() -> __setup_irq() will complain and bail out
>> if the IRQF_ONESHOT flag isn't set. Therefore check for the handler
>> being NULL and set IRQF_ONESHOT in this case.
>>
>> This change is needed to migrate the mei_me driver to
>> pci_alloc_irq_vectors() and pci_request_irq().
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> I'd like an ack from Thomas because this requirement about IRQF_ONESHOT
> usage isn't mentioned in the request_threaded_irq() function doc or
> Documentation/
> 
Sure.

That's the related comment in __setup_irq():

 * The interrupt was requested with handler = NULL, so
 * we use the default primary handler for it. But it
 * does not have the oneshot flag set. In combination
 * with level interrupts this is deadly, because the
 * default primary handler just wakes the thread, then
 * the irq lines is reenabled, but the device still
 * has the level irq asserted. Rinse and repeat....
 *
 * While this works for edge type interrupts, we play
 * it safe and reject unconditionally because we can't
 * say for sure which type this interrupt really
 * has. The type flags are unreliable as the
 * underlying chip implementation can override them.
 */


>> ---
>>  drivers/pci/irq.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/irq.c b/drivers/pci/irq.c
>> index 2a808e10..a1de501a 100644
>> --- a/drivers/pci/irq.c
>> +++ b/drivers/pci/irq.c
>> @@ -86,13 +86,17 @@ int pci_request_irq(struct pci_dev *dev, unsigned int nr, irq_handler_t handler,
>>  	va_list ap;
>>  	int ret;
>>  	char *devname;
>> +	unsigned long irqflags = IRQF_SHARED;
>> +
>> +	if (!handler)
>> +		irqflags |= IRQF_ONESHOT;
>>  
>>  	va_start(ap, fmt);
>>  	devname = kvasprintf(GFP_KERNEL, fmt, ap);
>>  	va_end(ap);
>>  
>>  	ret = request_threaded_irq(pci_irq_vector(dev, nr), handler, thread_fn,
>> -			IRQF_SHARED, devname, dev_id);
>> +				   irqflags, devname, dev_id);
>>  	if (ret)
>>  		kfree(devname);
>>  	return ret;
>> -- 
>> 2.18.0
>>
> 

Re: [PATCH] PCI: let pci_request_irq properly deal with threaded interrupts

[Andy Shevchenko]

On Tue, Jul 31, 2018 at 12:30 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> [+cc Thomas, Christoph, LKML]
>
> On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote:
>> If we have a threaded interrupt with the handler being NULL, then
>> request_threaded_irq() -> __setup_irq() will complain and bail out
>> if the IRQF_ONESHOT flag isn't set. Therefore check for the handler
>> being NULL and set IRQF_ONESHOT in this case.
>>
>> This change is needed to migrate the mei_me driver to
>> pci_alloc_irq_vectors() and pci_request_irq().
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>
> I'd like an ack from Thomas because this requirement about IRQF_ONESHOT
> usage isn't mentioned in the request_threaded_irq() function doc or
> Documentation/

 * IRQF_ONESHOT - Interrupt is not reenabled after the hardirq handler finished.
 *                Used by threaded interrupts which need to keep the
 *                irq line disabled until the threaded handler has been run.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] PCI: let pci_request_irq properly deal with threaded interrupts

[Thomas Gleixner]

On Mon, 30 Jul 2018, Bjorn Helgaas wrote:

> [+cc Thomas, Christoph, LKML]

+ Marc

> On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote:
> > If we have a threaded interrupt with the handler being NULL, then
> > request_threaded_irq() -> __setup_irq() will complain and bail out
> > if the IRQF_ONESHOT flag isn't set. Therefore check for the handler
> > being NULL and set IRQF_ONESHOT in this case.
> > 
> > This change is needed to migrate the mei_me driver to
> > pci_alloc_irq_vectors() and pci_request_irq().
> > 
> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> I'd like an ack from Thomas because this requirement about IRQF_ONESHOT
> usage isn't mentioned in the request_threaded_irq() function doc or
> Documentation/

Right. The documentation really needs some love and care. :(

Yes, request for pure threaded interrupts are rejected if the oneshot flag
is not set. The reason is that this would be deadly especially with level
triggered interrupts because the primary default handler just wakes the
thread and then reenables interrupts, which will make the interrupt come
back immediately and the thread won't have a chance to actually shut it up
in the device.

That made me look into that code again and I found that we added a flag for
irq chips to tell the core that the interrupt is one shot safe, i.e. that
it can be requested w/o IRQF_ONESHOT. That was initially added to optimize
MSI based interrupts which are oneshot safe by implementation.

  dc9b229a58dc ("genirq: Allow irq chips to mark themself oneshot safe")

The original patch added that flag to the x86 MSI irqchip code, but that
part was not applied for reasons which slipped from memory. It might be
worthwhile to revisit that in order to avoid the mask/unmask overhead for
such cases.

Anyway for the patch in question:

       Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Thanks,

	tglx

Re: [PATCH] PCI: let pci_request_irq properly deal with threaded interrupts

[Marc Zyngier]

On Mon, 30 Jul 2018 23:36:57 +0100,
Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> On Mon, 30 Jul 2018, Bjorn Helgaas wrote:
> 
> > [+cc Thomas, Christoph, LKML]
> 
> + Marc
> 
> > On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote:
> > > If we have a threaded interrupt with the handler being NULL, then
> > > request_threaded_irq() -> __setup_irq() will complain and bail out
> > > if the IRQF_ONESHOT flag isn't set. Therefore check for the handler
> > > being NULL and set IRQF_ONESHOT in this case.
> > > 
> > > This change is needed to migrate the mei_me driver to
> > > pci_alloc_irq_vectors() and pci_request_irq().
> > > 
> > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > 
> > I'd like an ack from Thomas because this requirement about IRQF_ONESHOT
> > usage isn't mentioned in the request_threaded_irq() function doc or
> > Documentation/
> 
> Right. The documentation really needs some love and care. :(
> 
> Yes, request for pure threaded interrupts are rejected if the oneshot flag
> is not set. The reason is that this would be deadly especially with level
> triggered interrupts because the primary default handler just wakes the
> thread and then reenables interrupts, which will make the interrupt come
> back immediately and the thread won't have a chance to actually shut it up
> in the device.
> 
> That made me look into that code again and I found that we added a flag for
> irq chips to tell the core that the interrupt is one shot safe, i.e. that
> it can be requested w/o IRQF_ONESHOT. That was initially added to optimize
> MSI based interrupts which are oneshot safe by implementation.
> 
>   dc9b229a58dc ("genirq: Allow irq chips to mark themself oneshot safe")
> 
> The original patch added that flag to the x86 MSI irqchip code, but that
> part was not applied for reasons which slipped from memory. It might be
> worthwhile to revisit that in order to avoid the mask/unmask overhead for
> such cases.

Yup, that would actually be beneficial to a range of interrupt
controllers (only an obscure GPIO driver makes use of this flag).

We could also consider extending this to support interrupt
hierarchies, as __setup_irq() seems only concerned with the top of the
stack (an IRQ provided by a generic MSI stack and backed by an irqchip
providing IRQCHIP_ONESHOT_SAFE would go unnoticed).

	M.

-- 
Jazz is not dead, it just smell funny.

Re: [PATCH] PCI: let pci_request_irq properly deal with threaded interrupts

[Heiner Kallweit]

On 31.07.2018 08:15, Marc Zyngier wrote:
> On Mon, 30 Jul 2018 23:36:57 +0100,
> Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>> On Mon, 30 Jul 2018, Bjorn Helgaas wrote:
>>
>>> [+cc Thomas, Christoph, LKML]
>>
>> + Marc
>>
>>> On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote:
>>>> If we have a threaded interrupt with the handler being NULL, then
>>>> request_threaded_irq() -> __setup_irq() will complain and bail out
>>>> if the IRQF_ONESHOT flag isn't set. Therefore check for the handler
>>>> being NULL and set IRQF_ONESHOT in this case.
>>>>
>>>> This change is needed to migrate the mei_me driver to
>>>> pci_alloc_irq_vectors() and pci_request_irq().
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>
>>> I'd like an ack from Thomas because this requirement about IRQF_ONESHOT
>>> usage isn't mentioned in the request_threaded_irq() function doc or
>>> Documentation/
>>
>> Right. The documentation really needs some love and care. :(
>>
>> Yes, request for pure threaded interrupts are rejected if the oneshot flag
>> is not set. The reason is that this would be deadly especially with level
>> triggered interrupts because the primary default handler just wakes the
>> thread and then reenables interrupts, which will make the interrupt come
>> back immediately and the thread won't have a chance to actually shut it up
>> in the device.
>>
>> That made me look into that code again and I found that we added a flag for
>> irq chips to tell the core that the interrupt is one shot safe, i.e. that
>> it can be requested w/o IRQF_ONESHOT. That was initially added to optimize
>> MSI based interrupts which are oneshot safe by implementation.
>>
>>   dc9b229a58dc ("genirq: Allow irq chips to mark themself oneshot safe")
>>
>> The original patch added that flag to the x86 MSI irqchip code, but that
>> part was not applied for reasons which slipped from memory. It might be
>> worthwhile to revisit that in order to avoid the mask/unmask overhead for
>> such cases.
> 
> Yup, that would actually be beneficial to a range of interrupt
> controllers (only an obscure GPIO driver makes use of this flag).
> 

I'm not an expert on that area, but if MSI is oneshot-safe in general,
then we could do the following in the framework instead of touching
every MSI irq_chip. Opinions?
I tested on X86 and at least my system is still running.

diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 4ca2fd46..ba6da943 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -289,6 +289,9 @@ struct irq_domain *msi_create_irq_domain(struct fwnode_handle *fwnode,
        if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS)
                msi_domain_update_chip_ops(info);

+       /* MSI is oneshot-safe in general */
+       info->chip->flags |= IRQCHIP_ONESHOT_SAFE;
+
        domain = irq_domain_create_hierarchy(parent, IRQ_DOMAIN_FLAG_MSI, 0,
                                             fwnode, &msi_domain_ops, info);

--
2.18.0

> We could also consider extending this to support interrupt
> hierarchies, as __setup_irq() seems only concerned with the top of the
> stack (an IRQ provided by a generic MSI stack and backed by an irqchip
> providing IRQCHIP_ONESHOT_SAFE would go unnoticed).
> 

Would it be sufficient if one irq_chip in the hierarchy is oneshot-safe?
Or what do we have to check for?

> 	M.
> 

Heiner

Re: [PATCH] PCI: let pci_request_irq properly deal with threaded interrupts

[Thomas Gleixner]

On Wed, 1 Aug 2018, Heiner Kallweit wrote:
> On 31.07.2018 08:15, Marc Zyngier wrote:
> > On Mon, 30 Jul 2018 23:36:57 +0100,
> > Thomas Gleixner <tglx@linutronix.de> wrote:
> >>
> >> Yes, request for pure threaded interrupts are rejected if the oneshot flag
> >> is not set. The reason is that this would be deadly especially with level
> >> triggered interrupts because the primary default handler just wakes the
> >> thread and then reenables interrupts, which will make the interrupt come
> >> back immediately and the thread won't have a chance to actually shut it up
> >> in the device.
> >>
> >> That made me look into that code again and I found that we added a flag for
> >> irq chips to tell the core that the interrupt is one shot safe, i.e. that
> >> it can be requested w/o IRQF_ONESHOT. That was initially added to optimize
> >> MSI based interrupts which are oneshot safe by implementation.
> >>
> >>   dc9b229a58dc ("genirq: Allow irq chips to mark themself oneshot safe")
> >>
> >> The original patch added that flag to the x86 MSI irqchip code, but that
> >> part was not applied for reasons which slipped from memory. It might be
> >> worthwhile to revisit that in order to avoid the mask/unmask overhead for
> >> such cases.
> > 
> > Yup, that would actually be beneficial to a range of interrupt
> > controllers (only an obscure GPIO driver makes use of this flag).
> > 
> 
> I'm not an expert on that area, but if MSI is oneshot-safe in general,
> then we could do the following in the framework instead of touching
> every MSI irq_chip. Opinions?
> I tested on X86 and at least my system is still running.
> 
> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> index 4ca2fd46..ba6da943 100644
> --- a/kernel/irq/msi.c
> +++ b/kernel/irq/msi.c
> @@ -289,6 +289,9 @@ struct irq_domain *msi_create_irq_domain(struct fwnode_handle *fwnode,
>         if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS)
>                 msi_domain_update_chip_ops(info);
> 
> +       /* MSI is oneshot-safe in general */
> +       info->chip->flags |= IRQCHIP_ONESHOT_SAFE;
> +
>         domain = irq_domain_create_hierarchy(parent, IRQ_DOMAIN_FLAG_MSI, 0,

Looks about right, though there might be dragons. MSI is not always as sane
as it should be...

> > We could also consider extending this to support interrupt
> > hierarchies, as __setup_irq() seems only concerned with the top of the
> > stack (an IRQ provided by a generic MSI stack and backed by an irqchip
> > providing IRQCHIP_ONESHOT_SAFE would go unnoticed).
> > 
> 
> Would it be sufficient if one irq_chip in the hierarchy is oneshot-safe?
> Or what do we have to check for?

I think the top most chip is the key, the rest of the hierarchy is
irrelevant because the top most chip is the one which is responsible for
not creating an interrupt storm after the interrupt got acknowledged.

Thanks,

	tglx

Re: [PATCH] PCI: let pci_request_irq properly deal with threaded interrupts

[Heiner Kallweit]

On 03.08.2018 16:09, Thomas Gleixner wrote:
> On Wed, 1 Aug 2018, Heiner Kallweit wrote:
>> On 31.07.2018 08:15, Marc Zyngier wrote:
>>> On Mon, 30 Jul 2018 23:36:57 +0100,
>>> Thomas Gleixner <tglx@linutronix.de> wrote:
>>>>
>>>> Yes, request for pure threaded interrupts are rejected if the oneshot flag
>>>> is not set. The reason is that this would be deadly especially with level
>>>> triggered interrupts because the primary default handler just wakes the
>>>> thread and then reenables interrupts, which will make the interrupt come
>>>> back immediately and the thread won't have a chance to actually shut it up
>>>> in the device.
>>>>
>>>> That made me look into that code again and I found that we added a flag for
>>>> irq chips to tell the core that the interrupt is one shot safe, i.e. that
>>>> it can be requested w/o IRQF_ONESHOT. That was initially added to optimize
>>>> MSI based interrupts which are oneshot safe by implementation.
>>>>
>>>>   dc9b229a58dc ("genirq: Allow irq chips to mark themself oneshot safe")
>>>>
>>>> The original patch added that flag to the x86 MSI irqchip code, but that
>>>> part was not applied for reasons which slipped from memory. It might be
>>>> worthwhile to revisit that in order to avoid the mask/unmask overhead for
>>>> such cases.
>>>
>>> Yup, that would actually be beneficial to a range of interrupt
>>> controllers (only an obscure GPIO driver makes use of this flag).
>>>
>>
>> I'm not an expert on that area, but if MSI is oneshot-safe in general,
>> then we could do the following in the framework instead of touching
>> every MSI irq_chip. Opinions?
>> I tested on X86 and at least my system is still running.
>>
>> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
>> index 4ca2fd46..ba6da943 100644
>> --- a/kernel/irq/msi.c
>> +++ b/kernel/irq/msi.c
>> @@ -289,6 +289,9 @@ struct irq_domain *msi_create_irq_domain(struct fwnode_handle *fwnode,
>>         if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS)
>>                 msi_domain_update_chip_ops(info);
>>
>> +       /* MSI is oneshot-safe in general */
>> +       info->chip->flags |= IRQCHIP_ONESHOT_SAFE;
>> +
>>         domain = irq_domain_create_hierarchy(parent, IRQ_DOMAIN_FLAG_MSI, 0,
> 
> Looks about right, though there might be dragons. MSI is not always as sane
> as it should be...
> 
When saying "MSI isn't always sane", are you referring to the hardware or
the controller driver implementation? Basically for me the question is
whether we would be able to fix the issue if we meet such a dragon,
or whether we would have to revert the change.
Do you think the chance of a dragon is low enough? Or better add the
flag only to the X86 PCI MSI irqchip for now?

>>> We could also consider extending this to support interrupt
>>> hierarchies, as __setup_irq() seems only concerned with the top of the
>>> stack (an IRQ provided by a generic MSI stack and backed by an irqchip
>>> providing IRQCHIP_ONESHOT_SAFE would go unnoticed).
>>>
>>
>> Would it be sufficient if one irq_chip in the hierarchy is oneshot-safe?
>> Or what do we have to check for?
> 
> I think the top most chip is the key, the rest of the hierarchy is
> irrelevant because the top most chip is the one which is responsible for
> not creating an interrupt storm after the interrupt got acknowledged.
> 
Thanks for the explanation, then I'll submit a patch implementing
this logic.

> Thanks,
> 
> 	tglx
> 
Heiner

Re: [PATCH] PCI: let pci_request_irq properly deal with threaded interrupts

[Thomas Gleixner]

On Fri, 3 Aug 2018, Heiner Kallweit wrote:
> On 03.08.2018 16:09, Thomas Gleixner wrote:
> > On Wed, 1 Aug 2018, Heiner Kallweit wrote:
> >> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
> >> index 4ca2fd46..ba6da943 100644
> >> --- a/kernel/irq/msi.c
> >> +++ b/kernel/irq/msi.c
> >> @@ -289,6 +289,9 @@ struct irq_domain *msi_create_irq_domain(struct fwnode_handle *fwnode,
> >>         if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS)
> >>                 msi_domain_update_chip_ops(info);
> >>
> >> +       /* MSI is oneshot-safe in general */
> >> +       info->chip->flags |= IRQCHIP_ONESHOT_SAFE;
> >> +
> >>         domain = irq_domain_create_hierarchy(parent, IRQ_DOMAIN_FLAG_MSI, 0,
> > 
> > Looks about right, though there might be dragons. MSI is not always as sane
> > as it should be...
> > 
> When saying "MSI isn't always sane", are you referring to the hardware or
> the controller driver implementation? Basically for me the question is
> whether we would be able to fix the issue if we meet such a dragon,
> or whether we would have to revert the change.

It's hardware unfortunately, so it might be a revert. PCI-MSI should be
safe, but the wild MSI variants in SoCs might be the actual dragon caves.

> Do you think the chance of a dragon is low enough? Or better add the
> flag only to the X86 PCI MSI irqchip for now?

I think PCI-MSI in general would be not too risky. Famous last words.

Thanks,

	tglx

Re: [PATCH] PCI: let pci_request_irq properly deal with threaded interrupts

[Heiner Kallweit]

On 03.08.2018 21:40, Thomas Gleixner wrote:
> On Fri, 3 Aug 2018, Heiner Kallweit wrote:
>> On 03.08.2018 16:09, Thomas Gleixner wrote:
>>> On Wed, 1 Aug 2018, Heiner Kallweit wrote:
>>>> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
>>>> index 4ca2fd46..ba6da943 100644
>>>> --- a/kernel/irq/msi.c
>>>> +++ b/kernel/irq/msi.c
>>>> @@ -289,6 +289,9 @@ struct irq_domain *msi_create_irq_domain(struct fwnode_handle *fwnode,
>>>>         if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS)
>>>>                 msi_domain_update_chip_ops(info);
>>>>
>>>> +       /* MSI is oneshot-safe in general */
>>>> +       info->chip->flags |= IRQCHIP_ONESHOT_SAFE;
>>>> +
>>>>         domain = irq_domain_create_hierarchy(parent, IRQ_DOMAIN_FLAG_MSI, 0,
>>>
>>> Looks about right, though there might be dragons. MSI is not always as sane
>>> as it should be...
>>>
>> When saying "MSI isn't always sane", are you referring to the hardware or
>> the controller driver implementation? Basically for me the question is
>> whether we would be able to fix the issue if we meet such a dragon,
>> or whether we would have to revert the change.
> 
> It's hardware unfortunately, so it might be a revert. PCI-MSI should be
> safe, but the wild MSI variants in SoCs might be the actual dragon caves.
> 

pci_msi_create_irq_domain() is used by controller drivers for PCI(e)
controllers in SoCs like Mediatek, Armada, ..
I assume you're referring to some other (non-PCI) SoC-internal
MSI mechanism when saying "wild variants", right?

>> Do you think the chance of a dragon is low enough? Or better add the
>> flag only to the X86 PCI MSI irqchip for now?
> 
> I think PCI-MSI in general would be not too risky. Famous last words.
> 
> Thanks,
> 
> 	tglx
> 

Re: [PATCH] PCI: let pci_request_irq properly deal with threaded interrupts

[Bjorn Helgaas]

[+cc maintainers of possibly erroneous callers of request_threaded_irq()]

On Mon, Jul 30, 2018 at 04:30:28PM -0500, Bjorn Helgaas wrote:
> [+cc Thomas, Christoph, LKML]
> 
> On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote:
> > If we have a threaded interrupt with the handler being NULL, then
> > request_threaded_irq() -> __setup_irq() will complain and bail out
> > if the IRQF_ONESHOT flag isn't set. Therefore check for the handler
> > being NULL and set IRQF_ONESHOT in this case.
> > 
> > This change is needed to migrate the mei_me driver to
> > pci_alloc_irq_vectors() and pci_request_irq().
> > 
> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> I'd like an ack from Thomas because this requirement about IRQF_ONESHOT
> usage isn't mentioned in the request_threaded_irq() function doc or
> Documentation/

Possibly these other request_threaded_irq() callers are similarly
broken?  I can't tell for sure about tda998x_create(), but all the
others certainly call request_threaded_irq() with "handler == NULL"
and irqflags that do not contain IRQF_ONESHOT:

  max8997_muic_probe()
    request_threaded_irq(virq, NULL, ..., IRQF_NO_SUSPEND, ...)

  tda998x_create()
    request_threaded_irq(client->irq, NULL, ..., irqd_get_trigger_type(), ...)
    (I can't tell what irqd_get_trigger_type() does)

  ab8500_btemp_probe()
  ab8500_charger_probe()
    request_threaded_irq(irq, NULL, ..., IRQF_SHARED | IRQF_NO_SUSPEND, ...)

  lp8788_set_irqs()
    request_threaded_irq(virq, NULL, ..., 0, ...)

  max77686_rtc_probe()
    request_threaded_irq(info->virq, NULL, ..., 0, ...)

  wm8350_register_irq()
    request_threaded_irq(irq + wm8350->irq_base, NULL, ..., flags, ...)
    (I think all callers of wm8350_register_irq() supply 0 for "flags")

> > ---
> >  drivers/pci/irq.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/irq.c b/drivers/pci/irq.c
> > index 2a808e10..a1de501a 100644
> > --- a/drivers/pci/irq.c
> > +++ b/drivers/pci/irq.c
> > @@ -86,13 +86,17 @@ int pci_request_irq(struct pci_dev *dev, unsigned int nr, irq_handler_t handler,
> >  	va_list ap;
> >  	int ret;
> >  	char *devname;
> > +	unsigned long irqflags = IRQF_SHARED;
> > +
> > +	if (!handler)
> > +		irqflags |= IRQF_ONESHOT;
> >  
> >  	va_start(ap, fmt);
> >  	devname = kvasprintf(GFP_KERNEL, fmt, ap);
> >  	va_end(ap);
> >  
> >  	ret = request_threaded_irq(pci_irq_vector(dev, nr), handler, thread_fn,
> > -			IRQF_SHARED, devname, dev_id);
> > +				   irqflags, devname, dev_id);
> >  	if (ret)
> >  		kfree(devname);
> >  	return ret;
> > -- 
> > 2.18.0
> > 

Re: [PATCH] PCI: let pci_request_irq properly deal with threaded interrupts

[Thomas Gleixner]

On Mon, 30 Jul 2018, Bjorn Helgaas wrote:

> [+cc maintainers of possibly erroneous callers of request_threaded_irq()]
> 
> On Mon, Jul 30, 2018 at 04:30:28PM -0500, Bjorn Helgaas wrote:
> > [+cc Thomas, Christoph, LKML]
> > 
> > On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote:
> > > If we have a threaded interrupt with the handler being NULL, then
> > > request_threaded_irq() -> __setup_irq() will complain and bail out
> > > if the IRQF_ONESHOT flag isn't set. Therefore check for the handler
> > > being NULL and set IRQF_ONESHOT in this case.
> > > 
> > > This change is needed to migrate the mei_me driver to
> > > pci_alloc_irq_vectors() and pci_request_irq().
> > > 
> > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > 
> > I'd like an ack from Thomas because this requirement about IRQF_ONESHOT
> > usage isn't mentioned in the request_threaded_irq() function doc or
> > Documentation/
> 
> Possibly these other request_threaded_irq() callers are similarly
> broken?  I can't tell for sure about tda998x_create(), but all the
> others certainly call request_threaded_irq() with "handler == NULL"
> and irqflags that do not contain IRQF_ONESHOT:
> 
>   max8997_muic_probe()
>     request_threaded_irq(virq, NULL, ..., IRQF_NO_SUSPEND, ...)
> 
>   tda998x_create()
>     request_threaded_irq(client->irq, NULL, ..., irqd_get_trigger_type(), ...)
>     (I can't tell what irqd_get_trigger_type() does)

It reads the trigger type back from the irq chip (level/edge/polarity) but
does not return with the ONESHOT bit set.

>   ab8500_btemp_probe()
>   ab8500_charger_probe()
>     request_threaded_irq(irq, NULL, ..., IRQF_SHARED | IRQF_NO_SUSPEND, ...)

SHARED is interesting ....

>   lp8788_set_irqs()
>     request_threaded_irq(virq, NULL, ..., 0, ...)
> 
>   max77686_rtc_probe()
>     request_threaded_irq(info->virq, NULL, ..., 0, ...)
> 
>   wm8350_register_irq()
>     request_threaded_irq(irq + wm8350->irq_base, NULL, ..., flags, ...)
>     (I think all callers of wm8350_register_irq() supply 0 for "flags")

Indeed. This all looks pretty much wrong. No idea why nobody ever noticed.

Thanks,

	tglx

Re: [PATCH] PCI: let pci_request_irq properly deal with threaded interrupts

[Lee Jones]

On Tue, 31 Jul 2018, Thomas Gleixner wrote:

> On Mon, 30 Jul 2018, Bjorn Helgaas wrote:
> 
> > [+cc maintainers of possibly erroneous callers of request_threaded_irq()]
> > 
> > On Mon, Jul 30, 2018 at 04:30:28PM -0500, Bjorn Helgaas wrote:
> > > [+cc Thomas, Christoph, LKML]
> > > 
> > > On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote:
> > > > If we have a threaded interrupt with the handler being NULL, then
> > > > request_threaded_irq() -> __setup_irq() will complain and bail out
> > > > if the IRQF_ONESHOT flag isn't set. Therefore check for the handler
> > > > being NULL and set IRQF_ONESHOT in this case.
> > > > 
> > > > This change is needed to migrate the mei_me driver to
> > > > pci_alloc_irq_vectors() and pci_request_irq().
> > > > 
> > > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > > 
> > > I'd like an ack from Thomas because this requirement about IRQF_ONESHOT
> > > usage isn't mentioned in the request_threaded_irq() function doc or
> > > Documentation/
> > 
> > Possibly these other request_threaded_irq() callers are similarly
> > broken?  I can't tell for sure about tda998x_create(), but all the
> > others certainly call request_threaded_irq() with "handler == NULL"
> > and irqflags that do not contain IRQF_ONESHOT:
> > 
> >   max8997_muic_probe()
> >     request_threaded_irq(virq, NULL, ..., IRQF_NO_SUSPEND, ...)
> > 
> >   tda998x_create()
> >     request_threaded_irq(client->irq, NULL, ..., irqd_get_trigger_type(), ...)
> >     (I can't tell what irqd_get_trigger_type() does)
> 
> It reads the trigger type back from the irq chip (level/edge/polarity) but
> does not return with the ONESHOT bit set.
> 
> >   ab8500_btemp_probe()
> >   ab8500_charger_probe()
> >     request_threaded_irq(irq, NULL, ..., IRQF_SHARED | IRQF_NO_SUSPEND, ...)
> 
> SHARED is interesting ....
> 
> >   lp8788_set_irqs()
> >     request_threaded_irq(virq, NULL, ..., 0, ...)
> > 
> >   max77686_rtc_probe()
> >     request_threaded_irq(info->virq, NULL, ..., 0, ...)
> > 
> >   wm8350_register_irq()
> >     request_threaded_irq(irq + wm8350->irq_base, NULL, ..., flags, ...)
> >     (I think all callers of wm8350_register_irq() supply 0 for "flags")
> 
> Indeed. This all looks pretty much wrong. No idea why nobody ever noticed.

I guess not many people (myself included) know the intricacies well
enough to have noticed.  This probably coupled with insufficient
checking/warning about such practices in the IRQ subsystem.

There are quite a few different devices here, so my assumption would
be that interrupt handling still works in these cases.

If we know certain configurations are incorrect or incompatible, might
I suggest we add some form of alert to bring these to the attention of
unknowing contributors?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] PCI: let pci_request_irq properly deal with threaded interrupts

[Russell King - ARM Linux]

On Tue, Jul 31, 2018 at 12:50:21AM +0200, Thomas Gleixner wrote:
> On Mon, 30 Jul 2018, Bjorn Helgaas wrote:
> 
> > [+cc maintainers of possibly erroneous callers of request_threaded_irq()]
> > 
> > On Mon, Jul 30, 2018 at 04:30:28PM -0500, Bjorn Helgaas wrote:
> > > [+cc Thomas, Christoph, LKML]
> > > 
> > > On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote:
> > > > If we have a threaded interrupt with the handler being NULL, then
> > > > request_threaded_irq() -> __setup_irq() will complain and bail out
> > > > if the IRQF_ONESHOT flag isn't set. Therefore check for the handler
> > > > being NULL and set IRQF_ONESHOT in this case.
> > > > 
> > > > This change is needed to migrate the mei_me driver to
> > > > pci_alloc_irq_vectors() and pci_request_irq().
> > > > 
> > > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > > 
> > > I'd like an ack from Thomas because this requirement about IRQF_ONESHOT
> > > usage isn't mentioned in the request_threaded_irq() function doc or
> > > Documentation/
> > 
> > Possibly these other request_threaded_irq() callers are similarly
> > broken?  I can't tell for sure about tda998x_create(), but all the
> > others certainly call request_threaded_irq() with "handler == NULL"
> > and irqflags that do not contain IRQF_ONESHOT:
> > 
> >   tda998x_create()
> >     request_threaded_irq(client->irq, NULL, ..., irqd_get_trigger_type(), ...)
> >     (I can't tell what irqd_get_trigger_type() does)
> 
> It reads the trigger type back from the irq chip (level/edge/polarity) but
> does not return with the ONESHOT bit set.

What tree are you (Bjorn) using there?  Looking in my git history,
request_threaded_irq() was added in January 2014, as:

+               irqf_trigger =
+                       irqd_get_trigger_type(irq_get_irq_data(client->irq));
+               ret = request_threaded_irq(client->irq, NULL,
+                                          tda998x_irq_thread,
+                                          irqf_trigger | IRQF_ONESHOT,
+                                          "tda998x", priv);

and updated more recently for the CEC support in ae81553c30ef
("drm/i2c: tda998x: allow interrupt to be shared") to be:

-               irqf_trigger =
+               irq_flags =
                        irqd_get_trigger_type(irq_get_irq_data(client->irq));
+               irq_flags |= IRQF_SHARED | IRQF_ONESHOT;
                ret = request_threaded_irq(client->irq, NULL,
-                                          tda998x_irq_thread,
-                                          irqf_trigger | IRQF_ONESHOT,
+                                          tda998x_irq_thread, irq_flags,
                                           "tda998x", priv);

In all cases, IRQF_ONESHOT has been set for this threaded interrupt.

I've also checked drm-next, and it doesn't show anything like what
Bjorn quoted.

Confused.  Bjorn, can you double check that what you think are
erroneous request_threaded_irq() calls are actually as per what
you quoted please?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

Re: [PATCH] PCI: let pci_request_irq properly deal with threaded interrupts

[Bjorn Helgaas]

On Tue, Jul 31, 2018 at 09:01:12AM +0100, Russell King - ARM Linux wrote:
> On Tue, Jul 31, 2018 at 12:50:21AM +0200, Thomas Gleixner wrote:
> > On Mon, 30 Jul 2018, Bjorn Helgaas wrote:
> > 
> > > [+cc maintainers of possibly erroneous callers of request_threaded_irq()]
> > > 
> > > On Mon, Jul 30, 2018 at 04:30:28PM -0500, Bjorn Helgaas wrote:
> > > > [+cc Thomas, Christoph, LKML]
> > > > 
> > > > On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote:
> > > > > If we have a threaded interrupt with the handler being NULL, then
> > > > > request_threaded_irq() -> __setup_irq() will complain and bail out
> > > > > if the IRQF_ONESHOT flag isn't set. Therefore check for the handler
> > > > > being NULL and set IRQF_ONESHOT in this case.
> > > > > 
> > > > > This change is needed to migrate the mei_me driver to
> > > > > pci_alloc_irq_vectors() and pci_request_irq().
> > > > > 
> > > > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > > > 
> > > > I'd like an ack from Thomas because this requirement about IRQF_ONESHOT
> > > > usage isn't mentioned in the request_threaded_irq() function doc or
> > > > Documentation/
> > > 
> > > Possibly these other request_threaded_irq() callers are similarly
> > > broken?  I can't tell for sure about tda998x_create(), but all the
> > > others certainly call request_threaded_irq() with "handler == NULL"
> > > and irqflags that do not contain IRQF_ONESHOT:
> > > 
> > >   tda998x_create()
> > >     request_threaded_irq(client->irq, NULL, ..., irqd_get_trigger_type(), ...)
> > >     (I can't tell what irqd_get_trigger_type() does)
> > 
> > It reads the trigger type back from the irq chip (level/edge/polarity) but
> > does not return with the ONESHOT bit set.
> 
> What tree are you (Bjorn) using there?  Looking in my git history,
> request_threaded_irq() was added in January 2014, as:
> 
> +               irqf_trigger =
> +                       irqd_get_trigger_type(irq_get_irq_data(client->irq));
> +               ret = request_threaded_irq(client->irq, NULL,
> +                                          tda998x_irq_thread,
> +                                          irqf_trigger | IRQF_ONESHOT,
> +                                          "tda998x", priv);
> 
> and updated more recently for the CEC support in ae81553c30ef
> ("drm/i2c: tda998x: allow interrupt to be shared") to be:
> 
> -               irqf_trigger =
> +               irq_flags =
>                         irqd_get_trigger_type(irq_get_irq_data(client->irq));
> +               irq_flags |= IRQF_SHARED | IRQF_ONESHOT;
>                 ret = request_threaded_irq(client->irq, NULL,
> -                                          tda998x_irq_thread,
> -                                          irqf_trigger | IRQF_ONESHOT,
> +                                          tda998x_irq_thread, irq_flags,
>                                            "tda998x", priv);
> 
> In all cases, IRQF_ONESHOT has been set for this threaded interrupt.
> 
> I've also checked drm-next, and it doesn't show anything like what
> Bjorn quoted.
> 
> Confused.  Bjorn, can you double check that what you think are
> erroneous request_threaded_irq() calls are actually as per what
> you quoted please?

I was looking at v4.18-rc1, but in the case of tda998x_create(), I
overlooked this line:

  irq_flags |= IRQF_SHARED | IRQF_ONESHOT;

Sorry!

Re: [PATCH] PCI: let pci_request_irq properly deal with threaded interrupts

[Krzysztof Kozlowski]

On 31 July 2018 at 00:42, Bjorn Helgaas <helgaas@kernel.org> wrote:
> [+cc maintainers of possibly erroneous callers of request_threaded_irq()]
>
> On Mon, Jul 30, 2018 at 04:30:28PM -0500, Bjorn Helgaas wrote:
>> [+cc Thomas, Christoph, LKML]
>>
>> On Mon, Jul 30, 2018 at 12:03:42AM +0200, Heiner Kallweit wrote:
>> > If we have a threaded interrupt with the handler being NULL, then
>> > request_threaded_irq() -> __setup_irq() will complain and bail out
>> > if the IRQF_ONESHOT flag isn't set. Therefore check for the handler
>> > being NULL and set IRQF_ONESHOT in this case.
>> >
>> > This change is needed to migrate the mei_me driver to
>> > pci_alloc_irq_vectors() and pci_request_irq().
>> >
>> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>
>> I'd like an ack from Thomas because this requirement about IRQF_ONESHOT
>> usage isn't mentioned in the request_threaded_irq() function doc or
>> Documentation/
>
> Possibly these other request_threaded_irq() callers are similarly
> broken?  I can't tell for sure about tda998x_create(), but all the
> others certainly call request_threaded_irq() with "handler == NULL"
> and irqflags that do not contain IRQF_ONESHOT:
>
>   max8997_muic_probe()
>     request_threaded_irq(virq, NULL, ..., IRQF_NO_SUSPEND, ...)
>
>   tda998x_create()
>     request_threaded_irq(client->irq, NULL, ..., irqd_get_trigger_type(), ...)
>     (I can't tell what irqd_get_trigger_type() does)
>
>   ab8500_btemp_probe()
>   ab8500_charger_probe()
>     request_threaded_irq(irq, NULL, ..., IRQF_SHARED | IRQF_NO_SUSPEND, ...)
>
>   lp8788_set_irqs()
>     request_threaded_irq(virq, NULL, ..., 0, ...)
>
>   max77686_rtc_probe()
>     request_threaded_irq(info->virq, NULL, ..., 0, ...)

max77686 works fine because it it is nested IRQ. Parent has ONESHOT flag set.

Best regards,
Krzysztof