[PATCH] block/cpqarray.c: remove IRQF_DISABLED flag

[PATCH] block/cpqarray.c: remove IRQF_DISABLED flag

[Valentin Rothberg]

The IRQF_DISABLED is a NOOP and scheduled to be removed.  According to
Ingo Molnar (e58aa3d2d0cc01ad8d6f7f640a0670433f794922) running IRQ
handlers with interrupts enabled can cause stack overflows when the
interrupt line of the issuing device is still active.

Signed-off-by: Valentin Rothberg <Valentin.Rothberg@lip6.fr>
---
 drivers/block/cpqarray.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/cpqarray.c b/drivers/block/cpqarray.c
index 2b94403..9e92b2c 100644
--- a/drivers/block/cpqarray.c
+++ b/drivers/block/cpqarray.c
@@ -405,8 +405,8 @@ static int cpqarray_register_ctlr(int i, struct pci_dev *pdev)
 		goto Enomem4;
 	}
 	hba[i]->access.set_intr_mask(hba[i], 0);
-	if (request_irq(hba[i]->intr, do_ida_intr,
-		IRQF_DISABLED|IRQF_SHARED, hba[i]->devname, hba[i]))
+	if (request_irq(hba[i]->intr, do_ida_intr, IRQF_SHARED,
+		hba[i]->devname, hba[i]))
 	{
 		printk(KERN_ERR "cpqarray: Unable to get irq %d for %s\n",
 				hba[i]->intr, hba[i]->devname);
-- 
1.9.1

Re: [PATCH] block/cpqarray.c: remove IRQF_DISABLED flag

[Andrew Morton]

On Fri, 27 Feb 2015 13:32:55 +0100 Valentin Rothberg <Valentin.Rothberg@lip6.fr> wrote:

> The IRQF_DISABLED is a NOOP and scheduled to be removed.  According to
> Ingo Molnar (e58aa3d2d0cc01ad8d6f7f640a0670433f794922) running IRQ
> handlers with interrupts enabled can cause stack overflows when the
> interrupt line of the issuing device is still active.
> 

I suggest you prepare a patch which removes IRQF_DISABLED entirely. 
Several drivers still use it and it is possible that they have been
buggy for some time, so we should be careful to cc the relevant
maintainers (they probably don't exist) so they can check out what's
going on in their code.

Re: [PATCH] block/cpqarray.c: remove IRQF_DISABLED flag

[Valentin Rothberg]

On Mon, Mar 2, 2015 at 10:56 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Fri, 27 Feb 2015 13:32:55 +0100 Valentin Rothberg <Valentin.Rothberg@lip6.fr> wrote:
>
>> The IRQF_DISABLED is a NOOP and scheduled to be removed.  According to
>> Ingo Molnar (e58aa3d2d0cc01ad8d6f7f640a0670433f794922) running IRQ
>> handlers with interrupts enabled can cause stack overflows when the
>> interrupt line of the issuing device is still active.
>>
>
> I suggest you prepare a patch which removes IRQF_DISABLED entirely.
> Several drivers still use it and it is possible that they have been
> buggy for some time, so we should be careful to cc the relevant
> maintainers (they probably don't exist) so they can check out what's
> going on in their code.

That's a good idea.  I sent some patches that remove the usage of
IRQF_DISABLED during the last days.  Some of them have been applied,
others are pending.  I may wait a few days to receive answers.

Shall I make a patch series (i.e., one patch removes the definition of
IRQF_DISABLED and then one patch per usage)?

Thank you,
 Valentin

Re: [PATCH] block/cpqarray.c: remove IRQF_DISABLED flag

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Fri, 27 Feb 2015 13:32:55 +0100 Valentin Rothberg <Valentin.Rothberg@lip6.fr> wrote:
> 
> > The IRQF_DISABLED is a NOOP and scheduled to be removed.  According to
> > Ingo Molnar (e58aa3d2d0cc01ad8d6f7f640a0670433f794922) running IRQ
> > handlers with interrupts enabled can cause stack overflows when the
> > interrupt line of the issuing device is still active.
> > 
> 
> I suggest you prepare a patch which removes IRQF_DISABLED entirely. 

Yes.

> Several drivers still use it and it is possible that they have been 
> buggy for some time, [...]

Well, IRQF_DISABLED is now the unconditional default, so requesting 
irqs with IRQF_DISABLED is simply superfluous, not buggy, AFAICS.

> [...] so we should be careful to cc the relevant maintainers (they 
> probably don't exist) so they can check out what's going on in their 
> code.

I think part of it might be the stale spinlock related comment in 
Documentation/PCI/MSI-HOWTO.txt which explicitly recommends 
IRQF_DISABLED use. So that reference should be zapped as well.

So maybe drivers still using IRQF_DISABLED might not be potentially 
broken drivers at all, but are drivers written by exceptionally 
capable driver authors, who read kernel documentation!

Thanks,

	Ingo

Re: [PATCH] block/cpqarray.c: remove IRQF_DISABLED flag

[Valentin Rothberg]

On Tue, Mar 3, 2015 at 9:37 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Andrew Morton <akpm@linux-foundation.org> wrote:
>
>> On Fri, 27 Feb 2015 13:32:55 +0100 Valentin Rothberg <Valentin.Rothberg@lip6.fr> wrote:
>>
>> > The IRQF_DISABLED is a NOOP and scheduled to be removed.  According to
>> > Ingo Molnar (e58aa3d2d0cc01ad8d6f7f640a0670433f794922) running IRQ
>> > handlers with interrupts enabled can cause stack overflows when the
>> > interrupt line of the issuing device is still active.
>> >
>>
>> I suggest you prepare a patch which removes IRQF_DISABLED entirely.
>
> Yes.
>
>> Several drivers still use it and it is possible that they have been
>> buggy for some time, [...]
>
> Well, IRQF_DISABLED is now the unconditional default, so requesting
> irqs with IRQF_DISABLED is simply superfluous, not buggy, AFAICS.
>
>> [...] so we should be careful to cc the relevant maintainers (they
>> probably don't exist) so they can check out what's going on in their
>> code.
>
> I think part of it might be the stale spinlock related comment in
> Documentation/PCI/MSI-HOWTO.txt which explicitly recommends
> IRQF_DISABLED use. So that reference should be zapped as well.

I sent a patch that fixes that already.  See commit
b0e1ee8e1405ae033a0eac01ead275e65e4e6831 ("MSI-HOWTO.txt: remove
reference on IRQF_DISABLED").

> So maybe drivers still using IRQF_DISABLED might not be potentially
> broken drivers at all, but are drivers written by exceptionally
> capable driver authors, who read kernel documentation!

In the context of my PhD, I am diving a little into interrupt handling
in Linux.  I feel that a lot of information is scattered over
documentation, source code comments or no comments at all (i.e., the
code itself).  Currently I am writing a document (from hard IRQs to
workqueues).  As soon as it's finished I will think about how and
where it could be useful in documentation.

Kind regards,
 Valentin

> Thanks,
>
>         Ingo