Skip to contentSkip to conversation

coreboot CFR firmware attributes

PATCH Superseded Sean Rhodes 4 participants 13 messages Aug 4 – Aug 12

Subsystems (14)
DRM drivers DRM misc x86 platform drivers ChromeOS EC subdrivers +10 more subsystems

v11 of 14 Superseded Latest is v14 →

lore
Share & tools
lore

Sean Rhodes

author maintainer
#0 lore raw

v11 cover coreboot CFR firmware attributes

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 v11:
- Keep the scoped cleanup in the CFR driver, but use explicit goto cleanup for the nested sysfs group registration path.

Changes in v10:
- Address the nested cleanup-path review on CFR attribute registration.

Changes in v9:
- Add the missing kstrtox.h include used by kstrtou32().
- Use a single kobject rollback path when setting registration fails.
- Split a few helper locals for readability, including the pending reboot device lookup and enum-value population.
- Avoid an intermediate void pointer cast while parsing numeric CFR options.
- Check the possible-values buffer length before computing each label length.

Tested on a Star Labs Byte with firmware 26.07: runtime APM_CNT apply was verified with power_led, and reboot-needed behavior was verified with debug_level.

Sean Rhodes (4):
platform/x86: Sort firmware attributes class includes
firmware: Move firmware attributes class helper
firmware: Rename google firmware directory to coreboot
firmware: coreboot: Add CFR firmware attributes driver

 MAINTAINERS                                   |   18 +-
 arch/arm64/configs/defconfig                  |    6 +-
 drivers/firmware/Kconfig                      |    5 +-
 drivers/firmware/Makefile                     |    3 +-
 drivers/firmware/{google => coreboot}/Kconfig |   88 +-
 drivers/firmware/coreboot/Makefile            |   15 +
 drivers/firmware/{google => coreboot}/cbmem.c |    0
 drivers/firmware/coreboot/coreboot-cfr.c      | 1205 +++++++++++++++++
 .../{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/firmware/google/Makefile              |   14 -
 drivers/gpu/drm/sysfb/Kconfig                 |    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         |    9 +-
 drivers/platform/x86/hp/hp-bioscfg/bioscfg.c  |    9 +-
 drivers/platform/x86/lenovo/think-lmi.c       |    5 +-
 drivers/platform/x86/lenovo/wmi-other.c       |    2 +-
 drivers/platform/x86/samsung-galaxybook.c     |    3 +-
 .../linux/firmware_attributes.h               |    6 +-
 31 files changed, 1340 insertions(+), 65 deletions(-)
 rename drivers/firmware/{google => coreboot}/Kconfig (57%)
 create mode 100644 drivers/firmware/coreboot/Makefile
 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%)
 delete mode 100644 drivers/firmware/google/Makefile
 rename drivers/platform/x86/firmware_attributes_class.h => include/linux/firmware_attributes.h (60%)
author maintainer
#1 lore raw

PATCH v11 1/4 platform/x86: Sort firmware attributes class includes

Move firmware_attributes_class.h into the main include block and sort the affected includes. This lets the header move replace each include in place.

Suggested-by Derek J. Clark <[email protected]>
Signed-off-by Sean Rhodes <[email protected]>
---
 drivers/platform/x86/asus-armoury.c                | 2 +-
 drivers/platform/x86/dell/dell-wmi-sysman/sysman.c | 9 +++++----
 drivers/platform/x86/hp/hp-bioscfg/bioscfg.c       | 9 +++++----
 drivers/platform/x86/lenovo/think-lmi.c            | 5 +++--
 drivers/platform/x86/lenovo/wmi-other.c            | 2 +-
 drivers/platform/x86/samsung-galaxybook.c          | 3 ++-
 6 files changed, 17 insertions(+), 13 deletions(-)
drivers/platform/x86/asus-armoury.c +1 -1
index 495dc1e31d40..f3d8c5a85762 100644
@@ -20,6 +20,7 @@
 #include <linux/dmi.h>
 #include <linux/err.h>
 #include <linux/errno.h>
+#include "firmware_attributes_class.h"
 #include <linux/fs.h>
 #include <linux/kernel.h>
 #include <linux/kmod.h>

@@ -34,7 +35,6 @@
 #include <linux/sysfs.h>
 
 #include "asus-armoury.h"
-#include "firmware_attributes_class.h"
 
 #define ASUS_NB_WMI_EVENT_GUID "0B3CBB35-E3C2-45ED-91C2-4C5A6D195D1C"
