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 for Android: free password hash cracker in your pocket

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

Message-ID: <c58ee2ba-7800-4da0-81a3-faa971515cab@t-8ch.de>
Date: Thu, 9 Jan 2025 16:17:42 +0100
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Mario Limonciello <mario.limonciello@....com>, 
	Hans de Goede <hdegoede@...hat.com>, Armin Wolf <W_Armin@....de>, platform-driver-x86@...r.kernel.org, 
	LKML <linux-kernel@...r.kernel.org>, Joshua Grisham <josh@...huagrisham.com>
Subject: Re: [PATCH 2/2] platform/x86: firmware_attributes_class: Add test
 driver

On 2025-01-08 11:30:12+0200, Ilpo Järvinen wrote:
> On Tue, 7 Jan 2025, 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@...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.
> > 
> > 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.
> 
> Can't you like add comments to the optional ones to reduce the incentive 
> to fill them with random junk as it's a lot easier to just delete them than 
> generating some random junk. So if a developer is unsure what to do a 
> comment telling something is optional would help to lean towards 'I can 
> safely delete this'?

That would be possible. But I'm still not convinced.
If driver authors can't be expected to know how to implement their own
sysfs attribute groups from the similar provided examples as needed, we
would have to provide example code for sysfs attributes of all firmware
attributes. And that would be a lot of them.

Also the attributes themselves would be highly repetitive. The
interesting logic would be how to wire it up the the rest of the driver,
and the example code can't provide copy-paste code for that.