Date:	Wed, 15 Feb 2012 18:32:03 -0700
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Benjamin Herrenschmidt <benh@...nel.crashing.org>
Cc:	Stephen Rothwell <sfr@...b.auug.org.au>,
	linux-next <linux-next@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Arnd Bergmann <arnd@...db.de>,
	Russell King - ARM Linux <linux@....linux.org.uk>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: Please add irqdomain branch to linux-next

On Sun, Feb 5, 2012 at 11:15 PM, Grant Likely <grant.likely@...retlab.ca> wrote:
> On Mon, Feb 06, 2012 at 12:35:11PM +1100, Benjamin Herrenschmidt wrote:
>> On Thu, 2012-02-02 at 14:10 -0700, Grant Likely wrote:
>> > Hi Stephen,
>> >
>> > Can you please add the following branch to linux-next?  It contains
>> > the majority of the irqdomain rework that I've been doing.  I'd like
>> > to get it marinating in linux-next early so I'm sure it will be ready
>> > when the v3.4 merge window rolls around.
>>
>> Ho !
>>
>> I don't have v4 in my mailbox to reply to the individual patches,
>> but I've spotted some issues so here they are in no specific order.
>>
>> @@ -739,31 +712,36 @@ unsigned int irq_create_mapping(struct irq_domain *host,
>>
>>       /* Get a virtual interrupt number */
>>       if (host->revmap_type == IRQ_DOMAIN_MAP_LEGACY) {
>>               /* Handle legacy */
>>               virq = (unsigned int)hwirq;
>>               if (virq == 0 || virq >= NUM_ISA_INTERRUPTS)
>>                       return NO_IRQ;
>>               return virq;
>>       } else {
>>               /* Allocate a virtual interrupt number */
>>               hint = hwirq % irq_virq_count;
>> -             virq = irq_alloc_virt(host, 1, hint);
>> +             virq = irq_alloc_desc_from(hint, 0);
>> +             if (!virq)
>> +                     virq = irq_alloc_desc_from(1, 0);
>>               if (virq == NO_IRQ) {
>>                       pr_debug("irq: -> virq allocation failed\n");
>>                       return NO_IRQ;
>>               }
>>
>> So first, the way you "avoid" allocating irq 0 seems to be by ...
>> allocating irq 0 and then allocating again once you've done that :-)
>>
>> You should either make sure hint is non-0 to begin with or use
>> irq_reserve_irq() to reserve irq 0 (tho I don't know whether the later
>> could be an issue on x86).
>
> Okay, I'll ensure that hint != 0

Now fixed.  Will be in v5

>
>> Also, you no longer honor irq_virq_count. It's a limitation of
>> __irq_alloc_descs() to not be able to get an upper boundary, but you
>> need that for iseries and ps3 at least.
>
> I'll look at adding an upper limit to __irq_alloc_descs().  If that won't
> work, then I'll add an explicit test after allocation to make sure it is not
> over the limit.
>
>> Also the default for irq_virq_count should probably be changed when you
>> move to the core to use IRQ_BITMAP_BITS (so we get the 8192 additional
>> irqs on SPARSE_IRQ).
>
> Good catch.

Only nomap users will care about this, and of those 5, only iseries
and ps3 actually change it.  How about I add a max_virq parameter to
only be used by the nomap revmap?  That seems to be cleaner than a
global setting.  I've crafted a patch and will post it with v5 of the
series.

>
>>
>> Another thing I noticed, tho I'm still only half way through the series
>> so you -may- have fixed that, is that you allocate all descs on node 0
>> (not even the current node) and have no way to do otherwise.
>
> No, I've not fixed that.
>
>> Now, it's a bit of a nasty issue because ideally we should "move" the
>> descs around as we set the affinity of interrupts and we really can't do
>> that just yet, but at least having a way to allocate the desc with a
>> node number (adding a node argument to irq_create_mapping) would be
>> useful. For things like PCI we could make that use the node where the
>> device is, which is better than having everything on node 0.
>
> okay.

For now I'll use numa_node_id() at allocation time.  I'll craft a
follow-on patch to change the API since it touches a lot of call
sites.

>
>> Also you should probably make the whole match & xlate business
>> #ifdef CONFIG_OF (especially in the definition of the irq domain). There
>> is no reason why archs couldn't use the domain mapper without
>> device-tree support.
>
> It builds and runs fine without the CONFIG_OF wrappers, but I can trim stuff
> down.
>
>> +int irq_domain_xlate_pci(struct irq_domain *d, struct device_node *ctrlr,
>> +                      const u32 *intspec, unsigned int intsize,
>> +                      unsigned long *out_hwirq, unsigned int *out_type)
>> +{
>> +     if (WARN_ON(intsize != 1))
>> +             return -EINVAL;
>> +     *out_hwirq = intspec[0];
>> +     *out_type = IRQ_TYPE_LEVEL_HIGH;
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(irq_domain_xlate_pci);
>>
>> That's bogus. PCI interrupts are level -low-. However some bridges
>> internally invert them on the way to the PIC (this is actually common
>> with PCIe bridges where they are generated from messages). So if you are
>> to provide a default helper, make it LEVEL_LOW really.
>
> Haha, good point.  I'll fix that.

I've dropped irq_domain_xlate_pci()

I think I've addressed all the problems you've brought up.  I'm
testing now and I'll be posting v5 very shortly.

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

Powered by blists - more mailing lists