[PATCH v2 0/3] vc04_services: Tweak module autoloading support

[PATCH v2 0/3] vc04_services: Tweak module autoloading support

[Umang Jain]

As explained by Greg KH in [1], we should not be using MODULE_ALIAS
to auto load modules within VC04. Drop that, in favour of
MODULE_DEVICE_TABLE. This series addresses the suggestion.

Patch 1/3 adds vchiq_bus_device_id and supporting entries in 
script/mod/file2alias.c

Patch 2/3 drops MODULE_ALIAS from bcm2835-audio in favour
of MODULE_DEVICE_TABLE.

Patch 3/3 drops MODULE_ALIAS from bcm2835-camera in favour
of MODULE_DEVICE_TABLE.

[1]:  https://lore.kernel.org/linux-media/2023100955-stunt-equate-c6fa@gregkh/

--
Changes in v2: 
- NULL terminate device_id_table arrays (in 2/3 and 3/3)

Umang Jain (3):
  staging: vc04_services: Support module autoloading using
    MODULE_DEVICE_TABLE
  staging: vc04_services: bcm2835-audio: Drop MODULE_ALIAS
  staging: vc04_services: bcm2835-camera: Drop MODULE_ALIAS

 drivers/staging/vc04_services/bcm2835-audio/bcm2835.c    | 8 +++++++-
 .../vc04_services/bcm2835-camera/bcm2835-camera.c        | 8 +++++++-
 .../vc04_services/interface/vchiq_arm/vchiq_bus.c        | 2 +-
 .../vc04_services/interface/vchiq_arm/vchiq_bus.h        | 3 +++
 include/linux/mod_devicetable.h                          | 4 ++++
 scripts/mod/devicetable-offsets.c                        | 3 +++
 scripts/mod/file2alias.c                                 | 9 +++++++++
 7 files changed, 34 insertions(+), 3 deletions(-)


base-commit: a2fc3e8215313c8e4e42d4b2062830aaf1ef49c0
-- 
2.40.1

[PATCH v2 1/3] staging: vc04_services: Support module autoloading using MODULE_DEVICE_TABLE

[Umang Jain]

VC04 has now a independent bus vchiq_bus to register its devices.
However, the module auto-loading for bcm2835-audio and bcm2835-camera
currently happens through MODULE_ALIAS() macro specified explicitly.

The correct way to auto-load a module, is when the alias is picked
out from MODULE_DEVICE_TABLE(). In order to get there, we need to
introduce  vchiq_device_id and add relevant entries in file2alias.c
infrastructure so that aliases can be generated. This patch targets
adding vchiq_bus_device_id and do_vchiq_bus_entry, in order to
generate those alias using the /script/mod/file2alias.c.

Going forward the MODULE_ALIAS() from bcm2835-camera and bcm2835-audio
will be dropped, in favour of MODULE_DEVICE_TABLE being used there.

The alias format for vchiq_bus devices will be "vchiq_bus:<dev_name>".
Adjust the vchiq_bus_uevent() to reflect that.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../vc04_services/interface/vchiq_arm/vchiq_bus.c        | 2 +-
 .../vc04_services/interface/vchiq_arm/vchiq_bus.h        | 3 +++
 include/linux/mod_devicetable.h                          | 4 ++++
 scripts/mod/devicetable-offsets.c                        | 3 +++
 scripts/mod/file2alias.c                                 | 9 +++++++++
 5 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
