[PATCH 0/4] Allow coreboot modules to autoload and enable cbmem in the arm64 defconfig

[PATCH 0/4] Allow coreboot modules to autoload and enable cbmem in the arm64 defconfig

[Nícolas F. R. A. Prado]

This series adds the missing pieces to the coreboot bus and the module
alias generation to allow coreboot modules to be automatically loaded
when matching devices are detected.

The configs for cbmem coreboot entries are then enabled in the arm64
defconfig, as modules, to allow reading logs from coreboot on arm64
Chromebooks, which is useful for debugging the boot process.


Nícolas F. R. A. Prado (4):
  firmware: coreboot: Generate modalias uevent for devices
  firmware: coreboot: Generate aliases for coreboot modules
  firmware: google: cbmem: Add to module device table
  arm64: defconfig: Enable support for cbmem entries in the coreboot
    table

 arch/arm64/configs/defconfig             |  3 +++
 drivers/firmware/google/cbmem.c          |  7 +++++++
 drivers/firmware/google/coreboot_table.c |  9 +++++++++
 include/linux/mod_devicetable.h          |  8 ++++++++
 scripts/mod/devicetable-offsets.c        |  3 +++
 scripts/mod/file2alias.c                 | 10 ++++++++++
 6 files changed, 40 insertions(+)

-- 
2.43.0

[PATCH 0/4] Allow coreboot modules to autoload and enable cbmem in the arm64 defconfig

[Nícolas F. R. A. Prado]

This series adds the missing pieces to the coreboot bus and the module
alias generation to allow coreboot modules to be automatically loaded
when matching devices are detected.

The configs for cbmem coreboot entries are then enabled in the arm64
defconfig, as modules, to allow reading logs from coreboot on arm64
Chromebooks, which is useful for debugging the boot process.


Nícolas F. R. A. Prado (4):
  firmware: coreboot: Generate modalias uevent for devices
  firmware: coreboot: Generate aliases for coreboot modules
  firmware: google: cbmem: Add to module device table
  arm64: defconfig: Enable support for cbmem entries in the coreboot
    table

 arch/arm64/configs/defconfig             |  3 +++
 drivers/firmware/google/cbmem.c          |  7 +++++++
 drivers/firmware/google/coreboot_table.c |  9 +++++++++
 include/linux/mod_devicetable.h          |  8 ++++++++
 scripts/mod/devicetable-offsets.c        |  3 +++
 scripts/mod/file2alias.c                 | 10 ++++++++++
 6 files changed, 40 insertions(+)

-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/4] firmware: coreboot: Generate modalias uevent for devices

[Nícolas F. R. A. Prado]

Generate a modalias uevent for devices in the coreboot bus to allow
userspace to automatically load the corresponding modules.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---

 drivers/firmware/google/coreboot_table.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
index 2a4469bf1b81..c1b9a9e8e8ed 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -53,11 +53,20 @@ static void coreboot_bus_remove(struct device *dev)
 		driver->remove(device);
 }
 
+static int coreboot_bus_uevent(const struct device *dev, struct kobj_uevent_env *env)
+{
+	struct coreboot_device *device = CB_DEV(dev);
+	u32 tag = device->entry.tag;
+
+	return add_uevent_var(env, "MODALIAS=coreboot:t%08X", tag);
+}
+
 static struct bus_type coreboot_bus_type = {
 	.name		= "coreboot",
 	.match		= coreboot_bus_match,
 	.probe		= coreboot_bus_probe,
 	.remove		= coreboot_bus_remove,
+	.uevent		= coreboot_bus_uevent,
 };
 
 static void coreboot_device_release(struct device *dev)
-- 
2.43.0

[PATCH 2/4] firmware: coreboot: Generate aliases for coreboot modules

[Nícolas F. R. A. Prado]

Generate aliases for coreboot modules to allow automatic module probing.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---

 include/linux/mod_devicetable.h   |  8 ++++++++
 scripts/mod/devicetable-offsets.c |  3 +++
 scripts/mod/file2alias.c          | 10 ++++++++++
 3 files changed, 21 insertions(+)

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index f458469c5ce5..24e0dcfde809 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -960,4 +960,12 @@ struct vchiq_device_id {
 	char name[32];
 };
 
+/**
+ * struct coreboot_device_id - Identifies a coreboot table entry
+ * @tag: tag ID
+ */
+struct coreboot_device_id {
+	__u32 tag;
+};
+
 #endif /* LINUX_MOD_DEVICETABLE_H */
diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
index e91a3c38143b..518200813d4e 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -274,5 +274,8 @@ int main(void)
 	DEVID(vchiq_device_id);
 	DEVID_FIELD(vchiq_device_id, name);
 
