Re: [PATCH 1/9] platform/x86: dell-descriptor: Use new buffer-based WMI API

[Armin Wolf]

Am 10.03.26 um 11:46 schrieb Gergo Koteles:

> Hi Armin,
>
> On Mon, 2026-03-09 at 20:45 +0100, Armin Wolf wrote:
> > Am 09.03.26 um 18:23 schrieb Pali Rohár:
> >
> > > On Monday 09 March 2026 10:41:20 Mario Limonciello wrote:
> > > > On 3/7/2026 6:25 PM, Armin Wolf wrote:
> > > > > +	/* "DELL" */
> > > > > +	if (le32_to_cpu(desc->vendor_signature) != 0x4C4C4544) {
> > > > Do you think you could come up with a helper for matching an expected
> > > > "string" like this?  I /suspect/ it's going to be a common pattern that
> > > > vendors use and it will increase code readability going forward if a helper
> > > > is possible.  I see it at least twice in this patch alone.
> > > >
> > > > Something like this prototype:
> > > >
> > > > bool wmi_valid_signature(u32 field, const char* expected_str);
> > > When I read your comment, I come to another idea. What about introducing
> > > a macro which will convert 4-byte string to u32 number? For example:
> > >
> > >     #define str_to_u32(str) ({ _Static_assert(__builtin_types_compatible_p(__typeof__(str), char[5]), "wrong type"); (u32)(u8)(str)[0] | ((u32)((u8)(str)[1]) << 8) | ((u32)((u8)(str)[2]) << 16) | ((u32)((u8)(str)[3]) << 24); })
> > >
> > > The whole conversion would be done in compile time (with -O2) so should
> > > not cause any possible performance issues.
> > >
> > > With it, the condition could be written as:
> > >
> > >     if (le32_to_cpu(desc->vendor_signature) != str_to_u32("DELL")) {
> > >
> > > and no need to use magic number 0x4C4C4544 and write comment that this
> > > number in ASCII is the "DELL" string.
> > To be honest i do nothing think that having a helper function for this inside the WMI driver core
> > is useful, especially since most vendors other than Dell do not use such magic numbers.
> >
> >   From my perspective assembling the two constants ourself is fine here.
> But what about the other readers? :)
>
> Why don't you use a char array for the descriptors?
>
> struct descriptor {
> 	char vendor_signature[4];
> 	char object_signature[4];
> 	__le32 interface_version;
> 	__le32 buffer_length;
> 	__le32 hotfix_number;
> } __packed;
>
> Best regards,
> Gergo Koteles
Fine, i will model the vendor and object signatures using char arrays.

Thanks,
Armin Wolf