[PATCH] genirq: error reporting for non-percpu interrupt in request_percpu_irq()

[PATCH] genirq: error reporting for non-percpu interrupt in request_percpu_irq()

[Vinayak Kale]

Return a separate error code when a non-percpu interrupt is passed to
request_percpu_irq().

Suggested-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Vinayak Kale <vkale@apm.com>
---
 kernel/irq/manage.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 514bcfd..c2713ae 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1671,10 +1671,12 @@ int request_percpu_irq(unsigned int irq, irq_handler_t handler,
 		return -EINVAL;
 
 	desc = irq_to_desc(irq);
-	if (!desc || !irq_settings_can_request(desc) ||
-	    !irq_settings_is_per_cpu_devid(desc))
+	if (!desc || !irq_settings_can_request(desc))
 		return -EINVAL;
 
+	if (!irq_settings_is_per_cpu_devid(desc))
+		return -EPERM;
+
 	action = kzalloc(sizeof(struct irqaction), GFP_KERNEL);
 	if (!action)
 		return -ENOMEM;
-- 
1.7.9.5

Re: [PATCH] genirq: error reporting for non-percpu interrupt in request_percpu_irq()

[Vinayak Kale]

Need for this patch is discussed in mail-thread [1].

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/204414.html

Thanks
-Vinayak

On Fri, Oct 18, 2013 at 3:36 PM, Vinayak Kale <vkale@apm.com> wrote:
> Return a separate error code when a non-percpu interrupt is passed to
> request_percpu_irq().
>
> Suggested-by: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Vinayak Kale <vkale@apm.com>
> ---
>  kernel/irq/manage.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 514bcfd..c2713ae 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1671,10 +1671,12 @@ int request_percpu_irq(unsigned int irq, irq_handler_t handler,
>                 return -EINVAL;
>
>         desc = irq_to_desc(irq);
> -       if (!desc || !irq_settings_can_request(desc) ||
> -           !irq_settings_is_per_cpu_devid(desc))
> +       if (!desc || !irq_settings_can_request(desc))
>                 return -EINVAL;
>
> +       if (!irq_settings_is_per_cpu_devid(desc))
> +               return -EPERM;
> +
>         action = kzalloc(sizeof(struct irqaction), GFP_KERNEL);
>         if (!action)
>                 return -ENOMEM;
> --
> 1.7.9.5
>

Re: [PATCH] genirq: error reporting for non-percpu interrupt in request_percpu_irq()

[Thomas Gleixner]

On Fri, 18 Oct 2013, Vinayak Kale wrote:

> Need for this patch is discussed in mail-thread [1].
> 
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/204414.html
> 

I can't see anything which makes use of the separate error code in
that patch.

Thanks,

	tglx

> On Fri, Oct 18, 2013 at 3:36 PM, Vinayak Kale <vkale@apm.com> wrote:
> > Return a separate error code when a non-percpu interrupt is passed to
> > request_percpu_irq().
> >
> > Suggested-by: Will Deacon <will.deacon@arm.com>
> > Signed-off-by: Vinayak Kale <vkale@apm.com>
> > ---
> >  kernel/irq/manage.c |    6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> > index 514bcfd..c2713ae 100644
> > --- a/kernel/irq/manage.c
> > +++ b/kernel/irq/manage.c
> > @@ -1671,10 +1671,12 @@ int request_percpu_irq(unsigned int irq, irq_handler_t handler,
> >                 return -EINVAL;
> >
> >         desc = irq_to_desc(irq);
> > -       if (!desc || !irq_settings_can_request(desc) ||
> > -           !irq_settings_is_per_cpu_devid(desc))
> > +       if (!desc || !irq_settings_can_request(desc))
> >                 return -EINVAL;
> >
> > +       if (!irq_settings_is_per_cpu_devid(desc))
> > +               return -EPERM;
> > +
> >         action = kzalloc(sizeof(struct irqaction), GFP_KERNEL);
> >         if (!action)
> >                 return -ENOMEM;
> > --
> > 1.7.9.5
> >
> 

Re: [PATCH] genirq: error reporting for non-percpu interrupt in request_percpu_irq()

[Vinayak Kale]

Hi Thomas,

On Tue, Oct 29, 2013 at 6:29 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 18 Oct 2013, Vinayak Kale wrote:
>
>> Need for this patch is discussed in mail-thread [1].
>>
>> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/204414.html
>>
>
> I can't see anything which makes use of the separate error code in
> that patch.

It was suggested by Will in response to above mentioned patch.
Will's response can be found here:
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/204491.html

Sorry for wrong link.

Thanks
-Vinayak

Re: [PATCH] genirq: error reporting for non-percpu interrupt in request_percpu_irq()

[Will Deacon]

On Fri, Oct 18, 2013 at 11:06:29AM +0100, Vinayak Kale wrote:
> Return a separate error code when a non-percpu interrupt is passed to
> request_percpu_irq().
> 
> Suggested-by: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Vinayak Kale <vkale@apm.com>
> ---
>  kernel/irq/manage.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 514bcfd..c2713ae 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1671,10 +1671,12 @@ int request_percpu_irq(unsigned int irq, irq_handler_t handler,
>  		return -EINVAL;
>  
>  	desc = irq_to_desc(irq);
> -	if (!desc || !irq_settings_can_request(desc) ||
> -	    !irq_settings_is_per_cpu_devid(desc))
> +	if (!desc || !irq_settings_can_request(desc))
>  		return -EINVAL;
>  
> +	if (!irq_settings_is_per_cpu_devid(desc))
> +		return -EPERM;

<bikeshedding> I reckon -ENODEV is clearer here. </bikeshedding>

Also, there's a counterpart to this code in request_threaded_irq, where I
think we should do something similar.

Will

Re: [PATCH] genirq: error reporting for non-percpu interrupt in request_percpu_irq()

[Vinayak Kale]

Hi Will,


On Fri, Oct 18, 2013 at 4:01 PM, Will Deacon <will.deacon@arm.com> wrote:
> On Fri, Oct 18, 2013 at 11:06:29AM +0100, Vinayak Kale wrote:
>> Return a separate error code when a non-percpu interrupt is passed to
>> request_percpu_irq().
>>
>> Suggested-by: Will Deacon <will.deacon@arm.com>
>> Signed-off-by: Vinayak Kale <vkale@apm.com>
>> ---
>>  kernel/irq/manage.c |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>> index 514bcfd..c2713ae 100644
>> --- a/kernel/irq/manage.c
>> +++ b/kernel/irq/manage.c
>> @@ -1671,10 +1671,12 @@ int request_percpu_irq(unsigned int irq, irq_handler_t handler,
>>               return -EINVAL;
>>
>>       desc = irq_to_desc(irq);
>> -     if (!desc || !irq_settings_can_request(desc) ||
>> -         !irq_settings_is_per_cpu_devid(desc))
>> +     if (!desc || !irq_settings_can_request(desc))
>>               return -EINVAL;
>>
>> +     if (!irq_settings_is_per_cpu_devid(desc))
>> +             return -EPERM;
>
> <bikeshedding> I reckon -ENODEV is clearer here. </bikeshedding>

I looked at the issue as "operation not allowed since interrupt is
non-percpu" and I found below comment for EPERM in
'include/uapi/asm-generic/errno-base.h' . So thought this error code
could be more fitting.

#define EPERM            1      /* Operation not permitted */

>
> Also, there's a counterpart to this code in request_threaded_irq, where I
> think we should do something similar.

So in request_threaded_irq, we would still keep the WARN_ON for percpu
interrupt and return a different error code. Is my understanding
correct?

Re: [PATCH] genirq: error reporting for non-percpu interrupt in request_percpu_irq()

[Vinayak Kale]

On Fri, Oct 18, 2013 at 4:33 PM, Vinayak Kale <vkale@apm.com> wrote:
> Hi Will,
>
>
> On Fri, Oct 18, 2013 at 4:01 PM, Will Deacon <will.deacon@arm.com> wrote:
>> On Fri, Oct 18, 2013 at 11:06:29AM +0100, Vinayak Kale wrote:
>>> Return a separate error code when a non-percpu interrupt is passed to
>>> request_percpu_irq().
>>>
>>> Suggested-by: Will Deacon <will.deacon@arm.com>
>>> Signed-off-by: Vinayak Kale <vkale@apm.com>
>>> ---
>>>  kernel/irq/manage.c |    6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>>> index 514bcfd..c2713ae 100644
>>> --- a/kernel/irq/manage.c
>>> +++ b/kernel/irq/manage.c
>>> @@ -1671,10 +1671,12 @@ int request_percpu_irq(unsigned int irq, irq_handler_t handler,
>>>               return -EINVAL;
>>>
>>>       desc = irq_to_desc(irq);
>>> -     if (!desc || !irq_settings_can_request(desc) ||
>>> -         !irq_settings_is_per_cpu_devid(desc))
>>> +     if (!desc || !irq_settings_can_request(desc))
>>>               return -EINVAL;
>>>
>>> +     if (!irq_settings_is_per_cpu_devid(desc))
>>> +             return -EPERM;
>>
>> <bikeshedding> I reckon -ENODEV is clearer here. </bikeshedding>
>
> I looked at the issue as "operation not allowed since interrupt is
> non-percpu" and I found below comment for EPERM in
> 'include/uapi/asm-generic/errno-base.h' . So thought this error code
> could be more fitting.
>
> #define EPERM            1      /* Operation not permitted */
>
>>
>> Also, there's a counterpart to this code in request_threaded_irq, where I
>> think we should do something similar.
>
> So in request_threaded_irq, we would still keep the WARN_ON for percpu
> interrupt and return a different error code. Is my understanding
> correct?

Please ignore my this comment.
If we do something similar in request_threaded_irq then we will have
to remove WARN_ON for percpu interrupt.