Re: [PATCH 1/3] firmware/arm_ffa: change ffa_device_register()'s parameters and return

[Sudeep Holla <sudeep.holla@arm.com>]

On Mon, Nov 25, 2024 at 09:52:49AM +0000, Yeoreum Yun wrote:
> From: Levi Yun <yeoreum.yun@arm.com>
> 
> Currently, ffa_dev->properties is set after ffa_device_register() in
> ffa_setup_partitions().
> This means it couldn't validate partition's properties
> while probing ffa_device.
> 
> Change parameter of ffa_device_register() to receive ffa_partition_info
> so that before device_register(), ffa_device->properties can be setup
> and any other data.
> 
> Also, change return value of ffa_device_register() from NULL to ERR_PTR
> so that it passes error code.
> 
> Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
> ---
>  drivers/firmware/arm_ffa/bus.c    | 22 +++++++++++++++-------
>  drivers/firmware/arm_ffa/driver.c | 12 ++++--------
>  include/linux/arm_ffa.h           | 12 ++++++++----
>  3 files changed, 27 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/firmware/arm_ffa/bus.c b/drivers/firmware/arm_ffa/bus.c
> index eb17d03b66fe..95c0e9518556 100644
> --- a/drivers/firmware/arm_ffa/bus.c
> +++ b/drivers/firmware/arm_ffa/bus.c
> @@ -38,6 +38,7 @@ static int ffa_device_match(struct device *dev, const struct device_driver *drv)
>  
>  		if (uuid_equal(&ffa_dev->uuid, &id_table->uuid))
>  			return 1;
> +

Spurious, don't add formatting or style changes in functional change. We
can take it up later when there are other formatting issues.

>  		id_table++;
>  	}
>  
> @@ -187,21 +188,26 @@ bool ffa_device_is_valid(struct ffa_device *ffa_dev)
>  	return valid;
>  }
>  
> -struct ffa_device *ffa_device_register(const uuid_t *uuid, int vm_id,
> -				       const struct ffa_ops *ops)
> +struct ffa_device *ffa_device_register(
> +		const struct ffa_partition_info *part_info,
> +		const struct ffa_ops *ops)
>  {
>  	int id, ret;
> +	uuid_t uuid;
>  	struct device *dev;
>  	struct ffa_device *ffa_dev;
>  
> +	if (!part_info)
> +		return ERR_PTR(-EINVAL);
> +
>  	id = ida_alloc_min(&ffa_bus_id, 1, GFP_KERNEL);
>  	if (id < 0)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	ffa_dev = kzalloc(sizeof(*ffa_dev), GFP_KERNEL);
>  	if (!ffa_dev) {
>  		ida_free(&ffa_bus_id, id);
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);

I am not keen on changing the error from NULL here. -ENOMEM has its own
logging. ida_alloc failing is very unlikely unless you have so many
partitions in the system.

>  	}
>  
>  	dev = &ffa_dev->dev;
> @@ -210,16 +216,18 @@ struct ffa_device *ffa_device_register(const uuid_t *uuid, int vm_id,
>  	dev_set_name(&ffa_dev->dev, "arm-ffa-%d", id);
>  
>  	ffa_dev->id = id;
> -	ffa_dev->vm_id = vm_id;
> +	ffa_dev->vm_id = part_info->id;
> +	ffa_dev->properties = part_info->properties;
>  	ffa_dev->ops = ops;
> -	uuid_copy(&ffa_dev->uuid, uuid);
> +	import_uuid(&uuid, (u8 *)part_info->uuid);
> +	uuid_copy(&ffa_dev->uuid, &uuid);
>  
>  	ret = device_register(&ffa_dev->dev);
>  	if (ret) {
>  		dev_err(dev, "unable to register device %s err=%d\n",
>  			dev_name(dev), ret);

This error log will give the information you are adding in driver.c, so
it is again not needed to change that.

>  		put_device(dev);
> -		return NULL;
> +		return ERR_PTR(ret);
>  	}
>  
>  	return ffa_dev;
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index b14cbdae94e8..a3a9cdec7389 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -1406,23 +1406,19 @@ static int ffa_setup_partitions(void)
>  
>  	xa_init(&drv_info->partition_info);
>  	for (idx = 0, tpbuf = pbuf; idx < count; idx++, tpbuf++) {
> -		import_uuid(&uuid, (u8 *)tpbuf->uuid);
> -
>  		/* Note that if the UUID will be uuid_null, that will require
>  		 * ffa_bus_notifier() to find the UUID of this partition id
>  		 * with help of ffa_device_match_uuid(). FF-A v1.1 and above
>  		 * provides UUID here for each partition as part of the
>  		 * discovery API and the same is passed.
>  		 */
> -		ffa_dev = ffa_device_register(&uuid, tpbuf->id, &ffa_drv_ops);
> -		if (!ffa_dev) {
> -			pr_err("%s: failed to register partition ID 0x%x\n",
> -			       __func__, tpbuf->id);
> +		ffa_dev = ffa_device_register(tpbuf, &ffa_drv_ops);
> +		if (IS_ERR_OR_NULL(ffa_dev)) {
> +			pr_err("%s: failed to register partition ID 0x%x, error %ld\n",
> +			       __func__, tpbuf->id, PTR_ERR(ffa_dev));

Drop this additional error info you are adding as bus.c will provide it.

-- 
Regards,
Sudeep