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

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

Jason Gunthorpe <jgg@nvidia.com> writes:

>> [ ... 143 lines skipped ... ]
>> +struct arm_smccc_device *arm_smccc_device_register(const char *name)
>> +{
>> +	int id, ret;
>> +	struct arm_smccc_device *smccc_dev;
>> +
>> +	if (!name)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	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) {
>> +		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);
>
> The bus seems well constructed, but this is a little bit odd, was it
> deliberate?
>
> For identifying the module alias and labeling the bus devices it is
> typical to use a fixed HW value, because it tends to turn into
> uAPI. So the hex func_id would have been a logical choice:
>
> 		.func_id        = SMC_RSI_ABI_VERSION,
>
> Ie 0xc4000190 as the device label.
>
> For example lets imagine that ARM defines a call to give a list of
> (func_id, version) for everything the FW supports. This would be
> great, then no need to probe every item in the table anymore. However
> if you define strings here then it doesn't work so well, the string
> table all has to be built in..
>
> Though handling ARM's version scheme could be tricky.
>
> Not opposed to this, but think about it carefully since this is
> basically making a uABI decision that probably cannot be taken back.
> comment about that above the table at least.
>
> What is this idr and name mangling doing? The names have to
> be unique because they are 1:1 with func_id, which must be unique by
> how SMCCC works, so what is the purpose of the IDR?
>
> Now instead of getting a machine stable device name like c4000190 or
> even arm-rsi-dev we get arm-rsi-dev.X where X is unpredictable and
> might change on kernel upgrades. That's not cool.
>
> smccc_dev->id isn't used for anything else. Drop it?
>
>> [ ... 47 lines skipped ... ]
>> +struct arm_smccc_device {
>> +	int id;
>> +	char name[ARM_SMCCC_NAME_SIZE];
>> +	struct device dev;
>> +};
>
> It is common practice to put the containing struct at the top and is a
> micro optimization since container_of becomes a NOP. Same for the
> driver below.
>
> Why have two copies of name? dev->name is already enough?
>

I have updated the bus to match devices using func_id.

static int arm_smccc_bus_match(struct device *dev,
		const struct device_driver *drv)
{
....
	id_table = to_arm_smccc_driver(drv)->id_table;
	if (!id_table)
		return 0;

	while (id_table->func_id) {
		if (smccc_dev->func_id == id_table->func_id)
			return 1;
		id_table++;
	}
...

file2alias.c now have

modified    scripts/mod/file2alias.c
@@ -1351,9 +1351,9 @@
 
 static void do_arm_smccc_entry(struct module *mod, void *symval)
 {
-	DEF_FIELD_ADDR(symval, arm_smccc_device_id, name);
+	DEF_FIELD(symval, arm_smccc_device_id, func_id);
 
-	module_alias_printf(mod, false, ARM_SMCCC_MODULE_PREFIX "%s", *name);
+	module_alias_printf(mod, false, ARM_SMCCC_MODULE_PREFIX "f%08X", func_id);
 }
 
 /*