[PATCH v8 0/3] tpm_crb_ffa: handle tpm busy return code

[PATCH v8 0/3] tpm_crb_ffa: handle tpm busy return code

[Prachotan Bathi]

Platforms that support FF-A direct message request v2 can implement
Secure Partitions (SPs) that host multiple services. When the TPM
service shares its SP with other services, message requests from the
driver may fail with a BUSY response if another service is currently
active.

To improve robustness in such scenarios, we need to introduce retry
logic in the driver. When a BUSY error is received, the driver will
re-attempt the TPM request until it succeeds or a run-time configurable 
timeout(default: 2000 ms) is reached. This ensures reliable TPM access 
under shared-SP conditions.

Add a module parameter, `busy_timeout_ms`, which specifies the
maximum amount of time (in milliseconds) to retry on BUSY before giving
up.

This change builds on top of commit a85b55ee64a5, which introduced
support for TPM service communication using the FF-A direct message v2
path, in accordance with section 3.3 of the TPM Service Command
Response Buffer Interface specification.
https://developer.arm.com/documentation/den0138/latest/

This was tested with an FF-A based fTPM currently not publicly available. 
There are plans to open source the fTPM.

Changes in v8:
- Moved memzero macro to a separate patch.
- Changes to inline comments for consistency.

Prachotan Bathi (3):
  tpm_crb_ffa: Fix typos in function name
  tpm_crb_ffa:Introduce memzero macro to replace memset
  tpm_crb_ffa: handle tpm busy return code

 .../admin-guide/kernel-parameters.txt         |  8 +++
 drivers/char/tpm/tpm_crb_ffa.c                | 59 +++++++++++++++----
 2 files changed, 56 insertions(+), 11 deletions(-)

-- 
2.43.0

[PATCH v8 1/3] tpm_crb_ffa: Fix typos in function name

[Prachotan Bathi]

s/recieve/receive in __tpm_crb_ffa_send_receive

Signed-off-by: Prachotan Bathi <prachotan.bathi@arm.com>
---
 drivers/char/tpm/tpm_crb_ffa.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
index 4ead61f01299..089d1e54bb46 100644
--- a/drivers/char/tpm/tpm_crb_ffa.c
+++ b/drivers/char/tpm/tpm_crb_ffa.c
@@ -178,7 +178,7 @@ int tpm_crb_ffa_init(void)
 }
 EXPORT_SYMBOL_GPL(tpm_crb_ffa_init);
 
