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

[Armin Wolf <W_Armin@gmx.de>]

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.

Thanks,
Armin Wolf

>
>>> +		dev_err(&wdev->dev, "Dell descriptor buffer has invalid vendor signature (%u)\n",
>>> +			le32_to_cpu(desc->vendor_signature));
>>> +		ret = -ENOMSG;
>>>    		descriptor_valid = ret;
>>>    		goto out;
>>>    	}
>>> -	buffer = (u32 *)obj->buffer.pointer;
>>> -
>>> -	if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0) {
>>> -		dev_err(&wdev->dev, "Dell descriptor buffer has invalid signature (%8ph)\n",
>>> -			buffer);
>>> -		ret = -EINVAL;
>>> +	/* " WMI" */
>>> +	if (le32_to_cpu(desc->object_signature) != 0x494D5720) {
>>> +		dev_err(&wdev->dev, "Dell descriptor buffer has invalid object signature (%u)\n",
>>> +			le32_to_cpu(desc->object_signature));
>>> +		ret = -ENOMSG;
>>>    		descriptor_valid = ret;
>>>    		goto out;
>>>    	}