drivers/platform/x86/dell/dell-wmi-sysman/sysman.c +5 -4
index ab46a023cc34..4ff9003c6589 100644
@@ -8,18 +8,19 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/align.h>
-#include <linux/fs.h>
 #include <linux/dmi.h>
-#include <linux/module.h>
+#include "../../firmware_attributes_class.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"
 
 #define MAX_TYPES  4
-#include <linux/nls.h>
 
 struct wmi_sysman_priv wmi_priv = {
 	.mutex = __MUTEX_INITIALIZER(wmi_priv.mutex),
drivers/platform/x86/hp/hp-bioscfg/bioscfg.c +5 -4
index 27fd6cd21529..1965a329be48 100644
@@ -7,16 +7,17 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/errno.h>
+#include "../../firmware_attributes_class.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>
 
 MODULE_AUTHOR("Jorge Lopez <[email protected]>");
 MODULE_DESCRIPTION("HP BIOS Configuration Driver");
drivers/platform/x86/lenovo/think-lmi.c +3 -2
index e215e86e3db7..9e4b3e973de9 100644
@@ -13,14 +13,15 @@
 
 #include <linux/acpi.h>
 #include <linux/array_size.h>
+#include <linux/dmi.h>
 #include <linux/errno.h>
+#include "../firmware_attributes_class.h"
 #include <linux/fs.h>
 #include <linux/mutex.h>
 #include <linux/string_helpers.h>
 #include <linux/types.h>
-#include <linux/dmi.h>
 #include <linux/wmi.h>
-#include "../firmware_attributes_class.h"
+
 #include "think-lmi.h"
 
 static bool debug_support;
drivers/platform/x86/lenovo/wmi-other.c +1 -1
index fbb32bf404f2..fdebaa0a9992 100644
@@ -33,6 +33,7 @@
 #include <linux/container_of.h>
 #include <linux/device.h>
 #include <linux/export.h>
+#include "../firmware_attributes_class.h"
 #include <linux/gfp_types.h>
 #include <linux/hwmon.h>
 #include <linux/idr.h>

@@ -50,7 +51,6 @@
 #include "wmi-capdata.h"
 #include "wmi-events.h"
 #include "wmi-helpers.h"
-#include "../firmware_attributes_class.h"
 
 #define LENOVO_OTHER_MODE_GUID "DC2A8805-3A8C-41BA-A6F7-092E0089CD3B"
drivers/platform/x86/samsung-galaxybook.c +2 -1
index 6382af0b106c..74103c00b0c8 100644
@@ -14,6 +14,7 @@
 #include <linux/acpi.h>
 #include <linux/bits.h>
 #include <linux/err.h>
+#include "firmware_attributes_class.h"
 #include <linux/i8042.h>
 #include <linux/init.h>
 #include <linux/input.h>

@@ -27,8 +28,8 @@
 #include <linux/sysfs.h>
 #include <linux/uuid.h>
 #include <linux/workqueue.h>
+
 #include <acpi/battery.h>
-#include "firmware_attributes_class.h"
 
 #define DRIVER_NAME "samsung-galaxybook"
author maintainer
#2 lore raw

PATCH v11 2/4 firmware: Move firmware attributes class helper

In reply to 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 <[email protected]>
Reviewed-by Derek J. Clark <[email protected]>
Tested-by Oliver Lin <[email protected]>
Signed-off-by Sean Rhodes <[email protected]>
---
 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%)
MAINTAINERS +8 -0
index 2f9472c1a090..e6a8c271e79d 100644
@@ -10115,6 +10115,14 @@ F:	include/linux/firewire.h
 F:	include/uapi/linux/firewire*.h
 F:	tools/firewire/
 
+FIRMWARE ATTRIBUTES CLASS
+M:	Hans de Goede <[email protected]>
+M:	Ilpo Järvinen <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	drivers/firmware/firmware_attributes_class.c
+F:	include/linux/firmware_attributes.h
+
 FIRMWARE FRAMEWORK FOR ARMV8-A
 M:	Sudeep Holla <[email protected]>
 L:	[email protected] (moderated for non-subscribers)
