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
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.
On Fri, 3 Jun 2011, Milton Miller wrote:
> 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".
That does not prevent a caller with uses irq_alloc_descs() directly
with the wrong arguments. So we want to sanity check this.
Thanks,
tglx
[Replying to both Mark and Thomas]
On Fri, 3 Jun 2011 about 16:06:36 +0100, Mark Brown wrote:
> 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.
Actually I do consider this a new case. You are implementing some kind
of cascade, and your code does not know (or want to care) if it can use
any irq or needs a platform selected irq block. The decision of "Is
this system fully enabled for dynamic irq selection?" is not in the
code but in some externally supplied data.
>
> > 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
I don't know that I'd call an interrupt controller a standard driver
today, but agree we are heading towards that.
> > 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.
The irq domain concept addresses mapping, but a simple fully allocated
block will be one of the options. Having 60 irqdesc might be
pushing the limit for some platforms but that can be addressed later
when irq domains are upstream.
> > 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'd argue the user is the wrong level to make this decision. However,
saying the platform decides is valid, and the excerpt you had earlier
implied you were getting the irq argument from platform data, not the
user (eg via a module parameter)..
Also, read the last five paragraphs below.
> > > 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.
Well, eliminating the redundant information was a secondary goal.
On Fri, 3 Jun 2011 about 18:25:01 +0200 (CEST), Thomas Gleixner wrote:
> On Fri, 3 Jun 2011, Milton Miller wrote:
> > 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".
>
> That does not prevent a caller with uses irq_alloc_descs() directly
> with the wrong arguments. So we want to sanity check this.
I wasn't debating a sanity check. I was claiming that the incoming
argument combination was not sane and should be outright rejected
instead of given new meaning and fixed up.
As I said earlier, I saw two valid use cases: the caller needed a
fixed irq, and the caller didn't care about the irq. However, upon
the further discussion, it does appear that Mark has a third case:
the driver can handle either; the choice is up to the platform.
With the further discussion, and some time to sleep and reflect on
what the patch actually acomplishes and the use case, I withdraw
my objection. The patch changes the semantics of the case irq >= 0
and from != irq, but it defines it in a meaningful way that removes
the current insane combindation of events required for a successful
allocation.
Acked-by: Milton Miller <[email protected]>
I should point out that several archtectures currently allocate irqs
in a architecture layer before calling the irq allocator asking for
the exact irq they choose on what they thought was free. Among these
are x86 and powerpc. Not calling the architecture layer will causein
all future allocations by other drivers using the architecture layer
to fail..
That means that any driver allocating irqs directly is not generic if
it actually allocates irqs unconditionally! Instead it will cause
hotplug or even boot failures (if it allocates irqs too early).
I have a series in mind, inspired by Grant's initial proposal, to
change the master allocation for powerpc (and export its helpers as irq
domains), although I haven't been able to work on it for the last week
or so.
An alternative, that can be implemented easily today, is to add the
ability to specify from above what the architecture layer manages.
(eg depend on SPARSE_IRQ and set from to NR_IRQS).
Looking at my other patch I can hide this in irq_alloc_descs helper
as I already have a if ladder with two calls to irq_alloc_descs_range.
Presently I don't intend to offer this ambigious feature directly in
the _range call, but will support it through the wrapper for the
existing functions.
Thanks for the discussion
milton
On Fri, Jun 03, 2011 at 07:23:24PM -0500, Milton Miller wrote:
> On Fri, 3 Jun 2011 about 16:06:36 +0100, Mark Brown wrote:
> > I need either a specific IRQ or an allocated one. This is just a very
> > standard driver with an interrupt controller
> I don't know that I'd call an interrupt controller a standard driver
> today, but agree we are heading towards that.
Pretty much any modern ARM system will have a PMIC with an interrupt
controller in it.
> > 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.
> The irq domain concept addresses mapping, but a simple fully allocated
> block will be one of the options. Having 60 irqdesc might be
> pushing the limit for some platforms but that can be addressed later
> when irq domains are upstream.
I'm not really sure what an IRQ domain is (the patch series you pointed
at before does rather appear to assume one knows already) but given that
we're allocating from within a 32 bit type it seems like we should be
using a few orders of magnitude more interrupts in a system than we do
presently before this becomes much of an issue.
> > 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'd argue the user is the wrong level to make this decision. However,
> saying the platform decides is valid, and the excerpt you had earlier
> implied you were getting the irq argument from platform data, not the
> user (eg via a module parameter)..
It's platform data. Having interrupt numbers specified as module
parameters strikes me as insane - the particular integers we map
individual interrupts onto are very much an implementation decision of
the kernel.
> I should point out that several archtectures currently allocate irqs
> in a architecture layer before calling the irq allocator asking for
> the exact irq they choose on what they thought was free. Among these
> are x86 and powerpc. Not calling the architecture layer will causein
> all future allocations by other drivers using the architecture layer
> to fail..
That sounds like we should join the two IRQ allocators up with each
other.
On Sat, Jun 4, 2011 at 3:51 AM, Mark Brown
<[email protected]> wrote:
> On Fri, Jun 03, 2011 at 07:23:24PM -0500, Milton Miller wrote:
>> On Fri, 3 Jun 2011 about 16:06:36 +0100, Mark Brown wrote:
>
>> > I need either a specific IRQ or an allocated one. ?This is just a very
>> > standard driver with an interrupt controller
>
>> I don't know that I'd call an interrupt controller a standard driver
>> today, but agree we are heading towards that.
>
> Pretty much any modern ARM system will have a PMIC with an interrupt
> controller in it.
>
>> > 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.
>
>> The irq domain concept addresses mapping, but a simple fully allocated
>> block will be one of the options. ?Having 60 irqdesc might be
>> pushing the limit for some platforms but that can be addressed later
>> when irq domains are upstream.
>
> I'm not really sure what an IRQ domain is (the patch series you pointed
> at before does rather appear to assume one knows already)
(obviously I need to write some documentation). irq_domains is some
infrastructure to make managing a range of irq numbers easier, and
also to handle the mapping of hw irq numbers to linux irq numbers.
> but given that
> we're allocating from within a 32 bit type it seems like we should be
> using a few orders of magnitude more interrupts in a system than we do
> presently before this becomes much of an issue.
Also, with sparse irqs, irq numbers are effectively free. It may be
valuable to implement deferred allocation of the actual irq_desc
structures, but ranges of irq numbers are really cheap for discrete
irq controllers (say in the range of 1-256 irq inputs). It may be
different for MSI controllers that have programmable hw irq numbers,
but in that case the solution is to allocated them one at a time as
needed, or allocate a pool that the MSI controller driver can manage
on its own.
>
>> > 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'd argue the user is the wrong level to make this decision. ?However,
>> saying the platform decides is valid, and the excerpt you had earlier
>> implied you were getting the irq argument from platform data, not the
>> user (eg via a module parameter)..
>
> It's platform data. ?Having interrupt numbers specified as module
> parameters strikes me as insane - the particular integers we map
> individual interrupts onto are very much an implementation decision of
> the kernel.
More and more irq numbers are becoming internal implementation details
of the kernel. We should be moving in the direction of irq numbers
have less to no impact on anything in userspace or the bootloader.
There probably isn't much to be done about /proc/interrupts, but it
definitely sounds insane to specify irq numbers as module parameters.
or in the device tree. or anything else not linked directly to the
kernel.
>> I should point out that several archtectures currently allocate irqs
>> in a architecture layer before calling the irq allocator asking for
>> the exact irq they choose on what they thought was free. ?Among these
>> are x86 and powerpc. ? Not calling the architecture layer will causein
>> all future allocations by other drivers using the architecture layer
>> to fail..
Yes they do, but no it should not stay that way. The architectures
currently do this because there wasn't common code to handle it
sanely. Now that irq_alloc_desc*() does exist, the arch specific
allocation of irqs needs to be dropped and the architecture code
should rely entirely on irq_alloc_desc*() for managing ranges of irqs.
It does not make sense for drivers needing to allocating irq_descs
(gpio controllers are an excellent example) to call into arch code.
irq_alloc_desc*() is more than sufficient. Milton, later in your
email you mention changing the way powerpc allocates irqs. Is this
what you were talking about?
>
> That sounds like we should join the two IRQ allocators up with each
> other.
yes.
On Mon, Jun 06, 2011 at 01:42:33PM -0600, Grant Likely wrote:
> On Sat, Jun 4, 2011 at 3:51 AM, Mark Brown
> > I'm not really sure what an IRQ domain is (the patch series you pointed
> > at before does rather appear to assume one knows already)
> (obviously I need to write some documentation). irq_domains is some
> infrastructure to make managing a range of irq numbers easier, and
> also to handle the mapping of hw irq numbers to linux irq numbers.
Hrm, in that case it sounds rather like a lot of interrupt controllers
ought to be making themselves IRQ domains?
On Mon, Jun 6, 2011 at 2:57 PM, Mark Brown
<[email protected]> wrote:
> On Mon, Jun 06, 2011 at 01:42:33PM -0600, Grant Likely wrote:
>> On Sat, Jun 4, 2011 at 3:51 AM, Mark Brown
>
>> > I'm not really sure what an IRQ domain is (the patch series you pointed
>> > at before does rather appear to assume one knows already)
>
>> (obviously I need to write some documentation). ?irq_domains is some
>> infrastructure to make managing a range of irq numbers easier, and
>> also to handle the mapping of hw irq numbers to linux irq numbers.
>
> Hrm, in that case it sounds rather like a lot of interrupt controllers
> ought to be making themselves IRQ domains?
yup, and I'll probably craft patches for RFC to do exactly that when
I've got irq_domains sorted out.
g.