Re: PATCH[[vme/bridges/vme_ca91cx42.c:1382: Bad if test Bug Fix]‏‏

Re: PATCH[[vme/bridges/vme_ca91cx42.c:1382: Bad if test Bug Fix]‏‏

[nick]

Here is the fixed patch. Having issues with using Thunderbird
so just used Evolution for now.
Nick 
--- drivers/vme/bridges/vme_ca91cx42.h.orig	2014-06-11 22:50:29.339671939 -0400
+++ drivers/vme/bridges/vme_ca91cx42.h	2014-06-11 23:15:36.027685173 -0400
Fixes bug issues with wrong bus width in if statments in vme_ca91cx42.c
Signed-off-by: Nicholas Krause <nickkrause@sympatico.ca>
@@ -526,7 +526,7 @@ static const int CA91CX42_LINT_LM[] = {
 #define CA91CX42_VSI_CTL_SUPER_SUPR	(1<<21)
 
 #define CA91CX42_VSI_CTL_VAS_M		(7<<16)
-#define CA91CX42_VSI_CTL_VAS_A16	0
+#define CA91CX42_VSI_CTL_VAS_A16	(3<<16)
 #define CA91CX42_VSI_CTL_VAS_A24	(1<<16)
 #define CA91CX42_VSI_CTL_VAS_A32	(1<<17)
 #define CA91CX42_VSI_CTL_VAS_USER1	(3<<17)
@@ -549,7 +549,7 @@ static const int CA91CX42_LINT_LM[] = {
 #define CA91CX42_LM_CTL_SUPR		(1<<21)
 #define CA91CX42_LM_CTL_NPRIV		(1<<20)
 #define CA91CX42_LM_CTL_AS_M		(5<<16)
-#define CA91CX42_LM_CTL_AS_A16		0
+#define CA91CX42_LM_CTL_AS_A16		(3<<16)
 #define CA91CX42_LM_CTL_AS_A24		(1<<16)
 #define CA91CX42_LM_CTL_AS_A32		(1<<17)

Re: PATCH[[vme/bridges/vme_ca91cx42.c:1382: Bad if test Bug Fix]‏‏

[Dan Carpenter]

On Thu, Jun 12, 2014 at 10:33:09AM -0400, nick wrote:
> Here is the fixed patch. Having issues with using Thunderbird
> so just used Evolution for now.
> Nick 

Please read the first paragraph of:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/Documentation/email-clients.txt

It's not clear from reading this email why 0 is incorrect and 3<<16 is
correct.  Explain in the changelog.  What are the user visible effects
of this bug etc.

The patch must have a signed-off-by line.

Here is what typical patch emails should look like:
https://lkml.org/lkml/2014/6/12/377
https://lkml.org/lkml/2014/6/12/369
https://lkml.org/lkml/2014/6/12/299

The subject should be:

[PATCH] vme: ca91cx42: Bad if test something something

Make sure it apply with `git am` and review the log.

regards,
dan carpenter

Re: PATCH[[vme/bridges/vme_ca91cx42.c:1382: Bad if test Bug Fix]‏‏

[Dan Carpenter]

Martyn,

Nick Krause emailed me privately that he's not able to get git to work.
Normally for bug fixes, we would patch this up ourselves.

The offending code was introduced in:

commit 2b82beb8c1bc81b3dde69d16cacbc22546681acf
Author: Martyn Welch <martyn.welch@ge.com>
Date:   Thu Feb 18 15:13:19 2010 +0000

    Staging: vme: Add location monitor support for ca91cx42

The |= 0 does look very suspicious and I have a static checker warning
for code like that.  Certainly it sounds very authoritative to say that
instead of zero, CA91CX42_LM_CTL_AS_A16 should be (3 << 16).  But then
the condition:

	if ((lm_ctl & (5 << 16)) == (3 << 16))

is always false...  It is puzzling.

I don't know this code and I have no idea what is correct.  Was this a
real bug that Nick hit in testing, or was it a static checker fix?  He
hasn't told us any of this stuff...

What to do...

regards,
dan carpenter

Re: PATCH[[vme/bridges/vme_ca91cx42.c:1382: Bad if test Bug Fix]‏‏

[Martyn Welch]

Nick,

Sorry for the delay in responding.

I'm staring at the manual for the ca91c142 and the relevant bits in the 
VSIx_CTL registers definitely need to be set to 0 for A16, likewise with 
the LM_CTL register. The pattern (3<<16) would enable one of the 
"reserved" address spaces.

Martyn

On 12/06/14 15:33, nick wrote:
> Here is the fixed patch. Having issues with using Thunderbird
> so just used Evolution for now.
> Nick
> --- drivers/vme/bridges/vme_ca91cx42.h.orig	2014-06-11 22:50:29.339671939 -0400
> +++ drivers/vme/bridges/vme_ca91cx42.h	2014-06-11 23:15:36.027685173 -0400
> Fixes bug issues with wrong bus width in if statments in vme_ca91cx42.c
> Signed-off-by: Nicholas Krause <nickkrause@sympatico.ca>
> @@ -526,7 +526,7 @@ static const int CA91CX42_LINT_LM[] = {
>   #define CA91CX42_VSI_CTL_SUPER_SUPR	(1<<21)
>
>   #define CA91CX42_VSI_CTL_VAS_M		(7<<16)
> -#define CA91CX42_VSI_CTL_VAS_A16	0
> +#define CA91CX42_VSI_CTL_VAS_A16	(3<<16)
>   #define CA91CX42_VSI_CTL_VAS_A24	(1<<16)
>   #define CA91CX42_VSI_CTL_VAS_A32	(1<<17)
>   #define CA91CX42_VSI_CTL_VAS_USER1	(3<<17)
> @@ -549,7 +549,7 @@ static const int CA91CX42_LINT_LM[] = {
>   #define CA91CX42_LM_CTL_SUPR		(1<<21)
>   #define CA91CX42_LM_CTL_NPRIV		(1<<20)
>   #define CA91CX42_LM_CTL_AS_M		(5<<16)
> -#define CA91CX42_LM_CTL_AS_A16		0
> +#define CA91CX42_LM_CTL_AS_A16		(3<<16)
>   #define CA91CX42_LM_CTL_AS_A24		(1<<16)
>   #define CA91CX42_LM_CTL_AS_A32		(1<<17)
>
>
>

-- 
Martyn Welch (Lead Software Engineer)  | Registered in England and Wales
GE Intelligent Platforms               | (3828642) at 100 Barbirolli Square
T +44(0)1327322748                     | Manchester, M2 3AB
E martyn.welch@ge.com                  | VAT:GB 927559189

Re: PATCH[[vme/bridges/vme_ca91cx42.c:1382: Bad if test Bug Fix]‏‏

[Dan Carpenter]

On Mon, Jun 16, 2014 at 10:47:25AM +0100, Martyn Welch wrote:
> Nick,
> 
> Sorry for the delay in responding.
> 
> I'm staring at the manual for the ca91c142 and the relevant bits in
> the VSIx_CTL registers definitely need to be set to 0 for A16,
> likewise with the LM_CTL register. The pattern (3<<16) would enable
> one of the "reserved" address spaces.
> 

Nick emailed me privately that this was a static checker warning.  These
warnings are often false positives...  But I'm worried about the test:

	if ((ctl & CA91CX42_VSI_CTL_VAS_M) == CA91CX42_VSI_CTL_VAS_A16)
		*aspace = VME_A16;

That could be true when we didn't intend it.

regards,
dan carpenter

Re: PATCH[[vme/bridges/vme_ca91cx42.c:1382: Bad if test Bug Fix]‏‏

[Martyn Welch]

On 16/06/14 10:56, Dan Carpenter wrote:
> On Mon, Jun 16, 2014 at 10:47:25AM +0100, Martyn Welch wrote:
>> Nick,
>>
>> Sorry for the delay in responding.
>>
>> I'm staring at the manual for the ca91c142 and the relevant bits in
>> the VSIx_CTL registers definitely need to be set to 0 for A16,
>> likewise with the LM_CTL register. The pattern (3<<16) would enable
>> one of the "reserved" address spaces.
>>
>
> Nick emailed me privately that this was a static checker warning.  These
> warnings are often false positives...  But I'm worried about the test:
>
> 	if ((ctl & CA91CX42_VSI_CTL_VAS_M) == CA91CX42_VSI_CTL_VAS_A16)
> 		*aspace = VME_A16;
>
> That could be true when we didn't intend it.
>

If I'm not mistaken, CA91CX42_VSI_CTL_VAS_A16 is currently defined as 0.

So:
	if ((ctl & (7<<16) == 0)
		*aspace = VME_A16;

Which looks right to me, it's checking to see if the relevant bits in 
the register are all zero, am I missing something obvious?

Martyn


-- 
Martyn Welch (Lead Software Engineer)  | Registered in England and Wales
GE Intelligent Platforms               | (3828642) at 100 Barbirolli Square
T +44(0)1327322748                     | Manchester, M2 3AB
E martyn.welch@ge.com                  | VAT:GB 927559189

Re: PATCH[[vme/bridges/vme_ca91cx42.c:1382: Bad if test Bug Fix]‏

[gregkh]

On Thu, Jun 12, 2014 at 04:17:36AM +0000, Nick Krause wrote:
> Here is the fixed patch as per Greg's recommendations. Unforunalty my email
> client removes tabs so I will have to be sending it as a patch file if that's
> Ok.
> Nick

HTML is rejected by the mailing lists, and we can't take a base64
attachment either :(

Take a look at Documentation/email_clients.txt for ideas on how to fix
this up on your end.

thanks,

greg k-h

PATCH[vme/bridges/vme_ca91cx42.c:1382: Bad if test Bug Fix]

[Nick Krause]

Hey Fellow Developers, 
This is my first patch so if there are any errors please reply as i will 
fix them. Below is the patch.
-- drivers/vme/bridges/vme_ca91cx42.h.orig    2014-06-11 22:50:29.339671939 -0400
+++ drivers/vme/bridges/vme_ca91cx42.h    2014-06-11 23:15:36.027685173 -0400
@@ -526,7 +526,7 @@ static const int CA91CX42_LINT_LM[] = {
 #define CA91CX42_VSI_CTL_SUPER_SUPR    (1<<21)
 
 #define CA91CX42_VSI_CTL_VAS_M        (7<<16)
-#define CA91CX42_VSI_CTL_VAS_A16    0
+#define CA91CX42_VSI_CTL_VAS_A16    (3<<16)
 #define CA91CX42_VSI_CTL_VAS_A24    (1<<16)
 #define CA91CX42_VSI_CTL_VAS_A32    (1<<17)
 #define CA91CX42_VSI_CTL_VAS_USER1    (3<<17)
@@ -549,7 +549,7 @@ static const int CA91CX42_LINT_LM[] = {
 #define CA91CX42_LM_CTL_SUPR        (1<<21)
 #define CA91CX42_LM_CTL_NPRIV        (1<<20)
 #define CA91CX42_LM_CTL_AS_M        (5<<16)
-#define CA91CX42_LM_CTL_AS_A16        0
+#define CA91CX42_LM_CTL_AS_A16        (3<<16)
 #define CA91CX42_LM_CTL_AS_A24        (1<<16)
 #define CA91CX42_LM_CTL_AS_A32        (1<<17)
Signed-off-by: Nicholas Krause <nickkrause@sympatico.ca>
Nick
 		 	   		  

Re: PATCH[vme/bridges/vme_ca91cx42.c:1382: Bad if test Bug Fix]

[gregkh]

On Thu, Jun 12, 2014 at 03:44:34AM +0000, Nick Krause wrote:
> Hey Fellow Developers, 
> This is my first patch so if there are any errors please reply as i will 
> fix them. Below is the patch.
> -- drivers/vme/bridges/vme_ca91cx42.h.orig    2014-06-11 22:50:29.339671939 -0400
> +++ drivers/vme/bridges/vme_ca91cx42.h    2014-06-11 23:15:36.027685173 -0400
> @@ -526,7 +526,7 @@ static const int CA91CX42_LINT_LM[] = {
>  #define CA91CX42_VSI_CTL_SUPER_SUPR    (1<<21)
>  
>  #define CA91CX42_VSI_CTL_VAS_M        (7<<16)
> -#define CA91CX42_VSI_CTL_VAS_A16    0
> +#define CA91CX42_VSI_CTL_VAS_A16    (3<<16)
>  #define CA91CX42_VSI_CTL_VAS_A24    (1<<16)
>  #define CA91CX42_VSI_CTL_VAS_A32    (1<<17)
>  #define CA91CX42_VSI_CTL_VAS_USER1    (3<<17)
> @@ -549,7 +549,7 @@ static const int CA91CX42_LINT_LM[] = {
>  #define CA91CX42_LM_CTL_SUPR        (1<<21)
>  #define CA91CX42_LM_CTL_NPRIV        (1<<20)
>  #define CA91CX42_LM_CTL_AS_M        (5<<16)
> -#define CA91CX42_LM_CTL_AS_A16        0
> +#define CA91CX42_LM_CTL_AS_A16        (3<<16)
>  #define CA91CX42_LM_CTL_AS_A24        (1<<16)
>  #define CA91CX42_LM_CTL_AS_A32        (1<<17)
> Signed-off-by: Nicholas Krause <nickkrause@sympatico.ca>

Always run your patch through scripts/checkpatch.pl first to catch the
issues that are 'obvious'.

After that, the signed-off-by: needs to be up in the changelog area,
there needs to be a changelog explaining why this patch is needed, and
the tabs need to be put back in the patch (your email client ate them.)

Can you try again?

thanks,

greg k-h