Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1337120 > unrolled thread

[PATCH] rtc: max77686: fix irqf_oneshot.cocci warnings

Back to article view | Back to linux.kernel

Contents

  [PATCH] rtc: max77686: fix irqf_oneshot.cocci warnings Valentin Rothberg <valentin.rothberg@posteo.net> - 2016-02-18 09:10 +0100
    Re: [PATCH] rtc: max77686: fix irqf_oneshot.cocci warnings Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-02-18 09:20 +0100
      Re: [PATCH] rtc: max77686: fix irqf_oneshot.cocci warnings Valentin Rothberg <valentin.rothberg@posteo.net> - 2016-02-18 09:50 +0100
        Re: [PATCH] rtc: max77686: fix irqf_oneshot.cocci warnings Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-02-18 10:00 +0100
          Re: [PATCH] rtc: max77686: fix irqf_oneshot.cocci warnings Valentin Rothberg <valentin.rothberg@posteo.net> - 2016-02-18 10:10 +0100
      Re: [PATCH] rtc: max77686: fix irqf_oneshot.cocci warnings Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2016-02-18 10:00 +0100
        Re: [PATCH] rtc: max77686: fix irqf_oneshot.cocci warnings Valentin Rothberg <valentin.rothberg@posteo.net> - 2016-02-18 10:30 +0100

#1337120 — [PATCH] rtc: max77686: fix irqf_oneshot.cocci warnings

From: kbuild test robot <fengguang.wu@intel.com>

 Since commit 1c6c69525b40 ("genirq: Reject bogus threaded irq requests")
 threaded IRQs without a primary handler need to be requested with
 IRQF_ONESHOT, otherwise the request will fail.

 So pass the IRQF_ONESHOT flag in this case.

Generated by: scripts/coccinelle/misc/irqf_oneshot.cocci

CC: Laxman Dewangan <ldewangan@nvidia.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
Signed-off-by: Valentin Rothberg <valentin.rothberg@posteo.net>
---
 drivers/rtc/rtc-max77686.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-max77686.c b/drivers/rtc/rtc-max77686.c
index 5e924f3cde90..027ee7ebfff4 100644
--- a/drivers/rtc/rtc-max77686.c
+++ b/drivers/rtc/rtc-max77686.c
@@ -742,8 +742,8 @@ static int max77686_rtc_probe(struct platform_device *pdev)
 		goto err_rtc;
 	}
 
-	ret = request_threaded_irq(info->virq, NULL, max77686_rtc_alarm_irq, 0,
-				   "rtc-alarm1", info);
+	ret = request_threaded_irq(info->virq, NULL, max77686_rtc_alarm_irq,
+				   IRQF_ONESHOT, "rtc-alarm1", info);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
 			info->virq, ret);
-- 
2.5.0

[toc] | [next] | [standalone]

#1337125

On 18.02.2016 17:06, Valentin Rothberg wrote:
> From: kbuild test robot <fengguang.wu@intel.com>
> 
>  Since commit 1c6c69525b40 ("genirq: Reject bogus threaded irq requests")
>  threaded IRQs without a primary handler need to be requested with
>  IRQF_ONESHOT, otherwise the request will fail.
> 
>  So pass the IRQF_ONESHOT flag in this case.
> 
> Generated by: scripts/coccinelle/misc/irqf_oneshot.cocci
> 
> CC: Laxman Dewangan <ldewangan@nvidia.com>
> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
> Signed-off-by: Valentin Rothberg <valentin.rothberg@posteo.net>
> ---
>  drivers/rtc/rtc-max77686.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Nack, because:
1. AFAIR this is a false positive.
2. Was it tested? Was it reproduced? Was the bug actually spotted or
just coccicheck pointed this and you assumed that "request will fail"?

Coccicheck is a great tool... but not necessarily for pointing run-time
bugs.

Best regards,
Krzysztof

[toc] | [prev] | [next] | [standalone]

#1337151

Hi Krzysztof,

