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

[Mario Limonciello <mario.limonciello@amd.com>]

On 3/9/2026 2:45 PM, 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.
> 
> Thanks,
> Armin Wolf

Oh I didn't realize no one else is doing this.  Yeah if it's Dell only, 
agree - meh.


> 
>>
>>>> +        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;
>>>>        }
>