drivers/firmware/Kconfig +3 -0
index 12dc70254842..78a239f8bbef 100644
@@ -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
drivers/firmware/Makefile +1 -0
index 4ddec2820c96..385b6e8b7fc6 100644
@@ -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
drivers/firmware/firmware_attributes_class.c renamed from drivers/platform/x86/firmware_attributes_class.c +1 -1
similarity index 94%
rename from drivers/platform/x86/firmware_attributes_class.c
rename to drivers/firmware/firmware_attributes_class.c
index 736e96c186d9..503ecc288086 100644
@@ -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",
drivers/platform/x86/Kconfig +0 -3
index b54b5212b204..beed14c38b54 100644
@@ -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
drivers/platform/x86/Makefile +0 -2
index 872ac3842391..cab20a4145e3 100644
@@ -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
drivers/platform/x86/asus-armoury.c +1 -1
index f3d8c5a85762..e6af93608ec5 100644
@@ -20,7 +20,7 @@
 #include <linux/dmi.h>
 #include <linux/err.h>
 #include <linux/errno.h>
-#include "firmware_attributes_class.h"
+#include <linux/firmware_attributes.h>
 #include <linux/fs.h>
 #include <linux/kernel.h>
 #include <linux/kmod.h>
drivers/platform/x86/dell/dell-wmi-sysman/sysman.c +1 -1
index 4ff9003c6589..963b3473991d 100644
@@ -9,7 +9,7 @@
 
 #include <linux/align.h>
 #include <linux/dmi.h>
-#include "../../firmware_attributes_class.h"
+#include <linux/firmware_attributes.h>
 #include <linux/fs.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
drivers/platform/x86/hp/hp-bioscfg/bioscfg.c +1 -1
index 1965a329be48..f13f21580acd 100644
@@ -8,7 +8,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/errno.h>
-#include "../../firmware_attributes_class.h"
+#include <linux/firmware_attributes.h>
 #include <linux/fs.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
drivers/platform/x86/lenovo/think-lmi.c +1 -1
index 9e4b3e973de9..4437785aa1f2 100644
@@ -15,7 +15,7 @@
 #include <linux/array_size.h>
 #include <linux/dmi.h>
 #include <linux/errno.h>
-#include "../firmware_attributes_class.h"
+#include <linux/firmware_attributes.h>
 #include <linux/fs.h>
 #include <linux/mutex.h>
 #include <linux/string_helpers.h>
drivers/platform/x86/lenovo/wmi-other.c +1 -1
index fdebaa0a9992..b175971bb308 100644
@@ -33,7 +33,7 @@
 #include <linux/container_of.h>
 #include <linux/device.h>
 #include <linux/export.h>
-#include "../firmware_attributes_class.h"
+#include <linux/firmware_attributes.h>
 #include <linux/gfp_types.h>
 #include <linux/hwmon.h>
 #include <linux/idr.h>
drivers/platform/x86/samsung-galaxybook.c +1 -1
index 74103c00b0c8..3911f60c23ea 100644
@@ -14,7 +14,7 @@
 #include <linux/acpi.h>
 #include <linux/bits.h>
 #include <linux/err.h>
-#include "firmware_attributes_class.h"
+#include <linux/firmware_attributes.h>
 #include <linux/i8042.h>
 #include <linux/init.h>
 #include <linux/input.h>
include/linux/firmware_attributes.h renamed from drivers/platform/x86/firmware_attributes_class.h +3 -3
similarity index 60%
rename from drivers/platform/x86/firmware_attributes_class.h
rename to include/linux/firmware_attributes.h
index d27abe54fcf9..d4026ba1fcc2 100644
@@ -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 */
author maintainer
#3 lore raw

PATCH v11 3/4 firmware: Rename google firmware directory to coreboot

In reply to 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.

Rename the generic coreboot Kconfig symbols at the same time. Keep the old CONFIG_GOOGLE_* names as transitional symbols so existing configurations migrate to the new names.

Suggested-by Julius Werner <[email protected]>
Signed-off-by Sean Rhodes <[email protected]>
---
 MAINTAINERS                                   |  4 +-
 arch/arm64/configs/defconfig                  |  6 +-
 drivers/firmware/Kconfig                      |  2 +-
 drivers/firmware/Makefile                     |  2 +-
 drivers/firmware/{google => coreboot}/Kconfig | 74 ++++++++++++++-----
 .../firmware/{google => coreboot}/Makefile    | 10 +--
 drivers/firmware/{google => coreboot}/cbmem.c |  0
 .../{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
 drivers/gpu/drm/sysfb/Kconfig                 |  2 +-
 19 files changed, 72 insertions(+), 34 deletions(-)
 rename drivers/firmware/{google => coreboot}/Kconfig (64%)
 rename drivers/firmware/{google => coreboot}/Makefile (51%)
 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%)
