From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id C35482DBF66; Wed, 25 Jun 2025 19:10:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1750878605; cv=none; b=seflYlYJ1ZUnPHJTQ5f5C39/LM9PwHzwD0jWfjz7GumwRhiSZLSLbz2sLMuZRaN7orXS5gkEqIbhiAAlbWx6g1FAUf/TPj03PFab+ZzdmI89I5RlYVszI2Dh/66RGGAtjrqbrMhyDVkvIIMUg8zFiohORUUlrfuvaNlPRxcWPkY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1750878605; c=relaxed/simple; bh=FnMYmoSUgheFarqpQRcfgwPFxcfdXrUXOM0r5RbD6pA=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=K7d4Qp60QI3Mgoi/5kUFehnraPJqDaNYUul0dGxg0kGTJlY+tQYJas415jfHYzAFp+rmXWkSLZcBkumAqMcScvgy/FB+6ap48/yMGzRWE/SD64G9eTZVbRbTCJMkQJiGLnC+OEwzp+v+hXdbD8twTRrfg+pLXmWlfpr8TON7xTk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id B1FBF106F; Wed, 25 Jun 2025 12:09:41 -0700 (PDT) Received: from [10.118.109.149] (Prachotans-MBP.austin.arm.com [10.118.109.149]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id A0BB23F58B; Wed, 25 Jun 2025 12:09:59 -0700 (PDT) Message-ID: Date: Wed, 25 Jun 2025 14:09:50 -0500 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v6 2/2] tpm_crb_ffa: handle tpm busy return code To: Jarkko Sakkinen Cc: Peter Huewe , Jason Gunthorpe , Stuart Yoder , linux-integrity@vger.kernel.org, linux-kernel@vger.kernel.org References: <20250617160544.444321-1-prachotan.bathi@arm.com> <20250617160544.444321-3-prachotan.bathi@arm.com> Content-Language: en-US From: Prachotan Bathi In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 6/24/25 6:21 PM, Jarkko Sakkinen wrote: > On Tue, Jun 17, 2025 at 11:05:44AM -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 >> --- >> drivers/char/tpm/tpm_crb_ffa.c | 66 +++++++++++++++++++++++++++------- >> 1 file changed, 53 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c >> index 089d1e54bb46..4615347795fa 100644 >> --- a/drivers/char/tpm/tpm_crb_ffa.c >> +++ b/drivers/char/tpm/tpm_crb_ffa.c >> @@ -10,8 +10,12 @@ >> #define pr_fmt(fmt) "CRB_FFA: " fmt >> >> #include >> +#include >> +#include >> #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 >> @@ -178,22 +182,18 @@ 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)) { >> - memset(&tpm_crb_ffa->direct_msg_data2, 0x00, >> - sizeof(struct ffa_send_direct_data2)); >> + 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; >> @@ -201,12 +201,12 @@ static int __tpm_crb_ffa_send_receive(unsigned long func_id, >> tpm_crb_ffa->direct_msg_data2.data[3] = a2; >> >> ret = msg_ops->sync_send_receive2(tpm_crb_ffa->ffa_dev, >> - &tpm_crb_ffa->direct_msg_data2); >> + &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)); >> + 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; >> @@ -214,11 +214,51 @@ static int __tpm_crb_ffa_send_receive(unsigned long func_id, >> 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); >> + &tpm_crb_ffa->direct_msg_data); >> if (!ret) >> ret = tpm_crb_ffa_to_linux_errno(tpm_crb_ffa->direct_msg_data.data1); >> } >> >> + return ret; >> +} >> + >> +static unsigned int busy_timeout_ms = 2000; >> +/** >> + * busy_timeout_ms - Maximum time to retry before giving up on busy > nit: s/busy_timeout_ms/busy_timeout_ms()/ > >> + * >> + * 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"); > Patch lacks update to Documentation/admin-guide/kernel-parameters.rst > (and also document that the default value is two seconds). > >> + >> +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) { > This loop would be less convoluted if you instead: > > if (ret != -EBUSY) > break; > > /* ... */ > >> + usleep_range(50, 100); > I wonder where this range comes from. This range comes from a similar timeout defined in the crb_wait_for_reg_32 function. See: https://github.com/torvalds/linux/blob/c4dce0c094a89b1bc8fde1163342bd6fe29c0370/drivers/char/tpm/tpm_crb.c#L153 A TPM Service might have a discrete TPM (dtpm) that it communicates with, a busy SP might become available again and be able to proxy commands to a dtpm within a similar retry window. This window works well with current internal implementations and can be changed as future implementations and specifications evolve and define a more sophisticated retry window. >> + if (ktime_after(ktime_get(), stop)) { >> + dev_warn(&tpm_crb_ffa->ffa_dev->dev, >> + "Busy retry timed out\n"); >> + break; >> + } >> + } else { >> + break; >> + } >> + } >> >> return ret; >> } >> -- >> 2.43.0 >> > BR, Jarkko On 6/24/25 6:21 PM, Jarkko Sakkinen wrote: > On Tue, Jun 17, 2025 at 11:05:44AM -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 >> --- >> drivers/char/tpm/tpm_crb_ffa.c | 66 +++++++++++++++++++++++++++------- >> 1 file changed, 53 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/char/tpm/tpm_crb_ffa.c b/drivers/char/tpm/tpm_crb_ffa.c >> index 089d1e54bb46..4615347795fa 100644 >> --- a/drivers/char/tpm/tpm_crb_ffa.c >> +++ b/drivers/char/tpm/tpm_crb_ffa.c >> @@ -10,8 +10,12 @@ >> #define pr_fmt(fmt) "CRB_FFA: " fmt >> >> #include >> +#include >> +#include >> #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 >> @@ -178,22 +182,18 @@ 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)) { >> - memset(&tpm_crb_ffa->direct_msg_data2, 0x00, >> - sizeof(struct ffa_send_direct_data2)); >> + 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; >> @@ -201,12 +201,12 @@ static int __tpm_crb_ffa_send_receive(unsigned long func_id, >> tpm_crb_ffa->direct_msg_data2.data[3] = a2; >> >> ret = msg_ops->sync_send_receive2(tpm_crb_ffa->ffa_dev, >> - &tpm_crb_ffa->direct_msg_data2); >> + &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)); >> + 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; >> @@ -214,11 +214,51 @@ static int __tpm_crb_ffa_send_receive(unsigned long func_id, >> 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); >> + &tpm_crb_ffa->direct_msg_data); >> if (!ret) >> ret = tpm_crb_ffa_to_linux_errno(tpm_crb_ffa->direct_msg_data.data1); >> } >> >> + return ret; >> +} >> + >> +static unsigned int busy_timeout_ms = 2000; >> +/** >> + * busy_timeout_ms - Maximum time to retry before giving up on busy > nit: s/busy_timeout_ms/busy_timeout_ms()/ > >> + * >> + * 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"); > Patch lacks update to Documentation/admin-guide/kernel-parameters.rst > (and also document that the default value is two seconds). > >> + >> +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) { > This loop would be less convoluted if you instead: > > if (ret != -EBUSY) > break; > > /* ... */ > >> + usleep_range(50, 100); > I wonder where this range comes from. > >> + if (ktime_after(ktime_get(), stop)) { >> + dev_warn(&tpm_crb_ffa->ffa_dev->dev, >> + "Busy retry timed out\n"); >> + break; >> + } >> + } else { >> + break; >> + } >> + } >> >> return ret; >> } >> -- >> 2.43.0 >> > BR, Jarkko