diff mbox series

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

Message ID 20240111151226.842603-3-nfraprado@collabora.com
State New
Headers
Series Allow coreboot modules to autoload and enable cbmem in the arm64 defconfig |

Commit Message

Nícolas F. R. A. Prado Jan. 11, 2024, 3:11 p.m. UTC 
  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(+)

Comments

Andy Shevchenko Jan. 14, 2024, 5:08 p.m. UTC | #1 
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?
Nícolas F. R. A. Prado Jan. 17, 2024, 12:53 p.m. UTC | #2 
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
Andy Shevchenko Jan. 21, 2024, 12:41 p.m. UTC | #3 
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?
Nícolas F. R. A. Prado Jan. 22, 2024, 6:24 p.m. UTC | #4 
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
diff mbox series

Patch

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.