MAINTAINERS +2 -2
index e6a8c271e79d..62ddee473f55 100644
@@ -11069,14 +11069,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 <[email protected]>
 R:	Brian Norris <[email protected]>
 R:	Julius Werner <[email protected]>
 L:	[email protected]
 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
arch/arm64/configs/defconfig +3 -3
index 654a102cb5bc..32ec89a872d9 100644
@@ -267,9 +267,9 @@ CONFIG_RASPBERRYPI_FIRMWARE=y
 CONFIG_INTEL_STRATIX10_SERVICE=y
 CONFIG_INTEL_STRATIX10_RSU=m
 CONFIG_MTK_ADSP_IPC=m
-CONFIG_GOOGLE_FIRMWARE=y
-CONFIG_GOOGLE_CBMEM=m
-CONFIG_GOOGLE_COREBOOT_TABLE=m
+CONFIG_COREBOOT_FIRMWARE=y
+CONFIG_COREBOOT_CBMEM=m
+CONFIG_COREBOOT_TABLE=m
 CONFIG_EFI_CAPSULE_LOADER=y
 CONFIG_IMX_SCU=y
 CONFIG_QCOM_TZMEM_MODE_SHMBRIDGE=y
drivers/firmware/Kconfig +1 -1
index 78a239f8bbef..4df0a0ad18b8 100644
@@ -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"
drivers/firmware/Makefile +1 -1
index 385b6e8b7fc6..f8da96382680 100644
@@ -30,7 +30,7 @@ obj-y				+= broadcom/
 obj-y				+= cirrus/
 obj-y				+= meson/
 obj-y				+= microchip/
-obj-$(CONFIG_GOOGLE_FIRMWARE)	+= google/
+obj-$(CONFIG_COREBOOT_FIRMWARE)	+= coreboot/
 obj-y				+= efi/
 obj-y				+= imx/
 obj-y				+= psci/
drivers/firmware/coreboot/Kconfig renamed from drivers/firmware/google/Kconfig +56 -18

129 lines not on this page · raw message

drivers/firmware/coreboot/Makefile renamed from drivers/firmware/google/Makefile +5 -5
similarity index 51%
rename from drivers/firmware/google/Makefile
rename to drivers/firmware/coreboot/Makefile
index 8151e323cc43..e681f274417a 100644
@@ -1,14 +1,14 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_GOOGLE_SMI)		+= gsmi.o
-obj-$(CONFIG_GOOGLE_COREBOOT_TABLE)        += coreboot_table.o
-obj-$(CONFIG_GOOGLE_FRAMEBUFFER_COREBOOT)  += framebuffer-coreboot.o
-obj-$(CONFIG_GOOGLE_MEMCONSOLE)            += memconsole.o
-obj-$(CONFIG_GOOGLE_MEMCONSOLE_COREBOOT)   += memconsole-coreboot.o
+obj-$(CONFIG_COREBOOT_TABLE)               += coreboot_table.o
+obj-$(CONFIG_COREBOOT_FRAMEBUFFER)         += framebuffer-coreboot.o
+obj-$(CONFIG_COREBOOT_MEMCONSOLE_COMMON)   += memconsole.o
+obj-$(CONFIG_COREBOOT_MEMCONSOLE)          += memconsole-coreboot.o
 obj-$(CONFIG_GOOGLE_MEMCONSOLE_X86_LEGACY) += memconsole-x86-legacy.o
 
 # Must come after coreboot_table.o, as this driver depends on that bus type.
-obj-$(CONFIG_GOOGLE_CBMEM)		+= cbmem.o
+obj-$(CONFIG_COREBOOT_CBMEM)		+= cbmem.o
 
 vpd-sysfs-y := vpd.o vpd_decode.o
 obj-$(CONFIG_GOOGLE_VPD)		+= vpd-sysfs.o
