| Message ID | 20200422115814.22205-7-rrichter@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | EDAC/mc/ghes: Fixes, cleanup and reworks | expand |
On Wed, Apr 22, 2020 at 01:58:10PM +0200, Robert Richter wrote: > The local variable rdr_mask serves as a static constant here. It hides > what the code is doing. Remove it and replace it with the actual logic > that checks some bits. > > Signed-off-by: Robert Richter <rrichter@marvell.com> > --- > drivers/edac/ghes_edac.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c > index a5890afa9c71..038e560fd332 100644 > --- a/drivers/edac/ghes_edac.c > +++ b/drivers/edac/ghes_edac.c > @@ -191,7 +191,6 @@ static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg) > if (dh->type == DMI_ENTRY_MEM_DEVICE) { > struct memdev_dmi_entry *entry = (struct memdev_dmi_entry *)dh; > struct dimm_info *dimm = edac_get_dimm(mci, dimm_fill->count, 0, 0); > - u16 rdr_mask = BIT(7) | BIT(13); > > if (entry->size == 0xffff) { > pr_info("Can't get DIMM%i size\n", > @@ -241,7 +240,8 @@ static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg) > default: > if (entry->type_detail & BIT(6)) > dimm->mtype = MEM_RMBS; > - else if ((entry->type_detail & rdr_mask) == rdr_mask) > + else if ((entry->type_detail & BIT(7)) && > + (entry->type_detail & BIT(13))) Well, "checks some bits" doesn't make it more telling than checking a descriptive name like "rdr_mask" but ok, since we're assigning MEM_RDR here, it is still clear what the check does. Btw, please write it like this: else if (entry->type_detail & (BIT(7) | BIT(13))) Thx.
On Mon, Apr 27, 2020 at 09:08:02AM +0200, Borislav Petkov wrote: > > if (entry->type_detail & BIT(6)) > > dimm->mtype = MEM_RMBS; > > - else if ((entry->type_detail & rdr_mask) == rdr_mask) > > + else if ((entry->type_detail & BIT(7)) && > > + (entry->type_detail & BIT(13))) > > Well, "checks some bits" doesn't make it more telling than checking a > descriptive name like "rdr_mask" but ok, since we're assigning MEM_RDR > here, it is still clear what the check does. > > Btw, please write it like this: > > else if (entry->type_detail & (BIT(7) | BIT(13))) That isn't the same. The previous version checked that BOTH bits 7 and 13 were set. Your version checks for either bit. Looks like the original with the local variable was checking for both bits set. -Tony
On Mon, Apr 27, 2020 at 10:24:08AM -0700, Luck, Tony wrote: > That isn't the same. The previous version checked that BOTH bits > 7 and 13 were set. Your version checks for either bit. Whoops, I'm confused again. ;-\ > Looks like the original with the local variable was checking for both > bits set. Yeah, let's leave it as it is. I prefer the rdr_mask thing. Thx.
On 27.04.20 19:34:02, Borislav Petkov wrote: > On Mon, Apr 27, 2020 at 10:24:08AM -0700, Luck, Tony wrote: > > That isn't the same. The previous version checked that BOTH bits > > 7 and 13 were set. Your version checks for either bit. > > Whoops, I'm confused again. ;-\ > > > Looks like the original with the local variable was checking for both > > bits set. > > Yeah, let's leave it as it is. I prefer the rdr_mask thing. I am dropping this patch from the series. Thanks, -Robert
diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c index a5890afa9c71..038e560fd332 100644 --- a/drivers/edac/ghes_edac.c +++ b/drivers/edac/ghes_edac.c @@ -191,7 +191,6 @@ static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg) if (dh->type == DMI_ENTRY_MEM_DEVICE) { struct memdev_dmi_entry *entry = (struct memdev_dmi_entry *)dh; struct dimm_info *dimm = edac_get_dimm(mci, dimm_fill->count, 0, 0); - u16 rdr_mask = BIT(7) | BIT(13); if (entry->size == 0xffff) { pr_info("Can't get DIMM%i size\n", @@ -241,7 +240,8 @@ static void ghes_edac_dmidecode(const struct dmi_header *dh, void *arg) default: if (entry->type_detail & BIT(6)) dimm->mtype = MEM_RMBS; - else if ((entry->type_detail & rdr_mask) == rdr_mask) + else if ((entry->type_detail & BIT(7)) && + (entry->type_detail & BIT(13))) dimm->mtype = MEM_RDR; else if (entry->type_detail & BIT(7)) dimm->mtype = MEM_SDR;
The local variable rdr_mask serves as a static constant here. It hides what the code is doing. Remove it and replace it with the actual logic that checks some bits. Signed-off-by: Robert Richter <rrichter@marvell.com> --- drivers/edac/ghes_edac.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)