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.ba...@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-wnsoltdv9...@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
