* [PATCH v7 0/3] firmware: Add coreboot CFR firmware attributes driver
@ 2026-07-17 8:50 Sean Rhodes
2026-07-17 8:50 ` [PATCH v7 1/3] firmware: Move firmware attributes class helper Sean Rhodes
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Sean Rhodes @ 2026-07-17 8:50 UTC (permalink / raw)
To: tzungbi
Cc: hansg, ilpo.jarvinen, corentin.chary, luke, denis.benato,
prasanth.ksr, jorge.lopez2, mpearson-lenovo, derekjohn.clark,
josh, briannorris, jwerner, tzimmermann, javierm, kees,
u.kleine-koenig, mst, chenhuacai, wenst, florian.fainelli,
titouan.ameline, oliver, linux-kernel, platform-driver-x86,
Dell.Client.Kernel, chrome-platform, Sean Rhodes
Move the firmware attributes class helper out of platform/x86, rename
the coreboot-table firmware driver directory from google to coreboot, and
add a coreboot CFR firmware attributes driver.
Changes in v7:
- Restore the existing include order in converted platform/x86 drivers
and add Oliver's Tested-by to patch 1.
- Use for loops for the CFR record walks, document pointer-based value
validation and intentionally ignored record types, and remove a
redundant PAGE_SIZE check.
- Validate the CFR payload checksum before parsing it.
- Make EFI writes transactional: lock the complete operation, restore
both the variable and live state on failure, and preserve a pending
reboot indication when rollback cannot fully restore the setting.
- Lock EFI reads and expose settings read-only when EFI writes are not
supported.
- Skip settings beneath suppressed or dependency-gated forms, propagate
enclosing read-only/inactive state, and bound CFR form nesting.
- Propagate transient probe failures instead of silently dropping
settings.
- Preserve APM token zero and skip an enumeration if its values cannot
fit in the sysfs output buffer.
- Simplify the device_create() error path.
Testing:
- x86_64 allmodconfig W=1 builds of the CFR driver, firmware attributes
helper, and all converted platform/x86 consumers.
- UML allmodconfig W=1 builds of the CFR driver and helper, exercising
the path compiled without CONFIG_X86.
Sean Rhodes (3):
firmware: Move firmware attributes class helper
firmware: Rename google firmware directory to coreboot
firmware: coreboot: Add CFR firmware attributes driver
MAINTAINERS | 18 +-
drivers/firmware/Kconfig | 5 +-
drivers/firmware/Makefile | 3 +-
drivers/firmware/{google => coreboot}/Kconfig | 20 +-
.../firmware/{google => coreboot}/Makefile | 1 +
drivers/firmware/{google => coreboot}/cbmem.c | 0
drivers/firmware/coreboot/coreboot-cfr.c | 1141 +++++++++++++++++
.../{google => coreboot}/coreboot_table.c | 0
.../{google => coreboot}/coreboot_table.h | 0
.../framebuffer-coreboot.c | 0
drivers/firmware/{google => coreboot}/gsmi.c | 0
.../memconsole-coreboot.c | 0
.../memconsole-x86-legacy.c | 0
.../{google => coreboot}/memconsole.c | 0
.../{google => coreboot}/memconsole.h | 6 +-
drivers/firmware/{google => coreboot}/vpd.c | 0
.../{google => coreboot}/vpd_decode.c | 0
.../{google => coreboot}/vpd_decode.h | 0
.../firmware_attributes_class.c | 2 +-
drivers/platform/x86/Kconfig | 3 -
drivers/platform/x86/Makefile | 2 -
drivers/platform/x86/asus-armoury.c | 2 +-
.../x86/dell/dell-wmi-sysman/sysman.c | 2 +-
drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 2 +-
drivers/platform/x86/lenovo/think-lmi.c | 2 +-
drivers/platform/x86/lenovo/wmi-other.c | 2 +-
drivers/platform/x86/samsung-galaxybook.c | 2 +-
.../linux/firmware_attributes.h | 6 +-
28 files changed, 1194 insertions(+), 25 deletions(-)
rename drivers/firmware/{google => coreboot}/Kconfig (82%)
rename drivers/firmware/{google => coreboot}/Makefile (92%)
rename drivers/firmware/{google => coreboot}/cbmem.c (100%)
create mode 100644 drivers/firmware/coreboot/coreboot-cfr.c
rename drivers/firmware/{google => coreboot}/coreboot_table.c (100%)
rename drivers/firmware/{google => coreboot}/coreboot_table.h (100%)
rename drivers/firmware/{google => coreboot}/framebuffer-coreboot.c (100%)
rename drivers/firmware/{google => coreboot}/gsmi.c (100%)
rename drivers/firmware/{google => coreboot}/memconsole-coreboot.c (100%)
rename drivers/firmware/{google => coreboot}/memconsole-x86-legacy.c (100%)
rename drivers/firmware/{google => coreboot}/memconsole.c (100%)
rename drivers/firmware/{google => coreboot}/memconsole.h (82%)
rename drivers/firmware/{google => coreboot}/vpd.c (100%)
rename drivers/firmware/{google => coreboot}/vpd_decode.c (100%)
rename drivers/firmware/{google => coreboot}/vpd_decode.h (100%)
rename drivers/{platform/x86 => firmware}/firmware_attributes_class.c (94%)
rename drivers/platform/x86/firmware_attributes_class.h => include/linux/firmware_attributes.h (60%)
Range-diff against v6:
1: 02edc6d05059 ! 1: c414cb12d7ac firmware: Move firmware attributes class helper
@@ Commit message
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com>
+ Tested-by: Oliver Lin <oliver@liuxiaozhen.dev>
Signed-off-by: Sean Rhodes <sean@starlabs.systems>
## MAINTAINERS ##
@@ drivers/platform/x86/Makefile: obj-$(CONFIG_X86_PLATFORM_DRIVERS_HP) += hp/
obj-$(CONFIG_SENSORS_HDAPS) += hdaps.o
## drivers/platform/x86/asus-armoury.c ##
-@@
- #include <linux/dmi.h>
- #include <linux/err.h>
- #include <linux/errno.h>
-+#include <linux/firmware_attributes.h>
- #include <linux/fs.h>
- #include <linux/kernel.h>
- #include <linux/kmod.h>
@@
#include <linux/sysfs.h>
#include "asus-armoury.h"
-#include "firmware_attributes_class.h"
++#include <linux/firmware_attributes.h>
#define ASUS_NB_WMI_EVENT_GUID "0B3CBB35-E3C2-45ED-91C2-4C5A6D195D1C"
## drivers/platform/x86/dell/dell-wmi-sysman/sysman.c ##
@@
- #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
- #include <linux/align.h>
--#include <linux/fs.h>
- #include <linux/dmi.h>
--#include <linux/module.h>
-+#include <linux/firmware_attributes.h>
-+#include <linux/fs.h>
- #include <linux/kernel.h>
-+#include <linux/module.h>
-+#include <linux/nls.h>
- #include <linux/string.h>
#include <linux/sysfs.h>
#include <linux/wmi.h>
-+
#include "dell-wmi-sysman.h"
-#include "../../firmware_attributes_class.h"
++#include <linux/firmware_attributes.h>
#define MAX_TYPES 4
--#include <linux/nls.h>
-
- struct wmi_sysman_priv wmi_priv = {
- .mutex = __MUTEX_INITIALIZER(wmi_priv.mutex),
+ #include <linux/nls.h>
## drivers/platform/x86/hp/hp-bioscfg/bioscfg.c ##
@@
-
- #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
-+#include <linux/errno.h>
-+#include <linux/firmware_attributes.h>
- #include <linux/fs.h>
--#include <linux/module.h>
- #include <linux/kernel.h>
-+#include <linux/module.h>
-+#include <linux/nls.h>
- #include <linux/printk.h>
#include <linux/string.h>
#include <linux/wmi.h>
-+
#include "bioscfg.h"
-#include "../../firmware_attributes_class.h"
--#include <linux/nls.h>
--#include <linux/errno.h>
++#include <linux/firmware_attributes.h>
+ #include <linux/nls.h>
+ #include <linux/errno.h>
- MODULE_AUTHOR("Jorge Lopez <jorge.lopez2@hp.com>");
- MODULE_DESCRIPTION("HP BIOS Configuration Driver");
## drivers/platform/x86/lenovo/think-lmi.c ##
@@
-
- #include <linux/acpi.h>
- #include <linux/array_size.h>
-+#include <linux/dmi.h>
- #include <linux/errno.h>
-+#include <linux/firmware_attributes.h>
- #include <linux/fs.h>
- #include <linux/mutex.h>
- #include <linux/string_helpers.h>
#include <linux/types.h>
--#include <linux/dmi.h>
+ #include <linux/dmi.h>
#include <linux/wmi.h>
-#include "../firmware_attributes_class.h"
-+
++#include <linux/firmware_attributes.h>
#include "think-lmi.h"
static bool debug_support;
## drivers/platform/x86/lenovo/wmi-other.c ##
-@@
- #include <linux/container_of.h>
- #include <linux/device.h>
- #include <linux/export.h>
-+#include <linux/firmware_attributes.h>
- #include <linux/gfp_types.h>
- #include <linux/hwmon.h>
- #include <linux/idr.h>
@@
#include "wmi-capdata.h"
#include "wmi-events.h"
#include "wmi-helpers.h"
-#include "../firmware_attributes_class.h"
++#include <linux/firmware_attributes.h>
#define LENOVO_OTHER_MODE_GUID "DC2A8805-3A8C-41BA-A6F7-092E0089CD3B"
## drivers/platform/x86/samsung-galaxybook.c ##
@@
- #include <linux/acpi.h>
- #include <linux/bits.h>
- #include <linux/err.h>
-+#include <linux/firmware_attributes.h>
- #include <linux/i8042.h>
- #include <linux/init.h>
- #include <linux/input.h>
-@@
- #include <linux/sysfs.h>
#include <linux/uuid.h>
#include <linux/workqueue.h>
-+
#include <acpi/battery.h>
-#include "firmware_attributes_class.h"
++#include <linux/firmware_attributes.h>
#define DRIVER_NAME "samsung-galaxybook"
2: d5d3d1334994 = 2: 198e2d4dff44 firmware: Rename google firmware directory to coreboot
3: 7a65c6b0e631 ! 3: f4842df09f6e firmware: coreboot: Add CFR firmware attributes driver
@@ drivers/firmware/coreboot/Kconfig: config GOOGLE_COREBOOT_TABLE
+ tristate "coreboot CFR firmware attributes"
+ depends on EFI
+ depends on GOOGLE_COREBOOT_TABLE
++ select CRC32
+ select FW_ATTR_CLASS
+ help
+ This driver parses the coreboot CFR table and exposes runtime EFI
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+#include <linux/array_size.h>
+#include <linux/bitops.h>
+#include <linux/cleanup.h>
++#include <linux/crc32.h>
+#include <linux/ctype.h>
+#include <linux/device.h>
+#include <linux/device-id/coreboot.h>
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+#define COREBOOT_CFR_APM_CNT_PORT 0xb2
+#define COREBOOT_CFR_APM_STS_PORT 0xb3
+#define COREBOOT_CFR_APM_APPLY_CMD 0xe3
++#define COREBOOT_CFR_MAX_FORM_DEPTH 16
+
+static efi_guid_t coreboot_cfr_guid = EFI_GUID(0xceae4c1d, 0x335b, 0x4685,
+ 0xa4, 0xa0, 0xfc, 0x4a,
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+ struct list_head settings;
+ /* Serializes EFI variable writes and the matching runtime apply hook. */
+ struct mutex lock;
++ bool efi_writes_supported;
+ bool pending_reboot;
+};
+
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+coreboot_cfr_child_entry(const void *base, size_t len, u32 tag)
+{
+ const struct coreboot_table_entry *entry;
-+ size_t off = 0;
++ size_t off;
+
-+ while (off < len) {
++ for (off = 0; off < len; off += entry->size) {
+ if (len - off < sizeof(*entry))
+ return NULL;
+
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+
+ if (entry->tag == tag)
+ return entry;
-+
-+ off += entry->size;
+ }
+
+ return NULL;
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+ return no_free_ptr(efi_name);
+}
+
-+static int coreboot_cfr_read_value(const struct coreboot_cfr_setting *setting,
-+ u32 *value, u32 *attrs)
++static int coreboot_cfr_read_efi_value_locked(efi_char16_t *efi_name,
++ u32 *value, u32 *attrs)
+{
+ unsigned long size = sizeof(__le32);
-+ efi_char16_t *efi_name;
+ efi_status_t status;
+ __le32 data;
+ u32 attr;
+
-+ efi_name = coreboot_cfr_efi_name(setting->name);
-+ if (IS_ERR(efi_name))
-+ return PTR_ERR(efi_name);
-+
+ status = efivar_get_variable(efi_name, &coreboot_cfr_guid, &attr,
+ &size, &data);
-+ kfree(efi_name);
+ if (status != EFI_SUCCESS)
+ return efi_status_to_err(status);
+
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+ return 0;
+}
+
-+static int coreboot_cfr_write_efi_value(struct coreboot_cfr_setting *setting,
-+ u32 value, u32 attrs)
++static int coreboot_cfr_read_value(const struct coreboot_cfr_setting *setting,
++ u32 *value, u32 *attrs)
+{
+ efi_char16_t *efi_name;
-+ efi_status_t status;
-+ __le32 data;
++ int ret;
+
+ efi_name = coreboot_cfr_efi_name(setting->name);
+ if (IS_ERR(efi_name))
+ return PTR_ERR(efi_name);
+
-+ data = cpu_to_le32(value);
-+ status = efivar_set_variable(efi_name, &coreboot_cfr_guid, attrs,
-+ sizeof(data), &data);
++ ret = efivar_lock();
++ if (ret) {
++ kfree(efi_name);
++ return ret;
++ }
++
++ ret = coreboot_cfr_read_efi_value_locked(efi_name, value, attrs);
++ efivar_unlock();
+ kfree(efi_name);
++
++ return ret;
++}
++
++static int coreboot_cfr_write_efi_value_locked(efi_char16_t *efi_name,
++ u32 value, u32 attrs)
++{
++ efi_status_t status;
++ __le32 data;
++
++ if (!efivar_supports_writes())
++ return -EROFS;
++
++ data = cpu_to_le32(value);
++ status = efivar_set_variable_locked(efi_name, &coreboot_cfr_guid, attrs,
++ sizeof(data), &data, false);
+ if (status != EFI_SUCCESS)
+ return efi_status_to_err(status);
+
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+#ifdef CONFIG_X86
+ u8 status;
+
-+ if (setting->runtime_apply_method != CFR_RUNTIME_APPLY_APM_CNT ||
-+ !setting->runtime_apply_id)
++ if (setting->runtime_apply_method != CFR_RUNTIME_APPLY_APM_CNT)
+ return -EOPNOTSUPP;
+
+ outb((u8)setting->runtime_apply_id, COREBOOT_CFR_APM_STS_PORT);
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+static int coreboot_cfr_write_value(struct coreboot_cfr_setting *setting,
+ u32 value)
+{
++ efi_char16_t *efi_name;
+ u32 attrs;
+ u32 old;
++ int restore_ret;
+ int ret;
+ bool changed = false;
+
+ if (setting->read_only)
+ return -EACCES;
+
-+ scoped_guard(mutex, &setting->drvdata->lock) {
-+ ret = coreboot_cfr_read_value(setting, &old, &attrs);
-+ if (ret)
-+ return ret;
++ efi_name = coreboot_cfr_efi_name(setting->name);
++ if (IS_ERR(efi_name))
++ return PTR_ERR(efi_name);
+
-+ if ((attrs & COREBOOT_CFR_EFI_ATTRS) != COREBOOT_CFR_EFI_ATTRS)
-+ return -EOPNOTSUPP;
++ mutex_lock(&setting->drvdata->lock);
++ ret = efivar_lock();
++ if (ret)
++ goto out_unlock_mutex;
++
++ ret = coreboot_cfr_read_efi_value_locked(efi_name, &old, &attrs);
++ if (ret)
++ goto out_unlock_efi;
+
-+ if (old == value)
-+ return 0;
++ if ((attrs & COREBOOT_CFR_EFI_ATTRS) != COREBOOT_CFR_EFI_ATTRS) {
++ ret = -EOPNOTSUPP;
++ goto out_unlock_efi;
++ }
+
-+ ret = coreboot_cfr_write_efi_value(setting, value, attrs);
-+ if (ret)
-+ return ret;
-+ changed = true;
++ if (old == value)
++ goto out_unlock_efi;
+
-+ ret = coreboot_cfr_apply_runtime(setting);
-+ if (ret == -EOPNOTSUPP) {
-+ /* EFI changed; firmware will consume it after reboot. */
++ ret = coreboot_cfr_write_efi_value_locked(efi_name, value, attrs);
++ if (ret)
++ goto out_unlock_efi;
++ changed = true;
++
++ ret = coreboot_cfr_apply_runtime(setting);
++ if (ret == -EOPNOTSUPP) {
++ /* EFI changed; firmware will consume it after reboot. */
++ setting->drvdata->pending_reboot = true;
++ ret = 0;
++ } else if (ret) {
++ restore_ret = coreboot_cfr_write_efi_value_locked(efi_name, old, attrs);
++ if (restore_ret) {
+ setting->drvdata->pending_reboot = true;
-+ ret = 0;
-+ } else if (ret) {
-+ int restore_ret;
-+
-+ restore_ret = coreboot_cfr_write_efi_value(setting, old, attrs);
-+ if (restore_ret)
-+ ret = restore_ret;
-+ else
-+ changed = false;
++ ret = restore_ret;
++ } else if (coreboot_cfr_apply_runtime(setting)) {
++ setting->drvdata->pending_reboot = true;
++ } else {
++ changed = false;
+ }
+ }
+
++out_unlock_efi:
++ efivar_unlock();
++out_unlock_mutex:
++ mutex_unlock(&setting->drvdata->lock);
++ kfree(efi_name);
++
+ if (changed)
+ kobject_uevent(&setting->drvdata->class_dev->kobj, KOBJ_CHANGE);
+ return ret;
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+{
+ u32 delta;
+
-+ if (setting->type != COREBOOT_CFR_SETTING_NUMBER)
++ if (setting->type != COREBOOT_CFR_SETTING_NUMBER) {
++ /* Enum and bool values are valid only if they have a label. */
+ return coreboot_cfr_label_from_value(setting, value);
++ }
+
+ if (value < setting->min || value > setting->max)
+ return false;
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+ for (i = 0; i < setting->n_values; i++) {
+ len += sysfs_emit_at(buf, len, "%s%s", i ? ";" : "",
+ setting->values[i].label);
-+ if (len >= PAGE_SIZE)
-+ return len;
+ }
+
+ len += sysfs_emit_at(buf, len, "\n");
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+static int coreboot_cfr_count_enum_values(const void *base, size_t len)
+{
+ const struct coreboot_table_entry *entry;
-+ size_t off = 0;
++ size_t off;
+ int count = 0;
+
-+ while (off < len) {
++ for (off = 0; off < len; off += entry->size) {
+ if (len - off < sizeof(*entry))
+ return -EINVAL;
+
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+
+ if (entry->tag == CFR_TAG_ENUM_VALUE)
+ count++;
-+
-+ off += entry->size;
+ }
+
+ return count;
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+ const struct lb_cfr_enum_value *enum_value;
+ const struct lb_cfr_varbinary *label;
+ const struct coreboot_table_entry *entry;
-+ size_t off = 0;
++ size_t off;
+ int count;
+
+ count = coreboot_cfr_count_enum_values(base, len);
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+ if (!setting->values)
+ return -ENOMEM;
+
-+ while (off < len) {
++ for (off = 0; off < len; off += entry->size) {
+ entry = base + off;
-+ if (entry->tag != CFR_TAG_ENUM_VALUE) {
-+ off += entry->size;
++ if (entry->tag != CFR_TAG_ENUM_VALUE)
+ continue;
-+ }
+
+ if (entry->size < sizeof(*enum_value))
+ return -EINVAL;
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+
+ setting->values[setting->n_values].value = enum_value->value;
+ setting->n_values++;
-+ off += entry->size;
+ }
+
+ return 0;
+}
+
++static bool
++coreboot_cfr_possible_values_fit(const struct coreboot_cfr_setting *setting)
++{
++ size_t len = 1; /* Trailing newline. */
++ unsigned int i;
++
++ for (i = 0; i < setting->n_values; i++) {
++ if (i)
++ len++;
++
++ if (strlen(setting->values[i].label) >= PAGE_SIZE - len)
++ return false;
++
++ len += strlen(setting->values[i].label);
++ }
++
++ return true;
++}
++
+static int coreboot_cfr_setting_is_usable(struct coreboot_cfr_setting *setting)
+{
+ u32 value;
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+}
+
+static int coreboot_cfr_add_numeric_option(struct coreboot_cfr_drvdata *data,
-+ const struct lb_cfr_numeric_option *option)
++ const struct lb_cfr_numeric_option *option,
++ bool parent_read_only)
+{
+ const struct lb_cfr_varbinary *name;
+ const struct lb_cfr_varbinary *display_name;
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+ setting->min = option->min;
+ setting->max = option->max;
+ setting->step = option->step ?: 1;
-+ setting->read_only = option->flags & COREBOOT_CFR_OPT_READ_ONLY_FLAGS;
++ setting->read_only =
++ (option->flags & COREBOOT_CFR_OPT_READ_ONLY_FLAGS) ||
++ !data->efi_writes_supported || parent_read_only;
+
+ runtime_apply = coreboot_cfr_child_runtime_apply(child_base, child_len);
+ if (IS_ERR(runtime_apply)) {
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+ goto err_put_setting;
+ }
+
-+ if (runtime_apply && runtime_apply->method == CFR_RUNTIME_APPLY_APM_CNT &&
-+ runtime_apply->id) {
++ if (runtime_apply && runtime_apply->method == CFR_RUNTIME_APPLY_APM_CNT) {
+ setting->runtime_apply_method = runtime_apply->method;
+ setting->runtime_apply_id = runtime_apply->id;
+ }
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+
+ if (ret)
+ goto err_put_setting;
++ if (setting->type != COREBOOT_CFR_SETTING_NUMBER &&
++ !coreboot_cfr_possible_values_fit(setting)) {
++ ret = 0;
++ goto err_put_setting;
++ }
+
+ ret = coreboot_cfr_setting_is_usable(setting);
+ if (ret) {
-+ ret = 0;
++ if (ret == -ENOENT || ret == -EINVAL || ret == -EOPNOTSUPP ||
++ ret == -ENAMETOOLONG)
++ ret = 0;
+ goto err_put_setting;
+ }
+
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+}
+
+static int coreboot_cfr_parse_records(struct coreboot_cfr_drvdata *data,
-+ const void *base, size_t len)
++ const void *base, size_t len,
++ unsigned int depth,
++ bool parent_read_only)
+{
++ const struct lb_cfr_option_form *form;
+ const struct coreboot_table_entry *entry;
+ const void *child_base;
+ size_t child_len;
-+ size_t off = 0;
++ size_t off;
+ int ret;
+
-+ while (off < len) {
++ for (off = 0; off < len; off += entry->size) {
+ if (len - off < sizeof(*entry))
+ return -EINVAL;
+
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+ if (entry->size < sizeof(struct lb_cfr_option_form))
+ return -EINVAL;
+
++ form = base + off;
++ if (form->dependency_id ||
++ form->flags & CFR_OPTFLAG_SUPPRESS)
++ break;
++
++ if (depth >= COREBOOT_CFR_MAX_FORM_DEPTH)
++ return -E2BIG;
++
+ child_base = base + off + sizeof(struct lb_cfr_option_form);
+ child_len = entry->size - sizeof(struct lb_cfr_option_form);
+ ret = coreboot_cfr_parse_records(data, child_base,
-+ child_len);
++ child_len, depth + 1,
++ parent_read_only ||
++ (form->flags &
++ COREBOOT_CFR_OPT_READ_ONLY_FLAGS));
+ if (ret)
+ return ret;
+ break;
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+ case CFR_TAG_OPTION_BOOL:
+ if (entry->size < sizeof(struct lb_cfr_numeric_option))
+ return -EINVAL;
-+ ret = coreboot_cfr_add_numeric_option(data, base + off);
++ ret = coreboot_cfr_add_numeric_option(data, base + off,
++ parent_read_only);
+ if (ret)
+ return ret;
+ break;
+ default:
++ /* Ignore unsupported, child-only and future record types. */
+ break;
+ }
-+
-+ off += entry->size;
+ }
+
+ return 0;
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+ if (root->size < sizeof(*root) || root->size > dev->entry.size)
+ return -EINVAL;
+
++ payload_len = root->size - sizeof(*root);
++ if (crc32_be(0, root + 1, payload_len) != root->checksum)
++ return -EBADMSG;
++
+ if (!efivar_is_available())
+ return -EPROBE_DEFER;
+
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+ if (!data)
+ return -ENOMEM;
+
++ ret = efivar_lock();
++ if (ret)
++ return ret;
++ data->efi_writes_supported = efivar_supports_writes();
++ efivar_unlock();
++
+ INIT_LIST_HEAD(&data->settings);
+ ret = devm_mutex_init(&dev->dev, &data->lock);
+ if (ret)
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+
+ data->class_dev = device_create(&firmware_attributes_class, NULL,
+ MKDEV(0, 0), NULL, DRIVER_NAME);
-+ if (IS_ERR(data->class_dev)) {
-+ ret = PTR_ERR(data->class_dev);
-+ goto err_clear_data;
-+ }
++ if (IS_ERR(data->class_dev))
++ return PTR_ERR(data->class_dev);
+ dev_set_drvdata(data->class_dev, data);
+
+ data->attrs_kset = kset_create_and_add("attributes", NULL,
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+ if (ret)
+ goto err_unregister_attrs;
+
-+ payload_len = root->size - sizeof(*root);
-+ ret = coreboot_cfr_parse_records(data, root + 1, payload_len);
++ ret = coreboot_cfr_parse_records(data, root + 1, payload_len, 0, false);
+ if (ret)
+ goto err_unregister_settings;
+
@@ drivers/firmware/coreboot/coreboot-cfr.c (new)
+ kset_unregister(data->attrs_kset);
+err_unregister_dev:
+ device_unregister(data->class_dev);
-+err_clear_data:
+ return ret;
+}
+
--
2.53.0
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v7 1/3] firmware: Move firmware attributes class helper 2026-07-17 8:50 [PATCH v7 0/3] firmware: Add coreboot CFR firmware attributes driver Sean Rhodes @ 2026-07-17 8:50 ` Sean Rhodes 2026-07-18 2:22 ` Derek J. Clark 2026-07-17 8:50 ` [PATCH v7 2/3] firmware: Rename google firmware directory to coreboot Sean Rhodes 2026-07-17 8:50 ` [PATCH v7 3/3] firmware: coreboot: Add CFR firmware attributes driver Sean Rhodes 2 siblings, 1 reply; 9+ messages in thread From: Sean Rhodes @ 2026-07-17 8:50 UTC (permalink / raw) To: tzungbi Cc: hansg, ilpo.jarvinen, corentin.chary, luke, denis.benato, prasanth.ksr, jorge.lopez2, mpearson-lenovo, derekjohn.clark, josh, briannorris, jwerner, tzimmermann, javierm, kees, u.kleine-koenig, mst, chenhuacai, wenst, florian.fainelli, titouan.ameline, oliver, linux-kernel, platform-driver-x86, Dell.Client.Kernel, chrome-platform, Sean Rhodes Move the firmware attributes class helper from drivers/platform/x86 to drivers/firmware and expose its class declaration through a public Linux header. The helper is not x86-specific. Keeping it in firmware lets coreboot firmware drivers use the standard firmware-attributes ABI without living under platform/x86. Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca> Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com> Tested-by: Oliver Lin <oliver@liuxiaozhen.dev> Signed-off-by: Sean Rhodes <sean@starlabs.systems> --- MAINTAINERS | 8 ++++++++ drivers/firmware/Kconfig | 3 +++ drivers/firmware/Makefile | 1 + .../x86 => firmware}/firmware_attributes_class.c | 2 +- drivers/platform/x86/Kconfig | 3 --- drivers/platform/x86/Makefile | 2 -- drivers/platform/x86/asus-armoury.c | 2 +- drivers/platform/x86/dell/dell-wmi-sysman/sysman.c | 2 +- drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 2 +- drivers/platform/x86/lenovo/think-lmi.c | 2 +- drivers/platform/x86/lenovo/wmi-other.c | 2 +- drivers/platform/x86/samsung-galaxybook.c | 2 +- .../linux/firmware_attributes.h | 6 +++--- 13 files changed, 22 insertions(+), 15 deletions(-) rename drivers/{platform/x86 => firmware}/firmware_attributes_class.c (94%) rename drivers/platform/x86/firmware_attributes_class.h => include/linux/firmware_attributes.h (60%) diff --git a/MAINTAINERS b/MAINTAINERS index f37a81950e25..07bdba98b1b1 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -10114,6 +10114,14 @@ F: include/linux/firewire.h F: include/uapi/linux/firewire*.h F: tools/firewire/ +FIRMWARE ATTRIBUTES CLASS +M: Hans de Goede <hansg@kernel.org> +M: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> +L: platform-driver-x86@vger.kernel.org +S: Maintained +F: drivers/firmware/firmware_attributes_class.c +F: include/linux/firmware_attributes.h + FIRMWARE FRAMEWORK FOR ARMV8-A M: Sudeep Holla <sudeep.holla@kernel.org> L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers) diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index 12dc70254842..78a239f8bbef 100644 --- a/drivers/firmware/Kconfig +++ b/drivers/firmware/Kconfig @@ -8,6 +8,9 @@ menu "Firmware Drivers" source "drivers/firmware/arm_scmi/Kconfig" +config FW_ATTR_CLASS + tristate + config ARM_SCPI_PROTOCOL tristate "ARM System Control and Power Interface (SCPI) Message Protocol" depends on ARM || ARM64 || COMPILE_TEST diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile index 4ddec2820c96..385b6e8b7fc6 100644 --- a/drivers/firmware/Makefile +++ b/drivers/firmware/Makefile @@ -16,6 +16,7 @@ obj-$(CONFIG_FIRMWARE_MEMMAP) += memmap.o obj-$(CONFIG_MTK_ADSP_IPC) += mtk-adsp-ipc.o obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o obj-$(CONFIG_FW_CFG_SYSFS) += qemu_fw_cfg.o +obj-$(CONFIG_FW_ATTR_CLASS) += firmware_attributes_class.o obj-$(CONFIG_SYSFB) += sysfb.o obj-$(CONFIG_SYSFB_SIMPLEFB) += sysfb_simplefb.o obj-$(CONFIG_TH1520_AON_PROTOCOL) += thead,th1520-aon.o diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/firmware/firmware_attributes_class.c similarity index 94% rename from drivers/platform/x86/firmware_attributes_class.c rename to drivers/firmware/firmware_attributes_class.c index 736e96c186d9..503ecc288086 100644 --- a/drivers/platform/x86/firmware_attributes_class.c +++ b/drivers/firmware/firmware_attributes_class.c @@ -3,7 +3,7 @@ /* Firmware attributes class helper module */ #include <linux/module.h> -#include "firmware_attributes_class.h" +#include <linux/firmware_attributes.h> const struct class firmware_attributes_class = { .name = "firmware-attributes", diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index b54b5212b204..beed14c38b54 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -926,9 +926,6 @@ config DASHARO_ACPI source "drivers/platform/x86/x86-android-tablets/Kconfig" -config FW_ATTR_CLASS - tristate - 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 872ac3842391..cab20a4145e3 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -63,8 +63,6 @@ obj-$(CONFIG_X86_PLATFORM_DRIVERS_HP) += hp/ # Hewlett Packard Enterprise obj-$(CONFIG_UV_SYSFS) += uv_sysfs.o -obj-$(CONFIG_FW_ATTR_CLASS) += firmware_attributes_class.o - # IBM Thinkpad (before 2005) obj-$(CONFIG_IBM_RTL) += ibm_rtl.o obj-$(CONFIG_SENSORS_HDAPS) += hdaps.o diff --git a/drivers/platform/x86/asus-armoury.c b/drivers/platform/x86/asus-armoury.c index 495dc1e31d40..93fac0d464d2 100644 --- a/drivers/platform/x86/asus-armoury.c +++ b/drivers/platform/x86/asus-armoury.c @@ -34,7 +34,7 @@ #include <linux/sysfs.h> #include "asus-armoury.h" -#include "firmware_attributes_class.h" +#include <linux/firmware_attributes.h> #define ASUS_NB_WMI_EVENT_GUID "0B3CBB35-E3C2-45ED-91C2-4C5A6D195D1C" diff --git a/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c b/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c index ab46a023cc34..85670be01647 100644 --- a/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c +++ b/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c @@ -16,7 +16,7 @@ #include <linux/sysfs.h> #include <linux/wmi.h> #include "dell-wmi-sysman.h" -#include "../../firmware_attributes_class.h" +#include <linux/firmware_attributes.h> #define MAX_TYPES 4 #include <linux/nls.h> diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c index 27fd6cd21529..44314ae4e5e3 100644 --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c @@ -14,7 +14,7 @@ #include <linux/string.h> #include <linux/wmi.h> #include "bioscfg.h" -#include "../../firmware_attributes_class.h" +#include <linux/firmware_attributes.h> #include <linux/nls.h> #include <linux/errno.h> diff --git a/drivers/platform/x86/lenovo/think-lmi.c b/drivers/platform/x86/lenovo/think-lmi.c index e215e86e3db7..a56f152b5f28 100644 --- a/drivers/platform/x86/lenovo/think-lmi.c +++ b/drivers/platform/x86/lenovo/think-lmi.c @@ -20,7 +20,7 @@ #include <linux/types.h> #include <linux/dmi.h> #include <linux/wmi.h> -#include "../firmware_attributes_class.h" +#include <linux/firmware_attributes.h> #include "think-lmi.h" static bool debug_support; diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c index fbb32bf404f2..e9a2c4684e5f 100644 --- a/drivers/platform/x86/lenovo/wmi-other.c +++ b/drivers/platform/x86/lenovo/wmi-other.c @@ -50,7 +50,7 @@ #include "wmi-capdata.h" #include "wmi-events.h" #include "wmi-helpers.h" -#include "../firmware_attributes_class.h" +#include <linux/firmware_attributes.h> #define LENOVO_OTHER_MODE_GUID "DC2A8805-3A8C-41BA-A6F7-092E0089CD3B" diff --git a/drivers/platform/x86/samsung-galaxybook.c b/drivers/platform/x86/samsung-galaxybook.c index 6382af0b106c..74b8c887dd79 100644 --- a/drivers/platform/x86/samsung-galaxybook.c +++ b/drivers/platform/x86/samsung-galaxybook.c @@ -28,7 +28,7 @@ #include <linux/uuid.h> #include <linux/workqueue.h> #include <acpi/battery.h> -#include "firmware_attributes_class.h" +#include <linux/firmware_attributes.h> #define DRIVER_NAME "samsung-galaxybook" diff --git a/drivers/platform/x86/firmware_attributes_class.h b/include/linux/firmware_attributes.h similarity index 60% rename from drivers/platform/x86/firmware_attributes_class.h rename to include/linux/firmware_attributes.h index d27abe54fcf9..d4026ba1fcc2 100644 --- a/drivers/platform/x86/firmware_attributes_class.h +++ b/include/linux/firmware_attributes.h @@ -2,11 +2,11 @@ /* Firmware attributes class helper module */ -#ifndef FW_ATTR_CLASS_H -#define FW_ATTR_CLASS_H +#ifndef _LINUX_FIRMWARE_ATTRIBUTES_H +#define _LINUX_FIRMWARE_ATTRIBUTES_H #include <linux/device/class.h> extern const struct class firmware_attributes_class; -#endif /* FW_ATTR_CLASS_H */ +#endif /* _LINUX_FIRMWARE_ATTRIBUTES_H */ -- 2.53.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v7 1/3] firmware: Move firmware attributes class helper 2026-07-17 8:50 ` [PATCH v7 1/3] firmware: Move firmware attributes class helper Sean Rhodes @ 2026-07-18 2:22 ` Derek J. Clark 0 siblings, 0 replies; 9+ messages in thread From: Derek J. Clark @ 2026-07-18 2:22 UTC (permalink / raw) To: Sean Rhodes, tzungbi Cc: hansg, ilpo.jarvinen, corentin.chary, luke, denis.benato, prasanth.ksr, jorge.lopez2, mpearson-lenovo, josh, briannorris, jwerner, tzimmermann, javierm, kees, u.kleine-koenig, mst, chenhuacai, wenst, florian.fainelli, titouan.ameline, oliver, linux-kernel, platform-driver-x86, Dell.Client.Kernel, chrome-platform On July 17, 2026 1:50:01 AM PDT, Sean Rhodes <sean@starlabs.systems> wrote: >Move the firmware attributes class helper from drivers/platform/x86 to >drivers/firmware and expose its class declaration through a public Linux >header. > >The helper is not x86-specific. Keeping it in firmware lets coreboot >firmware drivers use the standard firmware-attributes ABI without living >under platform/x86. > >Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca> >Reviewed-by: Derek J. Clark <derekjohn.clark@gmail.com> >Tested-by: Oliver Lin <oliver@liuxiaozhen.dev> >Signed-off-by: Sean Rhodes <sean@starlabs.systems> >--- > MAINTAINERS | 8 ++++++++ > drivers/firmware/Kconfig | 3 +++ > drivers/firmware/Makefile | 1 + > .../x86 => firmware}/firmware_attributes_class.c | 2 +- > drivers/platform/x86/Kconfig | 3 --- > drivers/platform/x86/Makefile | 2 -- > drivers/platform/x86/asus-armoury.c | 2 +- > drivers/platform/x86/dell/dell-wmi-sysman/sysman.c | 2 +- > drivers/platform/x86/hp/hp-bioscfg/bioscfg.c | 2 +- > drivers/platform/x86/lenovo/think-lmi.c | 2 +- > drivers/platform/x86/lenovo/wmi-other.c | 2 +- > drivers/platform/x86/samsung-galaxybook.c | 2 +- > .../linux/firmware_attributes.h | 6 +++--- > 13 files changed, 22 insertions(+), 15 deletions(-) > rename drivers/{platform/x86 => firmware}/firmware_attributes_class.c (94%) > rename drivers/platform/x86/firmware_attributes_class.h => include/linux/firmware_attributes.h (60%) > >diff --git a/MAINTAINERS b/MAINTAINERS >index f37a81950e25..07bdba98b1b1 100644 >--- a/MAINTAINERS >+++ b/MAINTAINERS >@@ -10114,6 +10114,14 @@ F: include/linux/firewire.h > F: include/uapi/linux/firewire*.h > F: tools/firewire/ > >+FIRMWARE ATTRIBUTES CLASS >+M: Hans de Goede <hansg@kernel.org> >+M: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> >+L: platform-driver-x86@vger.kernel.org >+S: Maintained >+F: drivers/firmware/firmware_attributes_class.c >+F: include/linux/firmware_attributes.h >+ > FIRMWARE FRAMEWORK FOR ARMV8-A > M: Sudeep Holla <sudeep.holla@kernel.org> > L: linux-arm-kernel@lists.infradead.org (moderated for non-subscribers) >diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig >index 12dc70254842..78a239f8bbef 100644 >--- a/drivers/firmware/Kconfig >+++ b/drivers/firmware/Kconfig >@@ -8,6 +8,9 @@ menu "Firmware Drivers" > > source "drivers/firmware/arm_scmi/Kconfig" > >+config FW_ATTR_CLASS >+ tristate >+ > config ARM_SCPI_PROTOCOL > tristate "ARM System Control and Power Interface (SCPI) Message Protocol" > depends on ARM || ARM64 || COMPILE_TEST >diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile >index 4ddec2820c96..385b6e8b7fc6 100644 >--- a/drivers/firmware/Makefile >+++ b/drivers/firmware/Makefile >@@ -16,6 +16,7 @@ obj-$(CONFIG_FIRMWARE_MEMMAP) += memmap.o > obj-$(CONFIG_MTK_ADSP_IPC) += mtk-adsp-ipc.o > obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o > obj-$(CONFIG_FW_CFG_SYSFS) += qemu_fw_cfg.o >+obj-$(CONFIG_FW_ATTR_CLASS) += firmware_attributes_class.o > obj-$(CONFIG_SYSFB) += sysfb.o > obj-$(CONFIG_SYSFB_SIMPLEFB) += sysfb_simplefb.o > obj-$(CONFIG_TH1520_AON_PROTOCOL) += thead,th1520-aon.o >diff --git a/drivers/platform/x86/firmware_attributes_class.c b/drivers/firmware/firmware_attributes_class.c >similarity index 94% >rename from drivers/platform/x86/firmware_attributes_class.c >rename to drivers/firmware/firmware_attributes_class.c >index 736e96c186d9..503ecc288086 100644 >--- a/drivers/platform/x86/firmware_attributes_class.c >+++ b/drivers/firmware/firmware_attributes_class.c >@@ -3,7 +3,7 @@ > /* Firmware attributes class helper module */ > > #include <linux/module.h> >-#include "firmware_attributes_class.h" >+#include <linux/firmware_attributes.h> > Hi Sean, I think there is a middle ground between just renaming these while knowingly having them all in the wrong spot, and reorganizing all the includes for drivers whose includes aren't properly alphabetized in a single patch. One of these options would be preferable: - Introduce a cleanup patch before the rename that does the reordering, then rename in the correct order. - Do the rename without a full reordering, placing the firmware attributes include right after its preceding alphabetical order location so that it wouldn't need to be moved if someone else does a full reordering later. Personally I think the first option is better, but if that's out of your desired scope or you're worried about additional effects in existing drivers then please at least do the second option. Thanks, Derek > const struct class firmware_attributes_class = { > .name = "firmware-attributes", >diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig >index b54b5212b204..beed14c38b54 100644 >--- a/drivers/platform/x86/Kconfig >+++ b/drivers/platform/x86/Kconfig >@@ -926,9 +926,6 @@ config DASHARO_ACPI > > source "drivers/platform/x86/x86-android-tablets/Kconfig" > >-config FW_ATTR_CLASS >- tristate >- > 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 872ac3842391..cab20a4145e3 100644 >--- a/drivers/platform/x86/Makefile >+++ b/drivers/platform/x86/Makefile >@@ -63,8 +63,6 @@ obj-$(CONFIG_X86_PLATFORM_DRIVERS_HP) += hp/ > # Hewlett Packard Enterprise > obj-$(CONFIG_UV_SYSFS) += uv_sysfs.o > >-obj-$(CONFIG_FW_ATTR_CLASS) += firmware_attributes_class.o >- > # IBM Thinkpad (before 2005) > obj-$(CONFIG_IBM_RTL) += ibm_rtl.o > obj-$(CONFIG_SENSORS_HDAPS) += hdaps.o >diff --git a/drivers/platform/x86/asus-armoury.c b/drivers/platform/x86/asus-armoury.c >index 495dc1e31d40..93fac0d464d2 100644 >--- a/drivers/platform/x86/asus-armoury.c >+++ b/drivers/platform/x86/asus-armoury.c >@@ -34,7 +34,7 @@ > #include <linux/sysfs.h> > > #include "asus-armoury.h" >-#include "firmware_attributes_class.h" >+#include <linux/firmware_attributes.h> > > #define ASUS_NB_WMI_EVENT_GUID "0B3CBB35-E3C2-45ED-91C2-4C5A6D195D1C" > >diff --git a/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c b/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c >index ab46a023cc34..85670be01647 100644 >--- a/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c >+++ b/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c >@@ -16,7 +16,7 @@ > #include <linux/sysfs.h> > #include <linux/wmi.h> > #include "dell-wmi-sysman.h" >-#include "../../firmware_attributes_class.h" >+#include <linux/firmware_attributes.h> > > #define MAX_TYPES 4 > #include <linux/nls.h> >diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c >index 27fd6cd21529..44314ae4e5e3 100644 >--- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c >+++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c >@@ -14,7 +14,7 @@ > #include <linux/string.h> > #include <linux/wmi.h> > #include "bioscfg.h" >-#include "../../firmware_attributes_class.h" >+#include <linux/firmware_attributes.h> > #include <linux/nls.h> > #include <linux/errno.h> > >diff --git a/drivers/platform/x86/lenovo/think-lmi.c b/drivers/platform/x86/lenovo/think-lmi.c >index e215e86e3db7..a56f152b5f28 100644 >--- a/drivers/platform/x86/lenovo/think-lmi.c >+++ b/drivers/platform/x86/lenovo/think-lmi.c >@@ -20,7 +20,7 @@ > #include <linux/types.h> > #include <linux/dmi.h> > #include <linux/wmi.h> >-#include "../firmware_attributes_class.h" >+#include <linux/firmware_attributes.h> > #include "think-lmi.h" > > static bool debug_support; >diff --git a/drivers/platform/x86/lenovo/wmi-other.c b/drivers/platform/x86/lenovo/wmi-other.c >index fbb32bf404f2..e9a2c4684e5f 100644 >--- a/drivers/platform/x86/lenovo/wmi-other.c >+++ b/drivers/platform/x86/lenovo/wmi-other.c >@@ -50,7 +50,7 @@ > #include "wmi-capdata.h" > #include "wmi-events.h" > #include "wmi-helpers.h" >-#include "../firmware_attributes_class.h" >+#include <linux/firmware_attributes.h> > > #define LENOVO_OTHER_MODE_GUID "DC2A8805-3A8C-41BA-A6F7-092E0089CD3B" > >diff --git a/drivers/platform/x86/samsung-galaxybook.c b/drivers/platform/x86/samsung-galaxybook.c >index 6382af0b106c..74b8c887dd79 100644 >--- a/drivers/platform/x86/samsung-galaxybook.c >+++ b/drivers/platform/x86/samsung-galaxybook.c >@@ -28,7 +28,7 @@ > #include <linux/uuid.h> > #include <linux/workqueue.h> > #include <acpi/battery.h> >-#include "firmware_attributes_class.h" >+#include <linux/firmware_attributes.h> > > #define DRIVER_NAME "samsung-galaxybook" > >diff --git a/drivers/platform/x86/firmware_attributes_class.h b/include/linux/firmware_attributes.h >similarity index 60% >rename from drivers/platform/x86/firmware_attributes_class.h >rename to include/linux/firmware_attributes.h >index d27abe54fcf9..d4026ba1fcc2 100644 >--- a/drivers/platform/x86/firmware_attributes_class.h >+++ b/include/linux/firmware_attributes.h >@@ -2,11 +2,11 @@ > > /* Firmware attributes class helper module */ > >-#ifndef FW_ATTR_CLASS_H >-#define FW_ATTR_CLASS_H >+#ifndef _LINUX_FIRMWARE_ATTRIBUTES_H >+#define _LINUX_FIRMWARE_ATTRIBUTES_H > > #include <linux/device/class.h> > > extern const struct class firmware_attributes_class; > >-#endif /* FW_ATTR_CLASS_H */ >+#endif /* _LINUX_FIRMWARE_ATTRIBUTES_H */ ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v7 2/3] firmware: Rename google firmware directory to coreboot 2026-07-17 8:50 [PATCH v7 0/3] firmware: Add coreboot CFR firmware attributes driver Sean Rhodes 2026-07-17 8:50 ` [PATCH v7 1/3] firmware: Move firmware attributes class helper Sean Rhodes @ 2026-07-17 8:50 ` Sean Rhodes 2026-07-18 0:39 ` Julius Werner 2026-07-17 8:50 ` [PATCH v7 3/3] firmware: coreboot: Add CFR firmware attributes driver Sean Rhodes 2 siblings, 1 reply; 9+ messages in thread From: Sean Rhodes @ 2026-07-17 8:50 UTC (permalink / raw) To: tzungbi Cc: hansg, ilpo.jarvinen, corentin.chary, luke, denis.benato, prasanth.ksr, jorge.lopez2, mpearson-lenovo, derekjohn.clark, josh, briannorris, jwerner, tzimmermann, javierm, kees, u.kleine-koenig, mst, chenhuacai, wenst, florian.fainelli, titouan.ameline, oliver, linux-kernel, platform-driver-x86, Dell.Client.Kernel, chrome-platform, Sean Rhodes Most drivers in drivers/firmware/google are coreboot table drivers rather than Google-specific firmware drivers. Rename the directory to coreboot and update the Kconfig prompt, build path and maintainer entry. Keep the existing CONFIG_GOOGLE_* symbols to avoid user configuration churn in this mechanical move. Signed-off-by: Sean Rhodes <sean@starlabs.systems> --- MAINTAINERS | 4 ++-- drivers/firmware/Kconfig | 2 +- drivers/firmware/Makefile | 2 +- drivers/firmware/{google => coreboot}/Kconfig | 6 +++--- drivers/firmware/{google => coreboot}/Makefile | 0 drivers/firmware/{google => coreboot}/cbmem.c | 0 drivers/firmware/{google => coreboot}/coreboot_table.c | 0 drivers/firmware/{google => coreboot}/coreboot_table.h | 0 .../firmware/{google => coreboot}/framebuffer-coreboot.c | 0 drivers/firmware/{google => coreboot}/gsmi.c | 0 drivers/firmware/{google => coreboot}/memconsole-coreboot.c | 0 .../firmware/{google => coreboot}/memconsole-x86-legacy.c | 0 drivers/firmware/{google => coreboot}/memconsole.c | 0 drivers/firmware/{google => coreboot}/memconsole.h | 6 +++--- drivers/firmware/{google => coreboot}/vpd.c | 0 drivers/firmware/{google => coreboot}/vpd_decode.c | 0 drivers/firmware/{google => coreboot}/vpd_decode.h | 0 17 files changed, 10 insertions(+), 10 deletions(-) rename drivers/firmware/{google => coreboot}/Kconfig (94%) rename drivers/firmware/{google => coreboot}/Makefile (100%) rename drivers/firmware/{google => coreboot}/cbmem.c (100%) rename drivers/firmware/{google => coreboot}/coreboot_table.c (100%) rename drivers/firmware/{google => coreboot}/coreboot_table.h (100%) rename drivers/firmware/{google => coreboot}/framebuffer-coreboot.c (100%) rename drivers/firmware/{google => coreboot}/gsmi.c (100%) rename drivers/firmware/{google => coreboot}/memconsole-coreboot.c (100%) rename drivers/firmware/{google => coreboot}/memconsole-x86-legacy.c (100%) rename drivers/firmware/{google => coreboot}/memconsole.c (100%) rename drivers/firmware/{google => coreboot}/memconsole.h (82%) rename drivers/firmware/{google => coreboot}/vpd.c (100%) rename drivers/firmware/{google => coreboot}/vpd_decode.c (100%) rename drivers/firmware/{google => coreboot}/vpd_decode.h (100%) diff --git a/MAINTAINERS b/MAINTAINERS index 07bdba98b1b1..8cf2766d515d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11068,14 +11068,14 @@ S: Maintained F: Documentation/networking/device_drivers/ethernet/google/gve.rst F: drivers/net/ethernet/google -GOOGLE FIRMWARE DRIVERS +COREBOOT FIRMWARE DRIVERS M: Tzung-Bi Shih <tzungbi@kernel.org> R: Brian Norris <briannorris@chromium.org> R: Julius Werner <jwerner@chromium.org> L: chrome-platform@lists.linux.dev S: Maintained T: git git://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git -F: drivers/firmware/google/ +F: drivers/firmware/coreboot/ F: include/linux/coreboot.h GOOGLE TENSOR SoC SUPPORT diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index 78a239f8bbef..4df0a0ad18b8 100644 --- a/drivers/firmware/Kconfig +++ b/drivers/firmware/Kconfig @@ -293,7 +293,7 @@ endif # TURRIS_MOX_RWTM source "drivers/firmware/arm_ffa/Kconfig" source "drivers/firmware/broadcom/Kconfig" source "drivers/firmware/cirrus/Kconfig" -source "drivers/firmware/google/Kconfig" +source "drivers/firmware/coreboot/Kconfig" source "drivers/firmware/efi/Kconfig" source "drivers/firmware/imx/Kconfig" source "drivers/firmware/meson/Kconfig" diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile index 385b6e8b7fc6..7a8fc3eeb3f0 100644 --- a/drivers/firmware/Makefile +++ b/drivers/firmware/Makefile @@ -30,7 +30,7 @@ obj-y += broadcom/ obj-y += cirrus/ obj-y += meson/ obj-y += microchip/ -obj-$(CONFIG_GOOGLE_FIRMWARE) += google/ +obj-$(CONFIG_GOOGLE_FIRMWARE) += coreboot/ obj-y += efi/ obj-y += imx/ obj-y += psci/ diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/coreboot/Kconfig similarity index 94% rename from drivers/firmware/google/Kconfig rename to drivers/firmware/coreboot/Kconfig index b78c644fa253..b248bafe8b6b 100644 --- a/drivers/firmware/google/Kconfig +++ b/drivers/firmware/coreboot/Kconfig @@ -1,10 +1,10 @@ # SPDX-License-Identifier: GPL-2.0-only menuconfig GOOGLE_FIRMWARE - bool "Google Firmware Drivers" + bool "coreboot firmware drivers" default n help - These firmware drivers are used by Google servers, - Chromebooks and other devices using coreboot firmware. + These firmware drivers are used by devices running coreboot + firmware, including Chromebooks and some Google servers. If in doubt, say "N". if GOOGLE_FIRMWARE diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/coreboot/Makefile similarity index 100% rename from drivers/firmware/google/Makefile rename to drivers/firmware/coreboot/Makefile diff --git a/drivers/firmware/google/cbmem.c b/drivers/firmware/coreboot/cbmem.c similarity index 100% rename from drivers/firmware/google/cbmem.c rename to drivers/firmware/coreboot/cbmem.c diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/coreboot/coreboot_table.c similarity index 100% rename from drivers/firmware/google/coreboot_table.c rename to drivers/firmware/coreboot/coreboot_table.c diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/coreboot/coreboot_table.h similarity index 100% rename from drivers/firmware/google/coreboot_table.h rename to drivers/firmware/coreboot/coreboot_table.h diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/coreboot/framebuffer-coreboot.c similarity index 100% rename from drivers/firmware/google/framebuffer-coreboot.c rename to drivers/firmware/coreboot/framebuffer-coreboot.c diff --git a/drivers/firmware/google/gsmi.c b/drivers/firmware/coreboot/gsmi.c similarity index 100% rename from drivers/firmware/google/gsmi.c rename to drivers/firmware/coreboot/gsmi.c diff --git a/drivers/firmware/google/memconsole-coreboot.c b/drivers/firmware/coreboot/memconsole-coreboot.c similarity index 100% rename from drivers/firmware/google/memconsole-coreboot.c rename to drivers/firmware/coreboot/memconsole-coreboot.c diff --git a/drivers/firmware/google/memconsole-x86-legacy.c b/drivers/firmware/coreboot/memconsole-x86-legacy.c similarity index 100% rename from drivers/firmware/google/memconsole-x86-legacy.c rename to drivers/firmware/coreboot/memconsole-x86-legacy.c diff --git a/drivers/firmware/google/memconsole.c b/drivers/firmware/coreboot/memconsole.c similarity index 100% rename from drivers/firmware/google/memconsole.c rename to drivers/firmware/coreboot/memconsole.c diff --git a/drivers/firmware/google/memconsole.h b/drivers/firmware/coreboot/memconsole.h similarity index 82% rename from drivers/firmware/google/memconsole.h rename to drivers/firmware/coreboot/memconsole.h index aaff2b72b606..58899c2abaaa 100644 --- a/drivers/firmware/google/memconsole.h +++ b/drivers/firmware/coreboot/memconsole.h @@ -7,8 +7,8 @@ * Copyright 2017 Google Inc. */ -#ifndef __FIRMWARE_GOOGLE_MEMCONSOLE_H -#define __FIRMWARE_GOOGLE_MEMCONSOLE_H +#ifndef __FIRMWARE_COREBOOT_MEMCONSOLE_H +#define __FIRMWARE_COREBOOT_MEMCONSOLE_H #include <linux/types.h> @@ -33,4 +33,4 @@ int memconsole_sysfs_init(void); */ void memconsole_exit(void); -#endif /* __FIRMWARE_GOOGLE_MEMCONSOLE_H */ +#endif /* __FIRMWARE_COREBOOT_MEMCONSOLE_H */ diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/coreboot/vpd.c similarity index 100% rename from drivers/firmware/google/vpd.c rename to drivers/firmware/coreboot/vpd.c diff --git a/drivers/firmware/google/vpd_decode.c b/drivers/firmware/coreboot/vpd_decode.c similarity index 100% rename from drivers/firmware/google/vpd_decode.c rename to drivers/firmware/coreboot/vpd_decode.c diff --git a/drivers/firmware/google/vpd_decode.h b/drivers/firmware/coreboot/vpd_decode.h similarity index 100% rename from drivers/firmware/google/vpd_decode.h rename to drivers/firmware/coreboot/vpd_decode.h -- 2.53.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v7 2/3] firmware: Rename google firmware directory to coreboot 2026-07-17 8:50 ` [PATCH v7 2/3] firmware: Rename google firmware directory to coreboot Sean Rhodes @ 2026-07-18 0:39 ` Julius Werner 0 siblings, 0 replies; 9+ messages in thread From: Julius Werner @ 2026-07-18 0:39 UTC (permalink / raw) To: Sean Rhodes Cc: tzungbi, hansg, ilpo.jarvinen, corentin.chary, luke, denis.benato, prasanth.ksr, jorge.lopez2, mpearson-lenovo, derekjohn.clark, josh, briannorris, jwerner, tzimmermann, javierm, kees, u.kleine-koenig, mst, chenhuacai, wenst, florian.fainelli, titouan.ameline, oliver, linux-kernel, platform-driver-x86, Dell.Client.Kernel, chrome-platform > Keep the existing CONFIG_GOOGLE_* symbols to avoid user configuration > churn in this mechanical move. Is this standard kernel development practice? It seems a bit weird to me to leave the config symbols mismatching the directory structure and actual meaning of the setting. How long would we leave them mismatched like this, forever? If you want to prevent user config churn, an alternative option would be to provide the old symbol names as transitional options, like this: menuconfig COREBOOT_FIRMWARE bool "coreboot firmware drivers" default n ... if COREBOOT_FIRMWARE config COREBOOT_CBMEM ... ... endif #COREBOOT_FIRMWARE # Deprecated symbol names, do not use menuconfig GOOGLE_FIRMWARE bool transitional select COREBOOT_FIRMWARE config GOOGLE_CBMEM bool transitional select COREBOOT_CBMEM ... ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v7 3/3] firmware: coreboot: Add CFR firmware attributes driver 2026-07-17 8:50 [PATCH v7 0/3] firmware: Add coreboot CFR firmware attributes driver Sean Rhodes 2026-07-17 8:50 ` [PATCH v7 1/3] firmware: Move firmware attributes class helper Sean Rhodes 2026-07-17 8:50 ` [PATCH v7 2/3] firmware: Rename google firmware directory to coreboot Sean Rhodes @ 2026-07-17 8:50 ` Sean Rhodes 2026-07-20 6:48 ` Tzung-Bi Shih 2026-07-21 9:56 ` Ilpo Järvinen 2 siblings, 2 replies; 9+ messages in thread From: Sean Rhodes @ 2026-07-17 8:50 UTC (permalink / raw) To: tzungbi Cc: hansg, ilpo.jarvinen, corentin.chary, luke, denis.benato, prasanth.ksr, jorge.lopez2, mpearson-lenovo, derekjohn.clark, josh, briannorris, jwerner, tzimmermann, javierm, kees, u.kleine-koenig, mst, chenhuacai, wenst, florian.fainelli, titouan.ameline, oliver, linux-kernel, platform-driver-x86, Dell.Client.Kernel, chrome-platform, Sean Rhodes coreboot can publish a CFR tree in its coreboot table to describe firmware setup options. Add a firmware-attributes driver for that table entry under the coreboot firmware driver directory. The driver exposes runtime EFI variable-backed boolean, enum and integer settings through the firmware-attributes ABI. It only registers settings with readable runtime EFI variables, skips suppressed, volatile, dependency-gated and non-runtime options, and keeps read-only or inactive options read-only in sysfs. For settings with an APM_CNT runtime-apply record, write the EFI variable and ask firmware to apply the setting immediately on x86. If firmware reports a failure, restore the old EFI variable value. Settings without a runtime apply hook, or non-x86 builds, keep the EFI variable update and set pending_reboot. Tested on a Star Labs Byte with firmware 26.07 and the driver built as a module against Linux 6.17: runtime APM_CNT apply was verified with power_led, and reboot-needed behavior was verified with debug_level. Link: https://doc.coreboot.org/drivers/cfr.html Link: https://doc.coreboot.org/drivers/cfr_internal.html Signed-off-by: Sean Rhodes <sean@starlabs.systems> --- MAINTAINERS | 6 + drivers/firmware/coreboot/Kconfig | 14 + drivers/firmware/coreboot/Makefile | 1 + drivers/firmware/coreboot/coreboot-cfr.c | 1141 ++++++++++++++++++++++ 4 files changed, 1162 insertions(+) create mode 100644 drivers/firmware/coreboot/coreboot-cfr.c diff --git a/MAINTAINERS b/MAINTAINERS index 8cf2766d515d..a00f54e0324e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6665,6 +6665,12 @@ F: tools/testing/selftests/cgroup/test_hugetlb_memcg.c F: tools/testing/selftests/cgroup/test_kmem.c F: tools/testing/selftests/cgroup/test_memcontrol.c +COREBOOT CFR FIRMWARE ATTRIBUTES DRIVER +M: Sean Rhodes <sean@starlabs.systems> +L: chrome-platform@lists.linux.dev +S: Maintained +F: drivers/firmware/coreboot/coreboot-cfr.c + CORETEMP HARDWARE MONITORING DRIVER L: linux-hwmon@vger.kernel.org S: Orphan diff --git a/drivers/firmware/coreboot/Kconfig b/drivers/firmware/coreboot/Kconfig index b248bafe8b6b..5c37bdc9c9fd 100644 --- a/drivers/firmware/coreboot/Kconfig +++ b/drivers/firmware/coreboot/Kconfig @@ -44,6 +44,20 @@ config GOOGLE_COREBOOT_TABLE device tree node /firmware/coreboot. If unsure say N. +config COREBOOT_CFR + tristate "coreboot CFR firmware attributes" + depends on EFI + depends on GOOGLE_COREBOOT_TABLE + select CRC32 + select FW_ATTR_CLASS + help + This driver parses the coreboot CFR table and exposes runtime EFI + variable-backed options through the firmware-attributes sysfs + interface. + + If you have a system running coreboot with CFR runtime + configuration support, say Y or M here. + config GOOGLE_MEMCONSOLE tristate depends on GOOGLE_MEMCONSOLE_X86_LEGACY || GOOGLE_MEMCONSOLE_COREBOOT diff --git a/drivers/firmware/coreboot/Makefile b/drivers/firmware/coreboot/Makefile index 8151e323cc43..1b9d0c0e7593 100644 --- a/drivers/firmware/coreboot/Makefile +++ b/drivers/firmware/coreboot/Makefile @@ -2,6 +2,7 @@ obj-$(CONFIG_GOOGLE_SMI) += gsmi.o obj-$(CONFIG_GOOGLE_COREBOOT_TABLE) += coreboot_table.o +obj-$(CONFIG_COREBOOT_CFR) += coreboot-cfr.o obj-$(CONFIG_GOOGLE_FRAMEBUFFER_COREBOOT) += framebuffer-coreboot.o obj-$(CONFIG_GOOGLE_MEMCONSOLE) += memconsole.o obj-$(CONFIG_GOOGLE_MEMCONSOLE_COREBOOT) += memconsole-coreboot.o diff --git a/drivers/firmware/coreboot/coreboot-cfr.c b/drivers/firmware/coreboot/coreboot-cfr.c new file mode 100644 index 000000000000..ba9708a434c6 --- /dev/null +++ b/drivers/firmware/coreboot/coreboot-cfr.c @@ -0,0 +1,1141 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * coreboot CFR firmware attributes driver. + * + * Parses LB_TAG_CFR_ROOT records from the coreboot table and exposes + * runtime EFI variable-backed options through the firmware-attributes class. + */ + +#include <linux/array_size.h> +#include <linux/bitops.h> +#include <linux/cleanup.h> +#include <linux/crc32.h> +#include <linux/ctype.h> +#include <linux/device.h> +#include <linux/device-id/coreboot.h> +#include <linux/efi.h> +#include <linux/err.h> +#include <linux/firmware_attributes.h> +#include <linux/io.h> +#include <linux/kdev_t.h> +#include <linux/kobject.h> +#include <linux/limits.h> +#include <linux/list.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/slab.h> +#include <linux/string.h> +#include <linux/sysfs.h> +#include <linux/types.h> + +#include "coreboot_table.h" + +#define DRIVER_NAME "coreboot-cfr" + +#define LB_TAG_CFR_ROOT 0x47 +#define CFR_VERSION 0 + +enum cfr_tags { + CFR_TAG_OPTION_FORM = 1, + CFR_TAG_ENUM_VALUE = 2, + CFR_TAG_OPTION_ENUM = 3, + CFR_TAG_OPTION_NUMBER = 4, + CFR_TAG_OPTION_BOOL = 5, + CFR_TAG_VARCHAR_OPT_NAME = 7, + CFR_TAG_VARCHAR_UI_NAME = 8, + CFR_TAG_RUNTIME_APPLY = 13, +}; + +enum cfr_option_flags { + CFR_OPTFLAG_READONLY = BIT(0), + CFR_OPTFLAG_INACTIVE = BIT(1), + CFR_OPTFLAG_SUPPRESS = BIT(2), + CFR_OPTFLAG_VOLATILE = BIT(3), + CFR_OPTFLAG_RUNTIME = BIT(4), +}; + +enum cfr_runtime_apply_method { + CFR_RUNTIME_APPLY_NONE = 0, + CFR_RUNTIME_APPLY_APM_CNT = 1, +}; + +struct lb_cfr { + u32 tag; + u32 size; + u32 version; + u32 checksum; +} __packed; + +struct lb_cfr_varbinary { + u32 tag; + u32 size; + u32 data_length; +} __packed; + +struct lb_cfr_enum_value { + u32 tag; + u32 size; + u32 value; +} __packed; + +struct lb_cfr_runtime_apply { + u32 tag; + u32 size; + u32 method; + u32 id; +} __packed; + +struct lb_cfr_numeric_option { + u32 tag; + u32 size; + cb_u64 object_id; + cb_u64 dependency_id; + u32 flags; + u32 default_value; + u32 min; + u32 max; + u32 step; + u32 display_flags; +} __packed; + +struct lb_cfr_option_form { + u32 tag; + u32 size; + cb_u64 object_id; + cb_u64 dependency_id; + u32 flags; +} __packed; + +#define COREBOOT_CFR_OPT_SKIP_FLAGS \ + (CFR_OPTFLAG_SUPPRESS | CFR_OPTFLAG_VOLATILE) + +#define COREBOOT_CFR_OPT_READ_ONLY_FLAGS \ + (CFR_OPTFLAG_READONLY | CFR_OPTFLAG_INACTIVE) + +#define COREBOOT_CFR_EFI_ATTRS \ + (EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | \ + EFI_VARIABLE_RUNTIME_ACCESS) + +#define COREBOOT_CFR_APM_CNT_PORT 0xb2 +#define COREBOOT_CFR_APM_STS_PORT 0xb3 +#define COREBOOT_CFR_APM_APPLY_CMD 0xe3 +#define COREBOOT_CFR_MAX_FORM_DEPTH 16 + +static efi_guid_t coreboot_cfr_guid = EFI_GUID(0xceae4c1d, 0x335b, 0x4685, + 0xa4, 0xa0, 0xfc, 0x4a, + 0x94, 0xee, 0xa0, 0x85); + +enum coreboot_cfr_setting_type { + COREBOOT_CFR_SETTING_ENUM, + COREBOOT_CFR_SETTING_NUMBER, + COREBOOT_CFR_SETTING_BOOL, +}; + +struct coreboot_cfr_enum { + char *label; + u32 value; +}; + +struct coreboot_cfr_setting { + struct kobject kobj; + struct list_head node; + struct coreboot_cfr_drvdata *drvdata; + enum coreboot_cfr_setting_type type; + char *name; + char *display_name; + struct coreboot_cfr_enum *values; + unsigned int n_values; + u32 default_value; + u32 min; + u32 max; + u32 step; + u32 runtime_apply_method; + u32 runtime_apply_id; + bool read_only; +}; + +struct coreboot_cfr_drvdata { + struct device *class_dev; + struct kset *attrs_kset; + struct list_head settings; + /* Serializes EFI variable writes and the matching runtime apply hook. */ + struct mutex lock; + bool efi_writes_supported; + bool pending_reboot; +}; + +static struct coreboot_cfr_setting *to_coreboot_cfr_setting(struct kobject *kobj) +{ + return container_of(kobj, struct coreboot_cfr_setting, kobj); +} + +static bool coreboot_cfr_string_is_valid_name(const char *name) +{ + return name && name[0] && !strchr(name, '/'); +} + +static char *coreboot_cfr_string_dup(const struct lb_cfr_varbinary *str) +{ + const char *data = (const char *)(str + 1); + size_t len = str->data_length; + + if (len && !data[len - 1]) + len--; + + return kmemdup_nul(data, len, GFP_KERNEL); +} + +static const struct coreboot_table_entry * +coreboot_cfr_child_entry(const void *base, size_t len, u32 tag) +{ + const struct coreboot_table_entry *entry; + size_t off; + + for (off = 0; off < len; off += entry->size) { + if (len - off < sizeof(*entry)) + return NULL; + + entry = base + off; + if (entry->size < sizeof(*entry) || entry->size > len - off) + return NULL; + + if (entry->tag == tag) + return entry; + } + + return NULL; +} + +static const struct lb_cfr_varbinary * +coreboot_cfr_child_string(const void *base, size_t len, u32 tag) +{ + const struct lb_cfr_varbinary *str; + const struct coreboot_table_entry *entry; + + entry = coreboot_cfr_child_entry(base, len, tag); + if (!entry) + return NULL; + + if (entry->size < sizeof(*str)) + return NULL; + + str = (const struct lb_cfr_varbinary *)entry; + if (str->data_length > entry->size - sizeof(*str)) + return NULL; + + return str; +} + +static const struct lb_cfr_runtime_apply * +coreboot_cfr_child_runtime_apply(const void *base, size_t len) +{ + const struct lb_cfr_runtime_apply *runtime_apply; + const struct coreboot_table_entry *entry; + + entry = coreboot_cfr_child_entry(base, len, CFR_TAG_RUNTIME_APPLY); + if (!entry) + return NULL; + + if (entry->size < sizeof(*runtime_apply)) + return ERR_PTR(-EINVAL); + + runtime_apply = (const struct lb_cfr_runtime_apply *)entry; + if (runtime_apply->method == CFR_RUNTIME_APPLY_APM_CNT && + runtime_apply->id > U8_MAX) + return ERR_PTR(-EINVAL); + + return runtime_apply; +} + +static efi_char16_t *coreboot_cfr_efi_name(const char *name) +{ + size_t len, i; + + len = strlen(name); + if (len >= EFI_VAR_NAME_LEN) + return ERR_PTR(-ENAMETOOLONG); + + efi_char16_t *efi_name __free(kfree) = + kcalloc(len + 1, sizeof(*efi_name), GFP_KERNEL); + if (!efi_name) + return ERR_PTR(-ENOMEM); + + for (i = 0; i < len; i++) { + if (!isascii(name[i])) + return ERR_PTR(-EINVAL); + efi_name[i] = name[i]; + } + + return no_free_ptr(efi_name); +} + +static int coreboot_cfr_read_efi_value_locked(efi_char16_t *efi_name, + u32 *value, u32 *attrs) +{ + unsigned long size = sizeof(__le32); + efi_status_t status; + __le32 data; + u32 attr; + + status = efivar_get_variable(efi_name, &coreboot_cfr_guid, &attr, + &size, &data); + if (status != EFI_SUCCESS) + return efi_status_to_err(status); + + if (size != sizeof(data)) + return -EINVAL; + + if (!(attr & EFI_VARIABLE_RUNTIME_ACCESS)) + return -EOPNOTSUPP; + + *value = le32_to_cpu(data); + if (attrs) + *attrs = attr; + + return 0; +} + +static int coreboot_cfr_read_value(const struct coreboot_cfr_setting *setting, + u32 *value, u32 *attrs) +{ + efi_char16_t *efi_name; + int ret; + + efi_name = coreboot_cfr_efi_name(setting->name); + if (IS_ERR(efi_name)) + return PTR_ERR(efi_name); + + ret = efivar_lock(); + if (ret) { + kfree(efi_name); + return ret; + } + + ret = coreboot_cfr_read_efi_value_locked(efi_name, value, attrs); + efivar_unlock(); + kfree(efi_name); + + return ret; +} + +static int coreboot_cfr_write_efi_value_locked(efi_char16_t *efi_name, + u32 value, u32 attrs) +{ + efi_status_t status; + __le32 data; + + if (!efivar_supports_writes()) + return -EROFS; + + data = cpu_to_le32(value); + status = efivar_set_variable_locked(efi_name, &coreboot_cfr_guid, attrs, + sizeof(data), &data, false); + if (status != EFI_SUCCESS) + return efi_status_to_err(status); + + return 0; +} + +static int coreboot_cfr_apply_runtime(struct coreboot_cfr_setting *setting) +{ +#ifdef CONFIG_X86 + u8 status; + + if (setting->runtime_apply_method != CFR_RUNTIME_APPLY_APM_CNT) + return -EOPNOTSUPP; + + outb((u8)setting->runtime_apply_id, COREBOOT_CFR_APM_STS_PORT); + outb(COREBOOT_CFR_APM_APPLY_CMD, COREBOOT_CFR_APM_CNT_PORT); + status = inb(COREBOOT_CFR_APM_STS_PORT); + if (status) + return -EIO; + + return 0; +#else + return -EOPNOTSUPP; +#endif +} + +static int coreboot_cfr_write_value(struct coreboot_cfr_setting *setting, + u32 value) +{ + efi_char16_t *efi_name; + u32 attrs; + u32 old; + int restore_ret; + int ret; + bool changed = false; + + if (setting->read_only) + return -EACCES; + + efi_name = coreboot_cfr_efi_name(setting->name); + if (IS_ERR(efi_name)) + return PTR_ERR(efi_name); + + mutex_lock(&setting->drvdata->lock); + ret = efivar_lock(); + if (ret) + goto out_unlock_mutex; + + ret = coreboot_cfr_read_efi_value_locked(efi_name, &old, &attrs); + if (ret) + goto out_unlock_efi; + + if ((attrs & COREBOOT_CFR_EFI_ATTRS) != COREBOOT_CFR_EFI_ATTRS) { + ret = -EOPNOTSUPP; + goto out_unlock_efi; + } + + if (old == value) + goto out_unlock_efi; + + ret = coreboot_cfr_write_efi_value_locked(efi_name, value, attrs); + if (ret) + goto out_unlock_efi; + changed = true; + + ret = coreboot_cfr_apply_runtime(setting); + if (ret == -EOPNOTSUPP) { + /* EFI changed; firmware will consume it after reboot. */ + setting->drvdata->pending_reboot = true; + ret = 0; + } else if (ret) { + restore_ret = coreboot_cfr_write_efi_value_locked(efi_name, old, attrs); + if (restore_ret) { + setting->drvdata->pending_reboot = true; + ret = restore_ret; + } else if (coreboot_cfr_apply_runtime(setting)) { + setting->drvdata->pending_reboot = true; + } else { + changed = false; + } + } + +out_unlock_efi: + efivar_unlock(); +out_unlock_mutex: + mutex_unlock(&setting->drvdata->lock); + kfree(efi_name); + + if (changed) + kobject_uevent(&setting->drvdata->class_dev->kobj, KOBJ_CHANGE); + return ret; +} + +static const char * +coreboot_cfr_label_from_value(const struct coreboot_cfr_setting *setting, + u32 value) +{ + unsigned int i; + + for (i = 0; i < setting->n_values; i++) { + if (setting->values[i].value == value) + return setting->values[i].label; + } + + return NULL; +} + +static int coreboot_cfr_parse_value(struct coreboot_cfr_setting *setting, + const char *label, u32 *value_out) +{ + unsigned int i; + + for (i = 0; i < setting->n_values; i++) { + if (!sysfs_streq(label, setting->values[i].label)) + continue; + + *value_out = setting->values[i].value; + return 0; + } + + return kstrtou32(label, 0, value_out); +} + +static bool coreboot_cfr_value_is_valid(struct coreboot_cfr_setting *setting, + u32 value) +{ + u32 delta; + + if (setting->type != COREBOOT_CFR_SETTING_NUMBER) { + /* Enum and bool values are valid only if they have a label. */ + return coreboot_cfr_label_from_value(setting, value); + } + + if (value < setting->min || value > setting->max) + return false; + + if (!setting->step) + return true; + + delta = value - setting->min; + return delta % setting->step == 0; +} + +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf) +{ + struct coreboot_cfr_setting *setting = to_coreboot_cfr_setting(kobj); + + if (setting->type == COREBOOT_CFR_SETTING_NUMBER) + return sysfs_emit(buf, "integer\n"); + + return sysfs_emit(buf, "enumeration\n"); +} + +static ssize_t display_name_language_code_show(struct kobject *kobj, + struct kobj_attribute *attr, + char *buf) +{ + return sysfs_emit(buf, "en_US.UTF-8\n"); +} + +static ssize_t display_name_show(struct kobject *kobj, + struct kobj_attribute *attr, char *buf) +{ + struct coreboot_cfr_setting *setting = to_coreboot_cfr_setting(kobj); + + return sysfs_emit(buf, "%s\n", setting->display_name); +} + +static ssize_t possible_values_show(struct kobject *kobj, + struct kobj_attribute *attr, char *buf) +{ + struct coreboot_cfr_setting *setting = to_coreboot_cfr_setting(kobj); + ssize_t len = 0; + unsigned int i; + + for (i = 0; i < setting->n_values; i++) { + len += sysfs_emit_at(buf, len, "%s%s", i ? ";" : "", + setting->values[i].label); + } + + len += sysfs_emit_at(buf, len, "\n"); + return len; +} + +static ssize_t min_value_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf) +{ + struct coreboot_cfr_setting *setting = to_coreboot_cfr_setting(kobj); + + return sysfs_emit(buf, "%u\n", setting->min); +} + +static ssize_t max_value_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf) +{ + struct coreboot_cfr_setting *setting = to_coreboot_cfr_setting(kobj); + + return sysfs_emit(buf, "%u\n", setting->max); +} + +static ssize_t scalar_increment_show(struct kobject *kobj, + struct kobj_attribute *attr, char *buf) +{ + struct coreboot_cfr_setting *setting = to_coreboot_cfr_setting(kobj); + + return sysfs_emit(buf, "%u\n", setting->step); +} + +static ssize_t default_value_show(struct kobject *kobj, + struct kobj_attribute *attr, char *buf) +{ + struct coreboot_cfr_setting *setting = to_coreboot_cfr_setting(kobj); + const char *label; + + if (setting->type == COREBOOT_CFR_SETTING_NUMBER) + return sysfs_emit(buf, "%u\n", setting->default_value); + + label = coreboot_cfr_label_from_value(setting, setting->default_value); + if (!label) + return sysfs_emit(buf, "%u\n", setting->default_value); + + return sysfs_emit(buf, "%s\n", label); +} + +static ssize_t current_value_show(struct kobject *kobj, + struct kobj_attribute *attr, char *buf) +{ + struct coreboot_cfr_setting *setting = to_coreboot_cfr_setting(kobj); + const char *label; + u32 value; + int ret; + + ret = coreboot_cfr_read_value(setting, &value, NULL); + if (ret) + return ret; + + if (setting->type == COREBOOT_CFR_SETTING_NUMBER) + return sysfs_emit(buf, "%u\n", value); + + label = coreboot_cfr_label_from_value(setting, value); + if (!label) + return -EINVAL; + + return sysfs_emit(buf, "%s\n", label); +} + +static ssize_t current_value_store(struct kobject *kobj, + struct kobj_attribute *attr, + const char *buf, size_t count) +{ + struct coreboot_cfr_setting *setting = to_coreboot_cfr_setting(kobj); + u32 value; + int ret; + + ret = coreboot_cfr_parse_value(setting, buf, &value); + if (ret) + return ret; + + if (!coreboot_cfr_value_is_valid(setting, value)) + return -EINVAL; + + ret = coreboot_cfr_write_value(setting, value); + if (ret) + return ret; + + return count; +} + +static struct kobj_attribute type_attr = __ATTR_RO(type); +static struct kobj_attribute display_name_language_code_attr = + __ATTR_RO(display_name_language_code); +static struct kobj_attribute display_name_attr = __ATTR_RO(display_name); +static struct kobj_attribute possible_values_attr = __ATTR_RO(possible_values); +static struct kobj_attribute min_value_attr = __ATTR_RO(min_value); +static struct kobj_attribute max_value_attr = __ATTR_RO(max_value); +static struct kobj_attribute scalar_increment_attr = __ATTR_RO(scalar_increment); +static struct kobj_attribute default_value_attr = __ATTR_RO(default_value); +static struct kobj_attribute current_value_attr = __ATTR_RW(current_value); + +static struct attribute *coreboot_cfr_setting_attrs[] = { + &type_attr.attr, + &display_name_language_code_attr.attr, + &display_name_attr.attr, + &possible_values_attr.attr, + &min_value_attr.attr, + &max_value_attr.attr, + &scalar_increment_attr.attr, + &default_value_attr.attr, + ¤t_value_attr.attr, + NULL, +}; + +static umode_t coreboot_cfr_attr_is_visible(struct kobject *kobj, + struct attribute *attr, int n) +{ + struct coreboot_cfr_setting *setting = to_coreboot_cfr_setting(kobj); + + if (setting->type == COREBOOT_CFR_SETTING_NUMBER && + attr == &possible_values_attr.attr) + return 0; + + if (setting->type != COREBOOT_CFR_SETTING_NUMBER && + (attr == &min_value_attr.attr || attr == &max_value_attr.attr || + attr == &scalar_increment_attr.attr)) + return 0; + + if (setting->read_only && attr == ¤t_value_attr.attr) + return 0444; + + return attr->mode; +} + +static const struct attribute_group coreboot_cfr_setting_group = { + .attrs = coreboot_cfr_setting_attrs, + .is_visible = coreboot_cfr_attr_is_visible, +}; + +static void coreboot_cfr_free_setting(struct coreboot_cfr_setting *setting) +{ + unsigned int i; + + for (i = 0; i < setting->n_values; i++) + kfree(setting->values[i].label); + + kfree(setting->values); + kfree(setting->display_name); + kfree(setting->name); + kfree(setting); +} + +static void coreboot_cfr_setting_release(struct kobject *kobj) +{ + coreboot_cfr_free_setting(to_coreboot_cfr_setting(kobj)); +} + +static const struct kobj_type coreboot_cfr_setting_ktype = { + .release = coreboot_cfr_setting_release, + .sysfs_ops = &kobj_sysfs_ops, +}; + +static ssize_t pending_reboot_show(struct kobject *kobj, + struct kobj_attribute *attr, char *buf) +{ + struct coreboot_cfr_drvdata *data; + + data = dev_get_drvdata(kobj_to_dev(kobj->parent)); + + return sysfs_emit(buf, "%d\n", data->pending_reboot); +} + +static struct kobj_attribute pending_reboot_attr = __ATTR_RO(pending_reboot); + +static int coreboot_cfr_copy_bool_values(struct coreboot_cfr_setting *setting) +{ + static const struct coreboot_cfr_enum bool_values[] = { + { .label = "Disabled", .value = 0 }, + { .label = "Enabled", .value = 1 }, + }; + unsigned int i; + + setting->values = kcalloc(ARRAY_SIZE(bool_values), sizeof(*setting->values), + GFP_KERNEL); + if (!setting->values) + return -ENOMEM; + + for (i = 0; i < ARRAY_SIZE(bool_values); i++) { + setting->values[i].label = kstrdup(bool_values[i].label, + GFP_KERNEL); + if (!setting->values[i].label) + return -ENOMEM; + setting->values[i].value = bool_values[i].value; + setting->n_values++; + } + + return 0; +} + +static int coreboot_cfr_count_enum_values(const void *base, size_t len) +{ + const struct coreboot_table_entry *entry; + size_t off; + int count = 0; + + for (off = 0; off < len; off += entry->size) { + if (len - off < sizeof(*entry)) + return -EINVAL; + + entry = base + off; + if (entry->size < sizeof(*entry) || entry->size > len - off) + return -EINVAL; + + if (entry->tag == CFR_TAG_ENUM_VALUE) + count++; + } + + return count; +} + +static int coreboot_cfr_copy_enum_values(struct coreboot_cfr_setting *setting, + const void *base, size_t len) +{ + const struct lb_cfr_enum_value *enum_value; + const struct lb_cfr_varbinary *label; + const struct coreboot_table_entry *entry; + size_t off; + int count; + + count = coreboot_cfr_count_enum_values(base, len); + if (count <= 0) + return count ?: -EINVAL; + + setting->values = kcalloc(count, sizeof(*setting->values), GFP_KERNEL); + if (!setting->values) + return -ENOMEM; + + for (off = 0; off < len; off += entry->size) { + entry = base + off; + if (entry->tag != CFR_TAG_ENUM_VALUE) + continue; + + if (entry->size < sizeof(*enum_value)) + return -EINVAL; + + enum_value = base + off; + label = coreboot_cfr_child_string(enum_value + 1, + enum_value->size - sizeof(*enum_value), + CFR_TAG_VARCHAR_UI_NAME); + if (!label) + return -EINVAL; + + setting->values[setting->n_values].label = + coreboot_cfr_string_dup(label); + if (!setting->values[setting->n_values].label) + return -ENOMEM; + + setting->values[setting->n_values].value = enum_value->value; + setting->n_values++; + } + + return 0; +} + +static bool +coreboot_cfr_possible_values_fit(const struct coreboot_cfr_setting *setting) +{ + size_t len = 1; /* Trailing newline. */ + unsigned int i; + + for (i = 0; i < setting->n_values; i++) { + if (i) + len++; + + if (strlen(setting->values[i].label) >= PAGE_SIZE - len) + return false; + + len += strlen(setting->values[i].label); + } + + return true; +} + +static int coreboot_cfr_setting_is_usable(struct coreboot_cfr_setting *setting) +{ + u32 value; + int ret; + + ret = coreboot_cfr_read_value(setting, &value, NULL); + if (ret) + return ret; + + if (!coreboot_cfr_value_is_valid(setting, value)) + return -EINVAL; + + return 0; +} + +static int coreboot_cfr_register_setting(struct coreboot_cfr_drvdata *data, + struct coreboot_cfr_setting *setting) +{ + int ret; + + ret = kobject_init_and_add(&setting->kobj, &coreboot_cfr_setting_ktype, + &data->attrs_kset->kobj, "%s", setting->name); + if (ret) { + kobject_put(&setting->kobj); + return ret; + } + + ret = sysfs_create_group(&setting->kobj, &coreboot_cfr_setting_group); + if (ret) { + kobject_put(&setting->kobj); + return ret; + } + + list_add_tail(&setting->node, &data->settings); + return 0; +} + +static int coreboot_cfr_add_numeric_option(struct coreboot_cfr_drvdata *data, + const struct lb_cfr_numeric_option *option, + bool parent_read_only) +{ + const struct lb_cfr_varbinary *name; + const struct lb_cfr_varbinary *display_name; + const struct lb_cfr_runtime_apply *runtime_apply; + const void *child_base = option + 1; + struct coreboot_cfr_setting *setting; + size_t child_len = option->size - sizeof(*option); + int ret; + + if (!(option->flags & CFR_OPTFLAG_RUNTIME)) + return 0; + + if (option->flags & COREBOOT_CFR_OPT_SKIP_FLAGS) + return 0; + + if (option->dependency_id) + return 0; + + setting = kzalloc_obj(*setting, GFP_KERNEL); + if (!setting) + return -ENOMEM; + + INIT_LIST_HEAD(&setting->node); + setting->drvdata = data; + setting->default_value = option->default_value; + setting->min = option->min; + setting->max = option->max; + setting->step = option->step ?: 1; + setting->read_only = + (option->flags & COREBOOT_CFR_OPT_READ_ONLY_FLAGS) || + !data->efi_writes_supported || parent_read_only; + + runtime_apply = coreboot_cfr_child_runtime_apply(child_base, child_len); + if (IS_ERR(runtime_apply)) { + ret = PTR_ERR(runtime_apply); + goto err_put_setting; + } + + if (runtime_apply && runtime_apply->method == CFR_RUNTIME_APPLY_APM_CNT) { + setting->runtime_apply_method = runtime_apply->method; + setting->runtime_apply_id = runtime_apply->id; + } + + name = coreboot_cfr_child_string(child_base, child_len, + CFR_TAG_VARCHAR_OPT_NAME); + if (!name) { + ret = -EINVAL; + goto err_put_setting; + } + + setting->name = coreboot_cfr_string_dup(name); + if (!setting->name) { + ret = -ENOMEM; + goto err_put_setting; + } + + if (!coreboot_cfr_string_is_valid_name(setting->name)) { + ret = -EINVAL; + goto err_put_setting; + } + + display_name = coreboot_cfr_child_string(child_base, child_len, + CFR_TAG_VARCHAR_UI_NAME); + if (display_name) + setting->display_name = coreboot_cfr_string_dup(display_name); + else + setting->display_name = kstrdup(setting->name, GFP_KERNEL); + if (!setting->display_name) { + ret = -ENOMEM; + goto err_put_setting; + } + + switch (option->tag) { + case CFR_TAG_OPTION_BOOL: + setting->type = COREBOOT_CFR_SETTING_BOOL; + setting->min = 0; + setting->max = 1; + setting->step = 1; + ret = coreboot_cfr_copy_bool_values(setting); + break; + case CFR_TAG_OPTION_ENUM: + setting->type = COREBOOT_CFR_SETTING_ENUM; + ret = coreboot_cfr_copy_enum_values(setting, child_base, + child_len); + break; + case CFR_TAG_OPTION_NUMBER: + setting->type = COREBOOT_CFR_SETTING_NUMBER; + if (!setting->min && !setting->max) + setting->max = U32_MAX; + else if (setting->max < setting->min) + setting->max = U32_MAX; + ret = 0; + break; + default: + ret = -EINVAL; + break; + } + + if (ret) + goto err_put_setting; + if (setting->type != COREBOOT_CFR_SETTING_NUMBER && + !coreboot_cfr_possible_values_fit(setting)) { + ret = 0; + goto err_put_setting; + } + + ret = coreboot_cfr_setting_is_usable(setting); + if (ret) { + if (ret == -ENOENT || ret == -EINVAL || ret == -EOPNOTSUPP || + ret == -ENAMETOOLONG) + ret = 0; + goto err_put_setting; + } + + ret = coreboot_cfr_register_setting(data, setting); + if (ret) + return ret; + + return 0; + +err_put_setting: + coreboot_cfr_free_setting(setting); + return ret; +} + +static int coreboot_cfr_parse_records(struct coreboot_cfr_drvdata *data, + const void *base, size_t len, + unsigned int depth, + bool parent_read_only) +{ + const struct lb_cfr_option_form *form; + const struct coreboot_table_entry *entry; + const void *child_base; + size_t child_len; + size_t off; + int ret; + + for (off = 0; off < len; off += entry->size) { + if (len - off < sizeof(*entry)) + return -EINVAL; + + entry = base + off; + if (entry->size < sizeof(*entry) || entry->size > len - off) + return -EINVAL; + + switch (entry->tag) { + case CFR_TAG_OPTION_FORM: + if (entry->size < sizeof(struct lb_cfr_option_form)) + return -EINVAL; + + form = base + off; + if (form->dependency_id || + form->flags & CFR_OPTFLAG_SUPPRESS) + break; + + if (depth >= COREBOOT_CFR_MAX_FORM_DEPTH) + return -E2BIG; + + child_base = base + off + sizeof(struct lb_cfr_option_form); + child_len = entry->size - sizeof(struct lb_cfr_option_form); + ret = coreboot_cfr_parse_records(data, child_base, + child_len, depth + 1, + parent_read_only || + (form->flags & + COREBOOT_CFR_OPT_READ_ONLY_FLAGS)); + if (ret) + return ret; + break; + case CFR_TAG_OPTION_ENUM: + case CFR_TAG_OPTION_NUMBER: + case CFR_TAG_OPTION_BOOL: + if (entry->size < sizeof(struct lb_cfr_numeric_option)) + return -EINVAL; + ret = coreboot_cfr_add_numeric_option(data, base + off, + parent_read_only); + if (ret) + return ret; + break; + default: + /* Ignore unsupported, child-only and future record types. */ + break; + } + } + + return 0; +} + +static void coreboot_cfr_unregister_settings(struct coreboot_cfr_drvdata *data) +{ + struct coreboot_cfr_setting *setting, *tmp; + + list_for_each_entry_safe(setting, tmp, &data->settings, node) { + sysfs_remove_group(&setting->kobj, &coreboot_cfr_setting_group); + list_del(&setting->node); + kobject_put(&setting->kobj); + } +} + +static int coreboot_cfr_probe(struct coreboot_device *dev) +{ + const struct lb_cfr *root = (const struct lb_cfr *)dev->raw; + struct coreboot_cfr_drvdata *data; + size_t payload_len; + int ret; + + if (dev->entry.size < sizeof(*root)) + return -EINVAL; + + if (root->tag != LB_TAG_CFR_ROOT || root->version != CFR_VERSION) + return -EINVAL; + + if (root->size < sizeof(*root) || root->size > dev->entry.size) + return -EINVAL; + + payload_len = root->size - sizeof(*root); + if (crc32_be(0, root + 1, payload_len) != root->checksum) + return -EBADMSG; + + if (!efivar_is_available()) + return -EPROBE_DEFER; + + data = devm_kzalloc(&dev->dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + ret = efivar_lock(); + if (ret) + return ret; + data->efi_writes_supported = efivar_supports_writes(); + efivar_unlock(); + + INIT_LIST_HEAD(&data->settings); + ret = devm_mutex_init(&dev->dev, &data->lock); + if (ret) + return ret; + + dev_set_drvdata(&dev->dev, data); + + data->class_dev = device_create(&firmware_attributes_class, NULL, + MKDEV(0, 0), NULL, DRIVER_NAME); + if (IS_ERR(data->class_dev)) + return PTR_ERR(data->class_dev); + dev_set_drvdata(data->class_dev, data); + + data->attrs_kset = kset_create_and_add("attributes", NULL, + &data->class_dev->kobj); + if (!data->attrs_kset) { + ret = -ENOMEM; + goto err_unregister_dev; + } + + ret = sysfs_create_file(&data->attrs_kset->kobj, + &pending_reboot_attr.attr); + if (ret) + goto err_unregister_attrs; + + ret = coreboot_cfr_parse_records(data, root + 1, payload_len, 0, false); + if (ret) + goto err_unregister_settings; + + if (list_empty(&data->settings)) { + ret = -ENODEV; + goto err_unregister_settings; + } + + return 0; + +err_unregister_settings: + coreboot_cfr_unregister_settings(data); + sysfs_remove_file(&data->attrs_kset->kobj, &pending_reboot_attr.attr); +err_unregister_attrs: + kset_unregister(data->attrs_kset); +err_unregister_dev: + device_unregister(data->class_dev); + return ret; +} + +static void coreboot_cfr_remove(struct coreboot_device *dev) +{ + struct coreboot_cfr_drvdata *data = dev_get_drvdata(&dev->dev); + + coreboot_cfr_unregister_settings(data); + sysfs_remove_file(&data->attrs_kset->kobj, &pending_reboot_attr.attr); + kset_unregister(data->attrs_kset); + device_unregister(data->class_dev); +} + +static const struct coreboot_device_id coreboot_cfr_ids[] = { + { .tag = LB_TAG_CFR_ROOT }, + { } +}; +MODULE_DEVICE_TABLE(coreboot, coreboot_cfr_ids); + +static struct coreboot_driver coreboot_cfr_driver = { + .probe = coreboot_cfr_probe, + .remove = coreboot_cfr_remove, + .drv = { + .name = DRIVER_NAME, + }, + .id_table = coreboot_cfr_ids, +}; +module_coreboot_driver(coreboot_cfr_driver); + +MODULE_AUTHOR("Sean Rhodes <sean@starlabs.systems>"); +MODULE_DESCRIPTION("coreboot CFR firmware attributes driver"); +MODULE_LICENSE("GPL"); +MODULE_IMPORT_NS("EFIVAR"); -- 2.53.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v7 3/3] firmware: coreboot: Add CFR firmware attributes driver 2026-07-17 8:50 ` [PATCH v7 3/3] firmware: coreboot: Add CFR firmware attributes driver Sean Rhodes @ 2026-07-20 6:48 ` Tzung-Bi Shih 2026-07-21 9:14 ` Ilpo Järvinen 2026-07-21 9:56 ` Ilpo Järvinen 1 sibling, 1 reply; 9+ messages in thread From: Tzung-Bi Shih @ 2026-07-20 6:48 UTC (permalink / raw) To: Sean Rhodes Cc: hansg, ilpo.jarvinen, corentin.chary, luke, denis.benato, prasanth.ksr, jorge.lopez2, mpearson-lenovo, derekjohn.clark, josh, briannorris, jwerner, tzimmermann, javierm, kees, u.kleine-koenig, mst, chenhuacai, wenst, florian.fainelli, titouan.ameline, oliver, linux-kernel, platform-driver-x86, Dell.Client.Kernel, chrome-platform On Fri, Jul 17, 2026 at 09:50:03AM +0100, Sean Rhodes wrote: > diff --git a/drivers/firmware/coreboot/coreboot-cfr.c b/drivers/firmware/coreboot/coreboot-cfr.c ... > +static int coreboot_cfr_read_efi_value_locked(efi_char16_t *efi_name, > + u32 *value, u32 *attrs) > +{ There is no coreboot_cfr_read_efi_value() and the symbol isn't exported anyway. The "_locked" suffix here is redundant. Instead, it can leave a note in the comment to indicate the lock requirement. > +static int coreboot_cfr_read_value(const struct coreboot_cfr_setting *setting, > + u32 *value, u32 *attrs) > +{ > + efi_char16_t *efi_name; > + int ret; > + > + efi_name = coreboot_cfr_efi_name(setting->name); > + if (IS_ERR(efi_name)) > + return PTR_ERR(efi_name); > + > + ret = efivar_lock(); > + if (ret) { > + kfree(efi_name); > + return ret; To be neat, use a goto statement to clean up. > +static int coreboot_cfr_write_efi_value_locked(efi_char16_t *efi_name, > + u32 value, u32 attrs) Same here. s/_locked//. > +static int coreboot_cfr_write_value(struct coreboot_cfr_setting *setting, > + u32 value) > +{ > + efi_char16_t *efi_name; > + u32 attrs; > + u32 old; > + int restore_ret; > + int ret; > + bool changed = false; The initialization can be eliminated. See comments below. > + > + if (setting->read_only) > + return -EACCES; > + > + efi_name = coreboot_cfr_efi_name(setting->name); > + if (IS_ERR(efi_name)) > + return PTR_ERR(efi_name); > + > + mutex_lock(&setting->drvdata->lock); Since it already includes cleanup.h, how about using a guard()? > + ret = coreboot_cfr_write_efi_value_locked(efi_name, value, attrs); > + if (ret) > + goto out_unlock_efi; > + changed = true; > + > + ret = coreboot_cfr_apply_runtime(setting); > + if (ret == -EOPNOTSUPP) { > + /* EFI changed; firmware will consume it after reboot. */ > + setting->drvdata->pending_reboot = true; > + ret = 0; > + } else if (ret) { > + restore_ret = coreboot_cfr_write_efi_value_locked(efi_name, old, attrs); > + if (restore_ret) { > + setting->drvdata->pending_reboot = true; > + ret = restore_ret; > + } else if (coreboot_cfr_apply_runtime(setting)) { Does it need to apply runtime again for old values? The previous coreboot_cfr_apply_runtime() for new values was just failed (i.e., the new values shouldn't take effect). > + setting->drvdata->pending_reboot = true; > + } else { > + changed = false; > + } > + } > + > +out_unlock_efi: > + efivar_unlock(); > +out_unlock_mutex: > + mutex_unlock(&setting->drvdata->lock); > + kfree(efi_name); > + > + if (changed) > + kobject_uevent(&setting->drvdata->class_dev->kobj, KOBJ_CHANGE); This shouldn't be in the cleanup path. Move it before the label "out_unlock_efi". `changed` only makes sense after calling coreboot_cfr_write_efi_value_locked(). > +static bool > +coreboot_cfr_possible_values_fit(const struct coreboot_cfr_setting *setting) The function needs some comments to explain why and what. > +{ > + size_t len = 1; /* Trailing newline. */ > + unsigned int i; > + > + for (i = 0; i < setting->n_values; i++) { > + if (i) > + len++; What is this for? For ';'? > + > + if (strlen(setting->values[i].label) >= PAGE_SIZE - len) > + return false; > + > + len += strlen(setting->values[i].label); A straightforward way: len += strlen(...); if (len >= PAGE_SIZE) ... > +static int coreboot_cfr_add_numeric_option(struct coreboot_cfr_drvdata *data, > + const struct lb_cfr_numeric_option *option, > + bool parent_read_only) > +{ ... > + if (setting->type != COREBOOT_CFR_SETTING_NUMBER && > + !coreboot_cfr_possible_values_fit(setting)) { > + ret = 0; > + goto err_put_setting; > + } Why it skips if the possible values can be truncated? Is it seen as a critical setting? This needs some comments to explain why and what. > + > + ret = coreboot_cfr_setting_is_usable(setting); > + if (ret) { > + if (ret == -ENOENT || ret == -EINVAL || ret == -EOPNOTSUPP || > + ret == -ENAMETOOLONG) > + ret = 0; Does it check for values from coreboot table have corresponding EFI vars? Why these errors aren't considered as failures? This needs some comments to explain why and what. > + goto err_put_setting; > + } > + > + ret = coreboot_cfr_register_setting(data, setting); > + if (ret) > + return ret; > + > + return 0; A simple way: return coreboot_cfr_register_setting(...); Also it may be worth some comments to mention that after calling coreboot_cfr_register_setting(), `coreboot_cfr_setting_ktype` takes care of the resource release. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v7 3/3] firmware: coreboot: Add CFR firmware attributes driver 2026-07-20 6:48 ` Tzung-Bi Shih @ 2026-07-21 9:14 ` Ilpo Järvinen 0 siblings, 0 replies; 9+ messages in thread From: Ilpo Järvinen @ 2026-07-21 9:14 UTC (permalink / raw) To: Tzung-Bi Shih, Sean Rhodes Cc: Hans de Goede, corentin.chary, luke, denis.benato, prasanth.ksr, jorge.lopez2, Mark Pearson, derekjohn.clark, josh, briannorris, jwerner, tzimmermann, javierm, kees, u.kleine-koenig, mst, chenhuacai, wenst, florian.fainelli, titouan.ameline, oliver, LKML, platform-driver-x86, Dell.Client.Kernel, chrome-platform On Mon, 20 Jul 2026, Tzung-Bi Shih wrote: > On Fri, Jul 17, 2026 at 09:50:03AM +0100, Sean Rhodes wrote: > > diff --git a/drivers/firmware/coreboot/coreboot-cfr.c b/drivers/firmware/coreboot/coreboot-cfr.c > ... > > +static int coreboot_cfr_read_efi_value_locked(efi_char16_t *efi_name, > > + u32 *value, u32 *attrs) > > +{ > > There is no coreboot_cfr_read_efi_value() and the symbol isn't exported > anyway. The "_locked" suffix here is redundant. Instead, it can leave a > note in the comment to indicate the lock requirement. > > > +static int coreboot_cfr_read_value(const struct coreboot_cfr_setting *setting, > > + u32 *value, u32 *attrs) > > +{ > > + efi_char16_t *efi_name; > > + int ret; > > + > > + efi_name = coreboot_cfr_efi_name(setting->name); > > + if (IS_ERR(efi_name)) > > + return PTR_ERR(efi_name); > > + > > + ret = efivar_lock(); > > + if (ret) { > > + kfree(efi_name); > > + return ret; > > To be neat, use a goto statement to clean up. It should use __free(), not goto. -- i. > > +static int coreboot_cfr_write_efi_value_locked(efi_char16_t *efi_name, > > + u32 value, u32 attrs) > > Same here. s/_locked//. > > > +static int coreboot_cfr_write_value(struct coreboot_cfr_setting *setting, > > + u32 value) > > +{ > > + efi_char16_t *efi_name; > > + u32 attrs; > > + u32 old; > > + int restore_ret; > > + int ret; > > + bool changed = false; > > The initialization can be eliminated. See comments below. > > > + > > + if (setting->read_only) > > + return -EACCES; > > + > > + efi_name = coreboot_cfr_efi_name(setting->name); > > + if (IS_ERR(efi_name)) > > + return PTR_ERR(efi_name); > > + > > + mutex_lock(&setting->drvdata->lock); > > Since it already includes cleanup.h, how about using a guard()? > > > + ret = coreboot_cfr_write_efi_value_locked(efi_name, value, attrs); > > + if (ret) > > + goto out_unlock_efi; > > + changed = true; > > + > > + ret = coreboot_cfr_apply_runtime(setting); > > + if (ret == -EOPNOTSUPP) { > > + /* EFI changed; firmware will consume it after reboot. */ > > + setting->drvdata->pending_reboot = true; > > + ret = 0; > > + } else if (ret) { > > + restore_ret = coreboot_cfr_write_efi_value_locked(efi_name, old, attrs); > > + if (restore_ret) { > > + setting->drvdata->pending_reboot = true; > > + ret = restore_ret; > > + } else if (coreboot_cfr_apply_runtime(setting)) { > > Does it need to apply runtime again for old values? The previous > coreboot_cfr_apply_runtime() for new values was just failed (i.e., the new > values shouldn't take effect). > > > + setting->drvdata->pending_reboot = true; > > + } else { > > + changed = false; > > + } > > + } > > + > > +out_unlock_efi: > > + efivar_unlock(); > > +out_unlock_mutex: > > + mutex_unlock(&setting->drvdata->lock); > > + kfree(efi_name); > > + > > + if (changed) > > + kobject_uevent(&setting->drvdata->class_dev->kobj, KOBJ_CHANGE); > > This shouldn't be in the cleanup path. Move it before the label > "out_unlock_efi". `changed` only makes sense after calling > coreboot_cfr_write_efi_value_locked(). > > > +static bool > > +coreboot_cfr_possible_values_fit(const struct coreboot_cfr_setting *setting) > > The function needs some comments to explain why and what. > > > +{ > > + size_t len = 1; /* Trailing newline. */ > > + unsigned int i; > > + > > + for (i = 0; i < setting->n_values; i++) { > > + if (i) > > + len++; > > What is this for? For ';'? > > > + > > + if (strlen(setting->values[i].label) >= PAGE_SIZE - len) > > + return false; > > + > > + len += strlen(setting->values[i].label); > > A straightforward way: > > len += strlen(...); > if (len >= PAGE_SIZE) > ... > > > +static int coreboot_cfr_add_numeric_option(struct coreboot_cfr_drvdata *data, > > + const struct lb_cfr_numeric_option *option, > > + bool parent_read_only) > > +{ > ... > > + if (setting->type != COREBOOT_CFR_SETTING_NUMBER && > > + !coreboot_cfr_possible_values_fit(setting)) { > > + ret = 0; > > + goto err_put_setting; > > + } > > Why it skips if the possible values can be truncated? Is it seen as a > critical setting? > > This needs some comments to explain why and what. > > > + > > + ret = coreboot_cfr_setting_is_usable(setting); > > + if (ret) { > > + if (ret == -ENOENT || ret == -EINVAL || ret == -EOPNOTSUPP || > > + ret == -ENAMETOOLONG) > > + ret = 0; > > Does it check for values from coreboot table have corresponding EFI vars? > Why these errors aren't considered as failures? > > This needs some comments to explain why and what. > > > + goto err_put_setting; > > + } > > + > > + ret = coreboot_cfr_register_setting(data, setting); > > + if (ret) > > + return ret; > > + > > + return 0; > > A simple way: > > return coreboot_cfr_register_setting(...); > > Also it may be worth some comments to mention that after calling > coreboot_cfr_register_setting(), `coreboot_cfr_setting_ktype` takes care of > the resource release. > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v7 3/3] firmware: coreboot: Add CFR firmware attributes driver 2026-07-17 8:50 ` [PATCH v7 3/3] firmware: coreboot: Add CFR firmware attributes driver Sean Rhodes 2026-07-20 6:48 ` Tzung-Bi Shih @ 2026-07-21 9:56 ` Ilpo Järvinen 1 sibling, 0 replies; 9+ messages in thread From: Ilpo Järvinen @ 2026-07-21 9:56 UTC (permalink / raw) To: Sean Rhodes Cc: tzungbi, Hans de Goede, corentin.chary, luke, denis.benato, prasanth.ksr, jorge.lopez2, Mark Pearson, derekjohn.clark, josh, briannorris, jwerner, tzimmermann, javierm, kees, u.kleine-koenig, mst, chenhuacai, wenst, florian.fainelli, titouan.ameline, oliver, LKML, platform-driver-x86, Dell.Client.Kernel, chrome-platform On Fri, 17 Jul 2026, Sean Rhodes wrote: > coreboot can publish a CFR tree in its coreboot table to describe > firmware setup options. Add a firmware-attributes driver for that table > entry under the coreboot firmware driver directory. > > The driver exposes runtime EFI variable-backed boolean, enum and integer > settings through the firmware-attributes ABI. It only registers settings > with readable runtime EFI variables, skips suppressed, volatile, > dependency-gated and non-runtime options, and keeps read-only or inactive > options read-only in sysfs. > > For settings with an APM_CNT runtime-apply record, write the EFI variable > and ask firmware to apply the setting immediately on x86. If firmware > reports a failure, restore the old EFI variable value. Settings without a > runtime apply hook, or non-x86 builds, keep the EFI variable update and > set pending_reboot. > > Tested on a Star Labs Byte with firmware 26.07 and the driver built as a > module against Linux 6.17: runtime APM_CNT apply was verified with > power_led, and reboot-needed behavior was verified with debug_level. > > Link: https://doc.coreboot.org/drivers/cfr.html > Link: https://doc.coreboot.org/drivers/cfr_internal.html > Signed-off-by: Sean Rhodes <sean@starlabs.systems> > --- > MAINTAINERS | 6 + > drivers/firmware/coreboot/Kconfig | 14 + > drivers/firmware/coreboot/Makefile | 1 + > drivers/firmware/coreboot/coreboot-cfr.c | 1141 ++++++++++++++++++++++ > 4 files changed, 1162 insertions(+) > create mode 100644 drivers/firmware/coreboot/coreboot-cfr.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 8cf2766d515d..a00f54e0324e 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -6665,6 +6665,12 @@ F: tools/testing/selftests/cgroup/test_hugetlb_memcg.c > F: tools/testing/selftests/cgroup/test_kmem.c > F: tools/testing/selftests/cgroup/test_memcontrol.c > > +COREBOOT CFR FIRMWARE ATTRIBUTES DRIVER > +M: Sean Rhodes <sean@starlabs.systems> > +L: chrome-platform@lists.linux.dev > +S: Maintained > +F: drivers/firmware/coreboot/coreboot-cfr.c > + > CORETEMP HARDWARE MONITORING DRIVER > L: linux-hwmon@vger.kernel.org > S: Orphan > diff --git a/drivers/firmware/coreboot/Kconfig b/drivers/firmware/coreboot/Kconfig > index b248bafe8b6b..5c37bdc9c9fd 100644 > --- a/drivers/firmware/coreboot/Kconfig > +++ b/drivers/firmware/coreboot/Kconfig > @@ -44,6 +44,20 @@ config GOOGLE_COREBOOT_TABLE > device tree node /firmware/coreboot. > If unsure say N. > > +config COREBOOT_CFR > + tristate "coreboot CFR firmware attributes" > + depends on EFI > + depends on GOOGLE_COREBOOT_TABLE > + select CRC32 > + select FW_ATTR_CLASS > + help > + This driver parses the coreboot CFR table and exposes runtime EFI > + variable-backed options through the firmware-attributes sysfs > + interface. > + > + If you have a system running coreboot with CFR runtime > + configuration support, say Y or M here. > + > config GOOGLE_MEMCONSOLE > tristate > depends on GOOGLE_MEMCONSOLE_X86_LEGACY || GOOGLE_MEMCONSOLE_COREBOOT > diff --git a/drivers/firmware/coreboot/Makefile b/drivers/firmware/coreboot/Makefile > index 8151e323cc43..1b9d0c0e7593 100644 > --- a/drivers/firmware/coreboot/Makefile > +++ b/drivers/firmware/coreboot/Makefile > @@ -2,6 +2,7 @@ > > obj-$(CONFIG_GOOGLE_SMI) += gsmi.o > obj-$(CONFIG_GOOGLE_COREBOOT_TABLE) += coreboot_table.o > +obj-$(CONFIG_COREBOOT_CFR) += coreboot-cfr.o > obj-$(CONFIG_GOOGLE_FRAMEBUFFER_COREBOOT) += framebuffer-coreboot.o > obj-$(CONFIG_GOOGLE_MEMCONSOLE) += memconsole.o > obj-$(CONFIG_GOOGLE_MEMCONSOLE_COREBOOT) += memconsole-coreboot.o > diff --git a/drivers/firmware/coreboot/coreboot-cfr.c b/drivers/firmware/coreboot/coreboot-cfr.c > new file mode 100644 > index 000000000000..ba9708a434c6 > --- /dev/null > +++ b/drivers/firmware/coreboot/coreboot-cfr.c > @@ -0,0 +1,1141 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * coreboot CFR firmware attributes driver. > + * > + * Parses LB_TAG_CFR_ROOT records from the coreboot table and exposes > + * runtime EFI variable-backed options through the firmware-attributes class. > + */ > + > +#include <linux/array_size.h> > +#include <linux/bitops.h> > +#include <linux/cleanup.h> > +#include <linux/crc32.h> > +#include <linux/ctype.h> > +#include <linux/device.h> > +#include <linux/device-id/coreboot.h> > +#include <linux/efi.h> > +#include <linux/err.h> > +#include <linux/firmware_attributes.h> > +#include <linux/io.h> > +#include <linux/kdev_t.h> > +#include <linux/kobject.h> > +#include <linux/limits.h> > +#include <linux/list.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/slab.h> > +#include <linux/string.h> > +#include <linux/sysfs.h> > +#include <linux/types.h> > + > +#include "coreboot_table.h" > + > +#define DRIVER_NAME "coreboot-cfr" > + > +#define LB_TAG_CFR_ROOT 0x47 > +#define CFR_VERSION 0 > + > +enum cfr_tags { > + CFR_TAG_OPTION_FORM = 1, > + CFR_TAG_ENUM_VALUE = 2, > + CFR_TAG_OPTION_ENUM = 3, > + CFR_TAG_OPTION_NUMBER = 4, > + CFR_TAG_OPTION_BOOL = 5, > + CFR_TAG_VARCHAR_OPT_NAME = 7, > + CFR_TAG_VARCHAR_UI_NAME = 8, > + CFR_TAG_RUNTIME_APPLY = 13, > +}; > + > +enum cfr_option_flags { > + CFR_OPTFLAG_READONLY = BIT(0), > + CFR_OPTFLAG_INACTIVE = BIT(1), > + CFR_OPTFLAG_SUPPRESS = BIT(2), > + CFR_OPTFLAG_VOLATILE = BIT(3), > + CFR_OPTFLAG_RUNTIME = BIT(4), > +}; > + > +enum cfr_runtime_apply_method { > + CFR_RUNTIME_APPLY_NONE = 0, > + CFR_RUNTIME_APPLY_APM_CNT = 1, > +}; > + > +struct lb_cfr { > + u32 tag; > + u32 size; > + u32 version; > + u32 checksum; > +} __packed; Add include for __packed. > +struct lb_cfr_varbinary { > + u32 tag; > + u32 size; > + u32 data_length; > +} __packed; > + > +struct lb_cfr_enum_value { > + u32 tag; > + u32 size; > + u32 value; > +} __packed; > + > +struct lb_cfr_runtime_apply { > + u32 tag; > + u32 size; > + u32 method; > + u32 id; > +} __packed; > + > +struct lb_cfr_numeric_option { > + u32 tag; > + u32 size; > + cb_u64 object_id; > + cb_u64 dependency_id; > + u32 flags; > + u32 default_value; > + u32 min; > + u32 max; > + u32 step; > + u32 display_flags; > +} __packed; > + > +struct lb_cfr_option_form { > + u32 tag; > + u32 size; > + cb_u64 object_id; > + cb_u64 dependency_id; > + u32 flags; > +} __packed; > + > +#define COREBOOT_CFR_OPT_SKIP_FLAGS \ > + (CFR_OPTFLAG_SUPPRESS | CFR_OPTFLAG_VOLATILE) > + > +#define COREBOOT_CFR_OPT_READ_ONLY_FLAGS \ > + (CFR_OPTFLAG_READONLY | CFR_OPTFLAG_INACTIVE) > + > +#define COREBOOT_CFR_EFI_ATTRS \ > + (EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_BOOTSERVICE_ACCESS | \ > + EFI_VARIABLE_RUNTIME_ACCESS) > + > +#define COREBOOT_CFR_APM_CNT_PORT 0xb2 > +#define COREBOOT_CFR_APM_STS_PORT 0xb3 > +#define COREBOOT_CFR_APM_APPLY_CMD 0xe3 > +#define COREBOOT_CFR_MAX_FORM_DEPTH 16 > + > +static efi_guid_t coreboot_cfr_guid = EFI_GUID(0xceae4c1d, 0x335b, 0x4685, > + 0xa4, 0xa0, 0xfc, 0x4a, > + 0x94, 0xee, 0xa0, 0x85); > + > +enum coreboot_cfr_setting_type { > + COREBOOT_CFR_SETTING_ENUM, > + COREBOOT_CFR_SETTING_NUMBER, > + COREBOOT_CFR_SETTING_BOOL, > +}; > + > +struct coreboot_cfr_enum { > + char *label; > + u32 value; > +}; > + > +struct coreboot_cfr_setting { > + struct kobject kobj; > + struct list_head node; > + struct coreboot_cfr_drvdata *drvdata; > + enum coreboot_cfr_setting_type type; > + char *name; > + char *display_name; > + struct coreboot_cfr_enum *values; > + unsigned int n_values; > + u32 default_value; > + u32 min; > + u32 max; > + u32 step; > + u32 runtime_apply_method; > + u32 runtime_apply_id; > + bool read_only; > +}; > + > +struct coreboot_cfr_drvdata { > + struct device *class_dev; > + struct kset *attrs_kset; > + struct list_head settings; > + /* Serializes EFI variable writes and the matching runtime apply hook. */ > + struct mutex lock; > + bool efi_writes_supported; > + bool pending_reboot; > +}; > + > +static struct coreboot_cfr_setting *to_coreboot_cfr_setting(struct kobject *kobj) > +{ > + return container_of(kobj, struct coreboot_cfr_setting, kobj); Add include for container_of(). > +} > + > +static bool coreboot_cfr_string_is_valid_name(const char *name) > +{ > + return name && name[0] && !strchr(name, '/'); > +} > + > +static char *coreboot_cfr_string_dup(const struct lb_cfr_varbinary *str) > +{ > + const char *data = (const char *)(str + 1); > + size_t len = str->data_length; > + > + if (len && !data[len - 1]) > + len--; > + > + return kmemdup_nul(data, len, GFP_KERNEL); > +} > + > +static const struct coreboot_table_entry * > +coreboot_cfr_child_entry(const void *base, size_t len, u32 tag) > +{ > + const struct coreboot_table_entry *entry; > + size_t off; > + > + for (off = 0; off < len; off += entry->size) { > + if (len - off < sizeof(*entry)) > + return NULL; > + > + entry = base + off; > + if (entry->size < sizeof(*entry) || entry->size > len - off) > + return NULL; > + > + if (entry->tag == tag) > + return entry; > + } > + > + return NULL; > +} > + > +static const struct lb_cfr_varbinary * > +coreboot_cfr_child_string(const void *base, size_t len, u32 tag) > +{ > + const struct lb_cfr_varbinary *str; > + const struct coreboot_table_entry *entry; > + > + entry = coreboot_cfr_child_entry(base, len, tag); > + if (!entry) > + return NULL; > + > + if (entry->size < sizeof(*str)) > + return NULL; > + > + str = (const struct lb_cfr_varbinary *)entry; > + if (str->data_length > entry->size - sizeof(*str)) > + return NULL; > + > + return str; > +} > + > +static const struct lb_cfr_runtime_apply * > +coreboot_cfr_child_runtime_apply(const void *base, size_t len) > +{ > + const struct lb_cfr_runtime_apply *runtime_apply; > + const struct coreboot_table_entry *entry; > + > + entry = coreboot_cfr_child_entry(base, len, CFR_TAG_RUNTIME_APPLY); > + if (!entry) > + return NULL; > + > + if (entry->size < sizeof(*runtime_apply)) > + return ERR_PTR(-EINVAL); > + > + runtime_apply = (const struct lb_cfr_runtime_apply *)entry; > + if (runtime_apply->method == CFR_RUNTIME_APPLY_APM_CNT && > + runtime_apply->id > U8_MAX) > + return ERR_PTR(-EINVAL); > + > + return runtime_apply; > +} > + > +static efi_char16_t *coreboot_cfr_efi_name(const char *name) > +{ > + size_t len, i; > + > + len = strlen(name); > + if (len >= EFI_VAR_NAME_LEN) > + return ERR_PTR(-ENAMETOOLONG); > + > + efi_char16_t *efi_name __free(kfree) = > + kcalloc(len + 1, sizeof(*efi_name), GFP_KERNEL); > + if (!efi_name) > + return ERR_PTR(-ENOMEM); > + > + for (i = 0; i < len; i++) { > + if (!isascii(name[i])) > + return ERR_PTR(-EINVAL); > + efi_name[i] = name[i]; > + } > + > + return no_free_ptr(efi_name); > +} > + > +static int coreboot_cfr_read_efi_value_locked(efi_char16_t *efi_name, > + u32 *value, u32 *attrs) > +{ > + unsigned long size = sizeof(__le32); > + efi_status_t status; > + __le32 data; > + u32 attr; > + > + status = efivar_get_variable(efi_name, &coreboot_cfr_guid, &attr, > + &size, &data); > + if (status != EFI_SUCCESS) > + return efi_status_to_err(status); > + > + if (size != sizeof(data)) > + return -EINVAL; > + > + if (!(attr & EFI_VARIABLE_RUNTIME_ACCESS)) > + return -EOPNOTSUPP; > + > + *value = le32_to_cpu(data); > + if (attrs) > + *attrs = attr; > + > + return 0; > +} > + > +static int coreboot_cfr_read_value(const struct coreboot_cfr_setting *setting, > + u32 *value, u32 *attrs) > +{ > + efi_char16_t *efi_name; > + int ret; > + > + efi_name = coreboot_cfr_efi_name(setting->name); > + if (IS_ERR(efi_name)) > + return PTR_ERR(efi_name); > + > + ret = efivar_lock(); > + if (ret) { > + kfree(efi_name); > + return ret; > + } > + > + ret = coreboot_cfr_read_efi_value_locked(efi_name, value, attrs); > + efivar_unlock(); > + kfree(efi_name); Please use __free() for efi_name. > + > + return ret; > +} > + > +static int coreboot_cfr_write_efi_value_locked(efi_char16_t *efi_name, > + u32 value, u32 attrs) > +{ > + efi_status_t status; > + __le32 data; > + > + if (!efivar_supports_writes()) > + return -EROFS; > + > + data = cpu_to_le32(value); > + status = efivar_set_variable_locked(efi_name, &coreboot_cfr_guid, attrs, > + sizeof(data), &data, false); > + if (status != EFI_SUCCESS) > + return efi_status_to_err(status); > + > + return 0; > +} > + > +static int coreboot_cfr_apply_runtime(struct coreboot_cfr_setting *setting) > +{ > +#ifdef CONFIG_X86 > + u8 status; > + > + if (setting->runtime_apply_method != CFR_RUNTIME_APPLY_APM_CNT) > + return -EOPNOTSUPP; > + > + outb((u8)setting->runtime_apply_id, COREBOOT_CFR_APM_STS_PORT); > + outb(COREBOOT_CFR_APM_APPLY_CMD, COREBOOT_CFR_APM_CNT_PORT); > + status = inb(COREBOOT_CFR_APM_STS_PORT); > + if (status) > + return -EIO; > + > + return 0; > +#else > + return -EOPNOTSUPP; > +#endif > +} > + > +static int coreboot_cfr_write_value(struct coreboot_cfr_setting *setting, > + u32 value) > +{ > + efi_char16_t *efi_name; > + u32 attrs; > + u32 old; > + int restore_ret; > + int ret; > + bool changed = false; > + > + if (setting->read_only) > + return -EACCES; > + > + efi_name = coreboot_cfr_efi_name(setting->name); > + if (IS_ERR(efi_name)) > + return PTR_ERR(efi_name); > + > + mutex_lock(&setting->drvdata->lock); > + ret = efivar_lock(); > + if (ret) > + goto out_unlock_mutex; After guard() conversion (see below), this can return directly. I suppose it would be possible to add DEFINE_LOCK_GUARD_0() for efivar_lock in a different patch as well so also it could use guard(). It would allow further simplifying the logic here. > + ret = coreboot_cfr_read_efi_value_locked(efi_name, &old, &attrs); > + if (ret) > + goto out_unlock_efi; > + > + if ((attrs & COREBOOT_CFR_EFI_ATTRS) != COREBOOT_CFR_EFI_ATTRS) { > + ret = -EOPNOTSUPP; > + goto out_unlock_efi; > + } > + > + if (old == value) > + goto out_unlock_efi; > + > + ret = coreboot_cfr_write_efi_value_locked(efi_name, value, attrs); > + if (ret) > + goto out_unlock_efi; > + changed = true; > + > + ret = coreboot_cfr_apply_runtime(setting); > + if (ret == -EOPNOTSUPP) { > + /* EFI changed; firmware will consume it after reboot. */ > + setting->drvdata->pending_reboot = true; > + ret = 0; > + } else if (ret) { > + restore_ret = coreboot_cfr_write_efi_value_locked(efi_name, old, attrs); > + if (restore_ret) { > + setting->drvdata->pending_reboot = true; > + ret = restore_ret; > + } else if (coreboot_cfr_apply_runtime(setting)) { > + setting->drvdata->pending_reboot = true; > + } else { > + changed = false; > + } > + } > + > +out_unlock_efi: > + efivar_unlock(); > +out_unlock_mutex: > + mutex_unlock(&setting->drvdata->lock); > + kfree(efi_name); Both the mutex and efi_name can use __free(). > + > + if (changed) > + kobject_uevent(&setting->drvdata->class_dev->kobj, KOBJ_CHANGE); > + return ret; > +} > + > +static const char * > +coreboot_cfr_label_from_value(const struct coreboot_cfr_setting *setting, > + u32 value) > +{ > + unsigned int i; > + > + for (i = 0; i < setting->n_values; i++) { > + if (setting->values[i].value == value) > + return setting->values[i].label; > + } > + > + return NULL; > +} > + > +static int coreboot_cfr_parse_value(struct coreboot_cfr_setting *setting, > + const char *label, u32 *value_out) > +{ > + unsigned int i; > + > + for (i = 0; i < setting->n_values; i++) { > + if (!sysfs_streq(label, setting->values[i].label)) > + continue; > + > + *value_out = setting->values[i].value; > + return 0; > + } > + > + return kstrtou32(label, 0, value_out); > +} > + > +static bool coreboot_cfr_value_is_valid(struct coreboot_cfr_setting *setting, > + u32 value) > +{ > + u32 delta; > + > + if (setting->type != COREBOOT_CFR_SETTING_NUMBER) { > + /* Enum and bool values are valid only if they have a label. */ > + return coreboot_cfr_label_from_value(setting, value); > + } > + > + if (value < setting->min || value > setting->max) > + return false; > + > + if (!setting->step) > + return true; > + > + delta = value - setting->min; > + return delta % setting->step == 0; > +} > + > +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf) > +{ > + struct coreboot_cfr_setting *setting = to_coreboot_cfr_setting(kobj); > + > + if (setting->type == COREBOOT_CFR_SETTING_NUMBER) > + return sysfs_emit(buf, "integer\n"); > + > + return sysfs_emit(buf, "enumeration\n"); > +} > + > +static ssize_t display_name_language_code_show(struct kobject *kobj, > + struct kobj_attribute *attr, > + char *buf) > +{ > + return sysfs_emit(buf, "en_US.UTF-8\n"); > +} > + > +static ssize_t display_name_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) > +{ > + struct coreboot_cfr_setting *setting = to_coreboot_cfr_setting(kobj); > + > + return sysfs_emit(buf, "%s\n", setting->display_name); > +} > + > +static ssize_t possible_values_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) > +{ > + struct coreboot_cfr_setting *setting = to_coreboot_cfr_setting(kobj); > + ssize_t len = 0; > + unsigned int i; > + > + for (i = 0; i < setting->n_values; i++) { > + len += sysfs_emit_at(buf, len, "%s%s", i ? ";" : "", > + setting->values[i].label); > + } > + > + len += sysfs_emit_at(buf, len, "\n"); > + return len; > +} > + > +static ssize_t min_value_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf) > +{ > + struct coreboot_cfr_setting *setting = to_coreboot_cfr_setting(kobj); > + > + return sysfs_emit(buf, "%u\n", setting->min); > +} > + > +static ssize_t max_value_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf) > +{ > + struct coreboot_cfr_setting *setting = to_coreboot_cfr_setting(kobj); > + > + return sysfs_emit(buf, "%u\n", setting->max); > +} > + > +static ssize_t scalar_increment_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) > +{ > + struct coreboot_cfr_setting *setting = to_coreboot_cfr_setting(kobj); > + > + return sysfs_emit(buf, "%u\n", setting->step); > +} > + > +static ssize_t default_value_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) > +{ > + struct coreboot_cfr_setting *setting = to_coreboot_cfr_setting(kobj); > + const char *label; > + > + if (setting->type == COREBOOT_CFR_SETTING_NUMBER) > + return sysfs_emit(buf, "%u\n", setting->default_value); > + > + label = coreboot_cfr_label_from_value(setting, setting->default_value); > + if (!label) > + return sysfs_emit(buf, "%u\n", setting->default_value); > + > + return sysfs_emit(buf, "%s\n", label); > +} > + > +static ssize_t current_value_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) > +{ > + struct coreboot_cfr_setting *setting = to_coreboot_cfr_setting(kobj); > + const char *label; > + u32 value; > + int ret; > + > + ret = coreboot_cfr_read_value(setting, &value, NULL); > + if (ret) > + return ret; > + > + if (setting->type == COREBOOT_CFR_SETTING_NUMBER) > + return sysfs_emit(buf, "%u\n", value); > + > + label = coreboot_cfr_label_from_value(setting, value); > + if (!label) > + return -EINVAL; > + > + return sysfs_emit(buf, "%s\n", label); > +} > + > +static ssize_t current_value_store(struct kobject *kobj, > + struct kobj_attribute *attr, > + const char *buf, size_t count) > +{ > + struct coreboot_cfr_setting *setting = to_coreboot_cfr_setting(kobj); > + u32 value; > + int ret; > + > + ret = coreboot_cfr_parse_value(setting, buf, &value); > + if (ret) > + return ret; > + > + if (!coreboot_cfr_value_is_valid(setting, value)) > + return -EINVAL; > + > + ret = coreboot_cfr_write_value(setting, value); > + if (ret) > + return ret; > + > + return count; > +} > + > +static struct kobj_attribute type_attr = __ATTR_RO(type); > +static struct kobj_attribute display_name_language_code_attr = > + __ATTR_RO(display_name_language_code); > +static struct kobj_attribute display_name_attr = __ATTR_RO(display_name); > +static struct kobj_attribute possible_values_attr = __ATTR_RO(possible_values); > +static struct kobj_attribute min_value_attr = __ATTR_RO(min_value); > +static struct kobj_attribute max_value_attr = __ATTR_RO(max_value); > +static struct kobj_attribute scalar_increment_attr = __ATTR_RO(scalar_increment); > +static struct kobj_attribute default_value_attr = __ATTR_RO(default_value); > +static struct kobj_attribute current_value_attr = __ATTR_RW(current_value); > + > +static struct attribute *coreboot_cfr_setting_attrs[] = { > + &type_attr.attr, > + &display_name_language_code_attr.attr, > + &display_name_attr.attr, > + &possible_values_attr.attr, > + &min_value_attr.attr, > + &max_value_attr.attr, > + &scalar_increment_attr.attr, > + &default_value_attr.attr, > + ¤t_value_attr.attr, > + NULL, > +}; > + > +static umode_t coreboot_cfr_attr_is_visible(struct kobject *kobj, > + struct attribute *attr, int n) > +{ > + struct coreboot_cfr_setting *setting = to_coreboot_cfr_setting(kobj); > + > + if (setting->type == COREBOOT_CFR_SETTING_NUMBER && > + attr == &possible_values_attr.attr) > + return 0; > + > + if (setting->type != COREBOOT_CFR_SETTING_NUMBER && > + (attr == &min_value_attr.attr || attr == &max_value_attr.attr || > + attr == &scalar_increment_attr.attr)) > + return 0; > + > + if (setting->read_only && attr == ¤t_value_attr.attr) > + return 0444; > + > + return attr->mode; > +} > + > +static const struct attribute_group coreboot_cfr_setting_group = { > + .attrs = coreboot_cfr_setting_attrs, > + .is_visible = coreboot_cfr_attr_is_visible, > +}; > + > +static void coreboot_cfr_free_setting(struct coreboot_cfr_setting *setting) > +{ > + unsigned int i; > + > + for (i = 0; i < setting->n_values; i++) > + kfree(setting->values[i].label); > + > + kfree(setting->values); > + kfree(setting->display_name); > + kfree(setting->name); > + kfree(setting); > +} > + > +static void coreboot_cfr_setting_release(struct kobject *kobj) > +{ > + coreboot_cfr_free_setting(to_coreboot_cfr_setting(kobj)); > +} > + > +static const struct kobj_type coreboot_cfr_setting_ktype = { > + .release = coreboot_cfr_setting_release, > + .sysfs_ops = &kobj_sysfs_ops, > +}; > + > +static ssize_t pending_reboot_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) > +{ > + struct coreboot_cfr_drvdata *data; > + > + data = dev_get_drvdata(kobj_to_dev(kobj->parent)); > + > + return sysfs_emit(buf, "%d\n", data->pending_reboot); > +} > + > +static struct kobj_attribute pending_reboot_attr = __ATTR_RO(pending_reboot); > + > +static int coreboot_cfr_copy_bool_values(struct coreboot_cfr_setting *setting) > +{ > + static const struct coreboot_cfr_enum bool_values[] = { > + { .label = "Disabled", .value = 0 }, > + { .label = "Enabled", .value = 1 }, > + }; > + unsigned int i; > + > + setting->values = kcalloc(ARRAY_SIZE(bool_values), sizeof(*setting->values), > + GFP_KERNEL); > + if (!setting->values) > + return -ENOMEM; > + > + for (i = 0; i < ARRAY_SIZE(bool_values); i++) { > + setting->values[i].label = kstrdup(bool_values[i].label, > + GFP_KERNEL); > + if (!setting->values[i].label) > + return -ENOMEM; > + setting->values[i].value = bool_values[i].value; > + setting->n_values++; > + } > + > + return 0; > +} > + > +static int coreboot_cfr_count_enum_values(const void *base, size_t len) > +{ > + const struct coreboot_table_entry *entry; > + size_t off; > + int count = 0; > + > + for (off = 0; off < len; off += entry->size) { > + if (len - off < sizeof(*entry)) > + return -EINVAL; > + > + entry = base + off; > + if (entry->size < sizeof(*entry) || entry->size > len - off) > + return -EINVAL; > + > + if (entry->tag == CFR_TAG_ENUM_VALUE) > + count++; > + } > + > + return count; > +} So if coreboot_cfr_child_entry() could continue from a non-starting position, you could reuse it here and avoid duplicating the entry walking code? It probably could already actually. The caller would just need to recalculate the remaining length to pass to it but maybe that too could be solved by passing it as "remaining" pointer or something along those lines. > + > +static int coreboot_cfr_copy_enum_values(struct coreboot_cfr_setting *setting, > + const void *base, size_t len) > +{ > + const struct lb_cfr_enum_value *enum_value; > + const struct lb_cfr_varbinary *label; > + const struct coreboot_table_entry *entry; > + size_t off; > + int count; > + > + count = coreboot_cfr_count_enum_values(base, len); > + if (count <= 0) > + return count ?: -EINVAL; > + > + setting->values = kcalloc(count, sizeof(*setting->values), GFP_KERNEL); > + if (!setting->values) > + return -ENOMEM; > + > + for (off = 0; off < len; off += entry->size) { > + entry = base + off; > + if (entry->tag != CFR_TAG_ENUM_VALUE) Based on other walking code checks, is this safe without size check first? Here it looks you have another copy of entry walking code, but is it now with different checks? It would be really useful to consolidate them to avoid each having their own bugs. > + continue; > + > + if (entry->size < sizeof(*enum_value)) > + return -EINVAL; > + > + enum_value = base + off; > + label = coreboot_cfr_child_string(enum_value + 1, > + enum_value->size - sizeof(*enum_value), > + CFR_TAG_VARCHAR_UI_NAME); > + if (!label) > + return -EINVAL; > + > + setting->values[setting->n_values].label = > + coreboot_cfr_string_dup(label); > + if (!setting->values[setting->n_values].label) > + return -ENOMEM; > + > + setting->values[setting->n_values].value = enum_value->value; > + setting->n_values++; > + } > + > + return 0; > +} > + > +static bool > +coreboot_cfr_possible_values_fit(const struct coreboot_cfr_setting *setting) > +{ > + size_t len = 1; /* Trailing newline. */ Could you move the code a bit to right to clearly separate it visually from the actual code. You should have enough space for that. > + unsigned int i; > + > + for (i = 0; i < setting->n_values; i++) { > + if (i) > + len++; > + > + if (strlen(setting->values[i].label) >= PAGE_SIZE - len) > + return false; > + > + len += strlen(setting->values[i].label); Calculate the length to an intermediate variable? > + } > + > + return true; > +} > + > +static int coreboot_cfr_setting_is_usable(struct coreboot_cfr_setting *setting) > +{ > + u32 value; > + int ret; > + > + ret = coreboot_cfr_read_value(setting, &value, NULL); > + if (ret) > + return ret; > + > + if (!coreboot_cfr_value_is_valid(setting, value)) > + return -EINVAL; > + > + return 0; > +} > + > +static int coreboot_cfr_register_setting(struct coreboot_cfr_drvdata *data, > + struct coreboot_cfr_setting *setting) > +{ > + int ret; > + > + ret = kobject_init_and_add(&setting->kobj, &coreboot_cfr_setting_ktype, > + &data->attrs_kset->kobj, "%s", setting->name); > + if (ret) { > + kobject_put(&setting->kobj); > + return ret; > + } > + > + ret = sysfs_create_group(&setting->kobj, &coreboot_cfr_setting_group); > + if (ret) { > + kobject_put(&setting->kobj); > + return ret; > + } > + > + list_add_tail(&setting->node, &data->settings); > + return 0; > +} > + > +static int coreboot_cfr_add_numeric_option(struct coreboot_cfr_drvdata *data, > + const struct lb_cfr_numeric_option *option, > + bool parent_read_only) > +{ > + const struct lb_cfr_varbinary *name; > + const struct lb_cfr_varbinary *display_name; > + const struct lb_cfr_runtime_apply *runtime_apply; > + const void *child_base = option + 1; > + struct coreboot_cfr_setting *setting; > + size_t child_len = option->size - sizeof(*option); > + int ret; > + > + if (!(option->flags & CFR_OPTFLAG_RUNTIME)) > + return 0; > + > + if (option->flags & COREBOOT_CFR_OPT_SKIP_FLAGS) > + return 0; > + > + if (option->dependency_id) > + return 0; > + > + setting = kzalloc_obj(*setting, GFP_KERNEL); > + if (!setting) > + return -ENOMEM; > + > + INIT_LIST_HEAD(&setting->node); > + setting->drvdata = data; > + setting->default_value = option->default_value; > + setting->min = option->min; > + setting->max = option->max; > + setting->step = option->step ?: 1; > + setting->read_only = > + (option->flags & COREBOOT_CFR_OPT_READ_ONLY_FLAGS) || > + !data->efi_writes_supported || parent_read_only; > + > + runtime_apply = coreboot_cfr_child_runtime_apply(child_base, child_len); > + if (IS_ERR(runtime_apply)) { > + ret = PTR_ERR(runtime_apply); > + goto err_put_setting; > + } > + > + if (runtime_apply && runtime_apply->method == CFR_RUNTIME_APPLY_APM_CNT) { > + setting->runtime_apply_method = runtime_apply->method; > + setting->runtime_apply_id = runtime_apply->id; > + } > + > + name = coreboot_cfr_child_string(child_base, child_len, > + CFR_TAG_VARCHAR_OPT_NAME); > + if (!name) { > + ret = -EINVAL; > + goto err_put_setting; > + } > + > + setting->name = coreboot_cfr_string_dup(name); > + if (!setting->name) { > + ret = -ENOMEM; > + goto err_put_setting; > + } > + > + if (!coreboot_cfr_string_is_valid_name(setting->name)) { > + ret = -EINVAL; > + goto err_put_setting; > + } > + > + display_name = coreboot_cfr_child_string(child_base, child_len, > + CFR_TAG_VARCHAR_UI_NAME); > + if (display_name) > + setting->display_name = coreboot_cfr_string_dup(display_name); > + else > + setting->display_name = kstrdup(setting->name, GFP_KERNEL); > + if (!setting->display_name) { > + ret = -ENOMEM; > + goto err_put_setting; > + } > + > + switch (option->tag) { > + case CFR_TAG_OPTION_BOOL: > + setting->type = COREBOOT_CFR_SETTING_BOOL; > + setting->min = 0; > + setting->max = 1; > + setting->step = 1; > + ret = coreboot_cfr_copy_bool_values(setting); > + break; > + case CFR_TAG_OPTION_ENUM: > + setting->type = COREBOOT_CFR_SETTING_ENUM; > + ret = coreboot_cfr_copy_enum_values(setting, child_base, > + child_len); > + break; > + case CFR_TAG_OPTION_NUMBER: > + setting->type = COREBOOT_CFR_SETTING_NUMBER; > + if (!setting->min && !setting->max) > + setting->max = U32_MAX; > + else if (setting->max < setting->min) > + setting->max = U32_MAX; > + ret = 0; > + break; > + default: > + ret = -EINVAL; > + break; > + } > + > + if (ret) > + goto err_put_setting; > + if (setting->type != COREBOOT_CFR_SETTING_NUMBER && > + !coreboot_cfr_possible_values_fit(setting)) { > + ret = 0; > + goto err_put_setting; > + } > + > + ret = coreboot_cfr_setting_is_usable(setting); > + if (ret) { > + if (ret == -ENOENT || ret == -EINVAL || ret == -EOPNOTSUPP || > + ret == -ENAMETOOLONG) > + ret = 0; > + goto err_put_setting; > + } > + > + ret = coreboot_cfr_register_setting(data, setting); > + if (ret) > + return ret; > + > + return 0; > + > +err_put_setting: > + coreboot_cfr_free_setting(setting); > + return ret; > +} > + > +static int coreboot_cfr_parse_records(struct coreboot_cfr_drvdata *data, > + const void *base, size_t len, > + unsigned int depth, > + bool parent_read_only) > +{ > + const struct lb_cfr_option_form *form; > + const struct coreboot_table_entry *entry; > + const void *child_base; > + size_t child_len; > + size_t off; > + int ret; > + > + for (off = 0; off < len; off += entry->size) { > + if (len - off < sizeof(*entry)) > + return -EINVAL; > + > + entry = base + off; > + if (entry->size < sizeof(*entry) || entry->size > len - off) > + return -EINVAL; If the entry walker can take some kind of ANY tag, this too could likely use it. You may also want to return ERR_PTR() from it to be able to differentiate end of entries from invalid entries. > + switch (entry->tag) { > + case CFR_TAG_OPTION_FORM: > + if (entry->size < sizeof(struct lb_cfr_option_form)) > + return -EINVAL; > + > + form = base + off; > + if (form->dependency_id || > + form->flags & CFR_OPTFLAG_SUPPRESS) > + break; > + > + if (depth >= COREBOOT_CFR_MAX_FORM_DEPTH) > + return -E2BIG; > + > + child_base = base + off + sizeof(struct lb_cfr_option_form); > + child_len = entry->size - sizeof(struct lb_cfr_option_form); > + ret = coreboot_cfr_parse_records(data, child_base, > + child_len, depth + 1, > + parent_read_only || > + (form->flags & > + COREBOOT_CFR_OPT_READ_ONLY_FLAGS)); > + if (ret) > + return ret; > + break; > + case CFR_TAG_OPTION_ENUM: > + case CFR_TAG_OPTION_NUMBER: > + case CFR_TAG_OPTION_BOOL: > + if (entry->size < sizeof(struct lb_cfr_numeric_option)) > + return -EINVAL; > + ret = coreboot_cfr_add_numeric_option(data, base + off, > + parent_read_only); > + if (ret) > + return ret; > + break; > + default: > + /* Ignore unsupported, child-only and future record types. */ > + break; > + } > + } > + > + return 0; > +} > + > +static void coreboot_cfr_unregister_settings(struct coreboot_cfr_drvdata *data) > +{ > + struct coreboot_cfr_setting *setting, *tmp; > + > + list_for_each_entry_safe(setting, tmp, &data->settings, node) { > + sysfs_remove_group(&setting->kobj, &coreboot_cfr_setting_group); > + list_del(&setting->node); > + kobject_put(&setting->kobj); > + } > +} > + > +static int coreboot_cfr_probe(struct coreboot_device *dev) > +{ > + const struct lb_cfr *root = (const struct lb_cfr *)dev->raw; > + struct coreboot_cfr_drvdata *data; > + size_t payload_len; > + int ret; > + > + if (dev->entry.size < sizeof(*root)) > + return -EINVAL; > + > + if (root->tag != LB_TAG_CFR_ROOT || root->version != CFR_VERSION) > + return -EINVAL; > + > + if (root->size < sizeof(*root) || root->size > dev->entry.size) > + return -EINVAL; > + > + payload_len = root->size - sizeof(*root); > + if (crc32_be(0, root + 1, payload_len) != root->checksum) > + return -EBADMSG; > + > + if (!efivar_is_available()) > + return -EPROBE_DEFER; > + > + data = devm_kzalloc(&dev->dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + ret = efivar_lock(); > + if (ret) > + return ret; > + data->efi_writes_supported = efivar_supports_writes(); > + efivar_unlock(); > + > + INIT_LIST_HEAD(&data->settings); > + ret = devm_mutex_init(&dev->dev, &data->lock); > + if (ret) > + return ret; > + > + dev_set_drvdata(&dev->dev, data); > + > + data->class_dev = device_create(&firmware_attributes_class, NULL, > + MKDEV(0, 0), NULL, DRIVER_NAME); > + if (IS_ERR(data->class_dev)) > + return PTR_ERR(data->class_dev); > + dev_set_drvdata(data->class_dev, data); > + > + data->attrs_kset = kset_create_and_add("attributes", NULL, > + &data->class_dev->kobj); > + if (!data->attrs_kset) { > + ret = -ENOMEM; > + goto err_unregister_dev; > + } > + > + ret = sysfs_create_file(&data->attrs_kset->kobj, > + &pending_reboot_attr.attr); > + if (ret) > + goto err_unregister_attrs; > + > + ret = coreboot_cfr_parse_records(data, root + 1, payload_len, 0, false); > + if (ret) > + goto err_unregister_settings; > + > + if (list_empty(&data->settings)) { > + ret = -ENODEV; > + goto err_unregister_settings; > + } > + > + return 0; > + > +err_unregister_settings: > + coreboot_cfr_unregister_settings(data); > + sysfs_remove_file(&data->attrs_kset->kobj, &pending_reboot_attr.attr); > +err_unregister_attrs: > + kset_unregister(data->attrs_kset); > +err_unregister_dev: > + device_unregister(data->class_dev); > + return ret; > +} > + > +static void coreboot_cfr_remove(struct coreboot_device *dev) > +{ > + struct coreboot_cfr_drvdata *data = dev_get_drvdata(&dev->dev); > + > + coreboot_cfr_unregister_settings(data); > + sysfs_remove_file(&data->attrs_kset->kobj, &pending_reboot_attr.attr); > + kset_unregister(data->attrs_kset); > + device_unregister(data->class_dev); > +} > + > +static const struct coreboot_device_id coreboot_cfr_ids[] = { > + { .tag = LB_TAG_CFR_ROOT }, > + { } > +}; > +MODULE_DEVICE_TABLE(coreboot, coreboot_cfr_ids); > + > +static struct coreboot_driver coreboot_cfr_driver = { > + .probe = coreboot_cfr_probe, > + .remove = coreboot_cfr_remove, > + .drv = { > + .name = DRIVER_NAME, > + }, > + .id_table = coreboot_cfr_ids, > +}; > +module_coreboot_driver(coreboot_cfr_driver); > + > +MODULE_AUTHOR("Sean Rhodes <sean@starlabs.systems>"); > +MODULE_DESCRIPTION("coreboot CFR firmware attributes driver"); > +MODULE_LICENSE("GPL"); > +MODULE_IMPORT_NS("EFIVAR"); > -- i. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-07-21 9:56 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-07-17 8:50 [PATCH v7 0/3] firmware: Add coreboot CFR firmware attributes driver Sean Rhodes 2026-07-17 8:50 ` [PATCH v7 1/3] firmware: Move firmware attributes class helper Sean Rhodes 2026-07-18 2:22 ` Derek J. Clark 2026-07-17 8:50 ` [PATCH v7 2/3] firmware: Rename google firmware directory to coreboot Sean Rhodes 2026-07-18 0:39 ` Julius Werner 2026-07-17 8:50 ` [PATCH v7 3/3] firmware: coreboot: Add CFR firmware attributes driver Sean Rhodes 2026-07-20 6:48 ` Tzung-Bi Shih 2026-07-21 9:14 ` Ilpo Järvinen 2026-07-21 9:56 ` Ilpo Järvinen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.