On Wed, Jun 11, 2025 at 02:30:09PM -0500, Prachotan Bathi wrote:
> Platforms supporting direct message request v2 can
> support SPs that support multiple services.
> If the TPM service is sharing the SP with another service,
What is "TPM service" and "SP"?
> it could get an error code of BUSY if the other service is
> in process.
> This adds a parameterized variable (default 2000ms)
What is "this"?
"Add a ..."
> that indicates the maximum time to keep retrying for.
> When the TPM service returns a BUSY error code, we sleep
s/we sleep/sleep/
> 50-100us before retrying again, until we are over the
> busy timeout.
>
> Signed-off-by: Prachotan Bathi <prachotan.ba...@arm.com>
I'd consider rewriting the commit message as it lacks any explanation
of the code change. You replace condition statement with a loop. Start
with that.
I'd strip some of the comments too i.e, those that state the obvious.
> ---
> drivers/char/tpm/tpm_crb_ffa.c | 78 +++++++++++++++++++++++-----------
> 1 file changed, 54 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c
> index 4ead61f01299..5f0a30a20b57 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"
>
> /* TPM service function status codes */
> @@ -178,6 +180,17 @@ int tpm_crb_ffa_init(void)
> }
> EXPORT_SYMBOL_GPL(tpm_crb_ffa_init);
>
> +static unsigned int busy_timeout = 2000;
> +/**
> + * busy_timeout - Maximum time to retry before giving up on busy
> + *
> + * This parameter defines the maximum time in milliseconds to retry
> + * sending a message to the TPM service before giving up.
> + */
> +module_param(busy_timeout, uint, 0644);
> +MODULE_PARM_DESC(busy_timeout,
> + "Maximum time to retry before giving up on busy");
> +
> static int __tpm_crb_ffa_send_recieve(unsigned long func_id,
> unsigned long a0,
> unsigned long a1,
> @@ -191,34 +204,51 @@ static int __tpm_crb_ffa_send_recieve(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,
> - sizeof(struct ffa_send_direct_data2));
> + ktime_t start;
> + ktime_t stop;
> +
> + start = ktime_get();
> + stop = ktime_add(start, ms_to_ktime(busy_timeout));
> +
> + do {
> + if
> (ffa_partition_supports_direct_req2_recv(tpm_crb_ffa->ffa_dev)) {
> + memset(&tpm_crb_ffa->direct_msg_data2, 0x00,
> + 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.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;
>
> - ret = msg_ops->sync_send_receive2(tpm_crb_ffa->ffa_dev,
> + 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 {
> - memset(&tpm_crb_ffa->direct_msg_data, 0x00,
> - 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;
> -
> - ret = msg_ops->sync_send_receive(tpm_crb_ffa->ffa_dev,
> - &tpm_crb_ffa->direct_msg_data);
> - if (!ret)
> - ret =
> tpm_crb_ffa_to_linux_errno(tpm_crb_ffa->direct_msg_data.data1);
> - }
> + 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,
> + 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;
> +
> + ret = msg_ops->sync_send_receive(tpm_crb_ffa->ffa_dev,
> + &tpm_crb_ffa->direct_msg_data);
> + if (!ret)
> + ret =
> tpm_crb_ffa_to_linux_errno(tpm_crb_ffa->direct_msg_data.data1);
> + }
> + /* Check if the TPM service returned busy */
Strip.
> + if (ret == -EBUSY)
> + pr_err("TPM says busy, trying again, value, ret: %d\n",
> + ret);
> + else
> + /* If TPM did not return busy, we are done, exit the
> loop */
Ditto.
> + break;
> + /* Sleep for a short time before retrying to avoid busy looping
> */
Ditto.
> + usleep_range(50, 100);
> + } while (ktime_before(ktime_get(), stop));
> + /* If we are here, we either got a valid response or over the busy
> timeout */
Ditto.
>
> return ret;
> }
> --
> 2.43.0
>
BR, Jarkko
