Re: [PATCH 1/2] module: add SCMI device table alias support

[Hans de Goede <johannes.goede@oss.qualcomm.com>]

Hi,

On 16-Jun-26 20:09, Bjorn Andersson wrote:
> SCMI client drivers already describe their bus match data with
> MODULE_DEVICE_TABLE(scmi, ...), but modpost does not know how to consume
> SCMI device tables. As a result, SCMI modules do not get generated module
> aliases from their id tables.
> 
> Move struct scmi_device_id to mod_devicetable.h so it has a fixed layout
> visible to modpost, add the corresponding generated offsets and teach
> file2alias to emit scmi:<protocol>:<name> aliases.
> 
> Use the same stable alias format for SCMI device uevents and sysfs
> modaliases. The previous string included the instance-specific device
> name, which is not useful for matching modules.
> 
> Assisted-by: Codex:GPT-5.5
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>

Thank you for this. One small nit, you add:

#include <linux/mod_devicetable.h>

to include/linux/scmi_protocol.h

But that header only declares pointers to struct scmi_device_id.
so you can just forward declare the struct type there and
then only include linux/mod_devicetable.h in places which actually
need it, rather then dragging all of linux/mod_devicetable.h
into any file which includes linux/scmi_protocol.h .

Some people are working on untangling the kernel headers for
faster compile times. So IMHO it would be good to not introduce
new cases of headers unnecessary including other headers.

Either way the patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans





