Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 29 Jul 2022 21:23:08 +0300
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Jean Delvare <jdelvare@suse.de>
Cc:     linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 1/1] firmware: dmi: Don't take garbage into
 consideration in dmi_smbios3_present()
Message-ID: <YuQljCM4LZXhSkbh@smile.fi.intel.com>
References: <20220726094329.1725-1-andriy.shevchenko@linux.intel.com>
 <20220727102504.6bbefcf9@endymion.delvare>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220727102504.6bbefcf9@endymion.delvare>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Wed, Jul 27, 2022 at 10:25:04AM +0200, Jean Delvare wrote:
> 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.

I don't know how to improve it, but it kinda a bug in a logic for non-prepared
reader, esp. as specification suggests different thing about version offset.

...

> > -		dmi_ver = get_unaligned_be32(buf + 6) & 0xFFFFFF;
> > +		dmi_ver = get_unaligned_be24(buf + 7);

> 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.

Good to know!
But here it's not a hot path, right?

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

For cold paths I think the correctness prevail the performance.

Alternatively we might add a comment in the code explaining the trick,
although I won't like to do it.

Another way is to have a subset of 24-/48-bit unaligned accessors that
use the trick specifically for hot paths with a caveat that they may
access data out of the 24-/48-bit boundaries.

-- 
With Best Regards,
Andy Shevchenko