* [PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from().
@ 2012-04-05 23:52 David Daney
[not found] ` <1333669933-25267-1-git-send-email-ddaney.cavm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2012-04-07 1:26 ` Grant Likely
0 siblings, 2 replies; 9+ messages in thread
From: David Daney @ 2012-04-05 23:52 UTC (permalink / raw)
To: devicetree-discuss, Grant Likely, Rob Herring,
Benjamin Herrenschmidt, Thomas Gleixner
Cc: linux-mips, linux-kernel, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread[parent not found: <1333669933-25267-1-git-send-email-ddaney.cavm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from().
[not found] ` <1333669933-25267-1-git-send-email-ddaney.cavm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2012-04-06 3:37 ` Rob Herring
[not found] ` <4F7E64E4.3080509-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 9+ messages in thread
From: Rob Herring @ 2012-04-06 3:37 UTC (permalink / raw)
To: David Daney
Cc: linux-mips-6z/3iImG2C8G8FEW9MqTrA, David Daney,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Thomas Gleixner
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);
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from().
2012-04-05 23:52 [PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from() David Daney
[not found] ` <1333669933-25267-1-git-send-email-ddaney.cavm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2012-04-07 1:26 ` Grant Likely
2012-04-09 16:56 ` David Daney
1 sibling, 1 reply; 9+ messages in thread
From: Grant Likely @ 2012-04-07 1:26 UTC (permalink / raw)
To: David Daney, devicetree-discuss, Rob Herring,
Benjamin Herrenschmidt, Thomas Gleixner
Cc: linux-mips, linux-kernel, David Daney
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.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from().
2012-04-07 1:26 ` Grant Likely
@ 2012-04-09 16:56 ` David Daney
2012-04-10 20:41 ` Grant Likely
0 siblings, 1 reply; 9+ messages in thread
From: David Daney @ 2012-04-09 16:56 UTC (permalink / raw)
To: Grant Likely
Cc: David Daney, devicetree-discuss, Rob Herring,
Benjamin Herrenschmidt, Thomas Gleixner, linux-mips, linux-kernel
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
>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from().
2012-04-09 16:56 ` David Daney
@ 2012-04-10 20:41 ` Grant Likely
0 siblings, 0 replies; 9+ messages in thread
From: Grant Likely @ 2012-04-10 20:41 UTC (permalink / raw)
Cc: David Daney, devicetree-discuss, Rob Herring,
Benjamin Herrenschmidt, Thomas Gleixner, linux-mips, linux-kernel
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-04-10 20:41 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-04-05 23:52 [PATCH] irq/irq_domain: Quit ignoring error returns from irq_alloc_desc_from() David Daney
[not found] ` <1333669933-25267-1-git-send-email-ddaney.cavm-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2012-04-06 3:37 ` Rob Herring
[not found] ` <4F7E64E4.3080509-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2012-04-06 16:37 ` David Daney
2012-04-06 17:32 ` Rob Herring
2012-04-06 23:56 ` Grant Likely
2012-04-09 16:52 ` David Daney
2012-04-07 1:26 ` Grant Likely
2012-04-09 16:56 ` David Daney
2012-04-10 20:41 ` Grant Likely
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).