| Message ID | 20250107-pdx86-firmware-attributes-v1-2-9d75c04a3b52@weissschuh.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | platform/x86: firmware_attributes_class: Provide a highlevel interface | expand |
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@weissschuh.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". > config INTEL_IMR > bool "Intel Isolated Memory Region support" > depends on X86_INTEL_QUARK && IOSF_MBI > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index e1b142947067475ee5472400a5a1cd20d79e12bd..610a1ca850a4353fd490e43b214a9e5872d2d28d 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -121,6 +121,7 @@ obj-$(CONFIG_TOPSTAR_LAPTOP) += topstar-laptop.o > > # Platform drivers > obj-$(CONFIG_FW_ATTR_CLASS) += firmware_attributes_class.o > +obj-$(CONFIG_FW_ATTR_TEST) += firmware_attributes_test.o > obj-$(CONFIG_SERIAL_MULTI_INSTANTIATE) += serial-multi-instantiate.o > obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o > obj-$(CONFIG_TOUCHSCREEN_DMI) += touchscreen_dmi.o > diff --git a/drivers/platform/x86/firmware_attributes_test.c b/drivers/platform/x86/firmware_attributes_test.c > new file mode 100644 > index 0000000000000000000000000000000000000000..84f6a92e5163378c655f30ac022d513d7df5a18c > --- /dev/null > +++ b/drivers/platform/x86/firmware_attributes_test.c > @@ -0,0 +1,78 @@ > +// SPDX-License-Identifier: GPL-2.0-only > + > +#include <linux/module.h> > +#include <linux/sysfs.h> > +#include "firmware_attributes_class.h" > + > +struct fw_attr_test_data { > + char attr1_value[PAGE_SIZE]; > +}; > + > +static ssize_t test_attr1_default_value_show(struct firmware_attributes_device *fwadev, > + const struct firmware_attribute *attr, char *buf) > +{ > + return sysfs_emit(buf, "default 1\n"); > +} > + > +static struct firmware_attribute test_attr1_default_value = __ATTR(default_value, 0444, > + test_attr1_default_value_show, > + NULL); > + > +static ssize_t test_attr1_current_value_show(struct firmware_attributes_device *fwadev, > + const struct firmware_attribute *attr, char *buf) > +{ > + struct fw_attr_test_data *data = fwadev->data; > + > + return sysfs_emit(buf, "%s\n", data->attr1_value); > +} > + > +static ssize_t test_attr1_current_value_store(struct firmware_attributes_device *fwadev, > + const struct firmware_attribute *attr, > + const char *buf, size_t count) > +{ > + struct fw_attr_test_data *data = fwadev->data; > + > + return strscpy(data->attr1_value, buf); > +} > + > +static struct firmware_attribute test_attr1_current_value = __ATTR(current_value, 0644, > + test_attr1_current_value_show, > + test_attr1_current_value_store); > + > +static struct attribute *test_attr1_attrs[] = { > + firmware_attribute_type_string, > + &test_attr1_default_value.attr, > + &test_attr1_current_value.attr, > + NULL > +}; > + > +static const struct attribute_group test_attr1_group = { > + .name = "attr1", > + .attrs = test_attr1_attrs, > +}; > + > +static const struct attribute_group *test_attr_groups[] = { > + &test_attr1_group, > + NULL > +}; > + > +static struct firmware_attributes_device *fwadev; > + > +static int __init fw_test_init(void) > +{ > + static struct fw_attr_test_data data = { > + .attr1_value = "attr1", > + }; > + > + fwadev = firmware_attributes_device_register(NULL, "test", test_attr_groups, &data); > + return PTR_ERR_OR_ZERO(fwadev); > +} > +module_init(fw_test_init); > + > +static void __exit fw_test_exit(void) > +{ > + firmware_attributes_device_unregister(fwadev); > +} > +module_exit(fw_test_exit); > + > +MODULE_LICENSE("GPL"); >
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@weissschuh.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". While "possible_values" certainly looks necessary for "enumeration", "min_length" for "strings" does so much less. Thomas
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@weissschuh.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.
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@weissschuh.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
On 1/7/2025 16:13, 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@weissschuh.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 How about just adding min_length and max_length? Since you're adding a string attribute then it makes a good example of a string attribute. It should be pretty trivial; AFAICT it's 0 to PAGE_SIZE.
On 2025-01-07 16:20:14-0600, Mario Limonciello wrote: > On 1/7/2025 16:13, 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@weissschuh.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 > > How about just adding min_length and max_length? Since you're adding a > string attribute then it makes a good example of a string attribute. > It should be pretty trivial; AFAICT it's 0 to PAGE_SIZE. IMO attributes should only exist if they provide useful, driver-specific information. The proposed values are exactly what I'd like to avoid being copied around. min_length=0 does not add any information. max_length=PAGE_SIZE exposes an implementation detail of sysfs. If that limit should become an issue for a driver they can switch to a bin_attribute. I would prefer using get_random_u32() for both attributes, so it's clear that they are only placeholders. But I'm also open to getting outvoted.
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@weissschuh.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. Hi, 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'?
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@weissschuh.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.
On 1/9/2025 09:17, Thomas Weißschuh wrote: > 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@weissschuh.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. Thinking about it a bit more what do you think about providing a macro helper for drivers to use? Think about how we have macros for pm ops for example and drivers can optionally populate all fields with callbacks. A macro for "enumeration" attributes, another for "string" attributes, and another for "integer" attributes. For string it could have optional values .min_length and .max_length, For enumeration it can have a callback that gets you a pointer to a string of possible options. For integer attributes it can have a field for scalar value etc.
On 2025-01-09 09:37:15-0600, Mario Limonciello wrote: > On 1/9/2025 09:17, Thomas Weißschuh wrote: > > 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@weissschuh.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. > > Thinking about it a bit more what do you think about providing a macro > helper for drivers to use? Think about how we have macros for pm ops for > example and drivers can optionally populate all fields with callbacks. This is what I tried, but came to the conclusion that it would be very complex. > A macro for "enumeration" attributes, another for "string" attributes, and > another for "integer" attributes. > > For string it could have optional values .min_length and .max_length, Then we need a flags field to track if the an attribute has either of these fields, because 0 is a valid value. And macros to initialize a static struct of it, with any combination of optional attributes. And more macros to do the same for dynamically allocated structs. > For enumeration it can have a callback that gets you a pointer to a string > of possible options. Callbacks would be better as there is a clear value when a attribute is missing. > For integer attributes it can have a field for scalar value etc. Same as for .min_length and .max_length. Let me try again with callbacks.
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. + config INTEL_IMR bool "Intel Isolated Memory Region support" depends on X86_INTEL_QUARK && IOSF_MBI diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index e1b142947067475ee5472400a5a1cd20d79e12bd..610a1ca850a4353fd490e43b214a9e5872d2d28d 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -121,6 +121,7 @@ obj-$(CONFIG_TOPSTAR_LAPTOP) += topstar-laptop.o # Platform drivers obj-$(CONFIG_FW_ATTR_CLASS) += firmware_attributes_class.o +obj-$(CONFIG_FW_ATTR_TEST) += firmware_attributes_test.o obj-$(CONFIG_SERIAL_MULTI_INSTANTIATE) += serial-multi-instantiate.o obj-$(CONFIG_MLX_PLATFORM) += mlx-platform.o obj-$(CONFIG_TOUCHSCREEN_DMI) += touchscreen_dmi.o diff --git a/drivers/platform/x86/firmware_attributes_test.c b/drivers/platform/x86/firmware_attributes_test.c new file mode 100644 index 0000000000000000000000000000000000000000..84f6a92e5163378c655f30ac022d513d7df5a18c --- /dev/null +++ b/drivers/platform/x86/firmware_attributes_test.c @@ -0,0 +1,78 @@ +// SPDX-License-Identifier: GPL-2.0-only + +#include <linux/module.h> +#include <linux/sysfs.h> +#include "firmware_attributes_class.h" + +struct fw_attr_test_data { + char attr1_value[PAGE_SIZE]; +}; + +static ssize_t test_attr1_default_value_show(struct firmware_attributes_device *fwadev, + const struct firmware_attribute *attr, char *buf) +{ + return sysfs_emit(buf, "default 1\n"); +} + +static struct firmware_attribute test_attr1_default_value = __ATTR(default_value, 0444, + test_attr1_default_value_show, + NULL); + +static ssize_t test_attr1_current_value_show(struct firmware_attributes_device *fwadev, + const struct firmware_attribute *attr, char *buf) +{ + struct fw_attr_test_data *data = fwadev->data; + + return sysfs_emit(buf, "%s\n", data->attr1_value); +} + +static ssize_t test_attr1_current_value_store(struct firmware_attributes_device *fwadev, + const struct firmware_attribute *attr, + const char *buf, size_t count) +{ + struct fw_attr_test_data *data = fwadev->data; + + return strscpy(data->attr1_value, buf); +} + +static struct firmware_attribute test_attr1_current_value = __ATTR(current_value, 0644, + test_attr1_current_value_show, + test_attr1_current_value_store); + +static struct attribute *test_attr1_attrs[] = { + firmware_attribute_type_string, + &test_attr1_default_value.attr, + &test_attr1_current_value.attr, + NULL +}; + +static const struct attribute_group test_attr1_group = { + .name = "attr1", + .attrs = test_attr1_attrs, +}; + +static const struct attribute_group *test_attr_groups[] = { + &test_attr1_group, + NULL +}; + +static struct firmware_attributes_device *fwadev; + +static int __init fw_test_init(void) +{ + static struct fw_attr_test_data data = { + .attr1_value = "attr1", + }; + + fwadev = firmware_attributes_device_register(NULL, "test", test_attr_groups, &data); + return PTR_ERR_OR_ZERO(fwadev); +} +module_init(fw_test_init); + +static void __exit fw_test_exit(void) +{ + firmware_attributes_device_unregister(fwadev); +} +module_exit(fw_test_exit); + +MODULE_LICENSE("GPL");
The driver showcases the use of the new subsystem API. Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- drivers/platform/x86/Kconfig | 12 ++++ drivers/platform/x86/Makefile | 1 + drivers/platform/x86/firmware_attributes_test.c | 78 +++++++++++++++++++++++++ 3 files changed, 91 insertions(+)