On 2/18/16 9:13 AM, Krzysztof Kozlowski wrote:
> On 18.02.2016 17:06, Valentin Rothberg wrote:
>> From: kbuild test robot <fengguang.wu@intel.com>
>>
>>  Since commit 1c6c69525b40 ("genirq: Reject bogus threaded irq requests")
>>  threaded IRQs without a primary handler need to be requested with
>>  IRQF_ONESHOT, otherwise the request will fail.
>>
>>  So pass the IRQF_ONESHOT flag in this case.
>>
>> Generated by: scripts/coccinelle/misc/irqf_oneshot.cocci
>>
>> CC: Laxman Dewangan <ldewangan@nvidia.com>
>> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
>> Signed-off-by: Valentin Rothberg <valentin.rothberg@posteo.net>
>> ---
>>  drivers/rtc/rtc-max77686.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
> 
> Nack, because:
> 1. AFAIR this is a false positive.

Looking at kernel/irq/manage.c +1250 such requests will be rejected
unconditionally when the primary handler is NULL, except when the chip
is marked to be oneshot safe.

Is there another semantic that I am not aware of?  In case the script
produces false positives, I will change it immediately.

> 2. Was it tested? Was it reproduced? Was the bug actually spotted or
> just coccicheck pointed this and you assumed that "request will fail"?
> 
> Coccicheck is a great tool... but not necessarily for pointing run-time
> bugs.

I did not test it.  To me the issue rather seems seems like something
where Coccinelle is really good at, static analysis.

Kind regards,
 Valentin

[toc] | [prev] | [next] | [standalone]

#1337163

On 18.02.2016 17:46, Valentin Rothberg wrote:
> 
> 
> Hi Krzysztof,
> 
> On 2/18/16 9:13 AM, Krzysztof Kozlowski wrote:
>> On 18.02.2016 17:06, Valentin Rothberg wrote:
>>> From: kbuild test robot <fengguang.wu@intel.com>
>>>
>>>  Since commit 1c6c69525b40 ("genirq: Reject bogus threaded irq requests")
>>>  threaded IRQs without a primary handler need to be requested with
>>>  IRQF_ONESHOT, otherwise the request will fail.
>>>
>>>  So pass the IRQF_ONESHOT flag in this case.
>>>
>>> Generated by: scripts/coccinelle/misc/irqf_oneshot.cocci
>>>
>>> CC: Laxman Dewangan <ldewangan@nvidia.com>
>>> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
>>> Signed-off-by: Valentin Rothberg <valentin.rothberg@posteo.net>
>>> ---
>>>  drivers/rtc/rtc-max77686.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>
>> Nack, because:
>> 1. AFAIR this is a false positive.
> 
> Looking at kernel/irq/manage.c +1250 such requests will be rejected
> unconditionally when the primary handler is NULL, except when the chip
> is marked to be oneshot safe.
> 
> Is there another semantic that I am not aware of?  In case the script
> produces false positives, I will change it immediately.

The handler is "irq_nested_primary_handler".

>> 2. Was it tested? Was it reproduced? Was the bug actually spotted or
>> just coccicheck pointed this and you assumed that "request will fail"?
>>
>> Coccicheck is a great tool... but not necessarily for pointing run-time
>> bugs.
> 
> I did not test it.  To me the issue rather seems seems like something
> where Coccinelle is really good at, static analysis.

Yet, this is somehow subtle (device inter-dependencies) so it falls out
of static into runtime (I mean runtime analysis is needed).

Best regards,
Krzysztof

[toc] | [prev] | [next] | [standalone]

#1337177

Hi Krzysztof,

On 2/18/16 9:50 AM, Krzysztof Kozlowski wrote:
> On 18.02.2016 17:46, Valentin Rothberg wrote:
>>
>>
>> Hi Krzysztof,
>>
>> On 2/18/16 9:13 AM, Krzysztof Kozlowski wrote:
>>> On 18.02.2016 17:06, Valentin Rothberg wrote:
>>>> From: kbuild test robot <fengguang.wu@intel.com>
>>>>
>>>>  Since commit 1c6c69525b40 ("genirq: Reject bogus threaded irq requests")
>>>>  threaded IRQs without a primary handler need to be requested with
>>>>  IRQF_ONESHOT, otherwise the request will fail.
>>>>
>>>>  So pass the IRQF_ONESHOT flag in this case.
>>>>
>>>> Generated by: scripts/coccinelle/misc/irqf_oneshot.cocci
>>>>
>>>> CC: Laxman Dewangan <ldewangan@nvidia.com>
>>>> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
>>>> Signed-off-by: Valentin Rothberg <valentin.rothberg@posteo.net>
>>>> ---
>>>>  drivers/rtc/rtc-max77686.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>
>>> Nack, because:
>>> 1. AFAIR this is a false positive.
>>
>> Looking at kernel/irq/manage.c +1250 such requests will be rejected
>> unconditionally when the primary handler is NULL, except when the chip
>> is marked to be oneshot safe.
>>
>> Is there another semantic that I am not aware of?  In case the script
>> produces false positives, I will change it immediately.
> 
> The handler is "irq_nested_primary_handler".
> 
>>> 2. Was it tested? Was it reproduced? Was the bug actually spotted or
>>> just coccicheck pointed this and you assumed that "request will fail"?
>>>
>>> Coccicheck is a great tool... but not necessarily for pointing run-time
>>> bugs.
>>
>> I did not test it.  To me the issue rather seems seems like something
>> where Coccinelle is really good at, static analysis.
> 
> Yet, this is somehow subtle (device inter-dependencies) so it falls out
> of static into runtime (I mean runtime analysis is needed).