drivers/firmware/coreboot/cbmem.c renamed from drivers/firmware/google/cbmem.c +0 -0
similarity index 100%
rename from drivers/firmware/google/cbmem.c
rename to drivers/firmware/coreboot/cbmem.c
drivers/firmware/coreboot/coreboot_table.c renamed from drivers/firmware/google/coreboot_table.c +0 -0
similarity index 100%
rename from drivers/firmware/google/coreboot_table.c
rename to drivers/firmware/coreboot/coreboot_table.c
drivers/firmware/coreboot/coreboot_table.h renamed from drivers/firmware/google/coreboot_table.h +0 -0
similarity index 100%
rename from drivers/firmware/google/coreboot_table.h
rename to drivers/firmware/coreboot/coreboot_table.h
drivers/firmware/coreboot/framebuffer-coreboot.c renamed from drivers/firmware/google/framebuffer-coreboot.c +0 -0
similarity index 100%
rename from drivers/firmware/google/framebuffer-coreboot.c
rename to drivers/firmware/coreboot/framebuffer-coreboot.c
drivers/firmware/coreboot/gsmi.c renamed from drivers/firmware/google/gsmi.c +0 -0
similarity index 100%
rename from drivers/firmware/google/gsmi.c
rename to drivers/firmware/coreboot/gsmi.c
drivers/firmware/coreboot/memconsole-coreboot.c renamed from drivers/firmware/google/memconsole-coreboot.c +0 -0
similarity index 100%
rename from drivers/firmware/google/memconsole-coreboot.c
rename to drivers/firmware/coreboot/memconsole-coreboot.c
drivers/firmware/coreboot/memconsole-x86-legacy.c renamed from drivers/firmware/google/memconsole-x86-legacy.c +0 -0
similarity index 100%
rename from drivers/firmware/google/memconsole-x86-legacy.c
rename to drivers/firmware/coreboot/memconsole-x86-legacy.c
drivers/firmware/coreboot/memconsole.c renamed from drivers/firmware/google/memconsole.c +0 -0
similarity index 100%
rename from drivers/firmware/google/memconsole.c
rename to drivers/firmware/coreboot/memconsole.c
drivers/firmware/coreboot/memconsole.h renamed from drivers/firmware/google/memconsole.h +3 -3
similarity index 82%
rename from drivers/firmware/google/memconsole.h
rename to drivers/firmware/coreboot/memconsole.h
index aaff2b72b606..58899c2abaaa 100644
@@ -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 */
drivers/firmware/coreboot/vpd.c renamed from drivers/firmware/google/vpd.c +0 -0
similarity index 100%
rename from drivers/firmware/google/vpd.c
rename to drivers/firmware/coreboot/vpd.c
drivers/firmware/coreboot/vpd_decode.c renamed from drivers/firmware/google/vpd_decode.c +0 -0
similarity index 100%
rename from drivers/firmware/google/vpd_decode.c
rename to drivers/firmware/coreboot/vpd_decode.c
drivers/firmware/coreboot/vpd_decode.h renamed from drivers/firmware/google/vpd_decode.h +0 -0
similarity index 100%
rename from drivers/firmware/google/vpd_decode.h
rename to drivers/firmware/coreboot/vpd_decode.h
drivers/gpu/drm/sysfb/Kconfig +1 -1
index 2559ead6cf1f..34d3fa8b30c9 100644
@@ -10,7 +10,7 @@ config DRM_SYSFB_HELPER
 config DRM_COREBOOTDRM
 	tristate "Coreboot framebuffer driver"
 	depends on DRM && MMU
-	depends on GOOGLE_FRAMEBUFFER_COREBOOT
+	depends on COREBOOT_FRAMEBUFFER
 	select APERTURE_HELPERS
 	select DRM_CLIENT_SELECTION
 	select DRM_GEM_SHMEM_HELPER
#4 lore raw

Re: [PATCH v11 3/4] firmware: Rename google firmware directory to coreboot

drivers/firmware/coreboot/Kconfig line 2

Quoted 9 lines · Sean Rhodes

rename to drivers/firmware/coreboot/Kconfig
index b78c644fa253..d273bd836245 100644

drivers/firmware/coreboot/Kconfig renamed from drivers/firmware/google/Kconfig +1 -2
@@ -1,13 +1,16 @@
 # SPDX-License-Identifier: GPL-2.0-only
-menuconfig GOOGLE_FIRMWARE
-	bool "Google Firmware Drivers"
+menuconfig COREBOOT_FIRMWARE

As pointed out by defconfig change, which is supposed to be separate commit, this will affect users of the kernel for no real gain.

Do not change this.

Best regards,
Krzysztof

#5 lore raw

