Re: [PATCH v7 1/6] firmware: smccc: Add an Arm SMCCC bus

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

Sudeep Holla <sudeep.holla@kernel.org> writes:

> On Thu, Jun 11, 2026 at 06:34:24PM +0530, Aneesh Kumar K.V (Arm) wrote:

...

>> +static int arm_smccc_bus_match(struct device *dev,
>> +		const struct device_driver *drv)
>> +{
>> +	const struct arm_smccc_device_id *id_table;
>> +	struct arm_smccc_device *smccc_dev = to_arm_smccc_device(dev);
>> +
>> +	id_table = to_arm_smccc_driver(drv)->id_table;
>> +	if (!id_table)
>> +		return 0;
>> +
>
> Any reason for not having this check when registering the driver also so
> that it becomes a must to register ? I remember vaguely adding this as fix to
> FF-A driver recently.
>

I will add

int arm_smccc_driver_register(struct arm_smccc_driver *driver,
		struct module *owner, const char *mod_name)
{
	if (!driver->probe || !driver->id_table)
		return -EINVAL;


>
>> +	while (id_table->name[0]) {
>> +		if (!strcmp(smccc_dev->name, id_table->name))
>> +			return 1;
>> +		id_table++;
>> +	}
>> +
>> +	return 0;
>> +}
>> +

...

>> +struct arm_smccc_device *arm_smccc_device_register(const char *name)
>> +{
>> +	struct arm_smccc_device *smccc_dev;
>> +	int id, ret;
>> +
>> +	id = ida_alloc_min(&arm_smccc_bus_id, 1, GFP_KERNEL);
>> +	if (id < 0)
>> +		return ERR_PTR(id);
>> +
>> +	smccc_dev = kzalloc_obj(*smccc_dev);
>> +	if (!smccc_dev) {
>> +		ida_free(&arm_smccc_bus_id, id);
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>> +
>> +	smccc_dev->id = id;
>> +	if (strscpy(smccc_dev->name, name) < 0) {
>
> Since this is exported symbol and one can call arm_smccc_device_register(ptr)
> where ptr = NULL, won't it blow up then ? IIUC strscpy() handles NULL dst
> but doesn't check for NULL src.
>

Will add the NULL check.

>> +		kfree(smccc_dev);
>> +		ida_free(&arm_smccc_bus_id, id);
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +	smccc_dev->dev.bus = &arm_smccc_bus_type;
>> +	smccc_dev->dev.release = arm_smccc_release_device;
>> +
>> +	ret = dev_set_name(&smccc_dev->dev, "%s-%d", smccc_dev->name, id);
>> +	if (ret) {
>> +		kfree(smccc_dev);
>> +		ida_free(&arm_smccc_bus_id, id);
>> +		return ERR_PTR(ret);
>> +	}
>> +
>> +	ret = device_register(&smccc_dev->dev);
>> +	if (ret) {
>> +		put_device(&smccc_dev->dev);
>> +		return ERR_PTR(ret);
>> +	}
>> +
>> +	return smccc_dev;
>> +}
>> +EXPORT_SYMBOL_GPL(arm_smccc_device_register);

...

>> +#define to_arm_smccc_driver(d) \
>> +	container_of_const(d, struct arm_smccc_driver, driver)
>> +
>> +int arm_smccc_driver_register(struct arm_smccc_driver *driver,
>> +		struct module *owner, const char *mod_name);
>> +void arm_smccc_driver_unregister(struct arm_smccc_driver *driver);
>> +struct arm_smccc_device *arm_smccc_device_register(const char *name);
>> +void arm_smccc_device_unregister(struct arm_smccc_device *smcc_dev);
>> +
>
> I may be overthinking but what will happen if HAVE_ARM_SMCCC_DISCOVERY=n
> and some driver is compiled using this header ? It should be fine if it
> fails to compile, just thinking out loud if we need to handle that are not.
> As long as all the drivers using these depends on HAVE_ARM_SMCCC_DISCOVERY
> it should be fine I think.

This will result in a build failure. The driver should either select
HAVE_ARM_SMCCC_DISCOVERY or depend on HAVE_ARM_SMCCC_DISCOVERY.

>
>> +#define arm_smccc_register(driver) \
>> +	arm_smccc_driver_register(driver, THIS_MODULE, KBUILD_MODNAME)
>> +#define arm_smccc_unregister(driver) \
>> +	arm_smccc_driver_unregister(driver)
>> +
>> +#define module_arm_smccc_driver(__arm_smccc_driver) \
>> +	module_driver(__arm_smccc_driver, arm_smccc_register, \
>> +		      arm_smccc_unregister)
>> +
>> +extern const struct bus_type arm_smccc_bus_type;
>> +
>> +#endif /* __LINUX_ARM_SMCCC_BUS_H */
>> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
>> index 23ff24080dfd..c9cee8c5a0b2 100644
>> --- a/include/linux/mod_devicetable.h
>> +++ b/include/linux/mod_devicetable.h
>
> This file seems to be reworked recently, so you need to rebase it moving
> smccc specific changes to separate file I think.
>

Do we need to add a separate file? I was able to rebase the series on
7.2-rc1

modified   include/linux/mod_devicetable.h
@@ -879,6 +879,19 @@ struct auxiliary_device_id {
 	kernel_ulong_t driver_data;
 };
 
+#define ARM_SMCCC_NAME_SIZE 40
+#define ARM_SMCCC_MODULE_PREFIX "arm_smccc:"
+
+/**
+ * struct arm_smccc_device_id - Arm SMCCC bus device identifier
+ * @name: SMCCC device name
+ * @driver_data: driver data
+ */
+struct arm_smccc_device_id {
+	char name[ARM_SMCCC_NAME_SIZE];
+	kernel_ulong_t driver_data;
+};
+
 /* Surface System Aggregator Module */
 
 #define SSAM_MATCH_TARGET	0x1


>> @@ -876,6 +876,19 @@ struct auxiliary_device_id {
>>  	kernel_ulong_t driver_data;
>>  };
>>  
>> +#define ARM_SMCCC_NAME_SIZE 40
>> +#define ARM_SMCCC_MODULE_PREFIX "arm_smccc:"
>> +
>> +/**
>> + * struct arm_smccc_device_id - Arm SMCCC bus device identifier
>> + * @name: SMCCC device name
>> + * @driver_data: driver data
>> + */
>> +struct arm_smccc_device_id {
>> +	char name[ARM_SMCCC_NAME_SIZE];
>> +	kernel_ulong_t driver_data;
>
> Can't find any users of the above driver_data ?
> Is it for future ? If so, can you add details on how it is supposed to be used
> if you don't want to drop it.
>

That would depend on the driver, wouldn’t it? I was comparing this with
struct auxiliary_device_id, and very few drivers use driver_data;
mei_gsc_probe() is one example.

I am not sure there is a generic rule here. IIUC, it provides a place
where a driver can add driver-specific data that can be used during the
probe routine.

>> +};
>> +
>>  /* Surface System Aggregator Module */
>>  
>>  #define SSAM_MATCH_TARGET	0x1
>> diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
>> index b4178c42d08f..a485011ff137 100644
>> --- a/scripts/mod/devicetable-offsets.c
>> +++ b/scripts/mod/devicetable-offsets.c
>> @@ -254,6 +254,9 @@ int main(void)
>>  	DEVID(auxiliary_device_id);
>>  	DEVID_FIELD(auxiliary_device_id, name);
>>  
>> +	DEVID(arm_smccc_device_id);
>> +	DEVID_FIELD(arm_smccc_device_id, name);
>> +
>>  	DEVID(ssam_device_id);
>>  	DEVID_FIELD(ssam_device_id, match_flags);
>>  	DEVID_FIELD(ssam_device_id, domain);

-aneesh