2009-04-08 21:58:33

by Jeff Haran

[permalink] [raw]
Subject: bug in drivers/edac/mpc85xx_edac.c:mpc85xx_mc_check() ?

Hi,

Recent versions of this function start off with:

static void mpc85xx_mc_check(struct mem_ctl_info *mci)
{
struct mpc85xx_mc_pdata *pdata = mci->pvt_info;
...

err_detect = in_be32(pdata->mc_vbase + MPC85XX_MC_ERR_DETECT);
if (err_detect)
return;

...
}

My reading of the Freescale 8548E Manual leads me to conclude that the
Memory Error Detect register (ERR_DETECT) will have various bits set if
the memory controller has detected an error since the last time it was
cleared. If no memory error has occurred, the register will contain 0.

Perhaps I am missing something very basic, but it seem to me that the
above "if" should be:

if (!err_detect)
return;

as the existing code would seem to read "if any errors have occurred,
ignore them", though perhaps testing has demonstrated that the Freescale
manual is in error.

Please include this email address in responses as I do not subscribe.

Thanks,

Jeff Haran
Brocade


2009-04-10 21:51:54

by Andrew Morton

[permalink] [raw]
Subject: Re: bug in drivers/edac/mpc85xx_edac.c:mpc85xx_mc_check() ?

(cc's added)

On Wed, 8 Apr 2009 14:57:42 -0700
"Jeff Haran" <[email protected]> wrote:

> Hi,
>
> Recent versions of this function start off with:
>
> static void mpc85xx_mc_check(struct mem_ctl_info *mci)
> {
> struct mpc85xx_mc_pdata *pdata = mci->pvt_info;
> ...
>
> err_detect = in_be32(pdata->mc_vbase + MPC85XX_MC_ERR_DETECT);
> if (err_detect)
> return;
>
> ...
> }
>
> My reading of the Freescale 8548E Manual leads me to conclude that the
> Memory Error Detect register (ERR_DETECT) will have various bits set if
> the memory controller has detected an error since the last time it was
> cleared. If no memory error has occurred, the register will contain 0.
>
> Perhaps I am missing something very basic, but it seem to me that the
> above "if" should be:
>
> if (!err_detect)
> return;
>
> as the existing code would seem to read "if any errors have occurred,
> ignore them", though perhaps testing has demonstrated that the Freescale
> manual is in error.
>
> Please include this email address in responses as I do not subscribe.
>
> Thanks,
>
> Jeff Haran
> Brocade

2009-04-13 17:10:52

by Dave Jiang

[permalink] [raw]
Subject: Re: bug in drivers/edac/mpc85xx_edac.c:mpc85xx_mc_check() ?

Jeff, you are correct. I will submit a patch to correct that.

Andrew Morton wrote:
> (cc's added)
>
> On Wed, 8 Apr 2009 14:57:42 -0700
> "Jeff Haran" <[email protected]> wrote:
>
>> Hi,
>>
>> Recent versions of this function start off with:
>>
>> static void mpc85xx_mc_check(struct mem_ctl_info *mci)
>> {
>> struct mpc85xx_mc_pdata *pdata = mci->pvt_info;
>> ...
>>
>> err_detect = in_be32(pdata->mc_vbase + MPC85XX_MC_ERR_DETECT);
>> if (err_detect)
>> return;
>>
>> ...
>> }
>>
>> My reading of the Freescale 8548E Manual leads me to conclude that the
>> Memory Error Detect register (ERR_DETECT) will have various bits set if
>> the memory controller has detected an error since the last time it was
>> cleared. If no memory error has occurred, the register will contain 0.
>>
>> Perhaps I am missing something very basic, but it seem to me that the
>> above "if" should be:
>>
>> if (!err_detect)
>> return;
>>
>> as the existing code would seem to read "if any errors have occurred,
>> ignore them", though perhaps testing has demonstrated that the Freescale
>> manual is in error.
>>
>> Please include this email address in responses as I do not subscribe.
>>
>> Thanks,
>>
>> Jeff Haran
>> Brocade
>


--

------------------------------------------------------
Dave Jiang
Software Engineer
MontaVista Software, Inc.
http://www.mvista.com
------------------------------------------------------