Re: [PATCH v2] coccinelle: api: check for propagation of error from platform_get_irq

[Sergei Shtylyov]

On 12/27/2015 9:13 AM, Julia Lawall wrote:


Well, looking again, the patch should be good. I just thought its goal was
to fix the code as well...


I could do that for the irq < 0 case, but I think that in that case, kbuild
will only run the patch version, and the <= cases will not be reported on.
I don't have a general fix for the <= 0.  Is it even correct to have < in
some cases and <= in others?


   That's a good question...
   In my prior fixes of this case I preferred to consider IRQ0 valid and so 
used 'irq < 0'. I myself don't share the "IRQ0 is invalid" sentiment...



julia


MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] coccinelle: api: check for propagation of error from platform_get_irq

[Julia Lawall]

On Sun, 27 Dec 2015, Sergei Shtylyov wrote:

> On 12/26/2015 11:58 PM, Julia Lawall wrote:
> 
> > The error return value of platform_get_irq seems to often get dropped.
> > 
> > Signed-off-by: Julia Lawall 
> > 
> > ---
> > 
> > v2: Check for the direct return case also.  Added some mailing lists of
> > common offenders.
> > 
> > diff --git a/scripts/coccinelle/api/platform_get_irq_return.cocci
> > b/scripts/coccinelle/api/platform_get_irq_return.cocci
> > new file mode 100644
> > index 000..44680d0
> > --- /dev/null
> > +++ b/scripts/coccinelle/api/platform_get_irq_return.cocci
> > @@ -0,0 +1,58 @@
> > +/// Propagate the return value of platform_get_irq.
> > +//# Sometimes the return value of platform_get_irq is tested using <= 0,
> > but 0
> > +//# might not be an appropriate return value in an error case.
> > +///
> > +// Confidence: Moderate
> > +// Copyright: (C) 2015 Julia Lawall, Inria. GPLv2.
> > +// URL: http://coccinelle.lip6.fr/
> > +// Options: --no-includes --include-headers
> > +
> > +virtual context
> > +virtual org
> > +virtual report
> > +
> > +//
> > 
> > +
> > +@r depends on context || org || report@
> > +constant C;
> > +statement S;
> > +expression e, ret;
> > +position j0, j1;
> > +@@
> > +
> > +* e@j0 = platform_get_irq(...);
> > +(
> > +if@j1 (...) {
> > +  ...
> > +  return -C;
> > +} else S
> > +|
> > +if@j1 (...) {
> > +  ...
> > +  ret = -C;
> > +  ...
> > +  return ret;
> > +} else S
> 
>Well, this seems to also cover the (e <= 0) checks which do make same sense
> in the light of Linus considering IRQ0 invalid. So I'd be more specific about
> the checks here -- 0 should indeed be overridden with something if it's
> considered invalid.

That's what the limitations section says (lines with #).  This doesn't 
make any changes, it only makes warnings, which should include the 
limitations information, so perhaps people can consider what it is that 
they really intend to do.

If you think this is not a good idea, then I can make the test more 
specific.

julia
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] coccinelle: api: check for propagation of error from platform_get_irq

[Sergei Shtylyov]

On 12/27/2015 01:32 AM, Julia Lawall wrote:


The error return value of platform_get_irq seems to often get dropped.

Signed-off-by: Julia Lawall 

---

v2: Check for the direct return case also.  Added some mailing lists of
common offenders.

diff --git a/scripts/coccinelle/api/platform_get_irq_return.cocci
b/scripts/coccinelle/api/platform_get_irq_return.cocci
new file mode 100644
index 000..44680d0
--- /dev/null
+++ b/scripts/coccinelle/api/platform_get_irq_return.cocci
@@ -0,0 +1,58 @@
+/// Propagate the return value of platform_get_irq.
+//# Sometimes the return value of platform_get_irq is tested using <= 0,
but 0
+//# might not be an appropriate return value in an error case.
+///
+// Confidence: Moderate
+// Copyright: (C) 2015 Julia Lawall, Inria. GPLv2.
+// URL: http://coccinelle.lip6.fr/
+// Options: --no-includes --include-headers
+
+virtual context
+virtual org
+virtual report
+
+//

+
+@r depends on context || org || report@
+constant C;
+statement S;
+expression e, ret;
+position j0, j1;
+@@
+
+* e@j0 = platform_get_irq(...);
+(
+if@j1 (...) {
+  ...
+  return -C;
+} else S
+|
+if@j1 (...) {
+  ...
+  ret = -C;
+  ...
+  return ret;
+} else S


Well, this seems to also cover the (e <= 0) checks which do make same sense
in the light of Linus considering IRQ0 invalid. So I'd be more specific about
the checks here -- 0 should indeed be overridden with something if it's
considered invalid.


That's what the limitations section says (lines with #).  This doesn't


   Ah, failed to notice those, only saw after replying.


make any changes, it only makes warnings, which should include the
limitations information, so perhaps people can consider what it is that
they really intend to do.

>

If you think this is not a good idea, then I can make the test more
specific.


   Well, looking again, the patch should be good. I just thought its goal was 
to fix the code as well...



julia


MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] coccinelle: api: check for propagation of error from platform_get_irq

[Sergei Shtylyov]

On 12/26/2015 11:58 PM, Julia Lawall wrote:


The error return value of platform_get_irq seems to often get dropped.

Signed-off-by: Julia Lawall 

---

v2: Check for the direct return case also.  Added some mailing lists of
common offenders.

diff --git a/scripts/coccinelle/api/platform_get_irq_return.cocci 
b/scripts/coccinelle/api/platform_get_irq_return.cocci
new file mode 100644
index 000..44680d0
--- /dev/null
+++ b/scripts/coccinelle/api/platform_get_irq_return.cocci
@@ -0,0 +1,58 @@
+/// Propagate the return value of platform_get_irq.
+//# Sometimes the return value of platform_get_irq is tested using <= 0, but 0
+//# might not be an appropriate return value in an error case.
+///
+// Confidence: Moderate
+// Copyright: (C) 2015 Julia Lawall, Inria. GPLv2.
+// URL: http://coccinelle.lip6.fr/
+// Options: --no-includes --include-headers
+
+virtual context
+virtual org
+virtual report
+
+// 
+
+@r depends on context || org || report@
+constant C;
+statement S;
+expression e, ret;
+position j0, j1;
+@@
+
+* e@j0 = platform_get_irq(...);
+(
+if@j1 (...) {
+  ...
+  return -C;
+} else S
+|
+if@j1 (...) {
+  ...
+  ret = -C;
+  ...
+  return ret;
+} else S


   Well, this seems to also cover the (e <= 0) checks which do make same 
sense in the light of Linus considering IRQ0 invalid. So I'd be more specific 
about the checks here -- 0 should indeed be overridden with something if it's 
considered invalid.


MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] coccinelle: api: check for propagation of error from platform_get_irq

[Julia Lawall]

>Well, looking again, the patch should be good. I just thought its goal was
> to fix the code as well...

I could do that for the irq < 0 case, but I think that in that case, kbuild
will only run the patch version, and the <= cases will not be reported on.
I don't have a general fix for the <= 0.  Is it even correct to have < in
some cases and <= in others?

julia
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html