+	DEVID(coreboot_device_id);
+	DEVID_FIELD(coreboot_device_id, tag);
+
 	return 0;
 }
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 4829680a0a6d..5d1c61fa5a55 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1494,6 +1494,15 @@ static int do_vchiq_entry(const char *filename, void *symval, char *alias)
 	return 1;
 }
 
+/* Looks like: coreboot:tN */
+static int do_coreboot_entry(const char *filename, void *symval, char *alias)
+{
+	DEF_FIELD(symval, coreboot_device_id, tag);
+	sprintf(alias, "coreboot:t%08X", tag);
+
+	return 1;
+}
+
 /* Does namelen bytes of name exactly match the symbol? */
 static bool sym_is(const char *name, unsigned namelen, const char *symbol)
 {
@@ -1575,6 +1584,7 @@ static const struct devtable devtable[] = {
 	{"ishtp", SIZE_ishtp_device_id, do_ishtp_entry},
 	{"cdx", SIZE_cdx_device_id, do_cdx_entry},
 	{"vchiq", SIZE_vchiq_device_id, do_vchiq_entry},
+	{"coreboot", SIZE_coreboot_device_id, do_coreboot_entry},
 };
 
 /* Create MODULE_ALIAS() statements.
-- 
2.43.0

[PATCH 3/4] firmware: google: cbmem: Add to module device table

[Nícolas F. R. A. Prado]

Create an id table and add it to the module device table to allow the
cbmem driver to be automatically loaded when a matching device is found.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---

 drivers/firmware/google/cbmem.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/firmware/google/cbmem.c b/drivers/firmware/google/cbmem.c
index 88e587ba1e0d..ceb89b4cdbe0 100644
--- a/drivers/firmware/google/cbmem.c
+++ b/drivers/firmware/google/cbmem.c
@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/kobject.h>
 #include <linux/module.h>
+#include <linux/mod_devicetable.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
@@ -114,6 +115,12 @@ static int cbmem_entry_probe(struct coreboot_device *dev)
 	return 0;
 }
 
+static const struct coreboot_device_id cbmem_ids[] = {
+	{ .tag = LB_TAG_CBMEM_ENTRY },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(coreboot, cbmem_ids);
+
 static struct coreboot_driver cbmem_entry_driver = {
 	.probe = cbmem_entry_probe,
 	.drv = {
-- 
2.43.0

[PATCH 4/4] arm64: defconfig: Enable support for cbmem entries in the coreboot table

[Nícolas F. R. A. Prado]

Enable the cbmem driver and dependencies in order to support reading
cbmem entries from the coreboot table, which are used to store logs from
coreboot on arm64 Chromebooks, and provide useful information for
debugging the boot process on those devices.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

---

 arch/arm64/configs/defconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 0b0ef6877a12..cd94d55b23b2 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -255,6 +255,9 @@ CONFIG_INTEL_STRATIX10_RSU=m
 CONFIG_MTK_ADSP_IPC=m
 CONFIG_QCOM_QSEECOM=y
 CONFIG_QCOM_QSEECOM_UEFISECAPP=y
+CONFIG_GOOGLE_FIRMWARE=y
+CONFIG_GOOGLE_CBMEM=m
+CONFIG_GOOGLE_COREBOOT_TABLE=m
 CONFIG_EFI_CAPSULE_LOADER=y
 CONFIG_IMX_SCU=y
 CONFIG_IMX_SCU_PD=y
-- 
2.43.0

[PATCH 4/4] arm64: defconfig: Enable support for cbmem entries in the coreboot table

[Nícolas F. R. A. Prado]

Enable the cbmem driver and dependencies in order to support reading
cbmem entries from the coreboot table, which are used to store logs from
coreboot on arm64 Chromebooks, and provide useful information for
debugging the boot process on those devices.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

---

 arch/arm64/configs/defconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 0b0ef6877a12..cd94d55b23b2 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -255,6 +255,9 @@ CONFIG_INTEL_STRATIX10_RSU=m
 CONFIG_MTK_ADSP_IPC=m
 CONFIG_QCOM_QSEECOM=y
 CONFIG_QCOM_QSEECOM_UEFISECAPP=y
+CONFIG_GOOGLE_FIRMWARE=y
+CONFIG_GOOGLE_CBMEM=m
+CONFIG_GOOGLE_COREBOOT_TABLE=m
 CONFIG_EFI_CAPSULE_LOADER=y
 CONFIG_IMX_SCU=y
 CONFIG_IMX_SCU_PD=y
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/4] firmware: coreboot: Generate modalias uevent for devices

[Brian Norris]

It would have been nice to get the whole series, or at least direct to
all the same mailing lists -- specifically, chrome-platform@lists.linux.dev

But I found the whole thing eventually.

On Thu, Jan 11, 2024 at 12:11:46PM -0300, Nícolas F. R. A. Prado wrote:
> Generate a modalias uevent for devices in the coreboot bus to allow
> userspace to automatically load the corresponding modules.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Acked-by: Brian Norris <briannorris@chromium.org>

Re: [PATCH 3/4] firmware: google: cbmem: Add to module device table

[Brian Norris]

On Thu, Jan 11, 2024 at 12:11:48PM -0300, Nícolas F. R. A. Prado wrote:
> Create an id table and add it to the module device table to allow the
> cbmem driver to be automatically loaded when a matching device is found.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Might as well also patch framebuffer-coreboot.c and
memconsole-coreboot.c while you're at it?

But for this one:

Acked-by: Brian Norris <briannorris@chromium.org>

Re: [PATCH 1/4] firmware: coreboot: Generate modalias uevent for devices

[Nícolas F. R. A. Prado]

On Thu, Jan 11, 2024 at 04:37:10PM -0800, Brian Norris wrote:
> It would have been nice to get the whole series, or at least direct to
> all the same mailing lists -- specifically, chrome-platform@lists.linux.dev

Yeah, that's an artifact of using patman for patch submission... But I'll make
sure that list gets the whole series for v2.

Thanks,
Nícolas

Re: [PATCH 3/4] firmware: google: cbmem: Add to module device table

[Nícolas F. R. A. Prado]

On Thu, Jan 11, 2024 at 04:38:44PM -0800, Brian Norris wrote:
> On Thu, Jan 11, 2024 at 12:11:48PM -0300, Nícolas F. R. A. Prado wrote:
> > Create an id table and add it to the module device table to allow the
> > cbmem driver to be automatically loaded when a matching device is found.
> > 
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> 
> Might as well also patch framebuffer-coreboot.c and
> memconsole-coreboot.c while you're at it?

Yeah, no reason not to. Will do in v2.

Thanks,
Nícolas

Re: [PATCH 2/4] firmware: coreboot: Generate aliases for coreboot modules

[Andy Shevchenko]

On Thu, Jan 11, 2024 at 12:11:47PM -0300, Nícolas F. R. A. Prado wrote:
> Generate aliases for coreboot modules to allow automatic module probing.

...

> +/**
> + * struct coreboot_device_id - Identifies a coreboot table entry
> + * @tag: tag ID
> + */
> +struct coreboot_device_id {
> +	__u32 tag;
> +};

Don't you want to have a driver data or so associated with this?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] firmware: google: cbmem: Add to module device table

[kernel test robot]

Hi Nícolas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on next-20240111]
[also build test WARNING on v6.7]
[cannot apply to chrome-platform/for-next chrome-platform/for-firmware-next masahiroy-kbuild/for-next masahiroy-kbuild/fixes arm64/for-next/core linus/master v6.7 v6.7-rc8 v6.7-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/N-colas-F-R-A-Prado/firmware-coreboot-Generate-modalias-uevent-for-devices/20240111-231841
base:   next-20240111
patch link:    https://lore.kernel.org/r/20240111151226.842603-4-nfraprado%40collabora.com
patch subject: [PATCH 3/4] firmware: google: cbmem: Add to module device table
config: i386-randconfig-013-20240115 (https://download.01.org/0day-ci/archive/20240115/202401151013.Xioj5wZo-lkp@intel.com/config)
compiler: ClangBuiltLinux clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240115/202401151013.Xioj5wZo-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401151013.Xioj5wZo-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/firmware/google/cbmem.c:118:40: warning: unused variable 'cbmem_ids' [-Wunused-const-variable]
     118 | static const struct coreboot_device_id cbmem_ids[] = {
         |                                        ^~~~~~~~~
   1 warning generated.


vim +/cbmem_ids +118 drivers/firmware/google/cbmem.c

   117	
 > 118	static const struct coreboot_device_id cbmem_ids[] = {
   119		{ .tag = LB_TAG_CBMEM_ENTRY },
   120		{ /* sentinel */ }
   121	};
   122	MODULE_DEVICE_TABLE(coreboot, cbmem_ids);
   123	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 3/4] firmware: google: cbmem: Add to module device table

[Brian Norris]

Hi Nicolas,

On Mon, Jan 15, 2024 at 10:53:48AM +0800, kernel test robot wrote:
> All warnings (new ones prefixed by >>):
> 
> >> drivers/firmware/google/cbmem.c:118:40: warning: unused variable 'cbmem_ids' [-Wunused-const-variable]
>      118 | static const struct coreboot_device_id cbmem_ids[] = {
>          |                                        ^~~~~~~~~
>    1 warning generated.
> 
> 
> vim +/cbmem_ids +118 drivers/firmware/google/cbmem.c
> 
>    117	
>  > 118	static const struct coreboot_device_id cbmem_ids[] = {
>    119		{ .tag = LB_TAG_CBMEM_ENTRY },
>    120		{ /* sentinel */ }
>    121	};
>    122	MODULE_DEVICE_TABLE(coreboot, cbmem_ids);
>    123	

I was wondering why we have a seemingly unique "unused variable" failure
mode in comparison to other similarly-structured device/bus drivers, and
I realized that's because we're not relying on the same structure for
both MODULE_DEVICE_TABLE (struct coreboot_device_id) and for the driver
definition (struct coreboot_driver -> 'u32 tag'). Thus, this structure
is only used for #define MODULE builds, and otherwise not used.

Rather than wrapping these definitions in "#ifdef MODULE", perhaps we
can settle on a single field, and replace `struct coreboot_driver::tag`
with an instance of `struct coreboot_device_id`? That would normally be
a breaking change that would require changing all drivers at the same
time as the bus (or else some kind of intermediate transition state),
but considering there are only 4 driver implementations and they all
live under the same maintainer tree, that seems like it should still be
OK (IMO).

If it makes the series more readable/incremental, perhaps the switchover
can be the last patch in the series, and there can remain some
duplication (and potential -Wunused-const-variable issues) for the
middle of the series.

Brian

Re: [PATCH 2/4] firmware: coreboot: Generate aliases for coreboot modules

[Nícolas F. R. A. Prado]

On Sun, Jan 14, 2024 at 07:08:13PM +0200, Andy Shevchenko wrote:
> On Thu, Jan 11, 2024 at 12:11:47PM -0300, Nícolas F. R. A. Prado wrote:
> > Generate aliases for coreboot modules to allow automatic module probing.
> 
> ...
> 
> > +/**
> > + * struct coreboot_device_id - Identifies a coreboot table entry
> > + * @tag: tag ID
> > + */
> > +struct coreboot_device_id {
> > +	__u32 tag;
> > +};
> 
> Don't you want to have a driver data or so associated with this?

There's no need for it currently in any driver. This struct is being created
simply to allow auto modprobe. So it seems reasonable to leave it out and add it
later when/if needed.

Thanks,
Nícolas

Re: [PATCH 2/4] firmware: coreboot: Generate aliases for coreboot modules

[Andy Shevchenko]

On Wed, Jan 17, 2024 at 09:53:23AM -0300, Nícolas F. R. A. Prado wrote:
> On Sun, Jan 14, 2024 at 07:08:13PM +0200, Andy Shevchenko wrote:
> > On Thu, Jan 11, 2024 at 12:11:47PM -0300, Nícolas F. R. A. Prado wrote:
> > > Generate aliases for coreboot modules to allow automatic module probing.

...

> > > +/**
> > > + * struct coreboot_device_id - Identifies a coreboot table entry
> > > + * @tag: tag ID
> > > + */
> > > +struct coreboot_device_id {
> > > +	__u32 tag;
> > > +};
> > 
> > Don't you want to have a driver data or so associated with this?
> 
> There's no need for it currently in any driver. This struct is being created
> simply to allow auto modprobe. So it seems reasonable to leave it out and add it
> later when/if needed.

The problem is that you introduce a kinda ABI here, how do you handle this later?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] firmware: coreboot: Generate aliases for coreboot modules

[Nícolas F. R. A. Prado]

On Sun, Jan 21, 2024 at 02:41:29PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 17, 2024 at 09:53:23AM -0300, Nícolas F. R. A. Prado wrote:
> > On Sun, Jan 14, 2024 at 07:08:13PM +0200, Andy Shevchenko wrote:
> > > On Thu, Jan 11, 2024 at 12:11:47PM -0300, Nícolas F. R. A. Prado wrote:
> > > > Generate aliases for coreboot modules to allow automatic module probing.
> 
> ...
> 
> > > > +/**
> > > > + * struct coreboot_device_id - Identifies a coreboot table entry
> > > > + * @tag: tag ID
> > > > + */
> > > > +struct coreboot_device_id {
> > > > +	__u32 tag;
> > > > +};
> > > 
> > > Don't you want to have a driver data or so associated with this?
> > 
> > There's no need for it currently in any driver. This struct is being created
> > simply to allow auto modprobe. So it seems reasonable to leave it out and add it
> > later when/if needed.
> 
> The problem is that you introduce a kinda ABI here, how do you handle this later?

Sorry, but I don't follow. What ABI is there to guarantee stability for here?
This header is not exported to userspace (not under uapi/). Only kernel code
will make use of this struct and it can be updated whenever this struct is
changed without anything breaking.

Thanks,
Nícolas