Re: [PATCH v11 3/4] firmware: Rename google firmware directory to coreboot

Quoted 14 lines · Krzysztof Kozlowski

drivers/firmware/coreboot/Kconfig line 2

Quoted 9 lines · Sean Rhodes

rename to drivers/firmware/coreboot/Kconfig
index b78c644fa253..d273bd836245 100644

drivers/firmware/coreboot/Kconfig renamed from drivers/firmware/google/Kconfig +1 -2
@@ -1,13 +1,16 @@
 # SPDX-License-Identifier: GPL-2.0-only
-menuconfig GOOGLE_FIRMWARE
-	bool "Google Firmware Drivers"
+menuconfig COREBOOT_FIRMWARE

As pointed out by defconfig change, which is supposed to be separate commit, this will affect users of the kernel for no real gain.

Quoted 1 line

Do not change this.

I am surprised that such change appeared at v10.

Changelog is quite incomplete and I do not see any explanation for this.

Best regards,
Krzysztof

author maintainer
#6 lore raw

Re: [PATCH v11 3/4] firmware: Rename google firmware directory to coreboot

Quoted 16 lines · Krzysztof Kozlowski
Quoted 14 lines · Krzysztof Kozlowski

drivers/firmware/coreboot/Kconfig line 2

Quoted 9 lines · Sean Rhodes

rename to drivers/firmware/coreboot/Kconfig
index b78c644fa253..d273bd836245 100644

drivers/firmware/coreboot/Kconfig renamed from drivers/firmware/google/Kconfig +1 -2
@@ -1,13 +1,16 @@
 # SPDX-License-Identifier: GPL-2.0-only
-menuconfig GOOGLE_FIRMWARE
-    bool "Google Firmware Drivers"
+menuconfig COREBOOT_FIRMWARE

As pointed out by defconfig change, which is supposed to be separate commit, this will affect users of the kernel for no real gain.

Quoted 5 lines
Quoted 1 line

Do not change this.

I am surprised that such change appeared at v10.

Changelog is quite incomplete and I do not see any explanation for this.

Sorry, don't follow - so put the names back, or split it into its own patch?

Quoted 3 lines

Best regards,
Krzysztof

#7 lore raw

Re: [PATCH v11 3/4] firmware: Rename google firmware directory to coreboot

Quoted 18 lines · Sean Rhodes
Quoted 16 lines · Krzysztof Kozlowski
Quoted 14 lines · Krzysztof Kozlowski

drivers/firmware/coreboot/Kconfig line 2

Quoted 9 lines · Sean Rhodes

rename to drivers/firmware/coreboot/Kconfig
index b78c644fa253..d273bd836245 100644

drivers/firmware/coreboot/Kconfig renamed from drivers/firmware/google/Kconfig +1 -2
@@ -1,13 +1,16 @@
 # SPDX-License-Identifier: GPL-2.0-only
-menuconfig GOOGLE_FIRMWARE
-    bool "Google Firmware Drivers"
+menuconfig COREBOOT_FIRMWARE

As pointed out by defconfig change, which is supposed to be separate commit, this will affect users of the kernel for no real gain.

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

Do not change this.

I am surprised that such change appeared at v10.

Changelog is quite incomplete and I do not see any explanation for this.

Sorry, don't follow - so put the names back, or split it into its own patch?

I did not find explanation of this in the changelog. It cuts at v9, so maybe that's the reason, although lore pointed me the v10 as the first version introducing new CONFIG symbols.

Anyway, IMO renaming is really not worth if the sole reason is Google name is disliked now.

Best regards,
Krzysztof

reviewer
#8 lore raw

Re: [PATCH v11 3/4] firmware: Rename google firmware directory to coreboot

Quoted 26 lines · Krzysztof Kozlowski
Quoted 24 lines · Sean Rhodes
Quoted 21 lines · Krzysztof Kozlowski
Quoted 14 lines · Krzysztof Kozlowski

drivers/firmware/coreboot/Kconfig line 2

Quoted 9 lines · Sean Rhodes

rename to drivers/firmware/coreboot/Kconfig
index b78c644fa253..d273bd836245 100644

drivers/firmware/coreboot/Kconfig renamed from drivers/firmware/google/Kconfig +1 -2
@@ -1,13 +1,16 @@
 # SPDX-License-Identifier: GPL-2.0-only
