[PATCH v1 1/1] tee: amdtee: Use %pUl printk() format specifier to print GUIDs

[PATCH v1 1/1] tee: amdtee: Use %pUl printk() format specifier to print GUIDs

[Andy Shevchenko]

Replace the custom approach with the %pUl printk() format specifier.
No functional change intended.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/tee/amdtee/core.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/tee/amdtee/core.c b/drivers/tee/amdtee/core.c
index e487231d25dc..d3201eff1b74 100644
--- a/drivers/tee/amdtee/core.c
+++ b/drivers/tee/amdtee/core.c
@@ -14,6 +14,7 @@
 #include <linux/mm.h>
 #include <linux/uaccess.h>
 #include <linux/firmware.h>
+#include <linux/uuid.h>
 #include "amdtee_private.h"
 #include <linux/psp-tee.h>
 
@@ -172,21 +173,11 @@ static int copy_ta_binary(struct tee_context *ctx, void *ptr, void **ta,
 {
 	const struct firmware *fw;
 	char fw_name[TA_PATH_MAX];
-	struct {
-		u32 lo;
-		u16 mid;
-		u16 hi_ver;
-		u8 seq_n[8];
-	} *uuid = ptr;
 	int n, rc = 0;
+	guid_t uuid;
 
-	n = snprintf(fw_name, TA_PATH_MAX,
-		     "%s/%08x-%04x-%04x-%02x%02x%02x%02x%02x%02x%02x%02x.bin",
-		     TA_LOAD_PATH, uuid->lo, uuid->mid, uuid->hi_ver,
-		     uuid->seq_n[0], uuid->seq_n[1],
-		     uuid->seq_n[2], uuid->seq_n[3],
-		     uuid->seq_n[4], uuid->seq_n[5],
-		     uuid->seq_n[6], uuid->seq_n[7]);
+	import_guid(&uuid, ptr);
+	n = snprintf(fw_name, TA_PATH_MAX, "%s/%pUl.bin", TA_LOAD_PATH, &uuid);
 	if (n < 0 || n >= TA_PATH_MAX) {
 		pr_err("failed to get firmware name\n");
 		return -EINVAL;
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 1/1] tee: amdtee: Use %pUl printk() format specifier to print GUIDs

[Jens Wiklander]

On Wed, Sep 11, 2024 at 10:41 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Replace the custom approach with the %pUl printk() format specifier.
> No functional change intended.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/tee/amdtee/core.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)

Thanks, the patch looks like a nice simplificatrion.

Rijo, Devaraj, does this work for you?

Cheers,
Jens

>
> diff --git a/drivers/tee/amdtee/core.c b/drivers/tee/amdtee/core.c
> index e487231d25dc..d3201eff1b74 100644
> --- a/drivers/tee/amdtee/core.c
> +++ b/drivers/tee/amdtee/core.c
> @@ -14,6 +14,7 @@
>  #include <linux/mm.h>
>  #include <linux/uaccess.h>
>  #include <linux/firmware.h>
> +#include <linux/uuid.h>
>  #include "amdtee_private.h"
>  #include <linux/psp-tee.h>
>
> @@ -172,21 +173,11 @@ static int copy_ta_binary(struct tee_context *ctx, void *ptr, void **ta,
>  {
>         const struct firmware *fw;
>         char fw_name[TA_PATH_MAX];
> -       struct {
> -               u32 lo;
> -               u16 mid;
> -               u16 hi_ver;
> -               u8 seq_n[8];
> -       } *uuid = ptr;
>         int n, rc = 0;
> +       guid_t uuid;
>
> -       n = snprintf(fw_name, TA_PATH_MAX,
> -                    "%s/%08x-%04x-%04x-%02x%02x%02x%02x%02x%02x%02x%02x.bin",
> -                    TA_LOAD_PATH, uuid->lo, uuid->mid, uuid->hi_ver,
> -                    uuid->seq_n[0], uuid->seq_n[1],
> -                    uuid->seq_n[2], uuid->seq_n[3],
> -                    uuid->seq_n[4], uuid->seq_n[5],
> -                    uuid->seq_n[6], uuid->seq_n[7]);
> +       import_guid(&uuid, ptr);
> +       n = snprintf(fw_name, TA_PATH_MAX, "%s/%pUl.bin", TA_LOAD_PATH, &uuid);
>         if (n < 0 || n >= TA_PATH_MAX) {
>                 pr_err("failed to get firmware name\n");
>                 return -EINVAL;
> --
> 2.43.0.rc1.1336.g36b5255a03ac
>

Re: [PATCH v1 1/1] tee: amdtee: Use %pUl printk() format specifier to print GUIDs

[Andy Shevchenko]

On Thu, Sep 12, 2024 at 07:50:08AM +0200, Jens Wiklander wrote:
> On Wed, Sep 11, 2024 at 10:41 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > Replace the custom approach with the %pUl printk() format specifier.
> > No functional change intended.

> Thanks, the patch looks like a nice simplificatrion.

Thank you for the review.

> Rijo, Devaraj, does this work for you?

Yes, please test, because seems others use uuid_t (UUID BE) for TEE,
but in this driver IIUC it's guid_t (UUID LE).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] tee: amdtee: Use %pUl printk() format specifier to print GUIDs

[Rijo Thomas]

On 9/13/2024 2:59 PM, Andy Shevchenko wrote:
> On Thu, Sep 12, 2024 at 07:50:08AM +0200, Jens Wiklander wrote:
>> On Wed, Sep 11, 2024 at 10:41 PM Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>>>
>>> Replace the custom approach with the %pUl printk() format specifier.
>>> No functional change intended.
> 
>> Thanks, the patch looks like a nice simplificatrion.
> 
> Thank you for the review.
> 
>> Rijo, Devaraj, does this work for you?
> 
> Yes, please test, because seems others use uuid_t (UUID BE) for TEE,
> but in this driver IIUC it's guid_t (UUID LE).
> 

No, this does not work for us. I tested this patch, it does not work as expected.

%pUl gives output in uuid format (%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x).
But, what we need, is a name with the format %08x-%04x-%04x-%02x%02x%02x%02x%02x%02x%02x%02x.

Endian-ness is not an issue here. uuid generates name with 4 hypens (-).
While, in our TA naming we are using 3 hyphens (-).

Thanks,
Rijo

Re: [PATCH v1 1/1] tee: amdtee: Use %pUl printk() format specifier to print GUIDs

[Andy Shevchenko]

On Mon, Sep 16, 2024 at 01:38:27PM +0530, Rijo Thomas wrote:
> On 9/13/2024 2:59 PM, Andy Shevchenko wrote:
> > On Thu, Sep 12, 2024 at 07:50:08AM +0200, Jens Wiklander wrote:
> >> On Wed, Sep 11, 2024 at 10:41 PM Andy Shevchenko
> >> <andriy.shevchenko@linux.intel.com> wrote:
> >>>
> >>> Replace the custom approach with the %pUl printk() format specifier.
> >>> No functional change intended.
> > 
> >> Thanks, the patch looks like a nice simplificatrion.
> > 
> > Thank you for the review.
> > 
> >> Rijo, Devaraj, does this work for you?
> > 
> > Yes, please test, because seems others use uuid_t (UUID BE) for TEE,
> > but in this driver IIUC it's guid_t (UUID LE).
> 
> No, this does not work for us. I tested this patch, it does not work as expected.
> 
> %pUl gives output in uuid format (%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x).
> But, what we need, is a name with the format %08x-%04x-%04x-%02x%02x%02x%02x%02x%02x%02x%02x.
> 
> Endian-ness is not an issue here. uuid generates name with 4 hypens (-).
> While, in our TA naming we are using 3 hyphens (-).

Ah, good catch! Can somebody add a comment there to explain that this uses
non-standard human-readable representation of GUID/UUID?

P.S. Thank you for testing!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] tee: amdtee: Use %pUl printk() format specifier to print GUIDs

[Andy Shevchenko]

On Mon, Sep 16, 2024 at 12:38:08PM +0300, Andy Shevchenko wrote:
> On Mon, Sep 16, 2024 at 01:38:27PM +0530, Rijo Thomas wrote:
> > On 9/13/2024 2:59 PM, Andy Shevchenko wrote:
> > > On Thu, Sep 12, 2024 at 07:50:08AM +0200, Jens Wiklander wrote:
> > >> On Wed, Sep 11, 2024 at 10:41 PM Andy Shevchenko
> > >> <andriy.shevchenko@linux.intel.com> wrote:
> > >>>
> > >>> Replace the custom approach with the %pUl printk() format specifier.
> > >>> No functional change intended.
> > > 
> > >> Thanks, the patch looks like a nice simplificatrion.
> > > 
> > > Thank you for the review.
> > > 
> > >> Rijo, Devaraj, does this work for you?
> > > 
> > > Yes, please test, because seems others use uuid_t (UUID BE) for TEE,
> > > but in this driver IIUC it's guid_t (UUID LE).
> > 
> > No, this does not work for us. I tested this patch, it does not work as expected.
> > 
> > %pUl gives output in uuid format (%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x).
> > But, what we need, is a name with the format %08x-%04x-%04x-%02x%02x%02x%02x%02x%02x%02x%02x.
> > 
> > Endian-ness is not an issue here. uuid generates name with 4 hypens (-).
> > While, in our TA naming we are using 3 hyphens (-).
> 
> Ah, good catch! Can somebody add a comment there to explain that this uses
> non-standard human-readable representation of GUID/UUID?
> 
> P.S. Thank you for testing!

Alternatively we may get rid of that hyphen. I can send a patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] tee: amdtee: Use %pUl printk() format specifier to print GUIDs

[Andy Shevchenko]

On Mon, Sep 16, 2024 at 12:39:14PM +0300, Andy Shevchenko wrote:
> On Mon, Sep 16, 2024 at 12:38:08PM +0300, Andy Shevchenko wrote:
> > On Mon, Sep 16, 2024 at 01:38:27PM +0530, Rijo Thomas wrote:
> > > On 9/13/2024 2:59 PM, Andy Shevchenko wrote:
> > > > On Thu, Sep 12, 2024 at 07:50:08AM +0200, Jens Wiklander wrote:
> > > >> On Wed, Sep 11, 2024 at 10:41 PM Andy Shevchenko
> > > >> <andriy.shevchenko@linux.intel.com> wrote:
> > > >>>
> > > >>> Replace the custom approach with the %pUl printk() format specifier.
> > > >>> No functional change intended.
> > > > 
> > > >> Thanks, the patch looks like a nice simplificatrion.
> > > > 
> > > > Thank you for the review.
> > > > 
> > > >> Rijo, Devaraj, does this work for you?
> > > > 
> > > > Yes, please test, because seems others use uuid_t (UUID BE) for TEE,
> > > > but in this driver IIUC it's guid_t (UUID LE).
> > > 
> > > No, this does not work for us. I tested this patch, it does not work as expected.
> > > 
> > > %pUl gives output in uuid format (%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x).
> > > But, what we need, is a name with the format %08x-%04x-%04x-%02x%02x%02x%02x%02x%02x%02x%02x.
> > > 
> > > Endian-ness is not an issue here. uuid generates name with 4 hypens (-).
> > > While, in our TA naming we are using 3 hyphens (-).
> > 
> > Ah, good catch! Can somebody add a comment there to explain that this uses
> > non-standard human-readable representation of GUID/UUID?
> > 
> > P.S. Thank you for testing!
> 
> Alternatively we may get rid of that hyphen. I can send a patch.

Something like this on top of this patch (meaning squashed at the end):

+	/*
+	 * Firmware name uses non-standard human-readable GUID representation:
+	 * %08x-%04x-%04x-%02x%02x%02x%02x%02x%02x%02x%02x (no last hyphen).
+	 */
+	p = strrchr(fw_name, '-');
+	memmove(p, p + 1, strlen(p + 1));
+
 	mutex_lock(&drv_mutex);
 	n = request_firmware(&fw, fw_name, &ctx->teedev->dev);

What do you think?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] tee: amdtee: Use %pUl printk() format specifier to print GUIDs

[Jerome Forissier]

On 9/16/24 11:38, Andy Shevchenko wrote:
> On Mon, Sep 16, 2024 at 01:38:27PM +0530, Rijo Thomas wrote:
>> On 9/13/2024 2:59 PM, Andy Shevchenko wrote:
>>> On Thu, Sep 12, 2024 at 07:50:08AM +0200, Jens Wiklander wrote:
>>>> On Wed, Sep 11, 2024 at 10:41 PM Andy Shevchenko
>>>> <andriy.shevchenko@linux.intel.com> wrote:
>>>>>
>>>>> Replace the custom approach with the %pUl printk() format specifier.
>>>>> No functional change intended.
>>>
>>>> Thanks, the patch looks like a nice simplificatrion.
>>>
>>> Thank you for the review.
>>>
>>>> Rijo, Devaraj, does this work for you?
>>>
>>> Yes, please test, because seems others use uuid_t (UUID BE) for TEE,
>>> but in this driver IIUC it's guid_t (UUID LE).
>>
>> No, this does not work for us. I tested this patch, it does not work as expected.
>>
>> %pUl gives output in uuid format (%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x).
>> But, what we need, is a name with the format %08x-%04x-%04x-%02x%02x%02x%02x%02x%02x%02x%02x.
>>
>> Endian-ness is not an issue here. uuid generates name with 4 hypens (-).
>> While, in our TA naming we are using 3 hyphens (-).
> 
> Ah, good catch! Can somebody add a comment there to explain that this uses
> non-standard human-readable representation of GUID/UUID?

Could this be due to some copying/pasting from the OP-TEE code base which had
a similar mistake prior to v2.3.0 [1][2][3]?

[1] https://github.com/OP-TEE/optee_os/blob/2.3.0/CHANGELOG.md?plain=1#L40-L45
[2] https://github.com/OP-TEE/optee_client/commit/a5b1ffcd26e3
[3] https://github.com/OP-TEE/optee_client/commit/365657667f89

> 
> P.S. Thank you for testing!
> 

Regards,
-- 
Jerome

Re: [PATCH v1 1/1] tee: amdtee: Use %pUl printk() format specifier to print GUIDs

[Andy Shevchenko]

On Tue, Sep 17, 2024 at 09:22:19AM +0200, Jerome Forissier wrote:
> On 9/16/24 11:38, Andy Shevchenko wrote:
> > On Mon, Sep 16, 2024 at 01:38:27PM +0530, Rijo Thomas wrote:
> >> On 9/13/2024 2:59 PM, Andy Shevchenko wrote:
> >>> On Thu, Sep 12, 2024 at 07:50:08AM +0200, Jens Wiklander wrote:
> >>>> On Wed, Sep 11, 2024 at 10:41 PM Andy Shevchenko
> >>>> <andriy.shevchenko@linux.intel.com> wrote:
> >>>>>
> >>>>> Replace the custom approach with the %pUl printk() format specifier.
> >>>>> No functional change intended.
> >>>
> >>>> Thanks, the patch looks like a nice simplificatrion.
> >>>
> >>> Thank you for the review.
> >>>
> >>>> Rijo, Devaraj, does this work for you?
> >>>
> >>> Yes, please test, because seems others use uuid_t (UUID BE) for TEE,
> >>> but in this driver IIUC it's guid_t (UUID LE).
> >>
> >> No, this does not work for us. I tested this patch, it does not work as expected.
> >>
> >> %pUl gives output in uuid format (%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x).
> >> But, what we need, is a name with the format %08x-%04x-%04x-%02x%02x%02x%02x%02x%02x%02x%02x.
> >>
> >> Endian-ness is not an issue here. uuid generates name with 4 hypens (-).
> >> While, in our TA naming we are using 3 hyphens (-).
> > 
> > Ah, good catch! Can somebody add a comment there to explain that this uses
> > non-standard human-readable representation of GUID/UUID?
> 
> Could this be due to some copying/pasting from the OP-TEE code base which had
> a similar mistake prior to v2.3.0 [1][2][3]?
> 
> [1] https://github.com/OP-TEE/optee_os/blob/2.3.0/CHANGELOG.md?plain=1#L40-L45
> [2] https://github.com/OP-TEE/optee_client/commit/a5b1ffcd26e3
> [3] https://github.com/OP-TEE/optee_client/commit/365657667f89

Interesting... Is somebody going to update this to follow the proper format?
(yes, I understand that the old one has still to be supported)

> > P.S. Thank you for testing!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] tee: amdtee: Use %pUl printk() format specifier to print GUIDs

