[PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from().

[PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from().

[David Daney]

From: David Daney <david.daney@cavium.com>

In commit 4bbdd45a (irq_domain/powerpc: eliminate irq_map; use
irq_alloc_desc() instead) code was added that ignores error returns
from irq_alloc_desc_from() by (silently) casting the return value to
unsigned.  The negitive value error return now suddenly looks like a
valid irq number.

Commits cc79ca69 (irq_domain: Move irq_domain code from powerpc to
kernel/irq) and 1bc04f2c (irq_domain: Add support for base irq and
hwirq in legacy mappings) move this code to its current location in
irqdomain.c

The result of all of this is a null pointer dereference OOPS if one of
the error cases is hit.

The fix: Don't cast away the negativeness of the return value and then
check for errors.

Signed-off-by: David Daney <david.daney@cavium.com>
---
 kernel/irq/irqdomain.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index af48e59..9d3e3ae 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -351,6 +351,7 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
 				irq_hw_number_t hwirq)
 {
 	unsigned int virq, hint;
+	int irq;
 
 	pr_debug("irq: irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq);
 
@@ -380,14 +381,14 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
 	hint = hwirq % irq_virq_count;
 	if (hint == 0)
 		hint++;
-	virq = irq_alloc_desc_from(hint, 0);
-	if (!virq)
-		virq = irq_alloc_desc_from(1, 0);
-	if (!virq) {
+	irq = irq_alloc_desc_from(hint, 0);
+	if (irq <= 0)
+		irq = irq_alloc_desc_from(1, 0);
+	if (irq <= 0) {
 		pr_debug("irq: -> virq allocation failed\n");
 		return 0;
 	}
-
+	virq = irq;
 	if (irq_setup_virq(domain, virq, hwirq)) {
 		if (domain->revmap_type != IRQ_DOMAIN_MAP_LEGACY)
 			irq_free_desc(virq);
-- 
1.7.2.3

Re: [PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from().

[Rob Herring]

On 04/05/2012 06:52 PM, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
> 
> In commit 4bbdd45a (irq_domain/powerpc: eliminate irq_map; use
> irq_alloc_desc() instead) code was added that ignores error returns
> from irq_alloc_desc_from() by (silently) casting the return value to
> unsigned.  The negitive value error return now suddenly looks like a
> valid irq number.
> 
> Commits cc79ca69 (irq_domain: Move irq_domain code from powerpc to
> kernel/irq) and 1bc04f2c (irq_domain: Add support for base irq and
> hwirq in legacy mappings) move this code to its current location in
> irqdomain.c
> 
> The result of all of this is a null pointer dereference OOPS if one of
> the error cases is hit.
> 
> The fix: Don't cast away the negativeness of the return value and then
> check for errors.
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  kernel/irq/irqdomain.c |   11 ++++++-----
>  1 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index af48e59..9d3e3ae 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -351,6 +351,7 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
>  				irq_hw_number_t hwirq)
>  {
>  	unsigned int virq, hint;
> +	int irq;
>  
>  	pr_debug("irq: irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq);
>  
> @@ -380,14 +381,14 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
>  	hint = hwirq % irq_virq_count;
>  	if (hint == 0)
>  		hint++;
> -	virq = irq_alloc_desc_from(hint, 0);

You are not looking at mainline. hint was removed in later versions, and
the referenced commit ids don't exist.

Rob

> -	if (!virq)
> -		virq = irq_alloc_desc_from(1, 0);
> -	if (!virq) {
> +	irq = irq_alloc_desc_from(hint, 0);
> +	if (irq <= 0)
> +		irq = irq_alloc_desc_from(1, 0);
> +	if (irq <= 0) {
>  		pr_debug("irq: -> virq allocation failed\n");
>  		return 0;
>  	}
> -
> +	virq = irq;
>  	if (irq_setup_virq(domain, virq, hwirq)) {
>  		if (domain->revmap_type != IRQ_DOMAIN_MAP_LEGACY)
>  			irq_free_desc(virq);

Re: [PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from().

[Rob Herring]

On 04/05/2012 06:52 PM, David Daney wrote:
> From: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> 
> In commit 4bbdd45a (irq_domain/powerpc: eliminate irq_map; use
> irq_alloc_desc() instead) code was added that ignores error returns
> from irq_alloc_desc_from() by (silently) casting the return value to
> unsigned.  The negitive value error return now suddenly looks like a
> valid irq number.
> 
> Commits cc79ca69 (irq_domain: Move irq_domain code from powerpc to
> kernel/irq) and 1bc04f2c (irq_domain: Add support for base irq and
> hwirq in legacy mappings) move this code to its current location in
> irqdomain.c
> 
> The result of all of this is a null pointer dereference OOPS if one of
> the error cases is hit.
> 
> The fix: Don't cast away the negativeness of the return value and then
> check for errors.
> 
> Signed-off-by: David Daney <david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> ---
>  kernel/irq/irqdomain.c |   11 ++++++-----
>  1 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index af48e59..9d3e3ae 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -351,6 +351,7 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
>  				irq_hw_number_t hwirq)
>  {
>  	unsigned int virq, hint;
> +	int irq;
>  
>  	pr_debug("irq: irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq);
>  
> @@ -380,14 +381,14 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
>  	hint = hwirq % irq_virq_count;
>  	if (hint == 0)
>  		hint++;
> -	virq = irq_alloc_desc_from(hint, 0);

You are not looking at mainline. hint was removed in later versions, and
the referenced commit ids don't exist.

Rob

> -	if (!virq)
> -		virq = irq_alloc_desc_from(1, 0);
> -	if (!virq) {
> +	irq = irq_alloc_desc_from(hint, 0);
> +	if (irq <= 0)
> +		irq = irq_alloc_desc_from(1, 0);
> +	if (irq <= 0) {
>  		pr_debug("irq: -> virq allocation failed\n");
>  		return 0;
>  	}
> -
> +	virq = irq;
>  	if (irq_setup_virq(domain, virq, hwirq)) {
>  		if (domain->revmap_type != IRQ_DOMAIN_MAP_LEGACY)
>  			irq_free_desc(virq);

Re: [PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from().

[David Daney]

Rob,

What the he*%? ...


On 04/05/2012 08:37 PM, Rob Herring wrote:
> On 04/05/2012 06:52 PM, David Daney wrote:
>> From: David Daney<david.daney@cavium.com>
>>
>> In commit 4bbdd45a (irq_domain/powerpc: eliminate irq_map; use
>> irq_alloc_desc() instead) code was added that ignores error returns
>> from irq_alloc_desc_from() by (silently) casting the return value to
>> unsigned.  The negitive value error return now suddenly looks like a
>> valid irq number.
>>
>> Commits cc79ca69 (irq_domain: Move irq_domain code from powerpc to
>> kernel/irq) and 1bc04f2c (irq_domain: Add support for base irq and
>> hwirq in legacy mappings) move this code to its current location in

That would be commits:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=4bbdd45afdae208a7c4ade89cf602f89a6397cff
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=cc79ca691c292e9fd44f589c7940b9654e22f2f6
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=1bc04f2cf8c2a1feadbd994f50c40bb145bf2989

>> irqdomain.c
>>
>> The result of all of this is a null pointer dereference OOPS if one of
>> the error cases is hit.
>>
>> The fix: Don't cast away the negativeness of the return value and then
>> check for errors.
>>
>> Signed-off-by: David Daney<david.daney@cavium.com>
>> ---
>>   kernel/irq/irqdomain.c |   11 ++++++-----
>>   1 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> index af48e59..9d3e3ae 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -351,6 +351,7 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
>>   				irq_hw_number_t hwirq)
>>   {
>>   	unsigned int virq, hint;
>> +	int irq;
>>
>>   	pr_debug("irq: irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq);
>>
>> @@ -380,14 +381,14 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
>>   	hint = hwirq % irq_virq_count;
>>   	if (hint == 0)
>>   		hint++;
>> -	virq = irq_alloc_desc_from(hint, 0);
>
> You are not looking at mainline. hint was removed in later versions, and
> the referenced commit ids don't exist.

Please look at Linus' tree before making incorrect statements about 
whether or not code exists on the 'mainline'

The current kernel.org tree contains the bug and will cause anything 
using irq_create_mapping() to crash in a semi-random manner.

David Daney

>
> Rob
>
>> -	if (!virq)
>> -		virq = irq_alloc_desc_from(1, 0);
>> -	if (!virq) {
>> +	irq = irq_alloc_desc_from(hint, 0);
>> +	if (irq<= 0)
>> +		irq = irq_alloc_desc_from(1, 0);
>> +	if (irq<= 0) {
>>   		pr_debug("irq: ->  virq allocation failed\n");
>>   		return 0;
>>   	}
>> -
>> +	virq = irq;
>>   	if (irq_setup_virq(domain, virq, hwirq)) {
>>   		if (domain->revmap_type != IRQ_DOMAIN_MAP_LEGACY)
>>   			irq_free_desc(virq);
>
>

Re: [PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from().

[David Daney]

Rob,

What the he*%? ...


On 04/05/2012 08:37 PM, Rob Herring wrote:
> On 04/05/2012 06:52 PM, David Daney wrote:
>> From: David Daney<david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>>
>> In commit 4bbdd45a (irq_domain/powerpc: eliminate irq_map; use
>> irq_alloc_desc() instead) code was added that ignores error returns
>> from irq_alloc_desc_from() by (silently) casting the return value to
>> unsigned.  The negitive value error return now suddenly looks like a
>> valid irq number.
>>
>> Commits cc79ca69 (irq_domain: Move irq_domain code from powerpc to
>> kernel/irq) and 1bc04f2c (irq_domain: Add support for base irq and
>> hwirq in legacy mappings) move this code to its current location in

That would be commits:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=4bbdd45afdae208a7c4ade89cf602f89a6397cff
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=cc79ca691c292e9fd44f589c7940b9654e22f2f6
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=1bc04f2cf8c2a1feadbd994f50c40bb145bf2989

>> irqdomain.c
>>
>> The result of all of this is a null pointer dereference OOPS if one of
>> the error cases is hit.
>>
>> The fix: Don't cast away the negativeness of the return value and then
>> check for errors.
>>
>> Signed-off-by: David Daney<david.daney-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
>> ---
>>   kernel/irq/irqdomain.c |   11 ++++++-----
>>   1 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> index af48e59..9d3e3ae 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -351,6 +351,7 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
>>   				irq_hw_number_t hwirq)
>>   {
>>   	unsigned int virq, hint;
>> +	int irq;
>>
>>   	pr_debug("irq: irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq);
>>
>> @@ -380,14 +381,14 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
>>   	hint = hwirq % irq_virq_count;
>>   	if (hint == 0)
>>   		hint++;
>> -	virq = irq_alloc_desc_from(hint, 0);
>
> You are not looking at mainline. hint was removed in later versions, and
> the referenced commit ids don't exist.

Please look at Linus' tree before making incorrect statements about 
whether or not code exists on the 'mainline'

The current kernel.org tree contains the bug and will cause anything 
using irq_create_mapping() to crash in a semi-random manner.

David Daney

>
> Rob
>
>> -	if (!virq)
>> -		virq = irq_alloc_desc_from(1, 0);
>> -	if (!virq) {
>> +	irq = irq_alloc_desc_from(hint, 0);
>> +	if (irq<= 0)
>> +		irq = irq_alloc_desc_from(1, 0);
>> +	if (irq<= 0) {
>>   		pr_debug("irq: ->  virq allocation failed\n");
>>   		return 0;
>>   	}
>> -
>> +	virq = irq;
>>   	if (irq_setup_virq(domain, virq, hwirq)) {
>>   		if (domain->revmap_type != IRQ_DOMAIN_MAP_LEGACY)
>>   			irq_free_desc(virq);
>
>

Re: [PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from().

[Rob Herring]

On 04/06/2012 11:37 AM, David Daney wrote:
> Rob,
> 
> What the he*%? ...
> 
> 
> On 04/05/2012 08:37 PM, Rob Herring wrote:
>> On 04/05/2012 06:52 PM, David Daney wrote:
>>> From: David Daney<david.daney@cavium.com>
>>>
>>> In commit 4bbdd45a (irq_domain/powerpc: eliminate irq_map; use
>>> irq_alloc_desc() instead) code was added that ignores error returns
>>> from irq_alloc_desc_from() by (silently) casting the return value to
>>> unsigned.  The negitive value error return now suddenly looks like a
>>> valid irq number.
>>>
>>> Commits cc79ca69 (irq_domain: Move irq_domain code from powerpc to
>>> kernel/irq) and 1bc04f2c (irq_domain: Add support for base irq and
>>> hwirq in legacy mappings) move this code to its current location in
> 
> That would be commits:
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=4bbdd45afdae208a7c4ade89cf602f89a6397cff
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=cc79ca691c292e9fd44f589c7940b9654e22f2f6
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=1bc04f2cf8c2a1feadbd994f50c40bb145bf2989
> 
> 
>>> irqdomain.c
>>>
>>> The result of all of this is a null pointer dereference OOPS if one of
>>> the error cases is hit.
>>>
>>> The fix: Don't cast away the negativeness of the return value and then
>>> check for errors.
>>>
>>> Signed-off-by: David Daney<david.daney@cavium.com>
>>> ---
>>>   kernel/irq/irqdomain.c |   11 ++++++-----
>>>   1 files changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>>> index af48e59..9d3e3ae 100644
>>> --- a/kernel/irq/irqdomain.c
>>> +++ b/kernel/irq/irqdomain.c
>>> @@ -351,6 +351,7 @@ unsigned int irq_create_mapping(struct irq_domain
>>> *domain,
>>>                   irq_hw_number_t hwirq)
>>>   {
>>>       unsigned int virq, hint;
>>> +    int irq;
>>>
>>>       pr_debug("irq: irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq);
>>>
>>> @@ -380,14 +381,14 @@ unsigned int irq_create_mapping(struct
>>> irq_domain *domain,
>>>       hint = hwirq % irq_virq_count;
>>>       if (hint == 0)
>>>           hint++;
>>> -    virq = irq_alloc_desc_from(hint, 0);
>>
>> You are not looking at mainline. hint was removed in later versions, and
>> the referenced commit ids don't exist.
> 
> Please look at Linus' tree before making incorrect statements about
> whether or not code exists on the 'mainline'
> 
> The current kernel.org tree contains the bug and will cause anything
> using irq_create_mapping() to crash in a semi-random manner.
> 

Doh, evidently I need git training... Sorry about that. I was looking at
a 3.2 branch even though I did check that. Plus I was remembering Grant
removed some of the hint code in later versions of the irq domain code.

My only comment is perhaps the pr_debug should be an error msg unless
you get an error message already. Otherwise, FWIW:

Acked-by: Rob Herring <rob.herring@calxeda.com>

Off to find my brain,
Rob


> David Daney
> 
>>
>> Rob
>>
>>> -    if (!virq)
>>> -        virq = irq_alloc_desc_from(1, 0);
>>> -    if (!virq) {
>>> +    irq = irq_alloc_desc_from(hint, 0);
>>> +    if (irq<= 0)
>>> +        irq = irq_alloc_desc_from(1, 0);
>>> +    if (irq<= 0) {
>>>           pr_debug("irq: ->  virq allocation failed\n");
>>>           return 0;
>>>       }
>>> -
>>> +    virq = irq;
>>>       if (irq_setup_virq(domain, virq, hwirq)) {
>>>           if (domain->revmap_type != IRQ_DOMAIN_MAP_LEGACY)
>>>               irq_free_desc(virq);
>>
>>
> 

Re: [PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from().

[Grant Likely]

On Fri, 06 Apr 2012 09:37:49 -0700, David Daney <ddaney.cavm@gmail.com> wrote:
> On 04/05/2012 08:37 PM, Rob Herring wrote:
> > On 04/05/2012 06:52 PM, David Daney wrote:
> >> From: David Daney<david.daney@cavium.com>
> >> @@ -380,14 +381,14 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
> >>   	hint = hwirq % irq_virq_count;
> >>   	if (hint == 0)
> >>   		hint++;
> >> -	virq = irq_alloc_desc_from(hint, 0);
> >
> > You are not looking at mainline. hint was removed in later versions, and
> > the referenced commit ids don't exist.
> 
> Please look at Linus' tree before making incorrect statements about 
> whether or not code exists on the 'mainline'

Rob is indeed mistaken here, but please let's keep things civil.  I
did write the patches to remove the hint, but I avoided merging them
because I think they are risky.  I wanted to get the core irq_domain
changes in first.  The removal of hint will be applied during the next
merge window.

I will take your bug fix and get it pushed up to Linus soon.

g.

Re: [PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from().

[Grant Likely]

On Fri, 06 Apr 2012 09:37:49 -0700, David Daney <ddaney.cavm@gmail.com> wrote:
> On 04/05/2012 08:37 PM, Rob Herring wrote:
> > On 04/05/2012 06:52 PM, David Daney wrote:
> >> From: David Daney<david.daney@cavium.com>
> >> @@ -380,14 +381,14 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
> >>   	hint = hwirq % irq_virq_count;
> >>   	if (hint == 0)
> >>   		hint++;
> >> -	virq = irq_alloc_desc_from(hint, 0);
> >
> > You are not looking at mainline. hint was removed in later versions, and
> > the referenced commit ids don't exist.
> 
> Please look at Linus' tree before making incorrect statements about 
> whether or not code exists on the 'mainline'

Rob is indeed mistaken here, but please let's keep things civil.  I
did write the patches to remove the hint, but I avoided merging them
because I think they are risky.  I wanted to get the core irq_domain
changes in first.  The removal of hint will be applied during the next
merge window.

I will take your bug fix and get it pushed up to Linus soon.

g.

Re: [PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from().

[Grant Likely]

On Fri, 06 Apr 2012 09:37:49 -0700, David Daney <ddaney.cavm@gmail.com> wrote:
> On 04/05/2012 08:37 PM, Rob Herring wrote:
> > On 04/05/2012 06:52 PM, David Daney wrote:
> >> From: David Daney<david.daney@cavium.com>
> >> @@ -380,14 +381,14 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
> >>   	hint = hwirq % irq_virq_count;
> >>   	if (hint == 0)
> >>   		hint++;
> >> -	virq = irq_alloc_desc_from(hint, 0);
> >
> > You are not looking at mainline. hint was removed in later versions, and
> > the referenced commit ids don't exist.
> 
> Please look at Linus' tree before making incorrect statements about 
> whether or not code exists on the 'mainline'

Rob is indeed mistaken here, but please let's keep things civil.  I
did write the patches to remove the hint, but I avoided merging them
because I think they are risky.  I wanted to get the core irq_domain
changes in first.  The removal of hint will be applied during the next
merge window.

I will take your bug fix and get it pushed up to Linus soon.

g.

Re: [PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from().

[Grant Likely]

On Thu,  5 Apr 2012 16:52:13 -0700, David Daney <ddaney.cavm@gmail.com> wrote:
> From: David Daney <david.daney@cavium.com>
> 
> In commit 4bbdd45a (irq_domain/powerpc: eliminate irq_map; use
> irq_alloc_desc() instead) code was added that ignores error returns
> from irq_alloc_desc_from() by (silently) casting the return value to
> unsigned.  The negitive value error return now suddenly looks like a
> valid irq number.
> 
> Commits cc79ca69 (irq_domain: Move irq_domain code from powerpc to
> kernel/irq) and 1bc04f2c (irq_domain: Add support for base irq and
> hwirq in legacy mappings) move this code to its current location in
> irqdomain.c
> 
> The result of all of this is a null pointer dereference OOPS if one of
> the error cases is hit.
> 
> The fix: Don't cast away the negativeness of the return value and then
> check for errors.
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  kernel/irq/irqdomain.c |   11 ++++++-----
>  1 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index af48e59..9d3e3ae 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -351,6 +351,7 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
>  				irq_hw_number_t hwirq)
>  {
>  	unsigned int virq, hint;
> +	int irq;

Merged, but I've dropped the new variable in favour of making virq an
int.  Makes for a smaller diffstat.

g.

>  
>  	pr_debug("irq: irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq);
>  
> @@ -380,14 +381,14 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
>  	hint = hwirq % irq_virq_count;
>  	if (hint == 0)
>  		hint++;
> -	virq = irq_alloc_desc_from(hint, 0);
> -	if (!virq)
> -		virq = irq_alloc_desc_from(1, 0);
> -	if (!virq) {
> +	irq = irq_alloc_desc_from(hint, 0);
> +	if (irq <= 0)
> +		irq = irq_alloc_desc_from(1, 0);
> +	if (irq <= 0) {
>  		pr_debug("irq: -> virq allocation failed\n");
>  		return 0;
>  	}
> -
> +	virq = irq;
>  	if (irq_setup_virq(domain, virq, hwirq)) {
>  		if (domain->revmap_type != IRQ_DOMAIN_MAP_LEGACY)
>  			irq_free_desc(virq);
> -- 
> 1.7.2.3
> 

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

Re: [PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from().

[David Daney]

On 04/06/2012 04:56 PM, Grant Likely wrote:
> On Fri, 06 Apr 2012 09:37:49 -0700, David Daney<ddaney.cavm@gmail.com>  wrote:
>> On 04/05/2012 08:37 PM, Rob Herring wrote:
>>> On 04/05/2012 06:52 PM, David Daney wrote:
>>>> From: David Daney<david.daney@cavium.com>
>>>> @@ -380,14 +381,14 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
>>>>    	hint = hwirq % irq_virq_count;
>>>>    	if (hint == 0)
>>>>    		hint++;
>>>> -	virq = irq_alloc_desc_from(hint, 0);
>>>
>>> You are not looking at mainline. hint was removed in later versions, and
>>> the referenced commit ids don't exist.
>>
>> Please look at Linus' tree before making incorrect statements about
>> whether or not code exists on the 'mainline'
>
> Rob is indeed mistaken here, but please let's keep things civil.

Sorry about that.  You are correct that it is not acceptable.

David Daney

Re: [PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from().

[David Daney]

On 04/06/2012 06:26 PM, Grant Likely wrote:
> On Thu,  5 Apr 2012 16:52:13 -0700, David Daney<ddaney.cavm@gmail.com>  wrote:
>> From: David Daney<david.daney@cavium.com>
>>
>> In commit 4bbdd45a (irq_domain/powerpc: eliminate irq_map; use
>> irq_alloc_desc() instead) code was added that ignores error returns
>> from irq_alloc_desc_from() by (silently) casting the return value to
>> unsigned.  The negitive value error return now suddenly looks like a
>> valid irq number.
>>
>> Commits cc79ca69 (irq_domain: Move irq_domain code from powerpc to
>> kernel/irq) and 1bc04f2c (irq_domain: Add support for base irq and
>> hwirq in legacy mappings) move this code to its current location in
>> irqdomain.c
>>
>> The result of all of this is a null pointer dereference OOPS if one of
>> the error cases is hit.
>>
>> The fix: Don't cast away the negativeness of the return value and then
>> check for errors.
>>
>> Signed-off-by: David Daney<david.daney@cavium.com>
>> ---
>>   kernel/irq/irqdomain.c |   11 ++++++-----
>>   1 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> index af48e59..9d3e3ae 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -351,6 +351,7 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
>>   				irq_hw_number_t hwirq)
>>   {
>>   	unsigned int virq, hint;
>> +	int irq;
>
> Merged, but I've dropped the new variable in favour of making virq an
> int.  Makes for a smaller diffstat.
>

Thanks Grant,

I had thought about that too, but since virq throughout all the rest of 
the code is unsigned, I didn't want to introduce an inconsistency.

After a little more thought, I think that the domain of virq and the irq 
used by the rest of the kernel are the same, so it might make sense to 
change virq to be int universally, and use the kernel convention that 
negative numbers indicate error conditions.  But that would be a much 
larger patch.

David Daney



> g.
>
>>
>>   	pr_debug("irq: irq_create_mapping(0x%p, 0x%lx)\n", domain, hwirq);
>>
>> @@ -380,14 +381,14 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
>>   	hint = hwirq % irq_virq_count;
>>   	if (hint == 0)
>>   		hint++;
>> -	virq = irq_alloc_desc_from(hint, 0);
>> -	if (!virq)
>> -		virq = irq_alloc_desc_from(1, 0);
>> -	if (!virq) {
>> +	irq = irq_alloc_desc_from(hint, 0);
>> +	if (irq<= 0)
>> +		irq = irq_alloc_desc_from(1, 0);
>> +	if (irq<= 0) {
>>   		pr_debug("irq: ->  virq allocation failed\n");
>>   		return 0;
>>   	}
>> -
>> +	virq = irq;
>>   	if (irq_setup_virq(domain, virq, hwirq)) {
>>   		if (domain->revmap_type != IRQ_DOMAIN_MAP_LEGACY)
>>   			irq_free_desc(virq);
>> --
>> 1.7.2.3
>>
>

Re: [PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from().

[Grant Likely]

On Mon, 09 Apr 2012 09:56:30 -0700, David Daney <ddaney.cavm@gmail.com> wrote:
> On 04/06/2012 06:26 PM, Grant Likely wrote:
> > On Thu,  5 Apr 2012 16:52:13 -0700, David Daney<ddaney.cavm@gmail.com>  wrote:
> >> From: David Daney<david.daney@cavium.com>
> >>
> >> In commit 4bbdd45a (irq_domain/powerpc: eliminate irq_map; use
> >> irq_alloc_desc() instead) code was added that ignores error returns
> >> from irq_alloc_desc_from() by (silently) casting the return value to
> >> unsigned.  The negitive value error return now suddenly looks like a
> >> valid irq number.
> >>
> >> Commits cc79ca69 (irq_domain: Move irq_domain code from powerpc to
> >> kernel/irq) and 1bc04f2c (irq_domain: Add support for base irq and
> >> hwirq in legacy mappings) move this code to its current location in
> >> irqdomain.c
> >>
> >> The result of all of this is a null pointer dereference OOPS if one of
> >> the error cases is hit.
> >>
> >> The fix: Don't cast away the negativeness of the return value and then
> >> check for errors.
> >>
> >> Signed-off-by: David Daney<david.daney@cavium.com>
> >> ---
> >>   kernel/irq/irqdomain.c |   11 ++++++-----
> >>   1 files changed, 6 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> >> index af48e59..9d3e3ae 100644
> >> --- a/kernel/irq/irqdomain.c
> >> +++ b/kernel/irq/irqdomain.c
> >> @@ -351,6 +351,7 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
> >>   				irq_hw_number_t hwirq)
> >>   {
> >>   	unsigned int virq, hint;
> >> +	int irq;
> >
> > Merged, but I've dropped the new variable in favour of making virq an
> > int.  Makes for a smaller diffstat.
> >
> 
> Thanks Grant,
> 
> I had thought about that too, but since virq throughout all the rest of 
> the code is unsigned, I didn't want to introduce an inconsistency.
> 
> After a little more thought, I think that the domain of virq and the irq 
> used by the rest of the kernel are the same, so it might make sense to 
> change virq to be int universally, and use the kernel convention that 
> negative numbers indicate error conditions.  But that would be a much 
> larger patch.

... touching pretty much *every* driver in the kernel!  Blech!

Yeah, that's not going to happen.  As a rule, irq numbers are always
unsigned, but there are a few apis that can return either '0' meaning
no irq, or a negative value indicating an error.  The irq_alloc_desc
apis unfortunately are one such case.

g.

Re: [PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from().

[Grant Likely]

On Mon, 09 Apr 2012 09:56:30 -0700, David Daney <ddaney.cavm@gmail.com> wrote:
> On 04/06/2012 06:26 PM, Grant Likely wrote:
> > On Thu,  5 Apr 2012 16:52:13 -0700, David Daney<ddaney.cavm@gmail.com>  wrote:
> >> From: David Daney<david.daney@cavium.com>
> >>
> >> In commit 4bbdd45a (irq_domain/powerpc: eliminate irq_map; use
> >> irq_alloc_desc() instead) code was added that ignores error returns
> >> from irq_alloc_desc_from() by (silently) casting the return value to
> >> unsigned.  The negitive value error return now suddenly looks like a
> >> valid irq number.
> >>
> >> Commits cc79ca69 (irq_domain: Move irq_domain code from powerpc to
> >> kernel/irq) and 1bc04f2c (irq_domain: Add support for base irq and
> >> hwirq in legacy mappings) move this code to its current location in
> >> irqdomain.c
> >>
> >> The result of all of this is a null pointer dereference OOPS if one of
> >> the error cases is hit.
> >>
> >> The fix: Don't cast away the negativeness of the return value and then
> >> check for errors.
> >>
> >> Signed-off-by: David Daney<david.daney@cavium.com>
> >> ---
> >>   kernel/irq/irqdomain.c |   11 ++++++-----
> >>   1 files changed, 6 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> >> index af48e59..9d3e3ae 100644
> >> --- a/kernel/irq/irqdomain.c
> >> +++ b/kernel/irq/irqdomain.c
> >> @@ -351,6 +351,7 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
> >>   				irq_hw_number_t hwirq)
> >>   {
> >>   	unsigned int virq, hint;
> >> +	int irq;
> >
> > Merged, but I've dropped the new variable in favour of making virq an
> > int.  Makes for a smaller diffstat.
> >
> 
> Thanks Grant,
> 
> I had thought about that too, but since virq throughout all the rest of 
> the code is unsigned, I didn't want to introduce an inconsistency.
> 
> After a little more thought, I think that the domain of virq and the irq 
> used by the rest of the kernel are the same, so it might make sense to 
> change virq to be int universally, and use the kernel convention that 
> negative numbers indicate error conditions.  But that would be a much 
> larger patch.

... touching pretty much *every* driver in the kernel!  Blech!

Yeah, that's not going to happen.  As a rule, irq numbers are always
unsigned, but there are a few apis that can return either '0' meaning
no irq, or a negative value indicating an error.  The irq_alloc_desc
apis unfortunately are one such case.

g.

Re: [PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from().

[Grant Likely]

On Mon, 09 Apr 2012 09:56:30 -0700, David Daney <ddaney.cavm@gmail.com> wrote:
> On 04/06/2012 06:26 PM, Grant Likely wrote:
> > On Thu,  5 Apr 2012 16:52:13 -0700, David Daney<ddaney.cavm@gmail.com>  wrote:
> >> From: David Daney<david.daney@cavium.com>
> >>
> >> In commit 4bbdd45a (irq_domain/powerpc: eliminate irq_map; use
> >> irq_alloc_desc() instead) code was added that ignores error returns
> >> from irq_alloc_desc_from() by (silently) casting the return value to
> >> unsigned.  The negitive value error return now suddenly looks like a
> >> valid irq number.
> >>
> >> Commits cc79ca69 (irq_domain: Move irq_domain code from powerpc to
> >> kernel/irq) and 1bc04f2c (irq_domain: Add support for base irq and
> >> hwirq in legacy mappings) move this code to its current location in
> >> irqdomain.c
> >>
> >> The result of all of this is a null pointer dereference OOPS if one of
> >> the error cases is hit.
> >>
> >> The fix: Don't cast away the negativeness of the return value and then
> >> check for errors.
> >>
> >> Signed-off-by: David Daney<david.daney@cavium.com>
> >> ---
> >>   kernel/irq/irqdomain.c |   11 ++++++-----
> >>   1 files changed, 6 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> >> index af48e59..9d3e3ae 100644
> >> --- a/kernel/irq/irqdomain.c
> >> +++ b/kernel/irq/irqdomain.c
> >> @@ -351,6 +351,7 @@ unsigned int irq_create_mapping(struct irq_domain *domain,
> >>   				irq_hw_number_t hwirq)
> >>   {
> >>   	unsigned int virq, hint;
> >> +	int irq;
> >
> > Merged, but I've dropped the new variable in favour of making virq an
> > int.  Makes for a smaller diffstat.
> >
> 
> Thanks Grant,
> 
> I had thought about that too, but since virq throughout all the rest of 
> the code is unsigned, I didn't want to introduce an inconsistency.
> 
> After a little more thought, I think that the domain of virq and the irq 
> used by the rest of the kernel are the same, so it might make sense to 
> change virq to be int universally, and use the kernel convention that 
> negative numbers indicate error conditions.  But that would be a much 
> larger patch.

... touching pretty much *every* driver in the kernel!  Blech!

Yeah, that's not going to happen.  As a rule, irq numbers are always
unsigned, but there are a few apis that can return either '0' meaning
no irq, or a negative value indicating an error.  The irq_alloc_desc
apis unfortunately are one such case.

g.