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;
+
 		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);
 	}
 
 	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);
 		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));
 			continue;
 		}
 
-		ffa_dev->properties = tpbuf->properties;
-
 		if (drv_info->version > FFA_VERSION_1_0 &&
 		    !(tpbuf->properties & FFA_PARTITION_AARCH64_EXEC))
 			ffa_mode_32bit_set(ffa_dev);
diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
index a28e2a6a13d0..3fb9c7a3453b 100644
--- a/include/linux/arm_ffa.h
+++ b/include/linux/arm_ffa.h
@@ -166,9 +166,12 @@ static inline void *ffa_dev_get_drvdata(struct ffa_device *fdev)
 	return dev_get_drvdata(&fdev->dev);
 }
 
+struct ffa_partition_info;
+
 #if IS_REACHABLE(CONFIG_ARM_FFA_TRANSPORT)
-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);
 void ffa_device_unregister(struct ffa_device *ffa_dev);
 int ffa_driver_register(struct ffa_driver *driver, struct module *owner,
 			const char *mod_name);
@@ -177,8 +180,9 @@ bool ffa_device_is_valid(struct ffa_device *ffa_dev);
 
 #else
 static inline
-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)
 {
 	return NULL;
 }
-- 
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}

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

Hi Sudeep.

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

Okay I'll restore.

> >  	}
> >
> >  	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.

Okay. Thanks!