index 4ac3491efe45..b4915bd76387 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
@@ -26,7 +26,7 @@ static int vchiq_bus_uevent(const struct device *dev, struct kobj_uevent_env *en
 {
 	const struct vchiq_device *device = container_of_const(dev, struct vchiq_device, dev);
 
-	return add_uevent_var(env, "MODALIAS=%s", dev_name(&device->dev));
+	return add_uevent_var(env, "MODALIAS=vchiq_bus:%s", dev_name(&device->dev));
 }
 
 static int vchiq_bus_probe(struct device *dev)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.h
index 7eaaf9a91cda..6e1fbee4b078 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.h
@@ -7,6 +7,7 @@
 #define _VCHIQ_DEVICE_H
 
 #include <linux/device.h>
+#include <linux/mod_devicetable.h>
 
 struct vchiq_device {
 	struct device dev;
@@ -18,6 +19,8 @@ struct vchiq_driver {
 	int		(*resume)(struct vchiq_device *device);
 	int		(*suspend)(struct vchiq_device *device,
 				   pm_message_t state);
+
+	const struct vchiq_bus_device_id *id_table;
 	struct device_driver driver;
 };
 
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index b0678b093cb2..f01e175f7f3d 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -946,4 +946,8 @@ struct cdx_device_id {
 	__u32 override_only;
 };
 
+struct vchiq_bus_device_id {
+	char name[32];
+};
+
 #endif /* LINUX_MOD_DEVICETABLE_H */
diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
index abe65f8968dd..c9a02d2c4f9e 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -267,5 +267,8 @@ int main(void)
 	DEVID_FIELD(cdx_device_id, device);
 	DEVID_FIELD(cdx_device_id, override_only);
 
+	DEVID(vchiq_bus_device_id);
+	DEVID_FIELD(vchiq_bus_device_id, name);
+
 	return 0;
 }
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 38120f932b0d..6908dc70b649 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1478,6 +1478,14 @@ static int do_cdx_entry(const char *filename, void *symval,
 	return 1;
 }
 
+static int do_vchiq_bus_entry(const char *filename, void *symval, char *alias)
+{
+	DEF_FIELD_ADDR(symval, vchiq_bus_device_id, name);
+	sprintf(alias, "vchiq_bus:%s", *name);
+
+	return 1;
+}
+
 /* Does namelen bytes of name exactly match the symbol? */
 static bool sym_is(const char *name, unsigned namelen, const char *symbol)
 {
@@ -1558,6 +1566,7 @@ static const struct devtable devtable[] = {
 	{"dfl", SIZE_dfl_device_id, do_dfl_entry},
 	{"ishtp", SIZE_ishtp_device_id, do_ishtp_entry},
 	{"cdx", SIZE_cdx_device_id, do_cdx_entry},
+	{"vchiq_bus", SIZE_vchiq_bus_device_id, do_vchiq_bus_entry},
 };
 
 /* Create MODULE_ALIAS() statements.
-- 
2.40.1

[PATCH v2 2/3] staging: vc04_services: bcm2835-audio: Drop MODULE_ALIAS

[Umang Jain]

Drop MODULE_ALIAS in favour of MODULE_DEVICE_TABLE as the module
alias should be dropped from there.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
index 06bdc7673d4b..d5bd8a36a010 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
@@ -311,12 +311,19 @@ static int snd_bcm2835_alsa_resume(struct vchiq_device *device)
 
 #endif
 
+static struct vchiq_bus_device_id device_id_table[] = {
+	{ .name = "bcm2835-audio" },
+	{}
+};
+MODULE_DEVICE_TABLE(vchiq_bus, device_id_table);
+
 static struct vchiq_driver bcm2835_alsa_driver = {
 	.probe = snd_bcm2835_alsa_probe,
 #ifdef CONFIG_PM
 	.suspend = snd_bcm2835_alsa_suspend,
 	.resume = snd_bcm2835_alsa_resume,
 #endif
+	.id_table = device_id_table,
 	.driver = {
 		.name = "bcm2835-audio",
 	},
@@ -326,4 +333,3 @@ module_vchiq_driver(bcm2835_alsa_driver);
 MODULE_AUTHOR("Dom Cobley");
 MODULE_DESCRIPTION("Alsa driver for BCM2835 chip");
 MODULE_LICENSE("GPL");
-MODULE_ALIAS("bcm2835-audio");
-- 
2.40.1

[PATCH v2 3/3] staging: vc04_services: bcm2835-camera: Drop MODULE_ALIAS

[Umang Jain]

Drop MODULE_ALIAS in favour of MODULE_DEVICE_TABLE as the module
alias should be picked from there.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../staging/vc04_services/bcm2835-camera/bcm2835-camera.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
index c873eace1437..44901aab637b 100644
--- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
+++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
@@ -1995,9 +1995,16 @@ static void bcm2835_mmal_remove(struct vchiq_device *device)
 	vchiq_mmal_finalise(instance);
 }
 
+static const struct vchiq_bus_device_id id_table[] = {
+	{ .name = "bcm2835-camera" },
+	{}
+};
+MODULE_DEVICE_TABLE(vchiq_bus, id_table);
+
 static struct vchiq_driver bcm2835_camera_driver = {
 	.probe		= bcm2835_mmal_probe,
 	.remove		= bcm2835_mmal_remove,
+	.id_table	= id_table,
 	.driver		= {
 		.name	= "bcm2835-camera",
 	},
@@ -2008,4 +2015,3 @@ module_vchiq_driver(bcm2835_camera_driver)
 MODULE_DESCRIPTION("Broadcom 2835 MMAL video capture");
 MODULE_AUTHOR("Vincent Sanders");
 MODULE_LICENSE("GPL");
-MODULE_ALIAS("bcm2835-camera");
-- 
2.40.1

Re: [PATCH v2 1/3] staging: vc04_services: Support module autoloading using MODULE_DEVICE_TABLE

[Laurent Pinchart]

On Wed, Oct 18, 2023 at 11:12:12AM +0530, Umang Jain wrote:
> VC04 has now a independent bus vchiq_bus to register its devices.
> However, the module auto-loading for bcm2835-audio and bcm2835-camera
> currently happens through MODULE_ALIAS() macro specified explicitly.
> 
> The correct way to auto-load a module, is when the alias is picked
> out from MODULE_DEVICE_TABLE(). In order to get there, we need to
> introduce  vchiq_device_id and add relevant entries in file2alias.c
> infrastructure so that aliases can be generated. This patch targets
> adding vchiq_bus_device_id and do_vchiq_bus_entry, in order to
> generate those alias using the /script/mod/file2alias.c.
> 
> Going forward the MODULE_ALIAS() from bcm2835-camera and bcm2835-audio
> will be dropped, in favour of MODULE_DEVICE_TABLE being used there.
> 
> The alias format for vchiq_bus devices will be "vchiq_bus:<dev_name>".
> Adjust the vchiq_bus_uevent() to reflect that.

None of the other buses have a "_bus" suffix in the alias or in the
*_device_id structure name. Is there a reason to make an exception here?

> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  .../vc04_services/interface/vchiq_arm/vchiq_bus.c        | 2 +-
>  .../vc04_services/interface/vchiq_arm/vchiq_bus.h        | 3 +++
>  include/linux/mod_devicetable.h                          | 4 ++++
>  scripts/mod/devicetable-offsets.c                        | 3 +++
>  scripts/mod/file2alias.c                                 | 9 +++++++++
>  5 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
> index 4ac3491efe45..b4915bd76387 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c
> @@ -26,7 +26,7 @@ static int vchiq_bus_uevent(const struct device *dev, struct kobj_uevent_env *en
>  {
>  	const struct vchiq_device *device = container_of_const(dev, struct vchiq_device, dev);
>  
> -	return add_uevent_var(env, "MODALIAS=%s", dev_name(&device->dev));
> +	return add_uevent_var(env, "MODALIAS=vchiq_bus:%s", dev_name(&device->dev));
>  }
>  
>  static int vchiq_bus_probe(struct device *dev)
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.h
> index 7eaaf9a91cda..6e1fbee4b078 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.h
> @@ -7,6 +7,7 @@
>  #define _VCHIQ_DEVICE_H
>  
>  #include <linux/device.h>
> +#include <linux/mod_devicetable.h>
>  
>  struct vchiq_device {
>  	struct device dev;
> @@ -18,6 +19,8 @@ struct vchiq_driver {
>  	int		(*resume)(struct vchiq_device *device);
>  	int		(*suspend)(struct vchiq_device *device,
>  				   pm_message_t state);
> +
> +	const struct vchiq_bus_device_id *id_table;
>  	struct device_driver driver;
>  };
>  
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index b0678b093cb2..f01e175f7f3d 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -946,4 +946,8 @@ struct cdx_device_id {
>  	__u32 override_only;
>  };
>  
> +struct vchiq_bus_device_id {
> +	char name[32];
> +};
> +
>  #endif /* LINUX_MOD_DEVICETABLE_H */
> diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
> index abe65f8968dd..c9a02d2c4f9e 100644
> --- a/scripts/mod/devicetable-offsets.c
> +++ b/scripts/mod/devicetable-offsets.c
> @@ -267,5 +267,8 @@ int main(void)
>  	DEVID_FIELD(cdx_device_id, device);
>  	DEVID_FIELD(cdx_device_id, override_only);
>  
> +	DEVID(vchiq_bus_device_id);
> +	DEVID_FIELD(vchiq_bus_device_id, name);
> +
>  	return 0;
>  }
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index 38120f932b0d..6908dc70b649 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -1478,6 +1478,14 @@ static int do_cdx_entry(const char *filename, void *symval,
>  	return 1;
>  }
>  
> +static int do_vchiq_bus_entry(const char *filename, void *symval, char *alias)
> +{
> +	DEF_FIELD_ADDR(symval, vchiq_bus_device_id, name);
> +	sprintf(alias, "vchiq_bus:%s", *name);
> +
> +	return 1;
> +}
> +
>  /* Does namelen bytes of name exactly match the symbol? */
>  static bool sym_is(const char *name, unsigned namelen, const char *symbol)
>  {
> @@ -1558,6 +1566,7 @@ static const struct devtable devtable[] = {
>  	{"dfl", SIZE_dfl_device_id, do_dfl_entry},
>  	{"ishtp", SIZE_ishtp_device_id, do_ishtp_entry},
>  	{"cdx", SIZE_cdx_device_id, do_cdx_entry},
> +	{"vchiq_bus", SIZE_vchiq_bus_device_id, do_vchiq_bus_entry},
>  };
>  
>  /* Create MODULE_ALIAS() statements.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] staging: vc04_services: bcm2835-audio: Drop MODULE_ALIAS

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Wed, Oct 18, 2023 at 11:12:13AM +0530, Umang Jain wrote:
> Drop MODULE_ALIAS in favour of MODULE_DEVICE_TABLE as the module
> alias should be dropped from there.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  drivers/staging/vc04_services/bcm2835-audio/bcm2835.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
> index 06bdc7673d4b..d5bd8a36a010 100644
> --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
> +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c
> @@ -311,12 +311,19 @@ static int snd_bcm2835_alsa_resume(struct vchiq_device *device)
>  
>  #endif
>  
> +static struct vchiq_bus_device_id device_id_table[] = {
> +	{ .name = "bcm2835-audio" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(vchiq_bus, device_id_table);

With the "_bus" suffix dropped (see review of 1/3),

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>  static struct vchiq_driver bcm2835_alsa_driver = {
>  	.probe = snd_bcm2835_alsa_probe,
>  #ifdef CONFIG_PM
>  	.suspend = snd_bcm2835_alsa_suspend,
>  	.resume = snd_bcm2835_alsa_resume,
>  #endif
> +	.id_table = device_id_table,
>  	.driver = {
>  		.name = "bcm2835-audio",
>  	},
> @@ -326,4 +333,3 @@ module_vchiq_driver(bcm2835_alsa_driver);
>  MODULE_AUTHOR("Dom Cobley");
>  MODULE_DESCRIPTION("Alsa driver for BCM2835 chip");
>  MODULE_LICENSE("GPL");
> -MODULE_ALIAS("bcm2835-audio");

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/3] staging: vc04_services: bcm2835-camera: Drop MODULE_ALIAS

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Wed, Oct 18, 2023 at 11:12:14AM +0530, Umang Jain wrote:
> Drop MODULE_ALIAS in favour of MODULE_DEVICE_TABLE as the module
> alias should be picked from there.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  .../staging/vc04_services/bcm2835-camera/bcm2835-camera.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
> index c873eace1437..44901aab637b 100644
> --- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
> +++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
> @@ -1995,9 +1995,16 @@ static void bcm2835_mmal_remove(struct vchiq_device *device)
>  	vchiq_mmal_finalise(instance);
>  }
>  
> +static const struct vchiq_bus_device_id id_table[] = {
> +	{ .name = "bcm2835-camera" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(vchiq_bus, id_table);

With the "_bus" suffix dropped (see review of 1/3),

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
>  static struct vchiq_driver bcm2835_camera_driver = {
>  	.probe		= bcm2835_mmal_probe,
>  	.remove		= bcm2835_mmal_remove,
> +	.id_table	= id_table,
>  	.driver		= {
>  		.name	= "bcm2835-camera",
>  	},
> @@ -2008,4 +2015,3 @@ module_vchiq_driver(bcm2835_camera_driver)
>  MODULE_DESCRIPTION("Broadcom 2835 MMAL video capture");
>  MODULE_AUTHOR("Vincent Sanders");
>  MODULE_LICENSE("GPL");
> -MODULE_ALIAS("bcm2835-camera");

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/3] staging: vc04_services: Support module autoloading using MODULE_DEVICE_TABLE

[Greg Kroah-Hartman]

On Wed, Oct 18, 2023 at 03:38:56PM +0300, Laurent Pinchart wrote:
> On Wed, Oct 18, 2023 at 11:12:12AM +0530, Umang Jain wrote:
> > VC04 has now a independent bus vchiq_bus to register its devices.
> > However, the module auto-loading for bcm2835-audio and bcm2835-camera
> > currently happens through MODULE_ALIAS() macro specified explicitly.
> > 
> > The correct way to auto-load a module, is when the alias is picked
> > out from MODULE_DEVICE_TABLE(). In order to get there, we need to
> > introduce  vchiq_device_id and add relevant entries in file2alias.c
> > infrastructure so that aliases can be generated. This patch targets
> > adding vchiq_bus_device_id and do_vchiq_bus_entry, in order to
> > generate those alias using the /script/mod/file2alias.c.
> > 
> > Going forward the MODULE_ALIAS() from bcm2835-camera and bcm2835-audio
> > will be dropped, in favour of MODULE_DEVICE_TABLE being used there.
> > 
> > The alias format for vchiq_bus devices will be "vchiq_bus:<dev_name>".
> > Adjust the vchiq_bus_uevent() to reflect that.
> 
> None of the other buses have a "_bus" suffix in the alias or in the
> *_device_id structure name. Is there a reason to make an exception here?

No, it should be dropped, good catch.

thanks,

greg k-h