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

[Jean Delvare]

Hi Andy,

On Fri, 29 Jul 2022 21:23:08 +0300, Andy Shevchenko wrote:
> 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.

I didn't consider that. Having code match the documentation is indeed
valuable.

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

True.

> > I'm not too sure what is preferred in such situations.
> 
> For cold paths I think the correctness prevail the performance.

You have a point.

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

I considered that, but that's too hackish for me and could easily cause
confusion leading to improper use. Not worth it. Let's keep things
simple and understandable.

So I'll apply your patch, thanks for contributing it.

-- 
Jean Delvare
SUSE L3 Support