Re: [PATCH v11 1/7] firmware: smccc: Add an Arm SMCCC bus

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

On Mon, 14 Sep 2026 11:35:05 +0530
"Aneesh Kumar K.V (Arm)" <aneesh.kumar@kernel.org> wrote:

> SMCCC-discovered firmware services are currently represented by separate
> platform devices, such as smccc_trng and arm-cca-dev. Those devices do not
> represent independent DT/ACPI-described platform resources; they are
> features of the SMCCC firmware interface.
> 
> Add an Arm SMCCC bus for services discovered through the SMCCC firmware
> interface. The bus provides SMCCC device and driver registration helpers,
> name-based matching, modalias generation, and a sysfs modalias attribute so
> SMCCC service drivers can bind to discovered firmware services and autoload
> as modules.
> 
> Follow-up changes can then register SMCCC firmware services as arm-smccc
> devices instead of creating independent per-feature platform devices.
> 
> Based on arm_ffa code

That code has the same issue as below and for bonus points doesn't check
if dev_set_name() succeeds or not.

> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Sudeep Holla <sudeep.holla@kernel.org>
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>

On tiny bit of fragility inline that I'd suggestion avoiding now rather than
later.  Down to dev_set_name() doing allocations in some cases under the hood
that are tricky to tidy up with device_register() being used.

With that tidied up

Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

> diff --git a/drivers/firmware/smccc/bus.c b/drivers/firmware/smccc/bus.c
> new file mode 100644
> index 000000000000..caba1a03c997
> --- /dev/null
> +++ b/drivers/firmware/smccc/bus.c

> +
> +struct arm_smccc_device *arm_smccc_device_register(const char *name, u32 func_id)
> +{
> +	int ret;
> +	struct arm_smccc_device *smccc_dev;
> +
> +	if (!name)
> +		return ERR_PTR(-EINVAL);
> +
> +	smccc_dev = kzalloc_obj(*smccc_dev);
> +	if (!smccc_dev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	smccc_dev->func_id = func_id;
> +	smccc_dev->dev.bus = &arm_smccc_bus_type;
> +	smccc_dev->dev.release = arm_smccc_release_device;
> +
> +	ret = dev_set_name(&smccc_dev->dev, "%s", name);

Does protecting the string defeat the nice underlying const handling?
e.g.
	ret = dev_set_name(&smccc_dev->dev, name);
might be better.  Not particular important - I only noticed because
you'd often see this between an device_initialize() and device_add()
and then we'd be relying on the device_put() to clean it up.

So as this stands this is fragile as any error paths that later
get added...


> +	if (ret) {
> +		kfree(smccc_dev);
> +		return ERR_PTR(ret);
> +	}
> +

here can't free the name allocation.

So I'd just switch this to a split device setup - something like...

	smccc_dev = kzalloc_obj(*smccc_dev);
	if (!smccc_dev)
		return ERR_PTR(-ENOMEM);

	smccc_dev->func_id = func_id;
	smccc_dev->dev.bus = &arm_smccc_bus_type;
	smccc_dev->dev.release = arm_smccc_release_device;

	device_initialize(&smccc_dev->dev);
	ret = dev_set_name(&smccc_dev->dev, "%s", name);

//up to you whether you care about const string handling.

	if (ret) {
		put_device(&smcc_dev->dev);
		return ret;
	}

	ret = device_register(&smccc_dev->dev);
	if (ret) {
		put_device(&smccc_dev->dev);
		return ERR_PTR(ret);
	}

	return smccc_dev;

> +	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);
> +
> +void arm_smccc_device_unregister(struct arm_smccc_device *smccc_dev)
> +{
> +	if (!smccc_dev)
> +		return;
> +
> +	device_unregister(&smccc_dev->dev);
> +}
> +EXPORT_SYMBOL_GPL(arm_smccc_device_unregister);
> +
> +static int __init arm_smccc_bus_init(void)
> +{
> +	return bus_register(&arm_smccc_bus_type);
> +}
> +subsys_initcall(arm_smccc_bus_init);