Re: [PATCH 2/2] platform/x86: firmware_attributes_class: Add test driver

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

On 1/7/2025 16:13, Thomas Weißschuh wrote:
> On 2025-01-07 15:18:21-0600, Mario Limonciello wrote:
>> On 1/7/2025 14:50, Thomas Weißschuh wrote:
>>> On 2025-01-07 13:29:08-0600, Mario Limonciello wrote:
>>>> On 1/7/2025 11:05, Thomas Weißschuh wrote:
>>>>> The driver showcases the use of the new subsystem API.
>>>>>
>>>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>>>> ---
>>>>>     drivers/platform/x86/Kconfig                    | 12 ++++
>>>>>     drivers/platform/x86/Makefile                   |  1 +
>>>>>     drivers/platform/x86/firmware_attributes_test.c | 78 +++++++++++++++++++++++++
>>>>>     3 files changed, 91 insertions(+)
>>>>>
>>>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>>>> index 0258dd879d64be389f4dd9bc309fe089f23cc798..2a0e828657d2f07074944d6c42dc204fc8825a42 100644
>>>>> --- a/drivers/platform/x86/Kconfig
>>>>> +++ b/drivers/platform/x86/Kconfig
>>>>> @@ -1065,6 +1065,18 @@ source "drivers/platform/x86/x86-android-tablets/Kconfig"
>>>>>     config FW_ATTR_CLASS
>>>>>     	tristate
>>>>> +config FW_ATTR_TEST
>>>>> +	tristate "Firmware attribute test driver"
>>>>> +	select FW_ATTR_CLASS
>>>>> +	help
>>>>> +	  This driver provides a test user of the firmware attribute subsystem.
>>>>> +
>>>>> +	  An instance is created at /sys/class/firmware-attributes/test/
>>>>> +	  container various example attributes.
>>>>> +
>>>>> +	  To compile this driver as a module, choose M here: the module
>>>>> +	  will be called firmware_attributes_test.
>>>>> +
>>>>
>>>> I think if you're going to be introducing a test driver it should be
>>>> compliant to what's in sysfs-class-firmware-attributes so that when it's
>>>> inevitably copy/pasted we end up with higher quality drivers.
>>>>
>>>> That is you need at a minimum 'type', 'current_value', 'default_value',
>>>> 'display_name' and 'display_name_language_code'.  Then individual types
>>>> employ additional requirements.
>>>>
>>>> I see 'type', 'current_value', and 'default_value, but I don't see
>>>> 'display_name' or 'display_name_language_code' here.
>>>>
>>>> Furthermore as this is a "string" attribute you're supposed to also have a
>>>> "max_length" and "min_length".
>>>
>>> Agreed that more examples are better.
>>>
>>> But are these attributes really mandatory?
>>> "This attribute is mandatory" is only specified for "type" and>
>> "current_value".
>>
>> Ah wow, I had thought they were, but you're right they're not!
>>
>>> While "possible_values" certainly looks necessary for "enumeration",
>>> "min_length" for "strings" does so much less.
>>
>> Even if they're not mandatory, I think it's better to include them for the
>> same copy/paste reason I mentioned though.
> 
> Thinking about it some more, which attributes should all be included?
> Having all of them in there could motivate driver authors to implement
> them all even it would mean filling in random values.
> The provided examples can already be copied-and-pasted and slightly
> adapted to add more attributes.
> 
> Also as for providing an even higher level interface. There exists a
> fairly big feature matrix. For example the display_name_language_code:
> * Is it used at all?
> * Is it the same for all attributes implemented by the driver and the
>    sysfs attribute can be reused for them all.
> * Should the same handler logic be reused between different settings which
>    only differ in their language code?
> 
> The answers seem to differ between each driver and having a uniform
> high-level interface that can handle all cases would look horrible.
> So I'd like to stick with the API provided currently for now and we can
> revisit it if there are more drivers.
> As mentioned before, the current API should be a decent baseline to
> build on top of.
> 
> 
> Thomas

How about just adding min_length and max_length?  Since you're adding a 
string attribute then it makes a good example of a string attribute.
It should be pretty trivial; AFAICT it's 0 to PAGE_SIZE.