> ---
>  drivers/firmware/arm_scmi/bus.c   | 19 +++++++++----------
>  include/linux/mod_devicetable.h   | 11 +++++++++++
>  include/linux/scmi_protocol.h     |  6 +-----
>  scripts/mod/devicetable-offsets.c |  4 ++++
>  scripts/mod/file2alias.c          | 11 +++++++++++
>  5 files changed, 36 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> index 793be9eabaed..7e344f2ee18d 100644
> --- a/drivers/firmware/arm_scmi/bus.c
> +++ b/drivers/firmware/arm_scmi/bus.c
> @@ -13,11 +13,12 @@
>  #include <linux/of.h>
>  #include <linux/kernel.h>
>  #include <linux/slab.h>
> +#include <linux/string.h>
>  #include <linux/device.h>
>  
>  #include "common.h"
>  
> -#define SCMI_UEVENT_MODALIAS_FMT	"%s:%02x:%s"
> +#define SCMI_UEVENT_MODALIAS_FMT	SCMI_MODULE_PREFIX "%02x:%s"
>  
>  BLOCKING_NOTIFIER_HEAD(scmi_requested_devices_nh);
>  EXPORT_SYMBOL_GPL(scmi_requested_devices_nh);
> @@ -141,7 +142,7 @@ static int scmi_protocol_table_register(const struct scmi_device_id *id_table)
>  	int ret = 0;
>  	const struct scmi_device_id *entry;
>  
> -	for (entry = id_table; entry->name && ret == 0; entry++)
> +	for (entry = id_table; entry->name[0] && ret == 0; entry++)
>  		ret = scmi_protocol_device_request(entry);
>  
>  	return ret;
> @@ -197,18 +198,18 @@ scmi_protocol_table_unregister(const struct scmi_device_id *id_table)
>  {
>  	const struct scmi_device_id *entry;
>  
> -	for (entry = id_table; entry->name; entry++)
> +	for (entry = id_table; entry->name[0]; entry++)
>  		scmi_protocol_device_unrequest(entry);
>  }
>  
>  static int scmi_dev_match_by_id_table(struct scmi_device *scmi_dev,
>  				      const struct scmi_device_id *id_table)
>  {
> -	if (!id_table || !id_table->name)
> +	if (!id_table || !id_table->name[0])
>  		return 0;
>  
>  	/* Always skip transport devices from matching */
> -	for (; id_table->protocol_id && id_table->name; id_table++)
> +	for (; id_table->protocol_id && id_table->name[0]; id_table++)
>  		if (id_table->protocol_id == scmi_dev->protocol_id &&
>  		    strncmp(scmi_dev->name, "__scmi_transport_device", 23) &&
>  		    !strcmp(id_table->name, scmi_dev->name))
> @@ -245,7 +246,7 @@ static struct scmi_device *scmi_child_dev_find(struct device *parent,
>  	struct device *dev;
>  
>  	id_table[0].protocol_id = prot_id;
> -	id_table[0].name = name;
> +	strscpy(id_table[0].name, name, sizeof(id_table[0].name));
>  
>  	dev = device_find_child(parent, &id_table, scmi_match_by_id_table);
>  	if (!dev)
> @@ -282,8 +283,7 @@ static int scmi_device_uevent(const struct device *dev, struct kobj_uevent_env *
>  	const struct scmi_device *scmi_dev = to_scmi_dev(dev);
>  
>  	return add_uevent_var(env, "MODALIAS=" SCMI_UEVENT_MODALIAS_FMT,
> -			      dev_name(&scmi_dev->dev), scmi_dev->protocol_id,
> -			      scmi_dev->name);
> +			      scmi_dev->protocol_id, scmi_dev->name);
>  }
>  
>  static ssize_t modalias_show(struct device *dev,
> @@ -292,8 +292,7 @@ static ssize_t modalias_show(struct device *dev,
>  	struct scmi_device *scmi_dev = to_scmi_dev(dev);
>  
>  	return sysfs_emit(buf, SCMI_UEVENT_MODALIAS_FMT,
> -			  dev_name(&scmi_dev->dev), scmi_dev->protocol_id,
> -			  scmi_dev->name);
> +			  scmi_dev->protocol_id, scmi_dev->name);
>  }
>  static DEVICE_ATTR_RO(modalias);
>  
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 3b0c9a251a2e..769382f2eadd 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -473,6 +473,17 @@ struct rpmsg_device_id {
>  	kernel_ulong_t driver_data;
>  };
>  
> +/* scmi */
> +
> +#define SCMI_NAME_SIZE		32
> +#define SCMI_MODULE_PREFIX	"scmi:"
> +
> +struct scmi_device_id {
> +	__u8 protocol_id;
> +	char name[SCMI_NAME_SIZE];
> +	kernel_ulong_t driver_data;
> +};
> +
>  /* i2c */
>  
>  #define I2C_NAME_SIZE	20
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index 5ab73b1ab9aa..48b346a26068 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -10,6 +10,7 @@
>  
>  #include <linux/bitfield.h>
>  #include <linux/device.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/notifier.h>
>  #include <linux/types.h>
>  
> @@ -951,11 +952,6 @@ struct scmi_device {
>  
>  #define to_scmi_dev(d) container_of_const(d, struct scmi_device, dev)
>  
> -struct scmi_device_id {
> -	u8 protocol_id;
> -	const char *name;
> -};
> -
>  struct scmi_driver {
>  	const char *name;
>  	int (*probe)(struct scmi_device *sdev);
> diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
> index b4178c42d08f..da5bd712c8da 100644
> --- a/scripts/mod/devicetable-offsets.c
> +++ b/scripts/mod/devicetable-offsets.c
> @@ -144,6 +144,10 @@ int main(void)
>  	DEVID(rpmsg_device_id);
>  	DEVID_FIELD(rpmsg_device_id, name);
>  
> +	DEVID(scmi_device_id);
> +	DEVID_FIELD(scmi_device_id, protocol_id);
> +	DEVID_FIELD(scmi_device_id, name);
> +
>  	DEVID(i2c_device_id);
>  	DEVID_FIELD(i2c_device_id, name);
>  
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index 8d36c74dec2d..a5283f4c8e6f 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -852,6 +852,16 @@ static void do_rpmsg_entry(struct module *mod, void *symval)
>  	module_alias_printf(mod, false, RPMSG_DEVICE_MODALIAS_FMT, *name);
>  }
>  
> +/* Looks like: scmi:NN:S */
> +static void do_scmi_entry(struct module *mod, void *symval)
> +{
> +	DEF_FIELD(symval, scmi_device_id, protocol_id);
> +	DEF_FIELD_ADDR(symval, scmi_device_id, name);
> +
> +	module_alias_printf(mod, false, SCMI_MODULE_PREFIX "%02x:%s",
> +			    protocol_id, *name);
> +}
> +
>  /* Looks like: i2c:S */
>  static void do_i2c_entry(struct module *mod, void *symval)
>  {
> @@ -1491,6 +1501,7 @@ static const struct devtable devtable[] = {
>  	{"virtio", SIZE_virtio_device_id, do_virtio_entry},
>  	{"vmbus", SIZE_hv_vmbus_device_id, do_vmbus_entry},
>  	{"rpmsg", SIZE_rpmsg_device_id, do_rpmsg_entry},
> +	{"scmi", SIZE_scmi_device_id, do_scmi_entry},
>  	{"i2c", SIZE_i2c_device_id, do_i2c_entry},
>  	{"i3c", SIZE_i3c_device_id, do_i3c_entry},
>  	{"slim", SIZE_slim_device_id, do_slim_entry},
>