[Rijo Thomas]

On 9/17/2024 12:52 PM, Jerome Forissier wrote:
> 
> 
> On 9/16/24 11:38, Andy Shevchenko wrote:
>> On Mon, Sep 16, 2024 at 01:38:27PM +0530, Rijo Thomas wrote:
>>> On 9/13/2024 2:59 PM, Andy Shevchenko wrote:
>>>> On Thu, Sep 12, 2024 at 07:50:08AM +0200, Jens Wiklander wrote:
>>>>> On Wed, Sep 11, 2024 at 10:41 PM Andy Shevchenko
>>>>> <andriy.shevchenko@linux.intel.com> wrote:
>>>>>>
>>>>>> Replace the custom approach with the %pUl printk() format specifier.
>>>>>> No functional change intended.
>>>>
>>>>> Thanks, the patch looks like a nice simplificatrion.
>>>>
>>>> Thank you for the review.
>>>>
>>>>> Rijo, Devaraj, does this work for you?
>>>>
>>>> Yes, please test, because seems others use uuid_t (UUID BE) for TEE,
>>>> but in this driver IIUC it's guid_t (UUID LE).
>>>
>>> No, this does not work for us. I tested this patch, it does not work as expected.
>>>
>>> %pUl gives output in uuid format (%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x).
>>> But, what we need, is a name with the format %08x-%04x-%04x-%02x%02x%02x%02x%02x%02x%02x%02x.
>>>
>>> Endian-ness is not an issue here. uuid generates name with 4 hypens (-).
>>> While, in our TA naming we are using 3 hyphens (-).
>>
>> Ah, good catch! Can somebody add a comment there to explain that this uses
>> non-standard human-readable representation of GUID/UUID?
> 
> Could this be due to some copying/pasting from the OP-TEE code base which had
> a similar mistake prior to v2.3.0 [1][2][3]?
> 
> [1] https://github.com/OP-TEE/optee_os/blob/2.3.0/CHANGELOG.md?plain=1#L40-L45
> [2] https://github.com/OP-TEE/optee_client/commit/a5b1ffcd26e3
> [3] https://github.com/OP-TEE/optee_client/commit/365657667f89
> 
>>
>> P.S. Thank you for testing!
>>
> 
> Regards,
> 
Thanks Jerome for the information. We will update and post a patch for this.

Regards,
Rijo