diff mbox series

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

Message ID 292d7702e946ca513af51236ca9e38bf1b1eb269.1716359578.git.quic_nguyenb@quicinc.com
State New
Headers show
Series Allow vendor drivers to update UIC command timeout | expand

Commit Message

Bao D. Nguyen May 22, 2024, 7:01 a.m. UTC
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(-)

Comments

Bao D. Nguyen May 22, 2024, 8:51 p.m. UTC | #1
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.
Manivannan Sadhasivam May 23, 2024, 4:38 a.m. UTC | #2
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
Bao D. Nguyen May 23, 2024, 5:40 a.m. UTC | #3
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
>
diff mbox series

Patch

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;
 };
 
 /**