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@xxxxxxxxxx> >> --- >> 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) >> { >