From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E6FC0CA9EAF for ; Mon, 21 Oct 2019 14:31:57 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C4F2B20640 for ; Mon, 21 Oct 2019 14:31:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728991AbfJUOb4 convert rfc822-to-8bit (ORCPT ); Mon, 21 Oct 2019 10:31:56 -0400 Received: from mx2.suse.de ([195.135.220.15]:57680 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727140AbfJUOb4 (ORCPT ); Mon, 21 Oct 2019 10:31:56 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 2197DC138; Mon, 21 Oct 2019 14:31:53 +0000 (UTC) Date: Mon, 21 Oct 2019 16:32:08 +0200 From: Jean Delvare To: "Erwan Velu" Cc: "Robert P. J. Day" , "Erwan Velu" , "Changbin Du" , "Boris Brezillon" , "Mauro Carvalho Chehab" , "Jens Wiklander" , "Sumit Garg" , "Andy Shevchenko" , "Michal Marek" , "Mattias Jacobsson" <2pi@mok.nu>, "Masahiro Yamada" , , Subject: Re: [PATCH 1/3] firmware/dmi_scan: Add dmi_save_release to save releases fields Message-ID: <20191021163208.5cd03d59@endymion> In-Reply-To: <20190918094323.17515-1-e.velu@criteo.com> References: <20190918094323.17515-1-e.velu@criteo.com> Organization: SUSE Linux X-Mailer: Claws Mail 3.17.3 (GTK+ 2.24.32; x86_64-suse-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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