Re: [PATCH v1 1/1] firmware: dmi: Don't take garbage into consideration in dmi_smbios3_present()

[Jean Delvare <jdelvare@suse.de>]

Hi Andy,

On Tue, 26 Jul 2022 12:43:29 +0300, Andy Shevchenko wrote:
> The byte at offset 6 represent length. Don't take it and drop it immediately
> by using proper accessor, i.e. get_unaligned_be24().

The subject sounds like you are fixing a bug, while this is only, at
best, a minor optimization.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/firmware/dmi_scan.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index b2ea318a10a4..24537ce29bc4 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -630,7 +630,7 @@ static int __init dmi_smbios3_present(const u8 *buf)
>  {
>  	if (memcmp(buf, "_SM3_", 5) == 0 &&
>  	    buf[6] < 32 && dmi_checksum(buf, buf[6])) {
> -		dmi_ver = get_unaligned_be32(buf + 6) & 0xFFFFFF;
> +		dmi_ver = get_unaligned_be24(buf + 7);
>  		dmi_num = 0;			/* No longer specified */
>  		dmi_len = get_unaligned_le32(buf + 12);
>  		dmi_base = get_unaligned_le64(buf + 16);

I admit I did not know about get_unaligned_be24(). While I agree that
it makes the source code look better, one downside is that it actually
increases the binary size on x86_64. The reason is that
get_unaligned_be32() is optimized by assembly instruction bswapl, while
get_unaligned_be24() is not. Situation appears to be the same on ia64
and arm. Only arm64 would apparently benefit from your proposed
change.

I'm not too sure what is preferred in such situations.

-- 
Jean Delvare
SUSE L3 Support