-static int __tpm_crb_ffa_send_recieve(unsigned long func_id,
+static int __tpm_crb_ffa_send_receive(unsigned long func_id,
 				      unsigned long a0,
 				      unsigned long a1,
 				      unsigned long a2)
@@ -251,7 +251,7 @@ int tpm_crb_ffa_get_interface_version(u16 *major, u16 *minor)
 
 	guard(mutex)(&tpm_crb_ffa->msg_data_lock);
 
-	rc = __tpm_crb_ffa_send_recieve(CRB_FFA_GET_INTERFACE_VERSION, 0x00, 0x00, 0x00);
+	rc = __tpm_crb_ffa_send_receive(CRB_FFA_GET_INTERFACE_VERSION, 0x00, 0x00, 0x00);
 	if (!rc) {
 		if (ffa_partition_supports_direct_req2_recv(tpm_crb_ffa->ffa_dev)) {
 			*major = CRB_FFA_MAJOR_VERSION(tpm_crb_ffa->direct_msg_data2.data[1]);
@@ -289,7 +289,7 @@ int tpm_crb_ffa_start(int request_type, int locality)
 
 	guard(mutex)(&tpm_crb_ffa->msg_data_lock);
 
-	return __tpm_crb_ffa_send_recieve(CRB_FFA_START, request_type, locality, 0x00);
+	return __tpm_crb_ffa_send_receive(CRB_FFA_START, request_type, locality, 0x00);
 }
 EXPORT_SYMBOL_GPL(tpm_crb_ffa_start);
 
-- 
2.43.0

[PATCH v8 2/3] tpm_crb_ffa:Introduce memzero macro to replace memset

[Prachotan Bathi]

Add a memzero macro to simplify and standardize zeroing
FF-A data args, replacing direct uses of memset for
improved readability and maintainability.

Signed-off-by: Prachotan Bathi <prachotan.bathi@arm.com>
---
 drivers/char/tpm/tpm_crb_ffa.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
index 089d1e54bb46..397cc3b0a478 100644
--- a/drivers/char/tpm/tpm_crb_ffa.c
+++ b/drivers/char/tpm/tpm_crb_ffa.c
@@ -12,6 +12,8 @@
 #include <linux/arm_ffa.h>
 #include "tpm_crb_ffa.h"
 
+#define memzero(s, n) memset((s), 0, (n))
+
 /* TPM service function status codes */
 #define CRB_FFA_OK			0x05000001
 #define CRB_FFA_OK_RESULTS_RETURNED	0x05000002
@@ -192,7 +194,7 @@ static int __tpm_crb_ffa_send_receive(unsigned long func_id,
 	msg_ops = tpm_crb_ffa->ffa_dev->ops->msg_ops;
 
 	if (ffa_partition_supports_direct_req2_recv(tpm_crb_ffa->ffa_dev)) {
-		memset(&tpm_crb_ffa->direct_msg_data2, 0x00,
+		memzero(&tpm_crb_ffa->direct_msg_data2,
 		       sizeof(struct ffa_send_direct_data2));
 
 		tpm_crb_ffa->direct_msg_data2.data[0] = func_id;
@@ -205,7 +207,7 @@ static int __tpm_crb_ffa_send_receive(unsigned long func_id,
 		if (!ret)
 			ret = tpm_crb_ffa_to_linux_errno(tpm_crb_ffa->direct_msg_data2.data[0]);
 	} else {
-		memset(&tpm_crb_ffa->direct_msg_data, 0x00,
+		memzero(&tpm_crb_ffa->direct_msg_data,
 		       sizeof(struct ffa_send_direct_data));
 
 		tpm_crb_ffa->direct_msg_data.data1 = func_id;
-- 
2.43.0

[PATCH v8 3/3] tpm_crb_ffa: handle tpm busy return code

[Prachotan Bathi]

Platforms supporting direct message request v2 [1] can support secure
partitions that support multiple services. For CRB over FF-A interface,
if the firmware TPM or TPM service [1] shares its Secure Partition (SP)
with another service, message requests may fail with a -EBUSY error.

To handle this, replace the single check and call with a retry loop
that attempts the TPM message send operation until it succeeds or a
configurable timeout is reached. Implement a _try_send_receive function
to do a single send/receive and modify the existing send_receive to
add this retry loop.
The retry mechanism introduces a module parameter (`busy_timeout_ms`,
default: 2000ms) to control how long to keep retrying on -EBUSY
responses. Between retries, the code waits briefly (50-100 microseconds)
to avoid busy-waiting and handling TPM BUSY conditions more gracefully.

The parameter can be modified at run-time as such:
echo 3000 | tee /sys/module/tpm_crb_ffa/parameters/busy_timeout_ms
This changes the timeout from the default 2000ms to 3000ms.

[1] TPM Service Command Response Buffer Interface Over FF-A
https://developer.arm.com/documentation/den0138/latest/

Signed-off-by: Prachotan Bathi <prachotan.bathi@arm.com>
---
 .../admin-guide/kernel-parameters.txt         |  8 +++
 drivers/char/tpm/tpm_crb_ffa.c                | 49 ++++++++++++++++---
 2 files changed, 50 insertions(+), 7 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index f1f2c0874da9..eb1ebbc328ab 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -7214,6 +7214,14 @@
 			causing a major performance hit, and the space where
 			machines are deployed is by other means guarded.
 
+	tpm_crb_ffa.busy_timeout_ms= [ARM64,TPM]
+			Maximum time in milliseconds to retry sending a message
+			to the TPM service before giving up. This parameter controls
+			how long the system will continue retrying when the TPM
+			service is busy.
+			Format: <unsigned int>
+			Default: 2000 (2 seconds)
+
 	tpm_suspend_pcr=[HW,TPM]
 			Format: integer pcr id
 			Specify that at suspend time, the tpm driver
diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
index 397cc3b0a478..a6c9e9c2c344 100644
--- a/drivers/char/tpm/tpm_crb_ffa.c
+++ b/drivers/char/tpm/tpm_crb_ffa.c
@@ -10,6 +10,8 @@
 #define pr_fmt(fmt) "CRB_FFA: " fmt
 
 #include <linux/arm_ffa.h>
+#include <linux/delay.h>
+#include <linux/moduleparam.h>
 #include "tpm_crb_ffa.h"
 
 #define memzero(s, n) memset((s), 0, (n))
@@ -180,17 +182,13 @@ int tpm_crb_ffa_init(void)
 }
 EXPORT_SYMBOL_GPL(tpm_crb_ffa_init);
 
-static int __tpm_crb_ffa_send_receive(unsigned long func_id,
-				      unsigned long a0,
-				      unsigned long a1,
-				      unsigned long a2)
+static int __tpm_crb_ffa_try_send_receive(unsigned long func_id,
+					  unsigned long a0, unsigned long a1,
+					  unsigned long a2)
 {
 	const struct ffa_msg_ops *msg_ops;
 	int ret;
 
-	if (!tpm_crb_ffa)
-		return -ENOENT;
-
 	msg_ops = tpm_crb_ffa->ffa_dev->ops->msg_ops;
 
 	if (ffa_partition_supports_direct_req2_recv(tpm_crb_ffa->ffa_dev)) {
@@ -221,6 +219,43 @@ static int __tpm_crb_ffa_send_receive(unsigned long func_id,
 			ret = tpm_crb_ffa_to_linux_errno(tpm_crb_ffa->direct_msg_data.data1);
 	}
 
+	return ret;
+}
+
+static unsigned int busy_timeout_ms = 2000;
+/**
+ * This parameter defines the maximum time in milliseconds to retry
+ * sending a message to the TPM service before giving up.
+ */
+module_param(busy_timeout_ms, uint, 0644);
+MODULE_PARM_DESC(busy_timeout_ms,
+		 "Maximum time in ms to retry before giving up on busy");
+
+static int __tpm_crb_ffa_send_receive(unsigned long func_id, unsigned long a0,
+				      unsigned long a1, unsigned long a2)
+{
+	ktime_t start, stop;
+	int ret;
+
+	if (!tpm_crb_ffa)
+		return -ENOENT;
+
+	start = ktime_get();
+	stop = ktime_add(start, ms_to_ktime(busy_timeout_ms));
+
+	for (;;) {
+		ret = __tpm_crb_ffa_try_send_receive(func_id, a0, a1, a2);
+
+		if (ret != -EBUSY)
+			break;
+
+		usleep_range(50, 100);
+		if (ktime_after(ktime_get(), stop)) {
+			dev_warn(&tpm_crb_ffa->ffa_dev->dev,
+				 "Busy retry timed out\n");
+			break;
+		}
+	}
 
 	return ret;
 }
-- 
2.43.0

Re: [PATCH v8 1/3] tpm_crb_ffa: Fix typos in function name

[Jarkko Sakkinen]

On Thu, Jun 26, 2025 at 01:45:19PM -0500, Prachotan Bathi wrote:
> s/recieve/receive in __tpm_crb_ffa_send_receive
> 
> Signed-off-by: Prachotan Bathi <prachotan.bathi@arm.com>
> ---
>  drivers/char/tpm/tpm_crb_ffa.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
> index 4ead61f01299..089d1e54bb46 100644
> --- a/drivers/char/tpm/tpm_crb_ffa.c
> +++ b/drivers/char/tpm/tpm_crb_ffa.c
> @@ -178,7 +178,7 @@ int tpm_crb_ffa_init(void)
>  }
>  EXPORT_SYMBOL_GPL(tpm_crb_ffa_init);
>  
> -static int __tpm_crb_ffa_send_recieve(unsigned long func_id,
> +static int __tpm_crb_ffa_send_receive(unsigned long func_id,
>  				      unsigned long a0,
>  				      unsigned long a1,
>  				      unsigned long a2)
> @@ -251,7 +251,7 @@ int tpm_crb_ffa_get_interface_version(u16 *major, u16 *minor)
>  
>  	guard(mutex)(&tpm_crb_ffa->msg_data_lock);
>  
> -	rc = __tpm_crb_ffa_send_recieve(CRB_FFA_GET_INTERFACE_VERSION, 0x00, 0x00, 0x00);
> +	rc = __tpm_crb_ffa_send_receive(CRB_FFA_GET_INTERFACE_VERSION, 0x00, 0x00, 0x00);
>  	if (!rc) {
>  		if (ffa_partition_supports_direct_req2_recv(tpm_crb_ffa->ffa_dev)) {
>  			*major = CRB_FFA_MAJOR_VERSION(tpm_crb_ffa->direct_msg_data2.data[1]);
> @@ -289,7 +289,7 @@ int tpm_crb_ffa_start(int request_type, int locality)
>  
>  	guard(mutex)(&tpm_crb_ffa->msg_data_lock);
>  
> -	return __tpm_crb_ffa_send_recieve(CRB_FFA_START, request_type, locality, 0x00);
> +	return __tpm_crb_ffa_send_receive(CRB_FFA_START, request_type, locality, 0x00);
>  }
>  EXPORT_SYMBOL_GPL(tpm_crb_ffa_start);
>  
> -- 
> 2.43.0
> 

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

Re: [PATCH v8 2/3] tpm_crb_ffa:Introduce memzero macro to replace memset

[Jarkko Sakkinen]

On Thu, Jun 26, 2025 at 01:45:20PM -0500, Prachotan Bathi wrote:
> Add a memzero macro to simplify and standardize zeroing
> FF-A data args, replacing direct uses of memset for
> improved readability and maintainability.
> 
> Signed-off-by: Prachotan Bathi <prachotan.bathi@arm.com>
> ---
>  drivers/char/tpm/tpm_crb_ffa.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
> index 089d1e54bb46..397cc3b0a478 100644
> --- a/drivers/char/tpm/tpm_crb_ffa.c
> +++ b/drivers/char/tpm/tpm_crb_ffa.c
> @@ -12,6 +12,8 @@
>  #include <linux/arm_ffa.h>
>  #include "tpm_crb_ffa.h"
>  
> +#define memzero(s, n) memset((s), 0, (n))
> +
>  /* TPM service function status codes */
>  #define CRB_FFA_OK			0x05000001
>  #define CRB_FFA_OK_RESULTS_RETURNED	0x05000002
> @@ -192,7 +194,7 @@ static int __tpm_crb_ffa_send_receive(unsigned long func_id,
>  	msg_ops = tpm_crb_ffa->ffa_dev->ops->msg_ops;
>  
>  	if (ffa_partition_supports_direct_req2_recv(tpm_crb_ffa->ffa_dev)) {
> -		memset(&tpm_crb_ffa->direct_msg_data2, 0x00,
> +		memzero(&tpm_crb_ffa->direct_msg_data2,
>  		       sizeof(struct ffa_send_direct_data2));
>  
>  		tpm_crb_ffa->direct_msg_data2.data[0] = func_id;
> @@ -205,7 +207,7 @@ static int __tpm_crb_ffa_send_receive(unsigned long func_id,
>  		if (!ret)
>  			ret = tpm_crb_ffa_to_linux_errno(tpm_crb_ffa->direct_msg_data2.data[0]);
>  	} else {
> -		memset(&tpm_crb_ffa->direct_msg_data, 0x00,
> +		memzero(&tpm_crb_ffa->direct_msg_data,
>  		       sizeof(struct ffa_send_direct_data));
>  
>  		tpm_crb_ffa->direct_msg_data.data1 = func_id;
> -- 
> 2.43.0
> 

It adds a ross-reference to the source code, meaning that you have to
jump back and forth in the source code just to see that there is a
function that wraps up a single memset() call.

How does that map to "readability"?

BR, Jarkko

Re: [PATCH v8 3/3] tpm_crb_ffa: handle tpm busy return code

[Jarkko Sakkinen]

On Thu, Jun 26, 2025 at 01:45:21PM -0500, Prachotan Bathi wrote:
> Platforms supporting direct message request v2 [1] can support secure
> partitions that support multiple services. For CRB over FF-A interface,
> if the firmware TPM or TPM service [1] shares its Secure Partition (SP)
> with another service, message requests may fail with a -EBUSY error.
> 
> To handle this, replace the single check and call with a retry loop
> that attempts the TPM message send operation until it succeeds or a
> configurable timeout is reached. Implement a _try_send_receive function
> to do a single send/receive and modify the existing send_receive to
> add this retry loop.
> The retry mechanism introduces a module parameter (`busy_timeout_ms`,
> default: 2000ms) to control how long to keep retrying on -EBUSY
> responses. Between retries, the code waits briefly (50-100 microseconds)
> to avoid busy-waiting and handling TPM BUSY conditions more gracefully.
> 
> The parameter can be modified at run-time as such:
> echo 3000 | tee /sys/module/tpm_crb_ffa/parameters/busy_timeout_ms
> This changes the timeout from the default 2000ms to 3000ms.
> 
> [1] TPM Service Command Response Buffer Interface Over FF-A
> https://developer.arm.com/documentation/den0138/latest/
> 
> Signed-off-by: Prachotan Bathi <prachotan.bathi@arm.com>
> ---
>  .../admin-guide/kernel-parameters.txt         |  8 +++
>  drivers/char/tpm/tpm_crb_ffa.c                | 49 ++++++++++++++++---
>  2 files changed, 50 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index f1f2c0874da9..eb1ebbc328ab 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -7214,6 +7214,14 @@
>  			causing a major performance hit, and the space where
>  			machines are deployed is by other means guarded.
>  
> +	tpm_crb_ffa.busy_timeout_ms= [ARM64,TPM]
> +			Maximum time in milliseconds to retry sending a message
> +			to the TPM service before giving up. This parameter controls
> +			how long the system will continue retrying when the TPM
> +			service is busy.
> +			Format: <unsigned int>
> +			Default: 2000 (2 seconds)
> +
>  	tpm_suspend_pcr=[HW,TPM]
>  			Format: integer pcr id
>  			Specify that at suspend time, the tpm driver
> diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
> index 397cc3b0a478..a6c9e9c2c344 100644
> --- a/drivers/char/tpm/tpm_crb_ffa.c
> +++ b/drivers/char/tpm/tpm_crb_ffa.c
> @@ -10,6 +10,8 @@
>  #define pr_fmt(fmt) "CRB_FFA: " fmt
>  
>  #include <linux/arm_ffa.h>
> +#include <linux/delay.h>
> +#include <linux/moduleparam.h>
>  #include "tpm_crb_ffa.h"
>  
>  #define memzero(s, n) memset((s), 0, (n))
> @@ -180,17 +182,13 @@ int tpm_crb_ffa_init(void)
>  }
>  EXPORT_SYMBOL_GPL(tpm_crb_ffa_init);
>  
> -static int __tpm_crb_ffa_send_receive(unsigned long func_id,
> -				      unsigned long a0,
> -				      unsigned long a1,
> -				      unsigned long a2)
> +static int __tpm_crb_ffa_try_send_receive(unsigned long func_id,
> +					  unsigned long a0, unsigned long a1,
> +					  unsigned long a2)
>  {
>  	const struct ffa_msg_ops *msg_ops;
>  	int ret;
>  
> -	if (!tpm_crb_ffa)
> -		return -ENOENT;
> -
>  	msg_ops = tpm_crb_ffa->ffa_dev->ops->msg_ops;
>  
>  	if (ffa_partition_supports_direct_req2_recv(tpm_crb_ffa->ffa_dev)) {
> @@ -221,6 +219,43 @@ static int __tpm_crb_ffa_send_receive(unsigned long func_id,
>  			ret = tpm_crb_ffa_to_linux_errno(tpm_crb_ffa->direct_msg_data.data1);
>  	}
>  
> +	return ret;
> +}
> +
> +static unsigned int busy_timeout_ms = 2000;

Add an empty line here.

> +/**
> + * This parameter defines the maximum time in milliseconds to retry
> + * sending a message to the TPM service before giving up.
> + */

Since it is already documented in kernel-parameters.rst, you could just
as well remove this comment. It is only extra burden to maintain later
on.

> +module_param(busy_timeout_ms, uint, 0644);
> +MODULE_PARM_DESC(busy_timeout_ms,
> +		 "Maximum time in ms to retry before giving up on busy");

It's somewhat de-facto to put module parameters to the beginning of the
file, which I'd advice to do also here.

> +
> +static int __tpm_crb_ffa_send_receive(unsigned long func_id, unsigned long a0,
> +				      unsigned long a1, unsigned long a2)
> +{
> +	ktime_t start, stop;
> +	int ret;
> +
> +	if (!tpm_crb_ffa)
> +		return -ENOENT;
> +
> +	start = ktime_get();
> +	stop = ktime_add(start, ms_to_ktime(busy_timeout_ms));
> +
> +	for (;;) {
> +		ret = __tpm_crb_ffa_try_send_receive(func_id, a0, a1, a2);
> +

I'd remove this newline (but it is your choice, not going to nak for this).

> +		if (ret != -EBUSY)
> +			break;
> +
> +		usleep_range(50, 100);
> +		if (ktime_after(ktime_get(), stop)) {
> +			dev_warn(&tpm_crb_ffa->ffa_dev->dev,
> +				 "Busy retry timed out\n");
> +			break;
> +		}
> +	}
>  
>  	return ret;
>  }
> -- 
> 2.43.0
> 

BR, Jarkko

Re: [PATCH v8 2/3] tpm_crb_ffa:Introduce memzero macro to replace memset

[Prachotan Bathi]

On 7/2/25 5:16 PM, Jarkko Sakkinen wrote:

> On Thu, Jun 26, 2025 at 01:45:20PM -0500, Prachotan Bathi wrote:
>> Add a memzero macro to simplify and standardize zeroing
>> FF-A data args, replacing direct uses of memset for
>> improved readability and maintainability.
>>
>> Signed-off-by: Prachotan Bathi <prachotan.bathi@arm.com>
>> ---
>>   drivers/char/tpm/tpm_crb_ffa.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
>> index 089d1e54bb46..397cc3b0a478 100644
>> --- a/drivers/char/tpm/tpm_crb_ffa.c
>> +++ b/drivers/char/tpm/tpm_crb_ffa.c
>> @@ -12,6 +12,8 @@
>>   #include <linux/arm_ffa.h>
>>   #include "tpm_crb_ffa.h"
>>   
>> +#define memzero(s, n) memset((s), 0, (n))
>> +
>>   /* TPM service function status codes */
>>   #define CRB_FFA_OK			0x05000001
>>   #define CRB_FFA_OK_RESULTS_RETURNED	0x05000002
>> @@ -192,7 +194,7 @@ static int __tpm_crb_ffa_send_receive(unsigned long func_id,
>>   	msg_ops = tpm_crb_ffa->ffa_dev->ops->msg_ops;
>>   
>>   	if (ffa_partition_supports_direct_req2_recv(tpm_crb_ffa->ffa_dev)) {
>> -		memset(&tpm_crb_ffa->direct_msg_data2, 0x00,
>> +		memzero(&tpm_crb_ffa->direct_msg_data2,
>>   		       sizeof(struct ffa_send_direct_data2));
>>   
>>   		tpm_crb_ffa->direct_msg_data2.data[0] = func_id;
>> @@ -205,7 +207,7 @@ static int __tpm_crb_ffa_send_receive(unsigned long func_id,
>>   		if (!ret)
>>   			ret = tpm_crb_ffa_to_linux_errno(tpm_crb_ffa->direct_msg_data2.data[0]);
>>   	} else {
>> -		memset(&tpm_crb_ffa->direct_msg_data, 0x00,
>> +		memzero(&tpm_crb_ffa->direct_msg_data,
>>   		       sizeof(struct ffa_send_direct_data));
>>   
>>   		tpm_crb_ffa->direct_msg_data.data1 = func_id;
>> -- 
>> 2.43.0
>>
> It adds a ross-reference to the source code, meaning that you have to
> jump back and forth in the source code just to see that there is a
> function that wraps up a single memset() call.
>
> How does that map to "readability"?
>
> BR, Jarkko

Hi Jarkko

I've implemented this change post your feedback on v4 of the initial patch
series, maybe this should've been a question at that point, but what was the
reasoning for recommending that I use memzero instead? I'll use the same
reasoning to rephrase the commit message.

Best
Prachotan.
Sorry for the spam, Thunderbird keeps defaulting to html emails.


On 7/2/25 5:16 PM, Jarkko Sakkinen wrote:
> On Thu, Jun 26, 2025 at 01:45:20PM -0500, Prachotan Bathi wrote:
>> Add a memzero macro to simplify and standardize zeroing
>> FF-A data args, replacing direct uses of memset for
>> improved readability and maintainability.
>>
>> Signed-off-by: Prachotan Bathi <prachotan.bathi@arm.com>
>> ---
>>   drivers/char/tpm/tpm_crb_ffa.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
>> index 089d1e54bb46..397cc3b0a478 100644
>> --- a/drivers/char/tpm/tpm_crb_ffa.c
>> +++ b/drivers/char/tpm/tpm_crb_ffa.c
>> @@ -12,6 +12,8 @@
>>   #include <linux/arm_ffa.h>
>>   #include "tpm_crb_ffa.h"
>>   
>> +#define memzero(s, n) memset((s), 0, (n))
>> +
>>   /* TPM service function status codes */
>>   #define CRB_FFA_OK			0x05000001
>>   #define CRB_FFA_OK_RESULTS_RETURNED	0x05000002
>> @@ -192,7 +194,7 @@ static int __tpm_crb_ffa_send_receive(unsigned long func_id,
>>   	msg_ops = tpm_crb_ffa->ffa_dev->ops->msg_ops;
>>   
>>   	if (ffa_partition_supports_direct_req2_recv(tpm_crb_ffa->ffa_dev)) {
>> -		memset(&tpm_crb_ffa->direct_msg_data2, 0x00,
>> +		memzero(&tpm_crb_ffa->direct_msg_data2,
>>   		       sizeof(struct ffa_send_direct_data2));
>>   
>>   		tpm_crb_ffa->direct_msg_data2.data[0] = func_id;
>> @@ -205,7 +207,7 @@ static int __tpm_crb_ffa_send_receive(unsigned long func_id,
>>   		if (!ret)
>>   			ret = tpm_crb_ffa_to_linux_errno(tpm_crb_ffa->direct_msg_data2.data[0]);
>>   	} else {
>> -		memset(&tpm_crb_ffa->direct_msg_data, 0x00,
>> +		memzero(&tpm_crb_ffa->direct_msg_data,
>>   		       sizeof(struct ffa_send_direct_data));
>>   
>>   		tpm_crb_ffa->direct_msg_data.data1 = func_id;
>> -- 
>> 2.43.0
>>
> It adds a ross-reference to the source code, meaning that you have to
> jump back and forth in the source code just to see that there is a
> function that wraps up a single memset() call.
>
> How does that map to "readability"?
>
> BR, Jarkko

Re: [PATCH v8 2/3] tpm_crb_ffa:Introduce memzero macro to replace memset

[Jarkko Sakkinen]

On Wed, Jul 02, 2025 at 10:58:56PM -0500, Prachotan Bathi wrote:
> On 7/2/25 5:16 PM, Jarkko Sakkinen wrote:
> 
> > On Thu, Jun 26, 2025 at 01:45:20PM -0500, Prachotan Bathi wrote:
> > > Add a memzero macro to simplify and standardize zeroing
> > > FF-A data args, replacing direct uses of memset for
> > > improved readability and maintainability.
> > > 
> > > Signed-off-by: Prachotan Bathi <prachotan.bathi@arm.com>
> > > ---
> > >   drivers/char/tpm/tpm_crb_ffa.c | 6 ++++--
> > >   1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
> > > index 089d1e54bb46..397cc3b0a478 100644
> > > --- a/drivers/char/tpm/tpm_crb_ffa.c
> > > +++ b/drivers/char/tpm/tpm_crb_ffa.c
> > > @@ -12,6 +12,8 @@
> > >   #include <linux/arm_ffa.h>
> > >   #include "tpm_crb_ffa.h"
> > > +#define memzero(s, n) memset((s), 0, (n))
> > > +
> > >   /* TPM service function status codes */
> > >   #define CRB_FFA_OK			0x05000001
> > >   #define CRB_FFA_OK_RESULTS_RETURNED	0x05000002
> > > @@ -192,7 +194,7 @@ static int __tpm_crb_ffa_send_receive(unsigned long func_id,
> > >   	msg_ops = tpm_crb_ffa->ffa_dev->ops->msg_ops;
> > >   	if (ffa_partition_supports_direct_req2_recv(tpm_crb_ffa->ffa_dev)) {
> > > -		memset(&tpm_crb_ffa->direct_msg_data2, 0x00,
> > > +		memzero(&tpm_crb_ffa->direct_msg_data2,
> > >   		       sizeof(struct ffa_send_direct_data2));
> > >   		tpm_crb_ffa->direct_msg_data2.data[0] = func_id;
> > > @@ -205,7 +207,7 @@ static int __tpm_crb_ffa_send_receive(unsigned long func_id,
> > >   		if (!ret)
> > >   			ret = tpm_crb_ffa_to_linux_errno(tpm_crb_ffa->direct_msg_data2.data[0]);
> > >   	} else {
> > > -		memset(&tpm_crb_ffa->direct_msg_data, 0x00,
> > > +		memzero(&tpm_crb_ffa->direct_msg_data,
> > >   		       sizeof(struct ffa_send_direct_data));
> > >   		tpm_crb_ffa->direct_msg_data.data1 = func_id;
> > > -- 
> > > 2.43.0
> > > 
> > It adds a ross-reference to the source code, meaning that you have to
> > jump back and forth in the source code just to see that there is a
> > function that wraps up a single memset() call.
> > 
> > How does that map to "readability"?
> > 
> > BR, Jarkko
> 
> Hi Jarkko
> 
> I've implemented this change post your feedback on v4 of the initial patch
> series, maybe this should've been a question at that point, but what was the
> reasoning for recommending that I use memzero instead? I'll use the same
> reasoning to rephrase the commit message.

OK I found what you were referring to:

https://lore.kernel.org/linux-integrity/aFF-WNSolTdV9PZG@kernel.org/

Well, that was some truly misguided advice from my side so all the shame
here is on me :-) There's no global memzero() and neither explicit
version makes much sense here. Sorry about that.

I gave it now (actual) thought, and here's what I'd propose:

diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
index 96746d5b03e3..e769f6143a7c 100644
--- a/drivers/char/tpm/tpm_crb_ffa.c
+++ b/drivers/char/tpm/tpm_crb_ffa.c
@@ -203,26 +203,20 @@ static int __tpm_crb_ffa_try_send_receive(unsigned long func_id,
 	msg_ops = tpm_crb_ffa->ffa_dev->ops->msg_ops;
 
 	if (ffa_partition_supports_direct_req2_recv(tpm_crb_ffa->ffa_dev)) {
-		memzero(&tpm_crb_ffa->direct_msg_data2,
-		       sizeof(struct ffa_send_direct_data2));
-
-		tpm_crb_ffa->direct_msg_data2.data[0] = func_id;
-		tpm_crb_ffa->direct_msg_data2.data[1] = a0;
-		tpm_crb_ffa->direct_msg_data2.data[2] = a1;
-		tpm_crb_ffa->direct_msg_data2.data[3] = a2;
+		tpm_crb_ffa->direct_msg_data2 = (struct ffa_send_direct_data2){
+			.data = { func_id, a0, a1, a2 },
+		};
 
 		ret = msg_ops->sync_send_receive2(tpm_crb_ffa->ffa_dev,
 				&tpm_crb_ffa->direct_msg_data2);
 		if (!ret)
 			ret = tpm_crb_ffa_to_linux_errno(tpm_crb_ffa->direct_msg_data2.data[0]);
 	} else {
-		memzero(&tpm_crb_ffa->direct_msg_data,
-		       sizeof(struct ffa_send_direct_data));
-
-		tpm_crb_ffa->direct_msg_data.data1 = func_id;
-		tpm_crb_ffa->direct_msg_data.data2 = a0;
-		tpm_crb_ffa->direct_msg_data.data3 = a1;
-		tpm_crb_ffa->direct_msg_data.data4 = a2;
+		tpm_crb_ffa->direct_msg_data = (struct ffa_send_direct_data){
+			.data1 = func_id,
+			.data2 = a0,
+			.data3 = a1,
+		};
 
 		ret = msg_ops->sync_send_receive(tpm_crb_ffa->ffa_dev,
 				&tpm_crb_ffa->direct_msg_data);

I tested the compilation with:

make CROSS_COMPILE=aarch64-linux-gnu- ARCH=arm64 tinyconfig && ./scripts/config --file .config -e CONFIG_KEYS -e CONFIG_TCG_TPM -e CONFIG_64BIT -e CONFIG_TRUSTED_KEYS -e CONFIG_TTY -e CONFIG_PROCFS -e CONFIG_SYSFS -e CONFIG_TCG_VTPM_PROXY -e CONFIG_EFI -e CONFIG_ACPI -e CONFIG_ARM_FFA_TRANSPORT -e CONFIG_TCG_CRB && yes '' | make CROSS_COMPILE=aarch64-linux-gnu- ARCH=arm64 oldconfig && make CROSS_COMPILE=aarch64-linux-gnu- ARCH=arm64 -j$(nproc)

BR, Jarkko

Re: [PATCH v8 2/3] tpm_crb_ffa:Introduce memzero macro to replace memset

[Jarkko Sakkinen]

On Fri, Jul 04, 2025 at 05:45:11AM +0300, Jarkko Sakkinen wrote:
> On Wed, Jul 02, 2025 at 10:58:56PM -0500, Prachotan Bathi wrote:
> > On 7/2/25 5:16 PM, Jarkko Sakkinen wrote:
> > 
> > > On Thu, Jun 26, 2025 at 01:45:20PM -0500, Prachotan Bathi wrote:
> > > > Add a memzero macro to simplify and standardize zeroing
> > > > FF-A data args, replacing direct uses of memset for
> > > > improved readability and maintainability.
> > > > 
> > > > Signed-off-by: Prachotan Bathi <prachotan.bathi@arm.com>
> > > > ---
> > > >   drivers/char/tpm/tpm_crb_ffa.c | 6 ++++--
> > > >   1 file changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
> > > > index 089d1e54bb46..397cc3b0a478 100644
> > > > --- a/drivers/char/tpm/tpm_crb_ffa.c
> > > > +++ b/drivers/char/tpm/tpm_crb_ffa.c
> > > > @@ -12,6 +12,8 @@
> > > >   #include <linux/arm_ffa.h>
> > > >   #include "tpm_crb_ffa.h"
> > > > +#define memzero(s, n) memset((s), 0, (n))
> > > > +
> > > >   /* TPM service function status codes */
> > > >   #define CRB_FFA_OK			0x05000001
> > > >   #define CRB_FFA_OK_RESULTS_RETURNED	0x05000002
> > > > @@ -192,7 +194,7 @@ static int __tpm_crb_ffa_send_receive(unsigned long func_id,
> > > >   	msg_ops = tpm_crb_ffa->ffa_dev->ops->msg_ops;
> > > >   	if (ffa_partition_supports_direct_req2_recv(tpm_crb_ffa->ffa_dev)) {
> > > > -		memset(&tpm_crb_ffa->direct_msg_data2, 0x00,
> > > > +		memzero(&tpm_crb_ffa->direct_msg_data2,
> > > >   		       sizeof(struct ffa_send_direct_data2));
> > > >   		tpm_crb_ffa->direct_msg_data2.data[0] = func_id;
> > > > @@ -205,7 +207,7 @@ static int __tpm_crb_ffa_send_receive(unsigned long func_id,
> > > >   		if (!ret)
> > > >   			ret = tpm_crb_ffa_to_linux_errno(tpm_crb_ffa->direct_msg_data2.data[0]);
> > > >   	} else {
> > > > -		memset(&tpm_crb_ffa->direct_msg_data, 0x00,
> > > > +		memzero(&tpm_crb_ffa->direct_msg_data,
> > > >   		       sizeof(struct ffa_send_direct_data));
> > > >   		tpm_crb_ffa->direct_msg_data.data1 = func_id;
> > > > -- 
> > > > 2.43.0
> > > > 
> > > It adds a ross-reference to the source code, meaning that you have to
> > > jump back and forth in the source code just to see that there is a
> > > function that wraps up a single memset() call.
> > > 
> > > How does that map to "readability"?
> > > 
> > > BR, Jarkko
> > 
> > Hi Jarkko
> > 
> > I've implemented this change post your feedback on v4 of the initial patch
> > series, maybe this should've been a question at that point, but what was the
> > reasoning for recommending that I use memzero instead? I'll use the same
> > reasoning to rephrase the commit message.
> 
> OK I found what you were referring to:
> 
> https://lore.kernel.org/linux-integrity/aFF-WNSolTdV9PZG@kernel.org/
> 
> Well, that was some truly misguided advice from my side so all the shame
> here is on me :-) There's no global memzero() and neither explicit
> version makes much sense here. Sorry about that.
> 
> I gave it now (actual) thought, and here's what I'd propose:
> 
> diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
> index 96746d5b03e3..e769f6143a7c 100644
> --- a/drivers/char/tpm/tpm_crb_ffa.c
> +++ b/drivers/char/tpm/tpm_crb_ffa.c
> @@ -203,26 +203,20 @@ static int __tpm_crb_ffa_try_send_receive(unsigned long func_id,
>  	msg_ops = tpm_crb_ffa->ffa_dev->ops->msg_ops;
>  
>  	if (ffa_partition_supports_direct_req2_recv(tpm_crb_ffa->ffa_dev)) {
> -		memzero(&tpm_crb_ffa->direct_msg_data2,
> -		       sizeof(struct ffa_send_direct_data2));
> -
> -		tpm_crb_ffa->direct_msg_data2.data[0] = func_id;
> -		tpm_crb_ffa->direct_msg_data2.data[1] = a0;
> -		tpm_crb_ffa->direct_msg_data2.data[2] = a1;
> -		tpm_crb_ffa->direct_msg_data2.data[3] = a2;
> +		tpm_crb_ffa->direct_msg_data2 = (struct ffa_send_direct_data2){
> +			.data = { func_id, a0, a1, a2 },
> +		};
>  
>  		ret = msg_ops->sync_send_receive2(tpm_crb_ffa->ffa_dev,
>  				&tpm_crb_ffa->direct_msg_data2);
>  		if (!ret)
>  			ret = tpm_crb_ffa_to_linux_errno(tpm_crb_ffa->direct_msg_data2.data[0]);
>  	} else {
> -		memzero(&tpm_crb_ffa->direct_msg_data,
> -		       sizeof(struct ffa_send_direct_data));
> -
> -		tpm_crb_ffa->direct_msg_data.data1 = func_id;
> -		tpm_crb_ffa->direct_msg_data.data2 = a0;
> -		tpm_crb_ffa->direct_msg_data.data3 = a1;
> -		tpm_crb_ffa->direct_msg_data.data4 = a2;
> +		tpm_crb_ffa->direct_msg_data = (struct ffa_send_direct_data){
> +			.data1 = func_id,
> +			.data2 = a0,
> +			.data3 = a1,

Oops, lacks a2 but you probably get the idea :-)

BR, Jarkko

Re: [PATCH v8 2/3] tpm_crb_ffa:Introduce memzero macro to replace memset

[David Laight]

On Fri, 4 Jul 2025 05:56:50 +0300
Jarkko Sakkinen <jarkko@kernel.org> wrote:

> On Fri, Jul 04, 2025 at 05:45:11AM +0300, Jarkko Sakkinen wrote:
...
> > Well, that was some truly misguided advice from my side so all the shame
> > here is on me :-) There's no global memzero() and neither explicit
> > version makes much sense here. Sorry about that.
> > 
> > I gave it now (actual) thought, and here's what I'd propose:
> > 
> > diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
> > index 96746d5b03e3..e769f6143a7c 100644
> > --- a/drivers/char/tpm/tpm_crb_ffa.c
> > +++ b/drivers/char/tpm/tpm_crb_ffa.c
> > @@ -203,26 +203,20 @@ static int __tpm_crb_ffa_try_send_receive(unsigned long func_id,
> >  	msg_ops = tpm_crb_ffa->ffa_dev->ops->msg_ops;
> >  
> >  	if (ffa_partition_supports_direct_req2_recv(tpm_crb_ffa->ffa_dev)) {
> > -		memzero(&tpm_crb_ffa->direct_msg_data2,
> > -		       sizeof(struct ffa_send_direct_data2));
> > -
> > -		tpm_crb_ffa->direct_msg_data2.data[0] = func_id;
> > -		tpm_crb_ffa->direct_msg_data2.data[1] = a0;
> > -		tpm_crb_ffa->direct_msg_data2.data[2] = a1;
> > -		tpm_crb_ffa->direct_msg_data2.data[3] = a2;
> > +		tpm_crb_ffa->direct_msg_data2 = (struct ffa_send_direct_data2){
> > +			.data = { func_id, a0, a1, a2 },
> > +		};

clang has a habit of compiling that as an un-named on-stack structure that
is initialised and then memcpy() used to copy it into place.
Often not was intended and blows the stack when the structure is large.

So probably not a pattern that should be encouraged.

	David

Re: [PATCH v8 2/3] tpm_crb_ffa:Introduce memzero macro to replace memset

[Jarkko Sakkinen]

On Fri, Jul 04, 2025 at 11:40:10AM +0100, David Laight wrote:
> On Fri, 4 Jul 2025 05:56:50 +0300
> Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> > On Fri, Jul 04, 2025 at 05:45:11AM +0300, Jarkko Sakkinen wrote:
> ...
> > > Well, that was some truly misguided advice from my side so all the shame
> > > here is on me :-) There's no global memzero() and neither explicit
> > > version makes much sense here. Sorry about that.
> > > 
> > > I gave it now (actual) thought, and here's what I'd propose:
> > > 
> > > diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
> > > index 96746d5b03e3..e769f6143a7c 100644
> > > --- a/drivers/char/tpm/tpm_crb_ffa.c
> > > +++ b/drivers/char/tpm/tpm_crb_ffa.c
> > > @@ -203,26 +203,20 @@ static int __tpm_crb_ffa_try_send_receive(unsigned long func_id,
> > >  	msg_ops = tpm_crb_ffa->ffa_dev->ops->msg_ops;
> > >  
> > >  	if (ffa_partition_supports_direct_req2_recv(tpm_crb_ffa->ffa_dev)) {
> > > -		memzero(&tpm_crb_ffa->direct_msg_data2,
> > > -		       sizeof(struct ffa_send_direct_data2));
> > > -
> > > -		tpm_crb_ffa->direct_msg_data2.data[0] = func_id;
> > > -		tpm_crb_ffa->direct_msg_data2.data[1] = a0;
> > > -		tpm_crb_ffa->direct_msg_data2.data[2] = a1;
> > > -		tpm_crb_ffa->direct_msg_data2.data[3] = a2;
> > > +		tpm_crb_ffa->direct_msg_data2 = (struct ffa_send_direct_data2){
> > > +			.data = { func_id, a0, a1, a2 },
> > > +		};
> 
> clang has a habit of compiling that as an un-named on-stack structure that
> is initialised and then memcpy() used to copy it into place.
> Often not was intended and blows the stack when the structure is large.
> 
> So probably not a pattern that should be encouraged.

This is interesting observation so I had to do some compilation tests to
verify the claim just to see how it plays out (and for the sake of
learning while doing it).

Note that I use GCC for the examples but I have high doubts that clang
would do worse. Please share the insight if that is a wrong assumption.

OK, so... here's the dissembly (using objdump) for the  unchanged version:

ffff8000801805a0:	8b020260 	add	x0, x19, x2
ffff8000801805a4:	94011819 	bl	ffff8000801c6608 <__memset>
ffff8000801805a8:	a9035a75 	stp	x21, x22, [x19, #48]
ffff8000801805ac:	aa1a03e1 	mov	x1, x26
ffff8000801805b0:	aa1903e0 	mov	x0, x25
ffff8000801805b4:	a9047e77 	stp	x23, xzr, [x19, #64]

[ Off-topic: note that how a2 gets optimized out with the zero register
  so that it is probably a parameter that we don't need at all in the
  first place? ]

However, in the changed version the matching snippet looks factors
better:

ffff800080180620:	a9017c7f 	stp	xzr, xzr, [x3, #16]
ffff800080180624:	f900107f 	str	xzr, [x3, #32]

Further, look at the stack size in the original version:

ffff800080180524 <__tpm_crb_ffa_send_receive.constprop.0>:
ffff800080180524:	a9ba7bfd 	stp	x29, x30, [sp, #-96]!

On the other hand, in the changed version:

ffff800080180524 <__tpm_crb_ffa_send_receive.constprop.0>:
ffff800080180524:	a9bb7bfd 	stp	x29, x30, [sp, #-80]!

I don't know, at least the figures I'm able to measure with my limited
ARM assembly knowledge look way better.

BR, Jarkko`

Re: [PATCH v8 2/3] tpm_crb_ffa:Introduce memzero macro to replace memset

[David Laight]

On Fri, 4 Jul 2025 17:04:02 +0300
Jarkko Sakkinen <jarkko@kernel.org> wrote:

> On Fri, Jul 04, 2025 at 11:40:10AM +0100, David Laight wrote:
> > On Fri, 4 Jul 2025 05:56:50 +0300
> > Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >   
> > > On Fri, Jul 04, 2025 at 05:45:11AM +0300, Jarkko Sakkinen wrote:  
> > ...  
> > > > Well, that was some truly misguided advice from my side so all the shame
> > > > here is on me :-) There's no global memzero() and neither explicit
> > > > version makes much sense here. Sorry about that.
> > > > 
> > > > I gave it now (actual) thought, and here's what I'd propose:
> > > > 
> > > > diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
> > > > index 96746d5b03e3..e769f6143a7c 100644
> > > > --- a/drivers/char/tpm/tpm_crb_ffa.c
> > > > +++ b/drivers/char/tpm/tpm_crb_ffa.c
> > > > @@ -203,26 +203,20 @@ static int __tpm_crb_ffa_try_send_receive(unsigned long func_id,
> > > >  	msg_ops = tpm_crb_ffa->ffa_dev->ops->msg_ops;
> > > >  
> > > >  	if (ffa_partition_supports_direct_req2_recv(tpm_crb_ffa->ffa_dev)) {
> > > > -		memzero(&tpm_crb_ffa->direct_msg_data2,
> > > > -		       sizeof(struct ffa_send_direct_data2));
> > > > -
> > > > -		tpm_crb_ffa->direct_msg_data2.data[0] = func_id;
> > > > -		tpm_crb_ffa->direct_msg_data2.data[1] = a0;
> > > > -		tpm_crb_ffa->direct_msg_data2.data[2] = a1;
> > > > -		tpm_crb_ffa->direct_msg_data2.data[3] = a2;
> > > > +		tpm_crb_ffa->direct_msg_data2 = (struct ffa_send_direct_data2){
> > > > +			.data = { func_id, a0, a1, a2 },
> > > > +		};  
> > 
> > clang has a habit of compiling that as an un-named on-stack structure that
> > is initialised and then memcpy() used to copy it into place.
> > Often not was intended and blows the stack when the structure is large.
> > 
> > So probably not a pattern that should be encouraged.  
> 
> This is interesting observation so I had to do some compilation tests to
> verify the claim just to see how it plays out (and for the sake of
> learning while doing it).
> 
> Note that I use GCC for the examples but I have high doubts that clang
> would do worse. Please share the insight if that is a wrong assumption.

It is a clang issue and may only affect builds with some of the 'memory
sanitiser' run-time checks.
Search through the mail archives for issues with overlarge stack frames.

	David

> 
> OK, so... here's the dissembly (using objdump) for the  unchanged version:
> 
> ffff8000801805a0:	8b020260 	add	x0, x19, x2
> ffff8000801805a4:	94011819 	bl	ffff8000801c6608 <__memset>
> ffff8000801805a8:	a9035a75 	stp	x21, x22, [x19, #48]
> ffff8000801805ac:	aa1a03e1 	mov	x1, x26
> ffff8000801805b0:	aa1903e0 	mov	x0, x25
> ffff8000801805b4:	a9047e77 	stp	x23, xzr, [x19, #64]
> 
> [ Off-topic: note that how a2 gets optimized out with the zero register
>   so that it is probably a parameter that we don't need at all in the
>   first place? ]
> 
> However, in the changed version the matching snippet looks factors
> better:
> 
> ffff800080180620:	a9017c7f 	stp	xzr, xzr, [x3, #16]
> ffff800080180624:	f900107f 	str	xzr, [x3, #32]
> 
> Further, look at the stack size in the original version:
> 
> ffff800080180524 <__tpm_crb_ffa_send_receive.constprop.0>:
> ffff800080180524:	a9ba7bfd 	stp	x29, x30, [sp, #-96]!
> 
> On the other hand, in the changed version:
> 
> ffff800080180524 <__tpm_crb_ffa_send_receive.constprop.0>:
> ffff800080180524:	a9bb7bfd 	stp	x29, x30, [sp, #-80]!
> 
> I don't know, at least the figures I'm able to measure with my limited
> ARM assembly knowledge look way better.
> 
> BR, Jarkko`
> 

Re: [PATCH v8 2/3] tpm_crb_ffa:Introduce memzero macro to replace memset

[Jarkko Sakkinen]

On Sat, Jul 05, 2025 at 08:10:03AM +0100, David Laight wrote:
> On Fri, 4 Jul 2025 17:04:02 +0300
> Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> > On Fri, Jul 04, 2025 at 11:40:10AM +0100, David Laight wrote:
> > > On Fri, 4 Jul 2025 05:56:50 +0300
> > > Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > >   
> > > > On Fri, Jul 04, 2025 at 05:45:11AM +0300, Jarkko Sakkinen wrote:  
> > > ...  
> > > > > Well, that was some truly misguided advice from my side so all the shame
> > > > > here is on me :-) There's no global memzero() and neither explicit
> > > > > version makes much sense here. Sorry about that.
> > > > > 
> > > > > I gave it now (actual) thought, and here's what I'd propose:
> > > > > 
> > > > > diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
> > > > > index 96746d5b03e3..e769f6143a7c 100644
> > > > > --- a/drivers/char/tpm/tpm_crb_ffa.c
> > > > > +++ b/drivers/char/tpm/tpm_crb_ffa.c
> > > > > @@ -203,26 +203,20 @@ static int __tpm_crb_ffa_try_send_receive(unsigned long func_id,
> > > > >  	msg_ops = tpm_crb_ffa->ffa_dev->ops->msg_ops;
> > > > >  
> > > > >  	if (ffa_partition_supports_direct_req2_recv(tpm_crb_ffa->ffa_dev)) {
> > > > > -		memzero(&tpm_crb_ffa->direct_msg_data2,
> > > > > -		       sizeof(struct ffa_send_direct_data2));
> > > > > -
> > > > > -		tpm_crb_ffa->direct_msg_data2.data[0] = func_id;
> > > > > -		tpm_crb_ffa->direct_msg_data2.data[1] = a0;
> > > > > -		tpm_crb_ffa->direct_msg_data2.data[2] = a1;
> > > > > -		tpm_crb_ffa->direct_msg_data2.data[3] = a2;
> > > > > +		tpm_crb_ffa->direct_msg_data2 = (struct ffa_send_direct_data2){
> > > > > +			.data = { func_id, a0, a1, a2 },
> > > > > +		};  
> > > 
> > > clang has a habit of compiling that as an un-named on-stack structure that
> > > is initialised and then memcpy() used to copy it into place.
> > > Often not was intended and blows the stack when the structure is large.
> > > 
> > > So probably not a pattern that should be encouraged.  
> > 
> > This is interesting observation so I had to do some compilation tests to
> > verify the claim just to see how it plays out (and for the sake of
> > learning while doing it).
> > 
> > Note that I use GCC for the examples but I have high doubts that clang
> > would do worse. Please share the insight if that is a wrong assumption.
> 
> It is a clang issue and may only affect builds with some of the 'memory
> sanitiser' run-time checks.
> Search through the mail archives for issues with overlarge stack frames.

If clang really did that, it would cost at worst 40 bytes of stack (aka
the size of struct ffa_send_direct_data. And if there is an issue, it
absolutely will get fixed.

That does not weight over making a code change that makes the most sense
for Linux in the long-term. And to add, I did show both the code and
the figures to support my claim.
 

> 
> 	David

BR, Jarkko