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

[Milton Miller <miltonm@bga.com>]

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

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