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;
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
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).
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
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!
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.
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?
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,
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!
op-tee@lists.trustedfirmware.org
-
Andy Shevchenko -
Jens Wiklander -
Jerome Forissier -
Rijo Thomas