Change in ...coreboot[master]: nb/intel/nehalem: Die if no memory ranks found

[Jacob Garber (Code Review)]

Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33403

Change subject: nb/intel/nehalem: Die if no memory ranks found
......................................................................

nb/intel/nehalem: Die if no memory ranks found

Die if there are no memory ranks found to prevent a division by zero.

Change-Id: I6146dd8420f3734d1a672a9f29a098f47fcb739c
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 1229628
---
M src/northbridge/intel/nehalem/raminit.c
1 file changed, 3 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/33403/1

diff --git a/src/northbridge/intel/nehalem/raminit.c b/src/northbridge/intel/nehalem/raminit.c
index 15d6abb..03831e2 100644
--- a/src/northbridge/intel/nehalem/raminit.c
+++ b/src/northbridge/intel/nehalem/raminit.c
@@ -1010,6 +1010,9 @@
    		}
    	}

+		if (count == 0)
+			die("No memory ranks found for channel %u\n", channel);
+
    	info->avg4044[channel] = sum / count;
    	info->max4048[channel] = max_of_unk;
    }

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33403 )

Change subject: nb/intel/nehalem: Die if no memory ranks found
......................................................................

Patch Set 1: Code-Review-1

This would break coreboot when only the second channel is populated.

[Jacob Garber (Code Review)]

Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33403 )

Change subject: nb/intel/nehalem: Die if no memory ranks found
......................................................................

Patch Set 1:
> Patch Set 1: Code-Review-1
>
> This would break coreboot when only the second channel is populated.

The iteration of the loop is skipped if the channel isn't populated (see the check on line 923), so I don't think(?) that would happen.

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33403 )

Change subject: nb/intel/nehalem: Die if no memory ranks found
......................................................................

Patch Set 1: Code-Review+1

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33403 )

Change subject: nb/intel/nehalem: Die if no memory ranks found
......................................................................

Patch Set 1: Code-Review+2

Replacing an unhandled exception (division by zero) with a die()
can't break anything more than it is already :)

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33403 )

Change subject: nb/intel/nehalem: Die if no memory ranks found
......................................................................

Patch Set 1: Code-Review+1
> Patch Set 1: Code-Review+2
>
> Replacing an unhandled exception (division by zero) with a die()
> can't break anything more than it is already :)

Sure, why not? If things happen to break later, it probably means the nehalem code would need some cleanup. I will take care of it when I try porting my stack of half a dozen nehalem laptops.

I was thinking of using a continue statement, but hell knows if the code will break later...

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33403 )

Change subject: nb/intel/nehalem: Die if no memory ranks found
......................................................................

Patch Set 1: Code-Review+2

(1 comment)
> Patch Set 1:
> > Patch Set 1: Code-Review-1
> >
> > This would break coreboot when only the second channel is populated.
>
> The iteration of the loop is skipped if the channel isn't populated (see the check on line 923), so I don't think(?) that would happen.

Yeah, I think that would do it. Think.

https://review.coreboot.org/#/c/33403/1/src/northbridge/intel/nehalem/ramini... 
File src/northbridge/intel/nehalem/raminit.c:

https://review.coreboot.org/#/c/33403/1/src/northbridge/intel/nehalem/ramini... 
PS1, Line 923: 		if (!info->populated_ranks_mask[channel])
             : 			continue;
Ah, there's this thing already

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33403 )

Change subject: nb/intel/nehalem: Die if no memory ranks found
......................................................................

Patch Set 1:
> > Replacing an unhandled exception (division by zero) with a die()
> > can't break anything more than it is already :)
>
> Sure, why not?

Because it was already broken. As much as possible. Before this change,
in the `count == 0` case, execution stopped in line 1013. Just not with
a useful error message.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33403 )

Change subject: nb/intel/nehalem: Die if no memory ranks found
......................................................................

Patch Set 1:
> Patch Set 1:
> > > Replacing an unhandled exception (division by zero) with a die()
> > > can't break anything more than it is already :)
> >
> > Sure, why not?
>
> Because it was already broken. As much as possible. Before this change,
> in the `count == 0` case, execution stopped in line 1013. Just not with
> a useful error message.

Ack

Change in coreboot[master]: nb/intel/nehalem: Die if no memory ranks found

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33403 )

Change subject: nb/intel/nehalem: Die if no memory ranks found
......................................................................

nb/intel/nehalem: Die if no memory ranks found

Die if there are no memory ranks found to prevent a division by zero.

Change-Id: I6146dd8420f3734d1a672a9f29a098f47fcb739c
Signed-off-by: Jacob Garber 
Found-by: Coverity CID 1229628
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33403
Tested-by: build bot (Jenkins) 
Reviewed-by: HAOUAS Elyes 
Reviewed-by: Nico Huber 
Reviewed-by: Angel Pons 
---
M src/northbridge/intel/nehalem/raminit.c
1 file changed, 3 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved
  HAOUAS Elyes: Looks good to me, but someone else must approve
  Angel Pons: Looks good to me, approved

diff --git a/src/northbridge/intel/nehalem/raminit.c b/src/northbridge/intel/nehalem/raminit.c
index 2b71c81..0b5f44c 100644
--- a/src/northbridge/intel/nehalem/raminit.c
+++ b/src/northbridge/intel/nehalem/raminit.c
@@ -1010,6 +1010,9 @@
    		}
    	}

+		if (count == 0)
+			die("No memory ranks found for channel %u\n", channel);
+
    	info->avg4044[channel] = sum / count;
    	info->max4048[channel] = max_of_unk;
    }