[LinuxBIOS] question on lx raminit.c

[ron minnich]

I offer the following diff and a question: can the use of && in these
if expressions be wrong?

Currently it is:
if ((MIN_MOD_BANKS > spd_byte) && (spd_byte > MAX_MOD_BANKS))

If spd_byte is 1, which is < MIN_MOD_BANKS, this if will fail, and it
should not do that, since the min module banks is 2!

I'm having trouble parsing this. BUT, lx raminit code works, so I am
reluctant to mess with it.

ron

Index: src/northbridge/amd/lx/raminit.c
===================================================================
--- src/northbridge/amd/lx/raminit.c    (revision 2876)
+++ src/northbridge/amd/lx/raminit.c    (working copy)
@@ -44,7 +44,7 @@
        /* Field: Module Banks per DIMM */
        /* EEPROM byte usage: (5) Number of DIMM Banks */
        spd_byte = spd_read_byte(dimm, SPD_NUM_DIMM_BANKS);
-       if ((MIN_MOD_BANKS > spd_byte) && (spd_byte > MAX_MOD_BANKS)) {
+       if ((MIN_MOD_BANKS > spd_byte) || (spd_byte > MAX_MOD_BANKS)) {
                print_debug("Number of module banks not compatible\n");
                POST_CODE(ERROR_BANK_SET);
                __asm__ __volatile__("hlt\n");
@@ -54,7 +54,7 @@
        /* Field: Banks per SDRAM device */
        /* EEPROM byte usage: (17) Number of Banks on SDRAM Device */
        spd_byte = spd_read_byte(dimm, SPD_NUM_BANKS_PER_SDRAM);
-       if ((MIN_DEV_BANKS > spd_byte) && (spd_byte > MAX_DEV_BANKS)) {
+       if ((MIN_DEV_BANKS > spd_byte) || (spd_byte > MAX_DEV_BANKS)) {
                print_debug("Number of device banks not compatible\n");
                POST_CODE(ERROR_BANK_SET);
                __asm__ __volatile__("hlt\n");

[Carl-Daniel Hailfinger]

On 20.10.2007 02:44, ron minnich wrote:
> I offer the following diff and a question: can the use of && in these
> if expressions be wrong?
>
> Currently it is:
> if ((MIN_MOD_BANKS > spd_byte) && (spd_byte > MAX_MOD_BANKS))

You want || because either condition should cause the error message. The
current statement will never be true because

(MIN_MOD_BANKS > MAX_MOD_BANKS) is never true.
> If spd_byte is 1, which is < MIN_MOD_BANKS, this if will fail, and it
> should not do that, since the min module banks is 2!
>
> I'm having trouble parsing this. BUT, lx raminit code works, so I am
> reluctant to mess with it.
>
> ron
> Index: src/northbridge/amd/lx/raminit.c
>
> --- src/northbridge/amd/lx/raminit.c    (revision 2876)
> +++ src/northbridge/amd/lx/raminit.c    (working copy)
> @@ -44,7 +44,7 @@
>         /* Field: Module Banks per DIMM */
>         /* EEPROM byte usage: (5) Number of DIMM Banks */
>         spd_byte = spd_read_byte(dimm, SPD_NUM_DIMM_BANKS);
>   if ((MIN_MOD_BANKS > spd_byte) && (spd_byte > MAX_MOD_BANKS)) {
>   if ((MIN_MOD_BANKS > spd_byte) || (spd_byte > MAX_MOD_BANKS)) {
>           print_debug("Number of module banks not compatible\n");
>           POST_CODE(ERROR_BANK_SET);
>           __asm__ __volatile__("hlt\n");
>
> @@ -54,7 +54,7 @@
>         /* Field: Banks per SDRAM device */
>         /* EEPROM byte usage: (17) Number of Banks on SDRAM Device */
>         spd_byte = spd_read_byte(dimm, SPD_NUM_BANKS_PER_SDRAM);
>   if ((MIN_DEV_BANKS > spd_byte) && (spd_byte > MAX_DEV_BANKS)) {
>   if ((MIN_DEV_BANKS > spd_byte) || (spd_byte > MAX_DEV_BANKS)) {
>           print_debug("Number of device banks not compatible\n");
>           POST_CODE(ERROR_BANK_SET);
>           __asm__ __volatile__("hlt\n");

Patch looks fine to me.
Acked-by: Carl-Daniel Hailfinger

Carl-Daniel

[Marc Jones]

Carl-Daniel Hailfinger wrote:
> On 20.10.2007 02:44, ron minnich wrote:
> > I offer the following diff and a question: can the use of && in these
> > if expressions be wrong?
> >
> > Currently it is:
> > if ((MIN_MOD_BANKS > spd_byte) && (spd_byte > MAX_MOD_BANKS))
>
> You want || because either condition should cause the error message. The
> current statement will never be true because
>
> (MIN_MOD_BANKS > MAX_MOD_BANKS) is never true.
>
> Patch looks fine to me.
> Acked-by: Carl-Daniel Hailfinger
>
> Carl-Daniel

Indeed,  I didn't test it with an non-supported dimm....
Marc