[PATCH] adc:cpcap-adc: Fix IRQ flags for using threaded handler

[PATCH] adc:cpcap-adc: Fix IRQ flags for using threaded handler

[venkat.prashanth2498]

From: Venkat Prashanth B U <venkat.prashanth2498@gmail.com>

Mark the request with ONESHOT using IRQF_ONESHOT.

Semantic patch information:
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

Signed-off-by: Venkat Prashanth B U <venkat.prashanth2498@gmail.com>
---
 drivers/iio/adc/cpcap-adc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/cpcap-adc.c b/drivers/iio/adc/cpcap-adc.c
index 9ad6042..b73fa46 100644
--- a/drivers/iio/adc/cpcap-adc.c
+++ b/drivers/iio/adc/cpcap-adc.c
@@ -1015,9 +1015,9 @@ static int cpcap_adc_probe(struct platform_device *pdev)
 		return -ENODEV;

 	error = devm_request_threaded_irq(&pdev->dev, ddata->irq, NULL,
-					  cpcap_adc_irq_thread,
-					  IRQF_TRIGGER_NONE,
-					  "cpcap-adc", indio_dev);
+				cpcap_adc_irq_thread,
+				IRQF_TRIGGER_NONE | IRQF_ONESHOT,
+				"cpcap-adc", indio_dev);
 	if (error) {
 		dev_err(&pdev->dev, "could not get irq: %i\n",
 			error);
--
1.9.1

Re: [PATCH] adc:cpcap-adc: Fix IRQ flags for using threaded handler

[Jonathan Cameron]

On Thu, 11 Jan 2018 12:41:00 +0530
+ CC: Tony as the author of the driver.

venkat.prashanth2498@gmail.com wrote:

> From: Venkat Prashanth B U <venkat.prashanth2498@gmail.com>
> 
> Mark the request with ONESHOT using IRQF_ONESHOT.
> 
> Semantic patch information:
> 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
> 
> Signed-off-by: Venkat Prashanth B U <venkat.prashanth2498@gmail.com>

This looks right but it is rather odd as this should have broken a long
time ago now when the irq core started getting fussy about this...

Back in 2012

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1c6c69525b

Given driver is from 2017 I'm rather surprised...

Jonathan
> ---
>  drivers/iio/adc/cpcap-adc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/cpcap-adc.c b/drivers/iio/adc/cpcap-adc.c
> index 9ad6042..b73fa46 100644
> --- a/drivers/iio/adc/cpcap-adc.c
> +++ b/drivers/iio/adc/cpcap-adc.c
> @@ -1015,9 +1015,9 @@ static int cpcap_adc_probe(struct platform_device *pdev)
>  		return -ENODEV;
> 
>  	error = devm_request_threaded_irq(&pdev->dev, ddata->irq, NULL,
> -					  cpcap_adc_irq_thread,
> -					  IRQF_TRIGGER_NONE,
> -					  "cpcap-adc", indio_dev);
> +				cpcap_adc_irq_thread,
> +				IRQF_TRIGGER_NONE | IRQF_ONESHOT,
> +				"cpcap-adc", indio_dev);
>  	if (error) {
>  		dev_err(&pdev->dev, "could not get irq: %i\n",
>  			error);
> --
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] adc:cpcap-adc: Fix IRQ flags for using threaded handler

[Tony Lindgren]

Hi,

* Jonathan Cameron <jic23@kernel.org> [180114 12:28]:
> On Thu, 11 Jan 2018 12:41:00 +0530
> + CC: Tony as the author of the driver.
> 
> venkat.prashanth2498@gmail.com wrote:
> 
> > From: Venkat Prashanth B U <venkat.prashanth2498@gmail.com>
> > 
> > Mark the request with ONESHOT using IRQF_ONESHOT.
> > 
> > Semantic patch information:
> > 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
> > 
> > Signed-off-by: Venkat Prashanth B U <venkat.prashanth2498@gmail.com>
> 
> This looks right but it is rather odd as this should have broken a long
> time ago now when the irq core started getting fussy about this...
> 
> Back in 2012
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1c6c69525b
> 
> Given driver is from 2017 I'm rather surprised...

I think this is now automatically set as the controller
flags don't have IRQCHIP_ONESHOT_SAFE set?

That being said, adding the IRQF_ONESHOT does not hurt either
so no objections to adding that if still considered valid.

But at least the patch description needs to be updated
though, things have been working just fine :)

Regards,

Tony

Re: [PATCH] adc:cpcap-adc: Fix IRQ flags for using threaded handler

[Jonathan Cameron]

On Fri, 19 Jan 2018 07:10:08 -0800
Tony Lindgren <tony@atomide.com> wrote:

> Hi,
> 
> * Jonathan Cameron <jic23@kernel.org> [180114 12:28]:
> > On Thu, 11 Jan 2018 12:41:00 +0530
> > + CC: Tony as the author of the driver.
> > 
> > venkat.prashanth2498@gmail.com wrote:
> >   
> > > From: Venkat Prashanth B U <venkat.prashanth2498@gmail.com>
> > > 
> > > Mark the request with ONESHOT using IRQF_ONESHOT.
> > > 
> > > Semantic patch information:
> > > 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
> > > 
> > > Signed-off-by: Venkat Prashanth B U <venkat.prashanth2498@gmail.com>  
> > 
> > This looks right but it is rather odd as this should have broken a long
> > time ago now when the irq core started getting fussy about this...
> > 
> > Back in 2012
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1c6c69525b
> > 
> > Given driver is from 2017 I'm rather surprised...  
> 
> I think this is now automatically set as the controller
> flags don't have IRQCHIP_ONESHOT_SAFE set?
As far as I can tell that's actually used the other way
around...  If ONESHOT_SAFE is set the checks for valid
oneshot are ignored.  There is nothing setting ONESHOT
up in the converse case.

> 
> That being said, adding the IRQF_ONESHOT does not hurt either
> so no objections to adding that if still considered valid.
> 
> But at least the patch description needs to be updated
> though, things have been working just fine :)

Which is curious!  Hmm. I wonder if it has anything to do
with it being IRQF_TYPE_NONE which is pretty rare...

... some digging later ...
Nope, it's because it is a nested interrupt and hence the
default handler is replaced anyway (and largely pointless).

So next question is do we want to fix the warnings that
result and how do we do it.  
We could request an any context irq which would suppress
the warning, but that feels wrong as it has to be a thread..

Maybe marking it (irrelevantly) as ONESHOT is the best
approach.  Perhaps what we should really have is a separate
requester when we know it is nested so that is apparent
to static analysers and they leave us alone.

+CC Thomas to see if he thinks this is crazy or not.

Jonathan

> Regards,
> 
> Tony