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

[Jason Gunthorpe]

> [ ... 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?

-- 
Jason