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

[Aneesh Kumar K . V]

Jason Gunthorpe <jgg@xxxxxxxxxx> writes:

> On Mon, Sep 14, 2026 at 01:32:49PM -0700, Jonathan Cameron wrote:
>> > +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.
>
> Pedenatically the %s is better as it doesn't restrict name to not
> include % characters. 
>
>> 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...
>
> Yes, but as written it is OK, and this is a common pattern in the
> kernel. I agree it is fragile tricky..
>
> Still it isn't an urgent reason to change it around, but the best
> pattern is to put the allocate, dev.release=, and device_initialize()
> in one 'alloc' function. Then the other function calls it and always
> unwinds with put_device. Use device_add().
>
> This avoids mixing the different kfree/put_device error unwind regimes
> into the same function..
>

Something like

+static struct arm_smccc_device *arm_smccc_device_alloc(u32 func_id)
+{
+	struct arm_smccc_device *smccc_dev;
+
+	smccc_dev = kzalloc_obj(*smccc_dev);
+	if (!smccc_dev)
+		return NULL;
+
+	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);
+
+	return smccc_dev;
+}
+
 struct arm_smccc_device *arm_smccc_device_register(const char *name, u32 func_id)
 {
+	struct arm_smccc_device *smccc_dev;
 	int ret;
-	struct arm_smccc_device *smccc_dev;
 
 	if (!name)
 		return ERR_PTR(-EINVAL);
 
-	smccc_dev = kzalloc_obj(*smccc_dev);
+	smccc_dev = arm_smccc_device_alloc(func_id);
 	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);
-	if (ret) {
-		kfree(smccc_dev);
-		return ERR_PTR(ret);
-	}
+	if (ret)
+		goto err_put_device;
 
-	ret = device_register(&smccc_dev->dev);
-	if (ret) {
-		put_device(&smccc_dev->dev);
-		return ERR_PTR(ret);
-	}
+	ret = device_add(&smccc_dev->dev);
+	if (ret)
+		goto err_put_device;
 
 	return smccc_dev;
+
+err_put_device:
+	put_device(&smccc_dev->dev);
+	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(arm_smccc_device_register);