There are two issues on UIC command handling related to auto hibern8.
https://lore.kernel.org/linux-scsi/[email protected]/
https://lore.kernel.org/linux-scsi/[email protected]/
Now I combine the two things into this thread.
And one thing added to make part of submitting a UIC command apart
from the critical section w/ host_lock.
Kiwoong Kim (3):
ufs: make __ufshcd_send_uic_cmd not wrapped by host_lock
ufs: poll HCS.UCRDY before issuing a UIC command
ufs: poll pmc until another pa request is completed
drivers/ufs/core/ufshcd.c | 109 ++++++++++++++++++++++++++++++++--------------
1 file changed, 77 insertions(+), 32 deletions(-)
--
2.7.4
__ufshcd_send_uic_cmd is wrapped uic_cmd_mutex and
its related contexts are accessed within the period wrappted
by uic_cmd_mutex. Thus, wrapping with host_lock is
redundant.
Signed-off-by: Kiwoong Kim <[email protected]>
---
drivers/ufs/core/ufshcd.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 9434328..e096401 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2457,7 +2457,6 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd,
bool completion)
{
lockdep_assert_held(&hba->uic_cmd_mutex);
- lockdep_assert_held(hba->host->host_lock);
if (!ufshcd_ready_for_uic_cmd(hba)) {
dev_err(hba->dev,
@@ -2493,9 +2492,7 @@ int ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
mutex_lock(&hba->uic_cmd_mutex);
ufshcd_add_delay_before_dme_cmd(hba);
- spin_lock_irqsave(hba->host->host_lock, flags);
ret = __ufshcd_send_uic_cmd(hba, uic_cmd, true);
- spin_unlock_irqrestore(hba->host->host_lock, flags);
if (!ret)
ret = ufshcd_wait_for_uic_cmd(hba, uic_cmd);
@@ -4180,8 +4177,8 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
wmb();
reenable_intr = true;
}
- ret = __ufshcd_send_uic_cmd(hba, cmd, false);
spin_unlock_irqrestore(hba->host->host_lock, flags);
+ ret = __ufshcd_send_uic_cmd(hba, cmd, false);
if (ret) {
dev_err(hba->dev,
"pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n",
--
2.7.4
v1 -> v2: replace usleep_range with udelay
because it's a sleepable period.
With auto hibern8 enabled, UIC could be working
for a while to process a hibern8 operation and HCI
reports UIC not ready for a short term through HCS.UCRDY.
And UFS driver can't recognize the operation.
UFSHCI spec specifies UCRDY like this:
whether the host controller is ready to process UIC COMMAND
The 'ready' could be seen as many different meanings. If the meaning
includes not processing any request from HCI, processing a hibern8
operation can be 'not ready'. In this situation, the driver needs to
wait until the operations is completed.
Signed-off-by: Kiwoong Kim <[email protected]>
---
drivers/ufs/core/ufshcd.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index e096401..a772f92 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2365,7 +2365,18 @@ static inline int ufshcd_hba_capabilities(struct ufs_hba *hba)
*/
static inline bool ufshcd_ready_for_uic_cmd(struct ufs_hba *hba)
{
- return ufshcd_readl(hba, REG_CONTROLLER_STATUS) & UIC_COMMAND_READY;
+ ktime_t timeout = ktime_add_ms(ktime_get(), UIC_CMD_TIMEOUT);
+ u32 val = 0;
+
+ do {
+ val = ufshcd_readl(hba, REG_CONTROLLER_STATUS) &
+ UIC_COMMAND_READY;
+ if (val)
+ break;
+ udelay(500);
+ } while (ktime_before(ktime_get(), timeout));
+
+ return val ? true : false;
}
/**
--
2.7.4
v2 -> v3
1) check time in the loop with jiffies, instead of miliseconds.
2) change a variable's name and fix a typo and wrong alignment.
v1 -> v2
1) remove clearing hba->active_uic_cmd at the end of __ufshcd_poll_uic_pwr
2) change commit message on the cited clause: 5.7.12.11 -> 5.7.12.1.1
3) add mdelay to avoid too many issueing
4) change the timeout for the polling to 100ms because I found it
sometimes takes much longer than expected.
Regarding 5.7.12.1.1 in Unipro v1.8, PA rejects sebsequent
requests following the first request from upper layer or remote.
In this situation, PA responds w/ BUSY in cases
when they come from upper layer and does nothing for
the requests. So HCI doesn't receive ind, a.k.a. indicator
for its requests and an interrupt, IS.UPMS isn't generated.
When LINERESET occurs, the error handler issues PMC which is
recognized as a request for PA. If a host's PA gets or raises
LINERESET, and a request for PMC, this could be a concurrent
situation mentioned above. And I found that situation w/ my
environment.
[ 222.929539]I[0:DefaultDispatch:20410] exynos-ufs 13500000.ufs: ufshcd_update_uic_error: uecdl : 0x80000002
[ 222.999009]I[0: arch_disk_io_1:20413] exynos-ufs 13500000.ufs: ufshcd_update_uic_error: uecpa : 0x80000010
[ 222.999200] [6: kworker/u16:2: 132] exynos-ufs 13500000.ufs: ufs_pwr_mode_restore_needed : mode = 0x15, pwr_rx = 1, pwr_tx = 1
[ 223.002876]I[0: arch_disk_io_3:20422] exynos-ufs 13500000.ufs: ufshcd_update_uic_error: uecpa : 0x80000010
[ 223.501050] [4: kworker/u16:2: 132] exynos-ufs 13500000.ufs: pwr ctrl cmd 0x2 with mode 0x11 completion timeout
[ 223.502305] [4: kworker/u16:2: 132] exynos-ufs 13500000.ufs: ufshcd_change_power_mode: power mode change failed -110
[ 223.502312] [4: kworker/u16:2: 132] exynos-ufs 13500000.ufs: ufshcd_err_handler: Failed to restore power mode, err = -110
[ 223.502392] [4: kworker/u16:2: 132] exynos-ufs 13500000.ufs: ufshcd_is_pwr_mode_restore_needed : mode = 0x11, pwr_rx = 1, pwr_tx = 1
This patch is to poll PMC's result w/o checking its ind until
the result is not busy, i.e. 09h, to avoid the rejection.
Signed-off-by: Kiwoong Kim <[email protected]>
---
drivers/ufs/core/ufshcd.c | 93 +++++++++++++++++++++++++++++++++--------------
1 file changed, 65 insertions(+), 28 deletions(-)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index a772f92..3cf2fbe 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -98,6 +98,9 @@
/* Polling time to wait for fDeviceInit */
#define FDEVICEINIT_COMPL_TIMEOUT 1500 /* millisecs */
+/* Polling time to wait until PA is ready */
+#define UIC_PA_RDY_TIMEOUT 100 /* millisecs */
+
/* UFSHC 4.0 compliant HC support this mode, refer param_set_mcq_mode() */
static bool use_mcq_mode = true;
@@ -4146,6 +4149,62 @@ int ufshcd_dme_get_attr(struct ufs_hba *hba, u32 attr_sel,
}
EXPORT_SYMBOL_GPL(ufshcd_dme_get_attr);
+static int __ufshcd_poll_uic_pwr(struct ufs_hba *hba, struct uic_command *cmd,
+ struct completion *cnf)
+{
+ unsigned long flags;
+ int ret;
+ u32 mode = cmd->argument3;
+ unsigned long deadline = jiffies +
+ msecs_to_jiffies(UIC_PA_RDY_TIMEOUT);
+
+ do {
+ spin_lock_irqsave(hba->host->host_lock, flags);
+ hba->active_uic_cmd = NULL;
+ if (ufshcd_is_link_broken(hba)) {
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+ ret = -ENOLINK;
+ goto out;
+ }
+ hba->uic_async_done = cnf;
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+
+ cmd->argument2 = 0;
+ cmd->argument3 = mode;
+ ret = __ufshcd_send_uic_cmd(hba, cmd, true);
+ if (ret) {
+ dev_err(hba->dev,
+ "pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n",
+ cmd->command, cmd->argument3, ret);
+ goto out;
+ }
+
+ /* This value is heuristic */
+ if (!wait_for_completion_timeout(&cmd->done,
+ msecs_to_jiffies(5))) {
+ ret = -ETIMEDOUT;
+ dev_err(hba->dev,
+ "pwr ctrl cmd 0x%x with mode 0x%x timeout\n",
+ cmd->command, cmd->argument3);
+ if (cmd->cmd_active)
+ goto out;
+
+ dev_info(hba->dev, "%s: pwr ctrl cmd has already been completed\n", __func__);
+ }
+
+ /* retry only for busy cases */
+ ret = cmd->argument2 & MASK_UIC_COMMAND_RESULT;
+ if (ret != UIC_CMD_RESULT_BUSY)
+ break;
+
+ dev_info(hba->dev, "%s: PA is busy and can't handle a requeest, %d\n", __func__, ret);
+ mdelay(1);
+
+ } while (time_before(jiffies, deadline));
+out:
+ return ret;
+}
+
/**
* ufshcd_uic_pwr_ctrl - executes UIC commands (which affects the link power
* state) and waits for it to take effect.
@@ -4168,33 +4227,13 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
unsigned long flags;
u8 status;
int ret;
- bool reenable_intr = false;
- mutex_lock(&hba->uic_cmd_mutex);
- ufshcd_add_delay_before_dme_cmd(hba);
-
- spin_lock_irqsave(hba->host->host_lock, flags);
- if (ufshcd_is_link_broken(hba)) {
- ret = -ENOLINK;
- goto out_unlock;
- }
- hba->uic_async_done = &uic_async_done;
- if (ufshcd_readl(hba, REG_INTERRUPT_ENABLE) & UIC_COMMAND_COMPL) {
- ufshcd_disable_intr(hba, UIC_COMMAND_COMPL);
- /*
- * Make sure UIC command completion interrupt is disabled before
- * issuing UIC command.
- */
- wmb();
- reenable_intr = true;
- }
- spin_unlock_irqrestore(hba->host->host_lock, flags);
- ret = __ufshcd_send_uic_cmd(hba, cmd, false);
+ ret = __ufshcd_poll_uic_pwr(hba, cmd, &uic_async_done);
if (ret) {
- dev_err(hba->dev,
- "pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n",
- cmd->command, cmd->argument3, ret);
- goto out;
+ if (ret == -ENOLINK)
+ goto out_unlock;
+ else
+ goto out;
}
if (!wait_for_completion_timeout(hba->uic_async_done,
@@ -4231,14 +4270,12 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
spin_lock_irqsave(hba->host->host_lock, flags);
hba->active_uic_cmd = NULL;
hba->uic_async_done = NULL;
- if (reenable_intr)
- ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
if (ret) {
ufshcd_set_link_broken(hba);
ufshcd_schedule_eh_work(hba);
}
-out_unlock:
spin_unlock_irqrestore(hba->host->host_lock, flags);
+out_unlock:
mutex_unlock(&hba->uic_cmd_mutex);
return ret;
--
2.7.4
Hi Kiwoong,
kernel test robot noticed the following build warnings:
[auto build test WARNING on v6.4-rc3]
[also build test WARNING on linus/master next-20230523]
[cannot apply to jejb-scsi/for-next mkp-scsi/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Kiwoong-Kim/ufs-poll-HCS-UCRDY-before-issuing-a-UIC-command/20230523-215413
base: 44c026a73be8038f03dbdeef028b642880cf1511
patch link: https://lore.kernel.org/r/c511c7f03ce77a604aa5211c3ffac96022c3ee40.1684822284.git.kwmad.kim%40samsung.com
patch subject: [PATCH v1 1/3] ufs: make __ufshcd_send_uic_cmd not wrapped by host_lock
config: x86_64-allmodconfig (https://download.01.org/0day-ci/archive/20230524/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/c3fdde6dc3c766e3141c3ed43ae136ceb345c1c1
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Kiwoong-Kim/ufs-poll-HCS-UCRDY-before-issuing-a-UIC-command/20230523-215413
git checkout c3fdde6dc3c766e3141c3ed43ae136ceb345c1c1
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/ufs/core/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
drivers/ufs/core/ufshcd.c: In function 'ufshcd_send_uic_cmd':
>> drivers/ufs/core/ufshcd.c:2479:23: warning: unused variable 'flags' [-Wunused-variable]
2479 | unsigned long flags;
| ^~~~~
vim +/flags +2479 drivers/ufs/core/ufshcd.c
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon 2013-06-26 2468
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon 2013-06-26 2469 /**
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon 2013-06-26 2470 * ufshcd_send_uic_cmd - Send UIC commands and retrieve the result
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon 2013-06-26 2471 * @hba: per adapter instance
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon 2013-06-26 2472 * @uic_cmd: UIC command
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon 2013-06-26 2473 *
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon 2013-06-26 2474 * Returns 0 only if success.
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon 2013-06-26 2475 */
e77044c5a8422e drivers/scsi/ufs/ufshcd.c Avri Altman 2018-10-07 2476 int ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon 2013-06-26 2477 {
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon 2013-06-26 2478 int ret;
57d104c153d3d6 drivers/scsi/ufs/ufshcd.c Subhash Jadavani 2014-09-25 @2479 unsigned long flags;
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon 2013-06-26 2480
a22bcfdbf10b41 drivers/scsi/ufs/ufshcd.c jongmin jeong 2021-10-18 2481 if (hba->quirks & UFSHCD_QUIRK_BROKEN_UIC_CMD)
a22bcfdbf10b41 drivers/scsi/ufs/ufshcd.c jongmin jeong 2021-10-18 2482 return 0;
a22bcfdbf10b41 drivers/scsi/ufs/ufshcd.c jongmin jeong 2021-10-18 2483
1ab27c9cf8b63d drivers/scsi/ufs/ufshcd.c Sahitya Tummala 2014-09-25 2484 ufshcd_hold(hba, false);
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon 2013-06-26 2485 mutex_lock(&hba->uic_cmd_mutex);
cad2e03d860779 drivers/scsi/ufs/ufshcd.c Yaniv Gardi 2015-03-31 2486 ufshcd_add_delay_before_dme_cmd(hba);
cad2e03d860779 drivers/scsi/ufs/ufshcd.c Yaniv Gardi 2015-03-31 2487
d75f7fe495cf57 drivers/scsi/ufs/ufshcd.c Yaniv Gardi 2016-02-01 2488 ret = __ufshcd_send_uic_cmd(hba, uic_cmd, true);
57d104c153d3d6 drivers/scsi/ufs/ufshcd.c Subhash Jadavani 2014-09-25 2489 if (!ret)
57d104c153d3d6 drivers/scsi/ufs/ufshcd.c Subhash Jadavani 2014-09-25 2490 ret = ufshcd_wait_for_uic_cmd(hba, uic_cmd);
57d104c153d3d6 drivers/scsi/ufs/ufshcd.c Subhash Jadavani 2014-09-25 2491
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon 2013-06-26 2492 mutex_unlock(&hba->uic_cmd_mutex);
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon 2013-06-26 2493
1ab27c9cf8b63d drivers/scsi/ufs/ufshcd.c Sahitya Tummala 2014-09-25 2494 ufshcd_release(hba);
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon 2013-06-26 2495 return ret;
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon 2013-06-26 2496 }
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon 2013-06-26 2497
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki