[PATCH v1 0/2] Allow vendor drivers to update UIC command timeout

[PATCH v1 0/2] Allow vendor drivers to update UIC command timeout

[Bao D. Nguyen]

The UIC command timeout default value remains as 500ms.

Allow vendor drivers to change the UIC command timeout as they wish.
During product development where a lot of debug logging/printing can
occur, the uart may print from different modules with interrupt disabled
for more than 500ms, causing interrupt starvation and UIC command timeout
as a result. The UIC command timeout may eventually cause a watchdog
timeout unnecessarily. With this change, the vendor drivers can set a
different UIC command timeout during their product development and
revert to the default 500ms when development completes as desired.

Bao D. Nguyen (2):
  scsi: ufs: core: Support Updating UIC Command Timeout
  scsi: ufs: qcom: Update the UIC Command Timeout

 drivers/ufs/core/ufshcd.c   | 9 ++++++---
 drivers/ufs/host/ufs-qcom.c | 3 +++
 include/ufs/ufshcd.h        | 2 ++
 3 files changed, 11 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH v1 1/2] scsi: ufs: core: Support Updating UIC Command Timeout

[Bao D. Nguyen]

The default UIC command timeout still remains 500ms.
Allow vendor drivers to override the UIC command timeout if desired.

In a real product, the 500ms timeout value is probably good enough.
However, during the product development where there are a lot of
logging and debug messages being printed to the uart console,
interrupt starvations happen occasionally because the uart may
print long debug messages from different modules in the system.
While printing, the uart may have interrupts disabled for more
than 500ms, causing UIC command timeout.
The UIC command timeout would trigger more printing from the
UFS driver, and eventually a watchdog timeout may occur unnecessarily.

Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
---
 drivers/ufs/core/ufshcd.c | 9 ++++++---
 include/ufs/ufshcd.h      | 2 ++
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 21429ee..c440caf 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2460,7 +2460,7 @@ static inline bool ufshcd_ready_for_uic_cmd(struct ufs_hba *hba)
 {
 	u32 val;
 	int ret = read_poll_timeout(ufshcd_readl, val, val & UIC_COMMAND_READY,
-				    500, UIC_CMD_TIMEOUT * 1000, false, hba,
+				    500, hba->uic_cmd_timeout * 1000, false, hba,
 				    REG_CONTROLLER_STATUS);
 	return ret == 0;
 }
@@ -2520,7 +2520,7 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
 	lockdep_assert_held(&hba->uic_cmd_mutex);
 
 	if (wait_for_completion_timeout(&uic_cmd->done,
-					msecs_to_jiffies(UIC_CMD_TIMEOUT))) {
+					msecs_to_jiffies(hba->uic_cmd_timeout))) {
 		ret = uic_cmd->argument2 & MASK_UIC_COMMAND_RESULT;
 	} else {
 		ret = -ETIMEDOUT;
@@ -4298,7 +4298,7 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
 	}
 
 	if (!wait_for_completion_timeout(hba->uic_async_done,
-					 msecs_to_jiffies(UIC_CMD_TIMEOUT))) {
+					 msecs_to_jiffies(hba->uic_cmd_timeout))) {
 		dev_err(hba->dev,
 			"pwr ctrl cmd 0x%x with mode 0x%x completion timeout\n",
 			cmd->command, cmd->argument3);
@@ -10690,6 +10690,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 			    FIELD_PREP(UFSHCI_AHIBERN8_SCALE_MASK, 3);
 	}
 
