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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Generate aliases for coreboot modules to allow automatic module probing.
Signed-off-by: Nícolas F. R. A. Prado <[email protected]>
---
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
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 <[email protected]>
---
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
It would have been nice to get the whole series, or at least direct to
all the same mailing lists -- specifically, [email protected]
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 <[email protected]>
Acked-by: Brian Norris <[email protected]>
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 <[email protected]>
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 <[email protected]>
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, [email protected]
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
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 <[email protected]>
>
> 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
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
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/[email protected]/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/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
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
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
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
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