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: <7f568cbd-b299-41c6-8786-25f225de8f4f@t-8ch.de>
Date: Tue, 7 Jan 2025 23:13:08 +0100
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Mario Limonciello <mario.limonciello@....com>
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 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.

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