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 <[email protected]>
@@ -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)
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
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 <[email protected]>
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
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
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 [email protected] | VAT:GB 927559189
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 <[email protected]>
> @@ -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 [email protected] | VAT:GB 927559189