[PATCH 0/3] small fixes for arm_ffa driver

[PATCH 0/3] small fixes for arm_ffa driver

[Yeoreum Yun]

This patchset small fixes for arm_ffa driver:
    - change ffa_device_register()'s declaration and its return value.
    - add direct message version 2 related properties bits and some fix.

Levi Yun (3):
  firmware/arm_ffa: change ffa_device_register()'s parameters and return
  arm_ffa.h: add properties bit related direct msg version 2
  firmware/arm_ffa: remove __le64_to_cpu() when set uuid for direct msg
    v2

 drivers/firmware/arm_ffa/bus.c    | 22 +++++++++++++++-------
 drivers/firmware/arm_ffa/driver.c | 18 +++++++-----------
 include/linux/arm_ffa.h           | 16 ++++++++++++----
 3 files changed, 34 insertions(+), 22 deletions(-)

--
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}

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

[Yeoreum Yun]

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}

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

[Sudeep Holla]

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

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

[Yeoreum Yun]

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!

[PATCH 2/3] arm_ffa.h: add properties bit related direct msg version 2

[Yeoreum Yun]

From: Levi Yun <yeoreum.yun@arm.com>

According to FF-A specificaiton [0], There are flags to be used to
check whether partition supports send/receive direct msg version 2.

Add related flags.

Link: https://developer.arm.com/documentation/den0077/latest [0]
Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
---
 include/linux/arm_ffa.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/arm_ffa.h b/include/linux/arm_ffa.h
index 3fb9c7a3453b..b697675c76ba 100644
--- a/include/linux/arm_ffa.h
+++ b/include/linux/arm_ffa.h
@@ -238,6 +238,10 @@ struct ffa_partition_info {
 #define FFA_PARTITION_NOTIFICATION_RECV	BIT(3)
 /* partition runs in the AArch64 execution state. */
 #define FFA_PARTITION_AARCH64_EXEC	BIT(8)
+/* partition supports receipt of direct requests version 2 */
+#define FFA_PARTITION_DIRECT_RECV_V2	BIT(9)
+/* partition can send direct requests. */
+#define FFA_PARTITION_DIRECT_SEND_V2	BIT(10)
 	u32 properties;
 	u32 uuid[4];
 };
-- 
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}

Re: [PATCH 2/3] arm_ffa.h: add properties bit related direct msg version 2

[Sudeep Holla]

On Mon, Nov 25, 2024 at 09:52:50AM +0000, Yeoreum Yun wrote:
> From: Levi Yun <yeoreum.yun@arm.com>
> 
> According to FF-A specificaiton [0], There are flags to be used to
> check whether partition supports send/receive direct msg version 2.
> 
> Add related flags.
>

I will delay this to next release, this is not a fix.

-- 
Regards,
Sudeep

[PATCH 3/3] firmware/arm_ffa: remove __le64_to_cpu() when set uuid for direct msg v2

[Yeoreum Yun]

From: Levi Yun <yeoreum.yun@arm.com>

UUID is saved in big endian format.
i.e) For uuid "378daedc-f06b-4446-8314-40ab933c87a3",

It should be saved in memory like:
    37 8d ae dc
    f0 6b 44 46
    83 14 40 ab
    93 3c 87 a3

Accoding to FF-A specification[0] 15.4 FFA_MSG_SEND_DRIECT_REQ2,
then UUID is saved in register:
    UUID Lo  x2  Bytes[0...7] of UUID with byte 0 in the low-order bits.
    UUID Hi  x3  Bytes[8...15] of UUID with byte 8 in the low-order bits.

That means, we don't need to swap the uuid when it send via direct
message request version 2, just send it as saved in memory.

Remove le64_to_cpu() for uuid in direct message request version 2,
and change uuid_regs' type to __be64 because UUID is saved in network
byte order.

Link: https://developer.arm.com/documentation/den0077/latest [0]
Signed-off-by: Yeoreum Yun <yeoreum.yun@arm.com>
---
 drivers/firmware/arm_ffa/driver.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index a3a9cdec7389..4a6bc6520d25 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -483,13 +483,13 @@ static int ffa_msg_send_direct_req2(u16 src_id, u16 dst_id, const uuid_t *uuid,
 	u32 src_dst_ids = PACK_TARGET_INFO(src_id, dst_id);
 	union {
 		uuid_t uuid;
-		__le64 regs[2];
+		__be64 regs[2];
 	} uuid_regs = { .uuid = *uuid };
 	ffa_value_t ret, args = {
 		.a0 = FFA_MSG_SEND_DIRECT_REQ2,
 		.a1 = src_dst_ids,
-		.a2 = le64_to_cpu(uuid_regs.regs[0]),
-		.a3 = le64_to_cpu(uuid_regs.regs[1]),
+		.a2 = uuid_regs.regs[0],
+		.a3 = uuid_regs.regs[1],
 	};
 	memcpy((void *)&args + offsetof(ffa_value_t, a4), data, sizeof(*data));
 
-- 
LEVI:{C3F47F37-75D8-414A-A8BA-3980EC8A46D7}

Re: [PATCH 3/3] firmware/arm_ffa: remove __le64_to_cpu() when set uuid for direct msg v2

[Sudeep Holla]

On Mon, Nov 25, 2024 at 09:52:51AM +0000, Yeoreum Yun wrote:
> From: Levi Yun <yeoreum.yun@arm.com>
> 
> UUID is saved in big endian format.
> i.e) For uuid "378daedc-f06b-4446-8314-40ab933c87a3",
> 
> It should be saved in memory like:
>     37 8d ae dc
>     f0 6b 44 46
>     83 14 40 ab
>     93 3c 87 a3
> 
> Accoding to FF-A specification[0] 15.4 FFA_MSG_SEND_DRIECT_REQ2,
> then UUID is saved in register:
>     UUID Lo  x2  Bytes[0...7] of UUID with byte 0 in the low-order bits.
>     UUID Hi  x3  Bytes[8...15] of UUID with byte 8 in the low-order bits.
> 
> That means, we don't need to swap the uuid when it send via direct
> message request version 2, just send it as saved in memory.
> 
> Remove le64_to_cpu() for uuid in direct message request version 2,
> and change uuid_regs' type to __be64 because UUID is saved in network
> byte order.
>

  |   warning: incorrect type in initializer (different base types)
  |      expected unsigned long a2
  |      got restricted __be64
  |   warning: incorrect type in initializer (different base types)
  |      expected unsigned long a3
  |      got restricted __be64

We will get this warning back with this change, wondering if we can take
up BE support separately.

--
Regards,
Sudeep

Re: [PATCH 3/3] firmware/arm_ffa: remove __le64_to_cpu() when set uuid for direct msg v2

[Yeoreum Yun]

Hi, Sudeep.
>
>   |   warning: incorrect type in initializer (different base types)
>   |      expected unsigned long a2
>   |      got restricted __be64
>   |   warning: incorrect type in initializer (different base types)
>   |      expected unsigned long a3
>   |      got restricted __be64
>
> We will get this warning back with this change, wondering if we can take
> up BE support separately.

Sorry. I'll change the type :\ Thanks to let me know!