Message-ID: <CAMF+KeYRpBFN=E=ZQm+Ho51QH4_-53VOe+Pup8SUiXOn6-sFRA@mail.gmail.com>
Date: Sat, 7 Jun 2025 18:38:38 +0200
From: Joshua Grisham <josh@...huagrisham.com>
To: Kurt Borja <kuurtb@...il.com>
Cc: Hans de Goede <hdegoede@...hat.com>, Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, 
	Thomas Weißschuh <linux@...ssschuh.net>, 
	Joshua Grisham <josh@...huagrisham.com>, Mark Pearson <mpearson-lenovo@...ebb.ca>, 
	Armin Wolf <W_Armin@....de>, Mario Limonciello <mario.limonciello@....com>, 
	Antheas Kapenekakis <lkml@...heas.dev>, "Derek J. Clark" <derekjohn.clark@...il.com>, 
	Prasanth Ksr <prasanth.ksr@...l.com>, Jorge Lopez <jorge.lopez2@...com>, 
	platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Dell.Client.Kernel@...l.com
Subject: Re: [PATCH v2 0/5] platform/x86: firmware_attributes_class: Add a
 high level API

Den lör 17 maj 2025 kl 10:52 skrev Kurt Borja <kuurtb@...il.com>:
>
> Hi all,
>
> These series adds the _long awaited_ API for the Firmware Attributes
> class.
>
> You'll find all the details in the commit messages and kernel-doc.
>
> I think it's easier to understand by example, so I used the
> samsung-galaxybook driver for this purpose (last patch). IMO code
> readibility, simplicity, maintainability, etc. is greatly improved, but
> there is still room for improvement of the API itself. For this reason I
> submitted this as an RFC.
>
> As always, your feedback is very appreciated :)
>
> Overview
> ========
>
> Patch 1-2: New API with docs included.
>   Patch 3: New firwmare attributes type
>   Patch 4: Misc Maintainers patch
>   Patch 5: samsung-galaxybook example
>
> Signed-off-by: Kurt Borja <kuurtb@...il.com>
> ---
> Changes in v2:
>
> [Patch 1]
>  - Include kdev_t.h header
>
> [Patch 2]
>  - Use one line comments in fwat_create_attrs()
>  - Check propagate errors in fwat_create_attrs()
>  - Add `mode` to fwat_attr_config and related macros to let users
>    configure the `current_value` attribute mode
>  - Use defined structs in fwat_attr_ops instead of anonymous ones
>  - Move fwat_attr_type from config to ops
>
> [Patch 5]
>  - Just transition to new API without chaing ABI
>
> - Link to v1: https://lore.kernel.org/r/20250509-fw-attrs-api-v1-0-258afed65bfa@gmail.com
>
> ---
> Kurt Borja (4):
>       platform/x86: firmware_attributes_class: Add a high level API
>       platform/x86: firmware_attributes_class: Add a boolean type
>       MAINTAINERS: Add FIRMWARE ATTRIBUTES CLASS entry
>       platform/x86: samsung-galaxybook: Transition to new firmware_attributes API
>
> Thomas Weißschuh (1):
>       platform/x86: firmware_attributes_class: Add device initialization methods
>
>  .../ABI/testing/sysfs-class-firmware-attributes    |   1 +
>  MAINTAINERS                                        |   7 +
>  drivers/platform/x86/firmware_attributes_class.c   | 454 +++++++++++++++++++++
>  drivers/platform/x86/firmware_attributes_class.h   | 276 +++++++++++++
>  drivers/platform/x86/samsung-galaxybook.c          | 308 ++++++--------
>  5 files changed, 861 insertions(+), 185 deletions(-)
> ---
> base-commit: 9f080c9f2099b5a81c85b3b7f95fd11fad428cc8
> change-id: 20250326-fw-attrs-api-0eea7c0225b6
> --
>  ~ Kurt
>

Hi Kurt! First let me begin by saying GREAT job in picking this up,
carrying on the work from Thomas, and really trying to glue all of the
various pieces together into a packaged solution that can finally see
the light of day :)