-menuconfig GOOGLE_FIRMWARE
-    bool "Google Firmware Drivers"
+menuconfig COREBOOT_FIRMWARE

As pointed out by defconfig change, which is supposed to be separate commit, this will affect users of the kernel for no real gain.

Quoted 1 line

Do not change this.

I am surprised that such change appeared at v10.

Changelog is quite incomplete and I do not see any explanation for this.

Quoted 8 lines
Quoted 1 line

Sorry, don't follow - so put the names back, or split it into its own patch?

I did not find explanation of this in the changelog. It cuts at v9, so maybe that's the reason, although lore pointed me the v10 as the first version introducing new CONFIG symbols.

Anyway, IMO renaming is really not worth if the sole reason is Google name is disliked now.

Krzysztof, the idea with this change was to introduce the new Kconfig symbols that more accurately represent the new directory structure while keeping the old symbol names available as fallback symbols for people who continue using old .config files. Doesn't that avoid the pain to users you're concerned about? I thought doing renames like this cleanly was the whole point of the transitional feature in the Kconfig language. Why do we have that lever if we don't want to use it for the kind of cases it was meant for?

#9 lore raw

Re: [PATCH v11 3/4] firmware: Rename google firmware directory to coreboot

Quoted 15 lines · Julius Werner
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Quoted 1 line

Do not change this.

I am surprised that such change appeared at v10.

Changelog is quite incomplete and I do not see any explanation for this.

Quoted 8 lines
Quoted 1 line

Sorry, don't follow - so put the names back, or split it into its own patch?

I did not find explanation of this in the changelog. It cuts at v9, so maybe that's the reason, although lore pointed me the v10 as the first version introducing new CONFIG symbols.

Anyway, IMO renaming is really not worth if the sole reason is Google name is disliked now.

Quoted 8 lines

Krzysztof, the idea with this change was to introduce the new Kconfig symbols that more accurately represent the new directory structure while keeping the old symbol names available as fallback symbols for people who continue using old .config files. Doesn't that avoid the pain to users you're concerned about? I thought doing renames like this cleanly was the whole point of the transitional feature in the Kconfig language. Why do we have that lever if we don't want to use it for the kind of cases it was meant for?

To me 'transitional' is when you actually make changes to drivers, their dependencies or to whatever Kconfig hierarchy is there.

I do not consider marketing or pure "I do not like this name" as valid reasons, because this is developer string (or kernel/distro configurator) not end-user visible thing. And for us, the developers, this marketing does not matter.

Anyway, defconfig change must be separate because it goes via different tree, unless this is non bisectable. Keeping it here initially confused me into thinking you are actually breaking use cases (which you are not).

Best regards,
Krzysztof

reviewer
#10 lore raw

Re: [PATCH v11 3/4] firmware: Rename google firmware directory to coreboot

Quoted 4 lines

I do not consider marketing or pure "I do not like this name" as valid reasons, because this is developer string (or kernel/distro configurator) not end-user visible thing. And for us, the developers, this marketing does not matter.

I'm not sure I'd agree that naming doesn't matter for developers? The style guide has an entire section about choosing good names for C symbols, I don't think Kconfig symbols are fundamentally different in that regard; they're still strings developers read to find and make sense of things, and leaving something with the wrong name makes it harder for the reading developer to find and understand it. (This isn't about "marketing", btw, Google the company doesn't care whether this driver is called GOOGLE or COREBOOT. I care as a kernel developer that this driver has been mostly used for something else than what it was originally labeled for for a long while and it's high time that we correct that to avoid confusing people.)

Quoted 3 lines

Anyway, defconfig change must be separate because it goes via different tree, unless this is non bisectable. Keeping it here initially confused me into thinking you are actually breaking use cases (which you are not).

Fair enough. So Sean, could you maybe split this out into this patch which does everything (the renaming and adding of transitional symbols) except for the arm64 defconfig change, and then the defconfig change as a separate patch at the end of the series so that it can go through a different tree?

author maintainer
#11 lore raw

PATCH v11 4/4 firmware: coreboot: Add CFR firmware attributes driver

In reply to 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: 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 <[email protected]>
---
 MAINTAINERS                              |    6 +
 drivers/firmware/coreboot/Kconfig        |   14 +
 drivers/firmware/coreboot/Makefile       |    1 +
 drivers/firmware/coreboot/coreboot-cfr.c | 1205 ++++++++++++++++++++++
 4 files changed, 1226 insertions(+)
 create mode 100644 drivers/firmware/coreboot/coreboot-cfr.c
