[PATCH] xen/arm: setup_dt_irq: don't enable the IRQ if the creation has failed

[PATCH] xen/arm: setup_dt_irq: don't enable the IRQ if the creation has failed

[Julien Grall]

For now __setup_dt_irq can only fail if the action is already set. If in the
future, the function is updated we don't want to enable the IRQ.

Assuming the function can fail with action = NULL, when Xen will receive the
IRQ it will segfault because do_IRQ doesn't check if action is NULL.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/gic.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index e6257a7..62510e3 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -605,8 +605,8 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
     rc = __setup_irq(desc, irq->irq, new);
     spin_unlock_irqrestore(&desc->lock, flags);
 
-    desc->handler->startup(desc);
-
+    if ( !rc )
+        desc->handler->startup(desc);
 
     return rc;
 }
-- 
1.7.10.4

Re: [PATCH] xen/arm: setup_dt_irq: don't enable the IRQ if the creation has failed

[Ian Campbell]

On Fri, 2014-01-10 at 20:49 +0000, Julien Grall wrote:
> For now __setup_dt_irq can only fail if the action is already set.

Can this ever happen with the current code base?

> If in the future, the function is updated we don't want to enable the IRQ.

Such an update is likely to be post-4.4 (unless there is a relationship
with "IRQ: Protect IRQ to be shared between domains and XEN" AND the RM
becomes convinced to grant a freeze exception for that patch).

On that basis this patch could also easily be post-4.4.

> Assuming the function can fail with action = NULL, when Xen will receive the
> IRQ it will segfault because do_IRQ doesn't check if action is NULL.

It seems unlikely that the system would be fully functional after such
an error even with this patch -- it would have failed to register either
timer, maintenance or the console interrupt.

> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/gic.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index e6257a7..62510e3 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -605,8 +605,8 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>      rc = __setup_irq(desc, irq->irq, new);
>      spin_unlock_irqrestore(&desc->lock, flags);
>  
> -    desc->handler->startup(desc);
> -
> +    if ( !rc )
> +        desc->handler->startup(desc);
>  
>      return rc;
>  }

Re: [PATCH] xen/arm: setup_dt_irq: don't enable the IRQ if the creation has failed

[Julien Grall]

On 01/15/2014 01:44 PM, Ian Campbell wrote:
> On Fri, 2014-01-10 at 20:49 +0000, Julien Grall wrote:
>> For now __setup_dt_irq can only fail if the action is already set.
> 
> Can this ever happen with the current code base?

No, that why I didn't ask for a freeze exception for Xen 4.4.

> 
>> If in the future, the function is updated we don't want to enable the IRQ.
> 
> Such an update is likely to be post-4.4 (unless there is a relationship
> with "IRQ: Protect IRQ to be shared between domains and XEN" AND the RM
> becomes convinced to grant a freeze exception for that patch).
> 
> On that basis this patch could also easily be post-4.4.
>
>> Assuming the function can fail with action = NULL, when Xen will receive the
>> IRQ it will segfault because do_IRQ doesn't check if action is NULL.
> 
> It seems unlikely that the system would be fully functional after such
> an error even with this patch -- it would have failed to register either
> timer, maintenance or the console interrupt.

Timer and maintenance code don't check the return of request_dt_irq
(which call setup_dt_irq).

For the console interrupt, the callback which initialize the interrupt
doesn't return an error... On every serial driver, only an error message
is printed.

In any case, it's wrong to enable this IRQ if the descriptor is not
correctly setup.

> 
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/arch/arm/gic.c |    4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index e6257a7..62510e3 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -605,8 +605,8 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>>      rc = __setup_irq(desc, irq->irq, new);
>>      spin_unlock_irqrestore(&desc->lock, flags);
>>  
>> -    desc->handler->startup(desc);
>> -
>> +    if ( !rc )
>> +        desc->handler->startup(desc);
>>  
>>      return rc;
>>  }
> 
> 


-- 
Julien Grall

Re: [PATCH] xen/arm: setup_dt_irq: don't enable the IRQ if the creation has failed

[Ian Campbell]

On Wed, 2014-01-15 at 14:01 +0000, Julien Grall wrote:
> On 01/15/2014 01:44 PM, Ian Campbell wrote:
> > On Fri, 2014-01-10 at 20:49 +0000, Julien Grall wrote:
> >> For now __setup_dt_irq can only fail if the action is already set.
> > 
> > Can this ever happen with the current code base?
> 
> No, that why I didn't ask for a freeze exception for Xen 4.4.

Ah ok. It is useful to tag patches which aren't for consideration (and
those which are for that matter, although the exception request suffices
there).

I'll put this in my 4.5 queue and consider it again later. Please ping
me a little while after 4.4 branches if I haven't done so.

Re: [PATCH] xen/arm: setup_dt_irq: don't enable the IRQ if the creation has failed

[Julien Grall]

On 01/15/2014 02:07 PM, Ian Campbell wrote:
> On Wed, 2014-01-15 at 14:01 +0000, Julien Grall wrote:
>> On 01/15/2014 01:44 PM, Ian Campbell wrote:
>>> On Fri, 2014-01-10 at 20:49 +0000, Julien Grall wrote:
>>>> For now __setup_dt_irq can only fail if the action is already set.
>>>
>>> Can this ever happen with the current code base?
>>
>> No, that why I didn't ask for a freeze exception for Xen 4.4.
> 
> Ah ok. It is useful to tag patches which aren't for consideration (and
> those which are for that matter, although the exception request suffices
> there).

Do you have a specific tag in my mind for this purpose?

> I'll put this in my 4.5 queue and consider it again later. Please ping
> me a little while after 4.4 branches if I haven't done so.

Thanks.

-- 
Julien Grall

Re: [PATCH] xen/arm: setup_dt_irq: don't enable the IRQ if the creation has failed

[Ian Campbell]

On Wed, 2014-01-15 at 14:08 +0000, Julien Grall wrote:
> On 01/15/2014 02:07 PM, Ian Campbell wrote:
> > On Wed, 2014-01-15 at 14:01 +0000, Julien Grall wrote:
> >> On 01/15/2014 01:44 PM, Ian Campbell wrote:
> >>> On Fri, 2014-01-10 at 20:49 +0000, Julien Grall wrote:
> >>>> For now __setup_dt_irq can only fail if the action is already set.
> >>>
> >>> Can this ever happen with the current code base?
> >>
> >> No, that why I didn't ask for a freeze exception for Xen 4.4.
> > 
> > Ah ok. It is useful to tag patches which aren't for consideration (and
> > those which are for that matter, although the exception request suffices
> > there).
> 
> Do you have a specific tag in my mind for this purpose?

Anything would do, either [PATCH for-4.5] in $subject or:
        ---
        This patch is for 4.5
after the commit message etc.

Ian.

Re: [PATCH] xen/arm: setup_dt_irq: don't enable the IRQ if the creation has failed

[Julien Grall]

On 01/15/2014 02:07 PM, Ian Campbell wrote:
> 
> I'll put this in my 4.5 queue and consider it again later. Please ping
> me a little while after 4.4 branches if I haven't done so.

I plan to send a bigger patch series today on interrupt management for
ARM. The patch will be include in this serie. You can remove the patch
from your 4.5 queue for now.

-- 
Julien Grall