Sorry it has taken some time for me to get back to you--work and other
life stuff seemed to always get in the way and I wanted to make sure I
took enough time to really think about this before I were to give any
feedback myself.

First off, the quick and easy one:  I applied all of your patches (on
top of 6.15.1), tested everything with samsung-galaxybook from my
device, and everything is still working without any failures and all
features work as I expect them to. I diffed everything under
/sys/class/firmware-attributes before vs after and everything is
exactly the same EXCEPT it looks like what is currently
"default_value" will now be called "default" with your patch. I assume
if the intention is to keep the ABI same as before then you would
probably want to change this? Specifically here:

> +static const char * const fwat_prop_labels[] = {
> +        [FWAT_PROP_DISPLAY_NAME] = "display_name",
> +        [FWAT_PROP_LANGUAGE_CODE] = "display_name_language_code",
> +        [FWAT_PROP_DEFAULT] = "default",

Assume the last line should instead be

        [FWAT_PROP_DEFAULT] = "default_value",

or maybe even for consistency to rename the fwat_property to also
match and then it could be like this?

        [FWAT_PROP_DEFAULT_VALUE] = "default_value",

FWIW I don't personally mind changing the ABI for samsung-galaxybook;
as you mentioned it is basically a brand new driver and the solutions
which exist "in the wild" for it are quite limited so better maybe
that it looks "right" going forward instead of carrying any
unnecessary baggage, but I can understand that this may not be the
case for all of the other drivers which have been using these
facilities for a longer time period.

Past that, I certainly think this is a big step forward as compared to
messing around with the lower level kset / kobj_attribute etc
facilities and trying to set everything up from scratch without so
many helper utilities. As you may have noticed, what I ended up doing
in samsung-galaxybook was essentially to create my own local
implementation of some kind of "standard" fw attributes (but only for
booleans), but it would be even better if this were standardized
across all drivers! There are a few things left over in
samsung-galaxybook that still need to be cleaned up from your
suggested change (e.g. the struct galaxybook_fw_attr can now be
totally removed, etc) which we can also address at some point, of
course!

But just to take a step back for a moment, and what I have been really
trying to think through and reflect on myself for a few hours with
this change...

(Please feel free to completely disregard the below if this has
already been brought up and ruled out, or anyone else has any opinions
against this; all of that feedback is welcome and most definitely
trumps my own meager opinions! ;) Also please remember that it is not
my intention at all to detract from any of the great work that has
already been done here -- just the stuff that my brain kind of gets
"stuck" on as I try to think through the bigger picture with this! )

If I think in terms of anyone who wants to come in and work on device
drivers in the kernel, then they will potentially need to learn
facilities for multiple different kind of "attributes" depending on
their use case: device attributes, driver attributes, hwmon's
sensor-related attributes, bus attributes, etc etc, and for the most
part, I think they ALL have basically the same kind of interface and
facilities. It feels very unified and relatively easy to work with all
of them once you have basically figured out the scheme and conventions
that have been implemented.

Now, when I look at the proposal from these patches, these "Firmware
Attributes" do not seem to have the same kind of "look, feel, and
smell" as the other type of attributes I just mentioned, but instead
feels like a totally new animal that must be learned separately. My
take on it would be that a desired/"dream" scenario for a device
driver developer is that all of these interfaces sort of look and
"smell" the same, it is just a matter of the name of the macro you
use, which device you attach the attributes to (which registration
function you need to execute??), and maybe some small subtle
differences in the facilities as appropriate to their context.

Specifically with firmware attributes vs the other kinds, I guess the
biggest differences are that:
1) There is a display_name with a language code
2) There are built-in restrictions on the input values depending on a
"type" (e.g. "enumeration" type has a predetermined list of values,
min/max values or str lengths for numeric or string values, etc)
3) There is a default_value
4) *Maybe* there should be some kind of inheritance and/or sub-groups
(e.g. the "authentication" and similar extensions that create a group
under the parent group...)

But at the end of the day, my hope as a developer would be to be able
to create these firmware attributes in much the same way as the other
types. E.g. maybe something like this quick and dirty pseudo example:


