[PATCH] 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


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/l...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
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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/l...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.
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Commit-ID: c5182b8867e189e14a8df5dbfcba1c73f286e061
Gitweb: http://git.kernel.org/tip/c5182b886...73f286e061
Author: Mark Brown
AuthorDate: Thu, 2 Jun 2011 18:55:13 +0100
Committer: Thomas Gleixner
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
Link: http://lkml.kernel.org/r/
Signed-off-by: Thomas Gleixner

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,
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Fri, 3 Jun 2011 about 11:42:17 +0100, Mark Brown wrote:

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.

In 2.6.39 all calls to irq_alloc_descs were from the helpers. Either
from irq_alloc_descriptor_at , which says "I need this exact irq",
or from irq_alloc_desc, which says "give me any irq".

I treated the arguments to irq_alloc_descs as having grown to
accomidate the two uses having a common allocator with the partially
redunant encoding. In one case an exact irq was specified (irq >= 0),
and one that allocates from anywhere (irq < 0, all callers passed -1).

Maybe you have a new case.

> 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

Do you need a specific irq or an allocated one?

Or do you have a case where you don't know?

> 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.

I am thinking two use cases: dynamic in a range, and pre specified
(often by an intermediate layer).

But your example seems to imply a driver used in different environments:

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.

Do you need a block of 60? or just 60 somewhere?

How do you know from = 0 is safe?

> 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/l...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.

Its relavant in that irq_alloc_descs_range no longer gets both irq and from;
the information is passed to the underling allocator in a different form.

Ie dynamic allocations will be in [x, y), static will be at x.

It may be possible to still pass this information without additional
arguments but I haven't had time to think about it yet.


I'm attending a conference and am quite busy but will see if I can
spend some break time on this.

milton
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Fri, Jun 03, 2011 at 09:43:42AM -0500, Milton Miller wrote:

I treated the arguments to irq_alloc_descs as having grown to
accomidate the two uses having a common allocator with the partially
redunant encoding. In one case an exact irq was specified (irq >= 0),
and one that allocates from anywhere (irq < 0, all callers passed -1).

Maybe you have a new case.

No, I'm only aware of those two cases. All my change does is make the
irq parameter be enough to select between the two - at the minute it's
just too weak.

Do you need a specific irq or an allocated one?

Or do you have a case where you don't know?

I need either a specific IRQ or an allocated one. This is just a very
standard driver with an interrupt controller (well, there's a bunch of
devices that are going to be doing the same thing - it's far from just
one driver), it doesn't care what base it gets but systems can specify a
base if they care for the externally visible interrupts (so that they
can be supplied to other devices or whatever).

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

Do you need a block of 60? or just 60 somewhere?

The driver assumes it's going to get a contiguous range, it'd be a lot
of bookkeeping for no gain to have to cope with them being splattered
all over the place.

How do you know from = 0 is safe?

If the user cares they can just pick a number for the base; if they're
going to pick a number they may as well pick the actual number.

> 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.

Its relavant in that irq_alloc_descs_range no longer gets both irq and from;
the information is passed to the underling allocator in a different form.

That's not the goal of the patch, it's just something the patch happens
to do as part of the implementation as far as I can see.
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/