Re: [PATCH v2 0/5] platform/x86: firmware_attributes_class: Add a high level API

[Joshua Grisham]

Den mån 9 juni 2025 kl 03:30 skrev Kurt Borja <kuurtb@xxxxxxxxx>:
>
> Hi Joshua,
>
> On Sat Jun 7, 2025 at 1:38 PM -03, Joshua Grisham wrote:
> > Den lör 17 maj 2025 kl 10:52 skrev Kurt Borja <kuurtb@xxxxxxxxx>:
> >>
> >> 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@xxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxx
> >>
> >> ---
> >> 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",
>
> Yes! You are correct. I completely missed this.
>
> >
> > 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.
>
> This was my first thought but I found out fwupd uses this interface.
> I'll leave the ABI as is to not incur in regressions.
>
> >
> > 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!
>
> Thanks! I'll clean them in the next revision.
>
> >
> > 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! )
>
> Don't worry, feedback is always appreciated :)
>
> >
> > 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...)
>
> I'm not sure what you mean by this. If you mean this API should also
> offer a way to create the Authentication group, I agree!
>
> I was just hoping to get feedback from other maintainers before doing
> that. I want to know if this approach passes the "smell" test for
> everyone.
>
> >
> > 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...),
>
> I agree and I believe this API has this capability. You can do this:
>
> static int power_on_lid_open_read(struct device *dev, long aux, const char **str)
> {
>         ...
> }
>
> static int power_on_lid_open_write(struct device *dev, long aux, const char *str, size_t count)
> {
>         ...
> }
>
> static ssize_t power_on_lid_open_prop_read(struct device *dev, long aux, enum fwat_property prop,
>                                            char *buf)
> {
>         ...
> }
>
> DEFINE_FWAT_OPS(power_on_lid_open, enumeration);
>
> ...
>
> static const struct fwat_attr_config * const galaxybook_fwat_config[] = {
>         FWAT_CONFIG_AUX("power_on_lid_open", 0644,
>                         GB_ATTR_POWER_ON_LID_OPEN,
>                         &power_on_lid_open_ops,
>                         galaxybook_fwat_props,
>                         ARRAY_SIZE(galaxybook_fwat_props)),
>         ...
>         NULL
> }
>
> I.e, you can define ops for each "firmware attribute" (aka
> attribute_group).
>
> I feel the _props approach is currently a bit ugly though, and there is
> room for improvement in the boilerplate department.
>
> In the samsung-galaxybook case I decided to define a single struct
> fwat_attr_ops because I didn't want to make the diff too ugly. The
> *_acpi_{get,set}() functions that already exist are used in other parts
> of the driver, and I would have to change a few lines to make it work.
>
> BTW, you can pass a drvdata pointer to devm_fwat_device_register().
>
> > - 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),
>
> I decided to make this a single callback defined in struct
> fwat_dev_config. I went for this because I didn't like the idea of a
> different function for each attribute_group because it would just be a
> bunch of functions.
>
> > - and then register my fw attr device with my attribute_group (the
> > register function would take care of all the rest..)
>
> Do you think the struct fwat_attr_config * list achieves this? Could it
> be improved in some way?
>

Hi again Kurt!

In case it helps for me to restate that, from my perspective, I can
say that I was maybe not thinking too terribly deeply, more at a
shallow level about the higher level workflow/process for a developer.
For example, with device attributes, it is loosely like this:

1. create functions to handle the actual getting and setting logic
which can become my show/store callback(s)

2. use DEVICE_ATTR_RW or similar macro to create my
device_attribute(s) -- the macro will expect callback functions that
follow specific naming convention from step 1

3. put one of more of my device_attribute(s) in a group (with optional
is_visible callback that also should be created)

4. my "device" is registered (through various mechanisms) while
passing in this attribute group as a parameter and everything is
created for me via the registration process

And this process is roughly the same with device attributes, driver
attributes, bus attributes, hwmon sensor attributes etc etc. so it is
very "familiar" to me (not saying that is "good" or "bad", but it is
nice that once you learn this process, you can sort of more quickly
understand what is happening as you encounter these various types used
in different places).

And from what I can tell as you mentioned in this patch, the process
for FW attributes would instead be something like .. ?

1. create functions or some way to handle the actual getting and
setting logic (but these are maybe not directly used as callbacks, but
instead used as one case within the logic of a more general
"show/store any property under the fw attr" callback)

2. create (or use an existing "standard"?)  enum with all properties
that should exist under my fw attr (display_name, current_value,
default_value, etc)

3. create said general show/store callback function for all properties
within the fw attr (display_name, current_value, default_value, etc)

4. use DEFINE_FWAT_OPS macro to create some fw attr ops for a specific
fw attr (passing the name and type)

