From: Jean Delvare <jdelvare@suse.de> To: "Erwan Velu" <erwanaliasr1@gmail.com> Cc: "Robert P. J. Day" <rpjday@crashcourse.ca>, "Erwan Velu" <e.velu@criteo.com>, "Changbin Du" <changbin.du@intel.com>, "Boris Brezillon" <bbrezillon@kernel.org>, "Mauro Carvalho Chehab" <mchehab+samsung@kernel.org>, "Jens Wiklander" <jens.wiklander@linaro.org>, "Sumit Garg" <sumit.garg@linaro.org>, "Andy Shevchenko" <andriy.shevchenko@linux.intel.com>, "Michal Marek" <michal.lkml@markovi.net>, "Mattias Jacobsson" <2pi@mok.nu>, "Masahiro Yamada" <yamada.masahiro@socionext.com>, <linux-kbuild@vger.kernel.org>, <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 1/3] firmware/dmi_scan: Add dmi_save_release to save releases fields Date: Mon, 21 Oct 2019 16:32:08 +0200 [thread overview] Message-ID: <20191021163208.5cd03d59@endymion> (raw) In-Reply-To: <20190918094323.17515-1-e.velu@criteo.com> 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) > { -- Jean Delvare SUSE L3 Support
WARNING: multiple messages have this Message-ID (diff)
From: Jean Delvare <jdelvare@suse.de> To: Erwan Velu <erwanaliasr1@gmail.com> Cc: "Robert P. J. Day" <rpjday@crashcourse.ca>, Erwan Velu <e.velu@criteo.com>, Changbin Du <changbin.du@intel.com>, Boris Brezillon <bbrezillon@kernel.org>, Mauro Carvalho Chehab <mchehab+samsung@kernel.org>, Jens Wiklander <jens.wiklander@linaro.org>, Sumit Garg <sumit.garg@linaro.org>, Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Michal Marek <michal.lkml@markovi.net>, Mattias Jacobsson <2pi@mok.nu>, Masahiro Yamada <yamada.masahiro@socionext.com>, linux-kbuild@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/3] firmware/dmi_scan: Add dmi_save_release to save releases fields Date: Mon, 21 Oct 2019 16:32:08 +0200 [thread overview] Message-ID: <20191021163208.5cd03d59@endymion> (raw) In-Reply-To: <20190918094323.17515-1-e.velu@criteo.com> 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) > { -- Jean Delvare SUSE L3 Support
next prev parent reply other threads:[~2019-10-21 14:31 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-09-18 9:43 [PATCH 1/3] firmware/dmi_scan: Add dmi_save_release to save releases fields Erwan Velu 2019-09-18 9:43 ` Erwan Velu 2019-09-18 9:43 ` [PATCH 2/3] firmware/dmi: Report DMI Bios release Erwan Velu 2019-09-18 9:43 ` Erwan Velu 2019-10-21 14:53 ` Jean Delvare 2019-10-21 14:53 ` Jean Delvare 2019-11-27 15:05 ` Erwan Velu 2019-09-18 9:43 ` [PATCH 3/3] firmware/dmi: Report DMI Embedded Firmware release Erwan Velu 2019-09-18 9:43 ` Erwan Velu 2019-10-21 14:55 ` Jean Delvare 2019-10-21 14:55 ` Jean Delvare 2019-10-21 14:32 ` Jean Delvare [this message] 2019-10-21 14:32 ` [PATCH 1/3] firmware/dmi_scan: Add dmi_save_release to save releases fields Jean Delvare 2019-11-27 15:04 ` Erwan Velu 2019-11-27 15:07 ` [PATCH 1/2] firmware/dmi: Report DMI Bios release Erwan Velu 2019-11-27 15:07 ` Erwan Velu 2019-11-27 15:07 ` [PATCH 2/2] firmware/dmi: Report DMI Embedded Firmware release Erwan Velu 2019-11-27 15:07 ` Erwan Velu 2020-02-06 12:24 ` Jean Delvare 2020-02-06 12:25 ` Erwan Velu 2020-02-07 8:38 ` Erwan Velu 2020-02-06 12:16 ` [PATCH 1/2] firmware/dmi: Report DMI Bios release Jean Delvare
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191021163208.5cd03d59@endymion \ --to=jdelvare@suse.de \ --cc=2pi@mok.nu \ --cc=andriy.shevchenko@linux.intel.com \ --cc=bbrezillon@kernel.org \ --cc=changbin.du@intel.com \ --cc=e.velu@criteo.com \ --cc=erwanaliasr1@gmail.com \ --cc=jens.wiklander@linaro.org \ --cc=linux-kbuild@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mchehab+samsung@kernel.org \ --cc=michal.lkml@markovi.net \ --cc=rpjday@crashcourse.ca \ --cc=sumit.garg@linaro.org \ --cc=yamada.masahiro@socionext.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.