Thanks for your answer.  I wasn't aware of this at all.

Best regards,
 Valentin

[toc] | [prev] | [next] | [standalone]

#1337169

On 18/02/2016 at 17:13:18 +0900, Krzysztof Kozlowski wrote :
> On 18.02.2016 17:06, Valentin Rothberg wrote:
> > From: kbuild test robot <fengguang.wu@intel.com>
> > 
> >  Since commit 1c6c69525b40 ("genirq: Reject bogus threaded irq requests")
> >  threaded IRQs without a primary handler need to be requested with
> >  IRQF_ONESHOT, otherwise the request will fail.
> > 
> >  So pass the IRQF_ONESHOT flag in this case.
> > 
> > Generated by: scripts/coccinelle/misc/irqf_oneshot.cocci
> > 
> > CC: Laxman Dewangan <ldewangan@nvidia.com>
> > Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
> > Signed-off-by: Valentin Rothberg <valentin.rothberg@posteo.net>
> > ---
> >  drivers/rtc/rtc-max77686.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> 
> Nack, because:
> 1. AFAIR this is a false positive.
> 2. Was it tested? Was it reproduced? Was the bug actually spotted or
> just coccicheck pointed this and you assumed that "request will fail"?
> 
> Coccicheck is a great tool... but not necessarily for pointing run-time
> bugs.
> 

Definitively a false positive.

Julia, I've been receiving quite a lot of those, is it possible to add a
note that this generates false positives to try to stop people from
blindly sending patches? I would have expected Valentin to know that
though.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1337190

Hi Alexandre,

On 2/18/16 9:51 AM, Alexandre Belloni wrote:
> On 18/02/2016 at 17:13:18 +0900, Krzysztof Kozlowski wrote :
>> On 18.02.2016 17:06, Valentin Rothberg wrote:
>>> From: kbuild test robot <fengguang.wu@intel.com>
>>>
>>>  Since commit 1c6c69525b40 ("genirq: Reject bogus threaded irq requests")
>>>  threaded IRQs without a primary handler need to be requested with
>>>  IRQF_ONESHOT, otherwise the request will fail.
>>>
>>>  So pass the IRQF_ONESHOT flag in this case.
>>>
>>> Generated by: scripts/coccinelle/misc/irqf_oneshot.cocci
>>>
>>> CC: Laxman Dewangan <ldewangan@nvidia.com>
>>> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
>>> Signed-off-by: Valentin Rothberg <valentin.rothberg@posteo.net>
>>> ---
>>>  drivers/rtc/rtc-max77686.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>
>> Nack, because:
>> 1. AFAIR this is a false positive.
>> 2. Was it tested? Was it reproduced? Was the bug actually spotted or
>> just coccicheck pointed this and you assumed that "request will fail"?
>>
>> Coccicheck is a great tool... but not necessarily for pointing run-time
>> bugs.
>>
> 
> Definitively a false positive.
> 
> Julia, I've been receiving quite a lot of those, is it possible to add a
> note that this generates false positives to try to stop people from
> blindly sending patches? I would have expected Valentin to know that
> though.

I don't have the device-specific knowledge for this issue.  It really
looked like a true positive to me, so I am sorry for the noise.

A warning about false positives seems promising.

Kind regards,
 Valentin

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web