Re: [PATCH 1/3] firmware/dmi_scan: Add dmi_save_release to save releases fields

[Erwan Velu]

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