2011-06-02 17:55:18

by Mark Brown

[permalink] [raw]
Subject: [PATCH] genirq: Ensure we locate the passed IRQ in irq_alloc_descs()

When irq_alloc_descs() is called with no base IRQ specified then it will
search for a range of IRQs starting from a specified base address. In the
case where an IRQ is specified it still does this search in order to ensure
that none of the requested range is already allocated and it still uses the
from parameter to specify the base for the search. This means that in the
case where a base is specified but from is zero (which is reasonable as
any IRQ number is in the range specified by a zero from) the function will
get confused and try to allocate the first suitably sized block of free IRQs
it finds.

Instead use a specified IRQ as the base address for the search, and insist
that any from that is specified can support that IRQ.

Signed-off-by: Mark Brown <[email protected]>
---
kernel/irq/irqdesc.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 886e803..bb53d6c 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -346,6 +346,12 @@ irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node)
if (!cnt)
return -EINVAL;

+ if (irq >= 0) {
+ if (from > irq)
+ return -EINVAL;
+ from = irq;
+ }
+
mutex_lock(&sparse_irq_lock);

start = bitmap_find_next_zero_area(allocated_irqs, IRQ_BITMAP_BITS,
--
1.7.5.3


2011-06-03 09:24:28

by Milton Miller

[permalink] [raw]
Subject: Re: genirq: Ensure we locate the passed IRQ in irq_alloc_descs()

On Thu, 02 Jun 2011 17:55:13 -0000, Mark Brown wrote:
> When irq_alloc_descs() is called with no base IRQ specified then it will
> search for a range of IRQs starting from a specified base address. In the
> case where an IRQ is specified it still does this search in order to ensure
> that none of the requested range is already allocated and it still uses the
> from parameter to specify the base for the search. This means that in the
> case where a base is specified but from is zero (which is reasonable as
> any IRQ number is in the range specified by a zero from) the function will
> get confused and try to allocate the first suitably sized block of free IRQs
> it finds.
>
> Instead use a specified IRQ as the base address for the search, and insist
> that any from that is specified can support that IRQ.
>
> Signed-off-by: Mark Brown <[email protected]>
>
> ---
> kernel/irq/irqdesc.c | 6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index 886e803..bb53d6c 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -346,6 +346,12 @@ irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node)
> if (!cnt)
> return -EINVAL;
>
> + if (irq >= 0) {
> + if (from > irq)
> + return -EINVAL;
> + from = irq;
> + }
> +
> mutex_lock(&sparse_irq_lock);
>
> start = bitmap_find_next_zero_area(allocated_irqs, IRQ_BITMAP_BITS,

and then right after this the code continues:

ret = -EEXIST;
if (irq >=0 && start != irq)
goto err;


This patch enables exactly the calls I want to forbid ! Why do
you need to verify that there are no irqs between from and irq ?
What is your use case?

Change your caller to specify the irq twice if you need a specific irq
block, or if you only need one then use the helper irq_alloc_desc_at.

If you want to change irq_alloc_descs, please make it return -EINVAL
if irq >=0 && from != irq (like I did).

See http://lkml.indiana.edu/hypermail/linux/kernel/1105.3/00739.html
[PATCH RFC 4/4] irq: allow a per-allocation upper limit when allocating irqs

(and yes, I have made the changes based on the feedback but haven't
had time to get back to the series).

Thanks,
milton
QUIT

2011-06-03 10:42:21

by Mark Brown

[permalink] [raw]
Subject: Re: genirq: Ensure we locate the passed IRQ in irq_alloc_descs()

On Fri, Jun 03, 2011 at 04:24:02AM -0500, Milton Miller wrote:
> On Thu, 02 Jun 2011 17:55:13 -0000, Mark Brown wrote:

> > start = bitmap_find_next_zero_area(allocated_irqs, IRQ_BITMAP_BITS,

> and then right after this the code continues:

> ret = -EEXIST;
> if (irq >=0 && start != irq)
> goto err;

> This patch enables exactly the calls I want to forbid ! Why do

Which you wish to forbid because...? You've not articulated any
motivation for doing this which makes it rather hard to engage here.

> you need to verify that there are no irqs between from and irq ?

I don't care if there are IRQs between from and the specified irq, all I
care about is that we get back the irq we requested (apart from anything
else the function will later error out if the allocated IRQ doesn't
match the one that was specified - it seems very clear from both the
code and documentation that if an IRQ is specified we're supposed to get
it back).

- The specified IRQ is ignored except

> What is your use case?

I've requested a base IRQ but the only attention that irq_alloc_descs()
is paying to it is that it generates an error rather than allocating
something

> Change your caller to specify the irq twice if you need a specific irq

This seems like a poor UI for the function, if the user specified an
irq_base and there's a suitable range of IRQs available at that base
what is the benefit in refusing to allocate there? That's just going to
make the system fragile against init ordering and driver disabling.

It's also going to be a bit more cumbersome to use:

if (pdata->irq_base > 0) {
irq_base = pdata->irq_base;
from = pdata->irq_base;
} else {
irq_base = -1;
from = 0;
}

> block, or if you only need one then use the helper irq_alloc_desc_at.

I need about 60 IRQs in the particular driver where I noticed this.

> If you want to change irq_alloc_descs, please make it return -EINVAL
> if irq >=0 && from != irq (like I did).

> See http://lkml.indiana.edu/hypermail/linux/kernel/1105.3/00739.html
> [PATCH RFC 4/4] irq: allow a per-allocation upper limit when allocating irqs

> (and yes, I have made the changes based on the feedback but haven't

I don't really see the relevance of this patch? You're adding
functionality for limiting the maximum IRQ number allocated which seems
orthogonal to the issue.

2011-06-03 12:59:18

by Mark Brown

[permalink] [raw]
Subject: [tip:irq/urgent] genirq: Ensure we locate the passed IRQ in irq_alloc_descs()

Commit-ID: c5182b8867e189e14a8df5dbfcba1c73f286e061
Gitweb: http://git.kernel.org/tip/c5182b8867e189e14a8df5dbfcba1c73f286e061
Author: Mark Brown <[email protected]>
AuthorDate: Thu, 2 Jun 2011 18:55:13 +0100
Committer: Thomas Gleixner <[email protected]>
CommitDate: Fri, 3 Jun 2011 14:53:16 +0200

genirq: Ensure we locate the passed IRQ in irq_alloc_descs()

When irq_alloc_descs() is called with no base IRQ specified then it will
search for a range of IRQs starting from a specified base address. In the
case where an IRQ is specified it still does this search in order to ensure
that none of the requested range is already allocated and it still uses the
from parameter to specify the base for the search. This means that in the
case where a base is specified but from is zero (which is reasonable as
any IRQ number is in the range specified by a zero from) the function will
get confused and try to allocate the first suitably sized block of free IRQs
it finds.

Instead use a specified IRQ as the base address for the search, and insist
that any from that is specified can support that IRQ.

Signed-off-by: Mark Brown <[email protected]>
Link: http://lkml.kernel.org/r/1307037313-15733-1-git-send-email-broonie@opensource.wolfsonmicro.com
Signed-off-by: Thomas Gleixner <[email protected]>
---
kernel/irq/irqdesc.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index c7ddc87..4c60a50 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -344,6 +344,12 @@ irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node)
if (!cnt)
return -EINVAL;

+ if (irq >= 0) {
+ if (from > irq)
+ return -EINVAL;
+ from = irq;
+ }
+
mutex_lock(&sparse_irq_lock);

start = bitmap_find_next_zero_area(allocated_irqs, IRQ_BITMAP_BITS,