Please add irqdomain branch to linux-next

Please add irqdomain branch to linux-next

[Grant Likely]

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.

git://git.secretlab.ca/git/linux-2.6.git irqdomain/next

Thanks,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: Please add irqdomain branch to linux-next

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1986 bytes --]

Hi Grant,

On Thu, 2 Feb 2012 14:10:31 -0700 Grant Likely <grant.likely@secretlab.ca> wrote:
>
> 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.
> 
> git://git.secretlab.ca/git/linux-2.6.git irqdomain/next

Added from today.

Thanks for adding your subsystem tree as a participant of linux-next.  As
you may know, this is not a judgment of your code.  The purpose of
linux-next is for integration testing and to lower the impact of
conflicts between subsystems in the next merge window. 

You will need to ensure that the patches/commits in your tree/series have
been:
     * submitted under GPL v2 (or later) and include the Contributor's
	Signed-off-by,
     * posted to the relevant mailing list,
     * reviewed by you (or another maintainer of your subsystem tree),
     * successfully unit tested, and 
     * destined for the current or next Linux merge window.

Basically, this should be just what you would send to Linus (or ask him
to fetch).  It is allowed to be rebased if you deem it necessary.

-- 
Cheers,
Stephen Rothwell 
sfr@canb.auug.org.au

Legal Stuff:
By participating in linux-next, your subsystem tree contributions are
public and will be included in the linux-next trees.  You may be sent
e-mail messages indicating errors or other issues when the
patches/commits from your subsystem tree are merged and tested in
linux-next.  These messages may also be cross-posted to the linux-next
mailing list, the linux-kernel mailing list, etc.  The linux-next tree
project and IBM (my employer) make no warranties regarding the linux-next
project, the testing procedures, the results, the e-mails, etc.  If you
don't agree to these ground rules, let me know and I'll remove your tree
from participation in linux-next.

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: Please add irqdomain branch to linux-next

[Benjamin Herrenschmidt]

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

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.

Those use no reverse mapping, because their HV can basically return the
virq ... but with a limited capacity, which is why they change
irq_virq_count to limit the virq numbers to what they can handle.

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

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.

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.

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.

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

Overall, I'm not that fan of those helpers... do they really "help" ?
IE, Is the call significantly smaller ?

Cheers,
Ben.

Re: Please add irqdomain branch to linux-next

[Grant Likely]

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

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

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

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

> Overall, I'm not that fan of those helpers... do they really "help" ?
> IE, Is the call significantly smaller ?

I think it makes the code a lot easier to read, and it makes it a lot
easier for a user to know what they are supposed to do I think.  The
build size change wasn't significant either way (but I've lost those
numbers, I'll need to recalculate them again to give specifics)..

Thanks for the review.

g.

Re: Please add irqdomain branch to linux-next

[Grant Likely]

On Sun, Feb 5, 2012 at 11:15 PM, Grant Likely <grant.likely@secretlab.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.

Re: Please add irqdomain branch to linux-next

[Benjamin Herrenschmidt]

On Wed, 2012-02-15 at 18:32 -0700, Grant Likely wrote:

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

Right, I don't see an obvious need elsewhere so it could be a flag
specific to nomap, tho it still needs to be taken into account in the
main allocation code.

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

But which node ? :-)

I'd rather you add a new API, no need to change the call sites:

Add foo_node(xxx,node); and have the existing foo() be implemented
as a static inline calling foo_node(xxx,0); or something like that, then
I can change the powerpc code to use the later & pass the PCI device
node (which should be in the pci_controller structure). We can add more
later.

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

Ok, let me know.

Cheers,
Ben.

Re: Please add irqdomain branch to linux-next

[Grant Likely]

On Thu, Feb 16, 2012 at 02:11:38PM +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2012-02-15 at 18:32 -0700, Grant Likely wrote:
> 
> > 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.
> 
> Right, I don't see an obvious need elsewhere so it could be a flag
> specific to nomap, tho it still needs to be taken into account in the
> main allocation code.
> 
> > 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.
> 
> But which node ? :-)
> 
> I'd rather you add a new API, no need to change the call sites:
> 
> Add foo_node(xxx,node); and have the existing foo() be implemented
> as a static inline calling foo_node(xxx,0); or something like that, then
> I can change the powerpc code to use the later & pass the PCI device
> node (which should be in the pci_controller structure). We can add more
> later.

Okay, fair enough.  Instead of changing the series then, adding the new
api can be the follow on patch.

g.