| Message ID | 20190918094323.17515-1-e.velu@criteo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] firmware/dmi_scan: Add dmi_save_release to save releases fields | expand |
Hi Erwan, Sorry for the late answer. On Wed, 18 Sep 2019 11:43:19 +0200, Erwan Velu wrote: > In DMI type 0, there is several fields that encodes a release. is -> are encodes -> encode > The dmi_save_release() function have the logic to check if the field is valid. > If so, it reports the actual value. > > Signed-off-by: Erwan Velu <e.velu@criteo.com> > --- > drivers/firmware/dmi_scan.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) This patch introduces a warning: drivers/firmware/dmi_scan.c:185:20: warning: ‘dmi_save_release’ defined but not used [-Wunused-function] static void __init dmi_save_release(const struct dmi_header *dm, int slot, ^~~~~~~~~~~~~~~~ because you add a static function with no user. I understand that you add a use later in the series, but it's not OK to introduce a warning even if temporary. It also makes little sense to split the changes that way as there is no way to cherry-pick one of the patches without the rest. And it makes things more difficult to review too, as I can't possibly judge if this function if right without also seeing where is will be called and how. So, please merge all the changes into a single patch. > diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c > index 35ed56b9c34f..202bd2c69d0f 100644 > --- a/drivers/firmware/dmi_scan.c > +++ b/drivers/firmware/dmi_scan.c > @@ -181,6 +181,32 @@ static void __init dmi_save_ident(const struct dmi_header *dm, int slot, > dmi_ident[slot] = p; > } > > +static void __init dmi_save_release(const struct dmi_header *dm, int slot, > + int index) > +{ > + const u8 *d; > + char *s; > + > + // If the table doesn't have the field, let's return Please stick to C89-style comments (/* */) as used everywhere else in this file. > + if (dmi_ident[slot] || dm->length < index) > + return; > + > + d = (u8 *) dm + index; > + > + // As per the specification, > + // if the system doesn't have the field, the value is FF > + if (d[0] == 0xFF) > + return; That's not exactly what the specification says. It says: "If the system does not support the use of [the System BIOS Major Release] field, the value is 0FFh for both this field and the System BIOS Minor Release field." So unused is when both fields are 0xFF. You can't test them independently. Same goes for the Embedded Controller Firmware Release fields, even if it is worded differently, the logic is the same. > + > + s = dmi_alloc(4); > + if (!s) > + return; > + > + sprintf(s, "%u", d[0]); > + > + dmi_ident[slot] = s; > +} > + > static void __init dmi_save_uuid(const struct dmi_header *dm, int slot, > int index) > {
Got all your points, sending a V2 with the fixes you asked. On 21/10/2019 16:32, Jean Delvare wrote: > Hi Erwan, > > Sorry for the late answer. > > On Wed, 18 Sep 2019 11:43:19 +0200, Erwan Velu wrote: >> In DMI type 0, there is several fields that encodes a release. > is -> are > encodes -> encode > >> The dmi_save_release() function have the logic to check if the field is valid. >> If so, it reports the actual value. >> >> Signed-off-by: Erwan Velu <e.velu@criteo.com> >> --- >> drivers/firmware/dmi_scan.c | 26 ++++++++++++++++++++++++++ >> 1 file changed, 26 insertions(+) > This patch introduces a warning: > > drivers/firmware/dmi_scan.c:185:20: warning: ‘dmi_save_release’ defined but not used [-Wunused-function] > static void __init dmi_save_release(const struct dmi_header *dm, int slot, > ^~~~~~~~~~~~~~~~ > > because you add a static function with no user. I understand that you > add a use later in the series, but it's not OK to introduce a warning > even if temporary. It also makes little sense to split the changes that > way as there is no way to cherry-pick one of the patches without the > rest. And it makes things more difficult to review too, as I can't > possibly judge if this function if right without also seeing where is > will be called and how. > > So, please merge all the changes into a single patch. > >> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c >> index 35ed56b9c34f..202bd2c69d0f 100644 >> --- a/drivers/firmware/dmi_scan.c >> +++ b/drivers/firmware/dmi_scan.c >> @@ -181,6 +181,32 @@ static void __init dmi_save_ident(const struct dmi_header *dm, int slot, >> dmi_ident[slot] = p; >> } >> >> +static void __init dmi_save_release(const struct dmi_header *dm, int slot, >> + int index) >> +{ >> + const u8 *d; >> + char *s; >> + >> + // If the table doesn't have the field, let's return > Please stick to C89-style comments (/* */) as used everywhere else in > this file. > >> + if (dmi_ident[slot] || dm->length < index) >> + return; >> + >> + d = (u8 *) dm + index; >> + >> + // As per the specification, >> + // if the system doesn't have the field, the value is FF >> + if (d[0] == 0xFF) >> + return; > That's not exactly what the specification says. It says: > > "If the system does not support the use of [the System BIOS Major > Release] field, the value is 0FFh for both this field and the System > BIOS Minor Release field." So unused is when both fields are 0xFF. You > can't test them independently. > > Same goes for the Embedded Controller Firmware Release fields, even if > it is worded differently, the logic is the same. > >> + >> + s = dmi_alloc(4); >> + if (!s) >> + return; >> + >> + sprintf(s, "%u", d[0]); >> + >> + dmi_ident[slot] = s; >> +} >> + >> static void __init dmi_save_uuid(const struct dmi_header *dm, int slot, >> int index) >> { >
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c index 35ed56b9c34f..202bd2c69d0f 100644 --- a/drivers/firmware/dmi_scan.c +++ b/drivers/firmware/dmi_scan.c @@ -181,6 +181,32 @@ static void __init dmi_save_ident(const struct dmi_header *dm, int slot, dmi_ident[slot] = p; } +static void __init dmi_save_release(const struct dmi_header *dm, int slot, + int index) +{ + const u8 *d; + char *s; + + // If the table doesn't have the field, let's return + if (dmi_ident[slot] || dm->length < index) + return; + + d = (u8 *) dm + index; + + // As per the specification, + // if the system doesn't have the field, the value is FF + if (d[0] == 0xFF) + return; + + s = dmi_alloc(4); + if (!s) + return; + + sprintf(s, "%u", d[0]); + + dmi_ident[slot] = s; +} + static void __init dmi_save_uuid(const struct dmi_header *dm, int slot, int index) {
In DMI type 0, there is several fields that encodes a release. The dmi_save_release() function have the logic to check if the field is valid. If so, it reports the actual value. Signed-off-by: Erwan Velu <e.velu@criteo.com> --- drivers/firmware/dmi_scan.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)