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

From: Jean Delvare
Date: Wed Jul 27 2022 - 04:25:14 EST


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@xxxxxxxxxxxxxxx>
> ---
> 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