static ssize_t power_on_lid_open_show(struct device *dev,
                                      struct device_attribute *attr,
                                      char *buf)
{
        // ...
}

static ssize_t power_on_lid_open_store(struct device *dev,
                                       struct device_attribute *attr,
                                       const char *buf, size_t count)
{
        // ...
}

static FW_BOOL_ATTR_RW(power_on_lid_open, "Power On Lid Open");

static struct attribute *galaxybook_fw_attrs[] = {
        // ... other fw attrs not shown above ...
       &fw_attr_power_on_lid_open.attr,
        NULL
};

static const struct attribute_group galaxybook_fw_attrs_group = {
        .attrs = galaxybook_fw_attrs,
        .is_visible = galaxybook_fw_attr_visible,
};

static int galaxybook_fw_attrs_init(struct samsung_galaxybook *galaxybook)
{
        // ...

        /* something like "devm_fw_attr_device_register" could be sort
of similar to
           how devm_hwmon_device_register_with_groups works ? */

        ret = devm_fw_attr_device_register(&galaxybook->platform->dev,
                                          DRIVER_NAME, galaxybook,
                                          &galaxybook_fw_attrs_group);
        return PTR_ERR_OR_ZERO(ret);
}


Or in other words:
- I create my callback functions for "show" and "store" with a certain
named prefix and then use a macro to create the struct for this fw
attr that relies on that these functions exist (e.g. in the above
example the macro would create this "fw_attr_power_on_lid_open" fw
attr structure instance) -- note here it might need to be a macro per
type and/or to include the type-related stuff (including value
constraints/enumeration arrays/default values/etc) as parameters to
the macro, plus maybe I would want to provide some kind of context
parameter e.g. I would maybe want a pointer to my samsung_galaxybook
ideally somehow to get to come along?? (that might affect the
signature of my above examples of course! they were just a
quick-and-dirty example...),
- put all of my desired attrs together in a group where I can specify
their is_visible callback (just like you do with DEVICE_ATTRs),
- and then register my fw attr device with my attribute_group (the
register function would take care of all the rest..)

And as sort of shown in the above example I certainly think it would
be nice if the naming convention lined up nicely with how the naming
convention works for the existing attribute stuff (e.g. DEVICE_ATTR_RW
vs DRIVER_ATTR_RW vs something like "FW_ATTR_RW" or "FIRMWARE_ATTR_RW"
seems like it falls into the same convention??)

Again I am not trying to "rock the boat" here, and I have not
necessarily *really* thought through all of the implications to the
existing fw attrs extensions and how they might be able to be
implemented with the above kind of example, ... I'm just taking a step
back and sharing my observations of the patch compared to how it
actually looks in the driver with the example vs how most of the other
existing attribute facilities have been designed.

One more final thing which I always felt a little "off" about -- is it
not the case that other type of platforms might could use firmware
attributes as well? Or is this considered ONLY an x86 thing (e.g. that
"firmware" relates to "BIOS" or something) ? Because I always thought
it a bit strange that the header file was only local to
./drivers/platform/x86/ instead of being part of the Linux headers
under ./include ..

And in the same vein as that, is it not the case that other attributes
could benefit from this "typing" mechanism with constraints (e.g.
DEVICE_ATTR of type "enumeration" that only allows for one of a list
of values ? or a number with min/max value / a string with min/max
length etc etc??). I understand this poses an even bigger question and
much larger change (now we are really talking a HUGE impact! ;) ), but
my first guess is that it would probably be sort of nice to have these
types and this automatic constraints mechanism to be somewhat
universal across other type of attributes, otherwise every single
driver author/maintainer has to write their own manual code for the
same kinds of verifications in every function of every driver (e.g.
write an if statement to check the value in every store function of
every attribute they create, and then otherwise return -EINVAL or
similar... this kind of code exists over and over and over in the
kernel today!).

Anyway I hope this all was of some use, and, as mentioned, please feel
free to take all I have said here "with a pinch of salt" -- I would
definitely hope and encourage that others with longer service records
here could chime in regarding this!

Thanks again for the contribution, great work, and have a nice weekend!

Best regards,
Joshua

Powered by blists - more mailing lists