linux-kernel - Re: [PATCH 2/2] platform/x86: firmware_attributes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1f5b77f3-c427-496e-9c1d-3150177f29d6@amd.com>
Date: Tue, 7 Jan 2025 15:18:21 -0600
From: Mario Limonciello <mario.limonciello@....com>
To: Thomas Weißschuh <linux@...ssschuh.net>
Cc: Hans de Goede <hdegoede@...hat.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 Armin Wolf <W_Armin@....de>, platform-driver-x86@...r.kernel.org,
 linux-kernel@...r.kernel.org, Joshua Grisham <josh@...huagrisham.com>
Subject: Re: [PATCH 2/2] platform/x86: firmware_attributes_class: Add test
 driver

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