+	if (!hba->uic_cmd_timeout)
+		hba->uic_cmd_timeout = UIC_CMD_TIMEOUT;
+
 	/* Hold auto suspend until async scan completes */
 	pm_runtime_get_sync(dev);
 	atomic_set(&hba->scsi_block_reqs_cnt, 0);
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index a35e12f..47e3bdf 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -917,6 +917,7 @@ enum ufshcd_mcq_opr {
  * @ufs_rtc_update_work: A work for UFS RTC periodic update
  * @pm_qos_req: PM QoS request handle
  * @pm_qos_enabled: flag to check if pm qos is enabled
+ * @uic_cmd_timeout: timeout in ms for UIC commands
  */
 struct ufs_hba {
 	void __iomem *mmio_base;
@@ -1085,6 +1086,7 @@ struct ufs_hba {
 	struct delayed_work ufs_rtc_update_work;
 	struct pm_qos_request pm_qos_req;
 	bool pm_qos_enabled;
+	u32 uic_cmd_timeout;
 };
 
 /**
-- 
2.7.4

Re: [PATCH v1 1/2] scsi: ufs: core: Support Updating UIC Command Timeout

[Bart Van Assche]

On 5/22/24 00:01, Bao D. Nguyen wrote:
> interrupt starvations happen occasionally because the uart may
> print long debug messages from different modules in the system.

I think that's a bug in the UART driver that should be fixed in the
UART driver.

Thanks,

Bart.

Re: [PATCH v1 1/2] scsi: ufs: core: Support Updating UIC Command Timeout

[Bao D. Nguyen]

On 5/22/2024 11:16 AM, Bart Van Assche wrote:
> On 5/22/24 00:01, Bao D. Nguyen wrote:
>> interrupt starvations happen occasionally because the uart may
>> print long debug messages from different modules in the system.
> 
> I think that's a bug in the UART driver that should be fixed in the
> UART driver.

Thanks Bart.
I am not familiar with the UART drivers. I looked at some UART code and 
it could be interpreted as their choice of implementation.
During product development, the UART may be used. However, when the 
development completes, most likely the UART logging is disabled due to 
performance reason.

This change is to give flexibility to the SoCs to use the UART 
implementation of their choice and to choose the desired UIC command 
timeout without affecting the system stability or the default hardcoded 
UIC timeout value of 500ms that others may be using.

Bao

> 
> Thanks,
> 
> Bart.

Re: [PATCH v1 1/2] scsi: ufs: core: Support Updating UIC Command Timeout

[Manivannan Sadhasivam]

On Wed, May 22, 2024 at 01:51:06PM -0700, Bao D. Nguyen wrote:
> On 5/22/2024 11:16 AM, Bart Van Assche wrote:
> > On 5/22/24 00:01, Bao D. Nguyen wrote:
> > > interrupt starvations happen occasionally because the uart may
> > > print long debug messages from different modules in the system.
> > 
> > I think that's a bug in the UART driver that should be fixed in the
> > UART driver.
> 
> Thanks Bart.
> I am not familiar with the UART drivers. I looked at some UART code and it
> could be interpreted as their choice of implementation.
> During product development, the UART may be used. However, when the
> development completes, most likely the UART logging is disabled due to
> performance reason.
> 
> This change is to give flexibility to the SoCs to use the UART
> implementation of their choice and to choose the desired UIC command timeout
> without affecting the system stability or the default hardcoded UIC timeout
> value of 500ms that others may be using.
> 

If UART runs in atomic context for 500ms, then it is doomed.

But for increasing the UIC timeout, I agree that the flexibility is acceptable.
In that case, please use a user configurable option like cmdline etc... instead
of hardcoding the value in glue drivers.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v1 1/2] scsi: ufs: core: Support Updating UIC Command Timeout

[Bao D. Nguyen]

On 5/22/2024 9:38 PM, Manivannan Sadhasivam wrote:
> On Wed, May 22, 2024 at 01:51:06PM -0700, Bao D. Nguyen wrote:
>> On 5/22/2024 11:16 AM, Bart Van Assche wrote:
>>> On 5/22/24 00:01, Bao D. Nguyen wrote:
>>>> interrupt starvations happen occasionally because the uart may
>>>> print long debug messages from different modules in the system.
>>>
>>> I think that's a bug in the UART driver that should be fixed in the
>>> UART driver.
>>
>> Thanks Bart.
>> I am not familiar with the UART drivers. I looked at some UART code and it
>> could be interpreted as their choice of implementation.
>> During product development, the UART may be used. However, when the
>> development completes, most likely the UART logging is disabled due to
>> performance reason.
>>
>> This change is to give flexibility to the SoCs to use the UART
>> implementation of their choice and to choose the desired UIC command timeout
>> without affecting the system stability or the default hardcoded UIC timeout
>> value of 500ms that others may be using.
>>
> 
> If UART runs in atomic context for 500ms, then it is doomed.
> 
> But for increasing the UIC timeout, I agree that the flexibility is acceptable.
> In that case, please use a user configurable option like cmdline etc... instead
> of hardcoding the value in glue drivers.

Thanks Mani. Bart also suggested something along that line.

Thanks, Bao

> 
> - Mani
> 

[PATCH v1 2/2] scsi: ufs: qcom: Update the UIC Command Timeout

[Bao D. Nguyen]

Change the UIC command timeout to 2 seconds.
This extra time is to allow the uart occasionally print long
debug messages and logging from different modules during
product development. With the default hardcoded 500ms timeout,
the uart printing with interrupt disabled may cause the UIC command
interrupt get starved, resulting in a UIC command timeout and
eventually a watchdog timeout.
When a product development completes, the vendors may
select a different UIC command timeout as desired.

Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
---
 drivers/ufs/host/ufs-qcom.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 79f8cb3..4649e0f 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -49,6 +49,7 @@ enum {
 
 #define QCOM_UFS_MAX_GEAR 4
 #define QCOM_UFS_MAX_LANE 2
+#define QCOM_UIC_CMD_TIMEOUT_MS 2000
 
 enum {
 	MODE_MIN,
@@ -1111,6 +1112,8 @@ static int ufs_qcom_init(struct ufs_hba *hba)
 		dev_warn(dev, "%s: failed to configure the testbus %d\n",
 				__func__, err);
 
+	hba->uic_cmd_timeout = QCOM_UIC_CMD_TIMEOUT_MS;
+
 	return 0;
 
 out_variant_clear:
-- 
2.7.4

Re: [PATCH v1 2/2] scsi: ufs: qcom: Update the UIC Command Timeout

[Bart Van Assche]

On 5/22/24 00:01, Bao D. Nguyen wrote:
> Change the UIC command timeout to 2 seconds.
> This extra time is to allow the uart occasionally print long
> debug messages and logging from different modules during
> product development. With the default hardcoded 500ms timeout,
> the uart printing with interrupt disabled may cause the UIC command
> interrupt get starved, resulting in a UIC command timeout and
> eventually a watchdog timeout.
> When a product development completes, the vendors may
> select a different UIC command timeout as desired.
> 
> Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
> ---
>   drivers/ufs/host/ufs-qcom.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 79f8cb3..4649e0f 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -49,6 +49,7 @@ enum {
>   
>   #define QCOM_UFS_MAX_GEAR 4
>   #define QCOM_UFS_MAX_LANE 2
> +#define QCOM_UIC_CMD_TIMEOUT_MS 2000
>   
>   enum {
>   	MODE_MIN,
> @@ -1111,6 +1112,8 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>   		dev_warn(dev, "%s: failed to configure the testbus %d\n",
>   				__func__, err);
>   
> +	hba->uic_cmd_timeout = QCOM_UIC_CMD_TIMEOUT_MS;
> +
>   	return 0;
>   
>   out_variant_clear:

Given the description of patch 1, the addressed issue is not specific to
a single vendor. Is that correct?

Since the described issue is only encountered during development, why to
modify the UIC command timeout unconditionally?

Thanks,

Bart.

Re: [PATCH v1 2/2] scsi: ufs: qcom: Update the UIC Command Timeout

[Bao D. Nguyen]

On 5/22/2024 11:18 AM, Bart Van Assche wrote:
> On 5/22/24 00:01, Bao D. Nguyen wrote:
>> Change the UIC command timeout to 2 seconds.
>> This extra time is to allow the uart occasionally print long
>> debug messages and logging from different modules during
>> product development. With the default hardcoded 500ms timeout,
>> the uart printing with interrupt disabled may cause the UIC command
>> interrupt get starved, resulting in a UIC command timeout and
>> eventually a watchdog timeout.
>> When a product development completes, the vendors may
>> select a different UIC command timeout as desired.
>>
>> Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
>> ---
>>   drivers/ufs/host/ufs-qcom.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index 79f8cb3..4649e0f 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -49,6 +49,7 @@ enum {
>>   #define QCOM_UFS_MAX_GEAR 4
>>   #define QCOM_UFS_MAX_LANE 2
>> +#define QCOM_UIC_CMD_TIMEOUT_MS 2000
>>   enum {
>>       MODE_MIN,
>> @@ -1111,6 +1112,8 @@ static int ufs_qcom_init(struct ufs_hba *hba)
>>           dev_warn(dev, "%s: failed to configure the testbus %d\n",
>>                   __func__, err);
>> +    hba->uic_cmd_timeout = QCOM_UIC_CMD_TIMEOUT_MS;
>> +
>>       return 0;
>>   out_variant_clear:
> 
> Given the description of patch 1, the addressed issue is not specific to
> a single vendor. Is that correct?

The issue we are trying to address is not specific to a single vendor.
Any vendor would have a freedom to choose the UIC command timeout. At 
different times during their product development, they can choose 
different UIC command timeout, or use the default 500ms. It's the 
flexibility we would like to give to vendors.

> 
> Since the described issue is only encountered during development, why to
> modify the UIC command timeout unconditionally?

The vendors can enjoy the default 500ms UIC timeout if they prefer.
As long as they don't write to hba->uic_cmd_timeout in the vendor's 
initialization routine, the default value of 500ms will be used.

Thanks,
Bao

> 
> Thanks,
> 
> Bart.

Re: [PATCH v1 2/2] scsi: ufs: qcom: Update the UIC Command Timeout

[Bart Van Assche]

On 5/22/24 13:56, Bao D. Nguyen wrote:
> On 5/22/2024 11:18 AM, Bart Van Assche wrote:
>> Since the described issue is only encountered during development, why to
>> modify the UIC command timeout unconditionally?
> 
> The vendors can enjoy the default 500ms UIC timeout if they prefer.
> As long as they don't write to hba->uic_cmd_timeout in the vendor's initialization routine, the default value of 500ms will be used.

Since this issue is not vendor specific, I think it would be better to
modify the UFSHCI core driver only. Has it been considered to introduce a
kernel module parameter for setting the UIC command timeout instead of the
approach of this patch? As you probably know there are multiple mechanisms
for specifying kernel module parameters, e.g. the bootargs parameter in the
device tree.

Thanks,

Bart.

RE: [PATCH v1 2/2] scsi: ufs: qcom: Update the UIC Command Timeout

[Avri Altman]

> On 5/22/24 13:56, Bao D. Nguyen wrote:
> > On 5/22/2024 11:18 AM, Bart Van Assche wrote:
> >> Since the described issue is only encountered during development, why to
> >> modify the UIC command timeout unconditionally?
> >
> > The vendors can enjoy the default 500ms UIC timeout if they prefer.
> > As long as they don't write to hba->uic_cmd_timeout in the vendor's
> initialization routine, the default value of 500ms will be used.
> 
> Since this issue is not vendor specific, I think it would be better to
> modify the UFSHCI core driver only. Has it been considered to introduce a
> kernel module parameter for setting the UIC command timeout instead of the
> approach of this patch? As you probably know there are multiple mechanisms
> for specifying kernel module parameters, e.g. the bootargs parameter in the
> device tree.
Since the problem statement is "During product development...", why not just a debugfs?

Thanks,
Avri

> 
> Thanks,
> 
> Bart.

Re: [PATCH v1 2/2] scsi: ufs: qcom: Update the UIC Command Timeout

[Bao D. Nguyen]

On 5/22/2024 8:18 PM, Avri Altman wrote:
>> On 5/22/24 13:56, Bao D. Nguyen wrote:
>>> On 5/22/2024 11:18 AM, Bart Van Assche wrote:
>>>> Since the described issue is only encountered during development, why to
>>>> modify the UIC command timeout unconditionally?
>>>
>>> The vendors can enjoy the default 500ms UIC timeout if they prefer.
>>> As long as they don't write to hba->uic_cmd_timeout in the vendor's
>> initialization routine, the default value of 500ms will be used.
>>
>> Since this issue is not vendor specific, I think it would be better to
>> modify the UFSHCI core driver only. Has it been considered to introduce a
>> kernel module parameter for setting the UIC command timeout instead of the
>> approach of this patch? As you probably know there are multiple mechanisms
>> for specifying kernel module parameters, e.g. the bootargs parameter in the
>> device tree.
> Since the problem statement is "During product development...", why not just a debugfs?

Hi Avri, if I am not mistaken, debugfs is not available at the initial 
stage of ufs probe/init time right?

> 
> Thanks,
> Avri
> 
>>
>> Thanks,
>>
>> Bart.
> 

Re: [PATCH v1 2/2] scsi: ufs: qcom: Update the UIC Command Timeout

[Bao D. Nguyen]

On 5/22/2024 2:01 PM, Bart Van Assche wrote:
> On 5/22/24 13:56, Bao D. Nguyen wrote:
>> On 5/22/2024 11:18 AM, Bart Van Assche wrote:
>>> Since the described issue is only encountered during development, why to
>>> modify the UIC command timeout unconditionally?
>>
>> The vendors can enjoy the default 500ms UIC timeout if they prefer.
>> As long as they don't write to hba->uic_cmd_timeout in the vendor's 
>> initialization routine, the default value of 500ms will be used.
> 
> Since this issue is not vendor specific, I think it would be better to
> modify the UFSHCI core driver only. Has it been considered to introduce a
> kernel module parameter for setting the UIC command timeout instead of the
> approach of this patch? As you probably know there are multiple mechanisms
> for specifying kernel module parameters, e.g. the bootargs parameter in the
> device tree.

The proposal here uses similar implementation as the auto hibern8 timer 
(hba->ahit). The ahit mechanism has been working well and stable, so I 
thought why not keep using it? Let see if other members have any 
comments about kernel module parameter/bootargs suggestion.

Thanks, Bao

> 
> Thanks,
> 
> Bart.
> 

Re: [PATCH v1 0/2] Allow vendor drivers to update UIC command timeout

[Christoph Hellwig]

There is no such thing as a vendor driver in Linux.

Re: [PATCH v1 0/2] Allow vendor drivers to update UIC command timeout

[Bao D. Nguyen]

On 5/23/2024 1:17 AM, Christoph Hellwig wrote:
> There is no such thing as a vendor driver in Linux.
> 
I will reword it in the next revision.

Thanks, Bao