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

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

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

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

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

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

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`

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`
>

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