From: Milton Miller <miltonm@bga.com>
To: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org
Subject: Re: genirq: Ensure we locate the passed IRQ in irq_alloc_descs()
Date: Fri, 03 Jun 2011 09:43:42 -0500 [thread overview]
Message-ID: <locate-from-more@mdm.bga.com> (raw)
In-Reply-To: 20110603104217.GA4777@opensource.wolfsonmicro.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
next prev parent reply other threads:[~2011-06-03 14:43 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-06-02 17:55 [PATCH] genirq: Ensure we locate the passed IRQ in irq_alloc_descs() Mark Brown
2011-06-03 9:24 ` Milton Miller
2011-06-03 10:42 ` Mark Brown
2011-06-03 14:43 ` Milton Miller [this message]
2011-06-03 15:06 ` Mark Brown
2011-06-03 16:25 ` Thomas Gleixner
2011-06-04 0:23 ` Milton Miller
2011-06-04 9:51 ` Mark Brown
2011-06-06 19:42 ` Grant Likely
2011-06-06 20:57 ` Mark Brown
2011-06-06 21:33 ` Grant Likely
2011-06-03 12:59 ` [tip:irq/urgent] " tip-bot for Mark Brown
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=locate-from-more@mdm.bga.com \
--to=miltonm@bga.com \
--cc=broonie@opensource.wolfsonmicro.com \
--cc=linux-kernel@vger.kernel.org \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).