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

["Kurt Borja" <kuurtb@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 16539 bytes --]

On Mon Jun 9, 2025 at 6:29 AM -03, Ilpo Järvinen wrote:
> On Sun, 8 Jun 2025, Kurt Borja wrote:
>> 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@gmail.com>:
>> >>
>> >> 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@gmail.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",
>> 
>> 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?
>> 
>> >
>> > 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.
>
> We do move code and headers around whenever we find out the initial 
> placement isn't good any more, it's business as usual.
>
> Usually when something feels off to you, it is off. But I understand 
> in these situations there's often the nagging voice telling inside one's 
> head 'Am I missing something obvious here?'; which rarely is the case ;-). 
> There's no need to assume the existing code is 'perfect' (including its 
> placement). :-)

Then I'll move it to ./include/. Thanks Ilpo :)

-- 
 ~ Kurt


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]