Should OP-TEE revision be printed as "string" in linux OP-TEE driver ?

[sahilnxp]

Since we have moved to 16bit sha1 len for OP-TEE revision in case of 64 bit architecture.

Should we print the revision as string in place of long int here

Observed one issue on 64 bit architectures, if the commit id is starting with 0 like 04d1c612ec7beaede073b8cad0f33a1f5ab9e2bc

It is printing revision as below removing leading 0.
2024-11-15T02:29:51.469317 [ 2.019585] optee: revision 4.4 (4d1c612ec7beaed)

[jforissier]

Hi @sahilnxp,

Should we print the revision as string in place of long int here

I think the format simply needs adjusting to cater to the size of the unsigned long:

pr_info("revision %lu.%lu (%0*lx)", res.result.major,
	res.result.minor, sizeof(res.result.build_id) * 2,
	res.result.build_id);

[sahilnxp]

Hi @sahilnxp,

Should we print the revision as string in place of long int here

I think the format simply needs adjusting to cater to the size of the unsigned long:

pr_info("revision %lu.%lu (%0*lx)", res.result.major,
	res.result.minor, sizeof(res.result.build_id) * 2,
	res.result.build_id);

Yes, that will work, but we need to pass the size like (16) too in this case like below:

pr_info("revision %lu.%lu (%0*lx)", res.result.major,
	res.result.minor, sizeof(res.result.build_id) * 2,
	16, res.result.build_id);

and how to decide on this size ? Otherwise, it is giving the following warning.

                 from drivers/tee/optee/smc_abi.c:12:
drivers/tee/optee/smc_abi.c: In function ‘optee_msg_get_os_revision’:
./include/linux/kern_levels.h:5:25: warning: field width specifier ‘*’ expects argument of type ‘int’, but argument 4 has type ‘long unsigned int’ [-Wformat=]
    5 | #define KERN_SOH        "\001"          /* ASCII Start Of Header */
      |                         ^~~~~~

[jenswi-linaro]

Casting the size argument to int should work.

[sahilnxp]

Sorry for confusion here.

There is no compilation warning in case of following code:

pr_info("revision %lu.%lu (%0*lx)", res.result.major,
	res.result.minor, sizeof(res.result.build_id) * 2,
	16, res.result.build_id);

But how we can decide if we should give the size as 8 (for 32bit arch) and 16 (for 64 bit arch) ?

[jforissier]

Sorry for confusion here.

There is no compilation warning in case of following code:

pr_info("revision %lu.%lu (%0*lx)", res.result.major,
	res.result.minor, sizeof(res.result.build_id) * 2,
	16, res.result.build_id);

But how we can decide if we should give the size as 8 (for 32bit arch) and 16 (for 64 bit arch) ?

res.result.build_id is unsigned long. 32-bit arch have sizeof(unsigned long) == 4 while 64-bit arch have sizeof(unsigned long) == 8.

[sahilnxp]

So it should be like this, right ?

pr_info("revision %lu.%lu (%0*lx)", res.result.major,
             res.result.minor, (int)sizeof(unsigned long),
             res.result.build_id);

[jforissier]

No, because in the Linux (and OP-TEE) coding style, the argument of sizeof() should be the variable rather than the type. This makes sense to make maintenance easier in case the type is changed for instance.

[sahilnxp]

In that case, it should be like below ?

pr_info("revision %lu.%lu (%0*lx)", res.result.major,
             res.result.minor, (int)sizeof(res.result.build_id),
             res.result.build_id);

[jforissier]

In that case, it should be like below ?

pr_info("revision %lu.%lu (%0*lx)", res.result.major,
             res.result.minor, (int)sizeof(res.result.build_id),
             res.result.build_id);

No. The * 2 is missing. Here is exactly how the code should be:

	if (res.result.build_id)
		pr_info("revision %lu.%lu (%08lx)", res.result.major,
			res.result.minor, (int)sizeof(res.result.build_id) * 2,
			res.result.build_id);
	else
		pr_info("revision %lu.%lu", res.result.major, res.result.minor);

[sahilnxp]

@jforissier Sorry, I am confused, why we want to double the size?

[jforissier]

Because for each byte of data, two characters are printed in hex

[sahilnxp]

Because for each byte of data, two characters are printed in hex

ohh, yeah. Thats right. I missed that part. Thanks.
I will send a patch for this on Linux kernel and update link here.

[sahilnxp]

Patch has been sent upstream: https://lore.kernel.org/linux-kernel/[email protected]/

[sahilnxp]

https://lore.kernel.org/linux-kernel/[email protected]/

@jforissier Do I need to update the tags in this one too ?

[jforissier]

https://lore.kernel.org/linux-kernel/[email protected]/

@jforissier Do I need to update the tags in this one too ?

I don't think one needs to re-send patches to the LKML just for the tags because the maintainers have different means to collect them. @jenswi-linaro is this correct?

[jenswi-linaro]

That's right, I'll apply all tags (and add my Signed-off-by) when I pick it up. It's a few weeks until the next merge window opens so there's no rush.

[sahilnxp]

Thanks @jenswi-linaro @jforissier for your inputs.

[github-actions]

This issue has been marked as a stale issue because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this issue will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.