MAINTAINERS +6 -0
index 62ddee473f55..50638f37e023 100644
@@ -6666,6 +6666,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 <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	drivers/firmware/coreboot/coreboot-cfr.c
+
 CORETEMP HARDWARE MONITORING DRIVER
 L:	[email protected]
 S:	Orphan
drivers/firmware/coreboot/Kconfig +14 -0
index d273bd836245..30ee5a814a88 100644
@@ -49,6 +49,20 @@ config COREBOOT_TABLE
 	  device tree node /firmware/coreboot.
 	  If unsure say N.
 
+config COREBOOT_CFR
+	tristate "coreboot CFR firmware attributes"
+	depends on EFI
+	depends on 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 COREBOOT_MEMCONSOLE_COMMON
 	tristate
 	depends on GOOGLE_MEMCONSOLE_X86_LEGACY || COREBOOT_MEMCONSOLE
drivers/firmware/coreboot/Makefile +1 -0
index e681f274417a..a08d167a95a8 100644
@@ -2,6 +2,7 @@
 
 obj-$(CONFIG_GOOGLE_SMI)		+= gsmi.o
 obj-$(CONFIG_COREBOOT_TABLE)               += coreboot_table.o
+obj-$(CONFIG_COREBOOT_CFR)		+= coreboot-cfr.o
 obj-$(CONFIG_COREBOOT_FRAMEBUFFER)         += framebuffer-coreboot.o
 obj-$(CONFIG_COREBOOT_MEMCONSOLE_COMMON)   += memconsole.o
 obj-$(CONFIG_COREBOOT_MEMCONSOLE)          += memconsole-coreboot.o
drivers/firmware/coreboot/coreboot-cfr.c new file +1205 -0

1,205 lines not on this page · raw message

AI review
#12 lore raw

Re: [PATCH v11 4/4] firmware: coreboot: Add CFR firmware attributes driver

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Memory leak of enum label string when it contains invalid characters. --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Memory leak of enum label string when it contains invalid characters. --

--- Patch [4]: [PATCH v11 4/4] firmware: coreboot: Add CFR firmware attributes driver --- commit ea61591b8dd6ac619938eac5285db7c41217e27e
Author: Sean Rhodes <[email protected]>

firmware: coreboot: Add CFR firmware attributes driver

This commit adds a firmware-attributes driver for the coreboot CFR table entry to describe firmware setup options. It exposes runtime EFI variable-backed boolean, enum, and integer settings through sysfs.

Link https://doc.coreboot.org/drivers/cfr.html
Link https://doc.coreboot.org/drivers/cfr_internal.html
Quoted 5 lines
drivers/firmware/coreboot/coreboot-cfr.c new file +0 -0
new file mode 100644
index 0000000000000..1dfb2d29046b6

[ ... ]

Quoted 3 lines
drivers/firmware/coreboot/coreboot-cfr.c new file +3 -0
+static int coreboot_cfr_copy_enum_values(struct coreboot_cfr_setting *setting,
+					 const void *base, size_t len)
+{

[ ... ]

Quoted 2 lines
drivers/firmware/coreboot/coreboot-cfr.c new file +2 -0
+	for (;;) {
+		entry = coreboot_cfr_next_entry(&iterator);

[ ... ]

drivers/firmware/coreboot/coreboot-cfr.c line 817

Quoted 6 lines
drivers/firmware/coreboot/coreboot-cfr.c new file +6 -0
+		value = &setting->values[setting->n_values];
+		value->label = coreboot_cfr_string_dup(label);
+		if (!value->label)
+			return -ENOMEM;
+		if (!coreboot_cfr_string_is_valid_label(value->label))
+			return -EINVAL;

[Severity: Medium]
Does this code leak the newly allocated value->label string?

When coreboot_cfr_string_is_valid_label() fails and returns -EINVAL, setting->n_values has not been incremented yet. If the caller then cleans up the error by calling coreboot_cfr_free_setting(), the cleanup loop:

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);
	...

will skip freeing the string that was just allocated because setting->n_values does not include it yet. Should kfree() be called on value->label before returning -EINVAL here?

Quoted 5 lines
drivers/firmware/coreboot/coreboot-cfr.c new file +5 -0
+
+		value->value = enum_value->value;
+		setting->n_values++;
+	}
+}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4