5. build an array of fwat_attr_config whose entry is another struct,
but that struct can/will be built by another macro FWAT_CONFIG_AUX (or
similar) with all of its own properties for things like mode, the ops
from step 4, the properties enum from step 2, etc

6. put the attrs config in a new fwat_dev_config (with optional
is_visible callback that also should be created)

7. register the fw attr device with devm_fwat_device_register, passing
the fwat_dev_config and optional drvdata

(of course I could have easily missed something obvious or gotten
something totally wrong here, but hopefully I have captured the gist
of it!)

So I guess what I was trying to say was, the first process seems
easier and more intuitive for my simple mind to understand, whereas
the second process is a fair bit different from the others. Not that
one is necessarily better or worse than the other, but I guess I would
say that the first does feel more "simple" to me (though admittedly
these other attributes are more "simple" by design, of course!).

What I tried to convey before was more that my own personal "dream
scenario" with this is that I as a developer could essentially still
follow the first process I mentioned from above (the one for other
device attrs) and then in a "happy path" scenario I would only need to
focus on callback functions for my getting and setting logic (show
and/or store callbacks) and not have to write ANY other code.
Basically like this:

1. create functions to handle the actual getting and setting logic
which can become my show/store callback(s)

2. use FW_BOOL_ATTR_RW(name, display_name, default_value) or similar
macro to create my fw_attribute(s) --> NOTE that because I chose
"bool" here (in that I used the "BOOL" variant of the macro) then I
will automatically get all of the standard attributes like
display_name, current_value, language_code (maybe even that it
defaults to English unless I want to change it??), fixed choice of
possible_values (as the possible values are always the same for
booleans, I don't need to do anything with this part..) etc etc i.e.
there are standard callback functions for these standard things like
display_name etc and the developer does not have to implement them
(and similar variants could exist for enum, string, and numeric fw
attrs that take care of min/max properties etc)

3. put one of more of my fw_attribute(s) in a group (with optional
is_visible callback that also should be created)

4. my fw attr "device" is registered and the group is passed and
everything is created for me via the registration process

So *essentially* the process of this kind of fw attr implementation
would still be roughly the same as other attribute types, the code
would "look/feel/smell" the same as other attribute types, but I would
just need to pass a few extra parameters to the attribute macro in
order to set the display_name, default_value, etc properties that
would then get picked up by the "standard" show callback functions for
those attributes. The only thing I as an implementer really bother
with is the show/store functions and their actual getting/setting
logic on the actual device.

And bonus if the "standard" logic also enforces the various
constraints (e.g. it should always block to store something that is
not one of my given possible_values, or enforce the various min/max
rules, etc) so that I do not even have to write any of the code for
that part, either!

Maybe I am thinking of this too simply? I have not as mentioned
thought through all of the implementation details of this, but my
first guess is at least for these standard types (enumeration, string,
boolean, etc) is that it does seem like it is *probably* possible??
(not the extensions e.g. "authentication" etc -- needs more thinking
on that!)

Does this make sense, or are we saying the same thing, or maybe
"talking past each other" as they say in Swedish? :)

Thanks again and keep up the good fight!

Joshua

> >
> > 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??)
>
> I can certainly add these macros, but they would be for "firmware
> attributes" defined entirely manually, using struct fwat_attribute.
> Actually I thought of adding these, but I didn't do it because I wanted
> to get something working at first and then add some of these extra
> helpers.
>
> >
> > 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.
>
> Thank you! As I said before, feedback is always welcome.
>
> I feel this API already accomplishes the requirements (which I agree
> with) you listed, albeit with some (maybe a bit too much) boilerplate.
> However your questions make me realise documentation is still lacking, I
> will make it better for the next revision.
>
> If you have more concrete areas of improvement, please let me know! I
> know there is room for improvement. Especially with naming.
>
> >
> > 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 ..
>
> I agree! I'd like to know maintainers opinion on this.
>
> >
> > 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!).
>
> Device attributes already have a lot of helpers for creating some
> common attributes, see [1].
>
> I feel like every driver, subsystem, interface, etc. Has VERY different
> requirements for how sysfs attributes/groups should work. IMO there
> wouldn't be a lot of benefit in providing this infrastructure for other
> subsystems, either because they already have something in place or
> because it wouldn't exactly fit their needs. Kernel ABI is very diverse.
>
> These syfs interfaces are very old and there are good reasons why they
> are the way they are now. I don't think is a bad think to have to
> develop infrastructures for each subsystem!
>
> >
> > 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!
>
> I hope so!
>
> >
> > Thanks again for the contribution, great work, and have a nice weekend!
>
> You too :)
>
> >
> > Best regards,
> > Joshua
>
>
> --
>  ~ Kurt
>