[PATCH v1] scsi: ufs: Change the error check sequence for auto hibern8

[PATCH v1] scsi: ufs: Change the error check sequence for auto hibern8

[HOYOUNG SEO]

In the case of auto hibern8, hibern8 entering or exiting occurs
without a cmd, so it is not correct to check 
for the presence of a cmd before checking for auto hibern8 errors.
Thus, the order of the auto hibern8 error check changed.

Signed-off-by: HOYOUNG SEO <hy50.seo@samsung.com>
---
 drivers/ufs/core/ufshcd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index bcc2512bf34b..c8a70d7d210d 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -5928,6 +5928,9 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
 	struct uic_command *cmd;
 
 	guard(spinlock_irqsave)(&hba->host->host_lock);
+	if (ufshcd_is_auto_hibern8_error(hba, intr_status))
+		hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);
+
 	cmd = hba->active_uic_cmd;
 	if (!cmd) {
 		dev_err(hba->dev,
@@ -5935,9 +5938,6 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
 		return retval;
 	}
 
-	if (ufshcd_is_auto_hibern8_error(hba, intr_status))
-		hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);
-
 	if (intr_status & UIC_COMMAND_COMPL) {
 		/*
 		 * Store the UIC command result in the lowest byte of
-- 
2.34.1

Re: [PATCH v1] scsi: ufs: Change the error check sequence for auto hibern8

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: ufs: fallthrough after auto hibern8 error in ufshcd_uic_cmd_compl()
--

commit 5bb3d85e5945c6dc3a3a1bb37e02acd5c7703a06
Author: HOYOUNG SEO <hy50.seo@samsung.com>

scsi: ufs: Change the error check sequence for auto hibern8

This commit moves the ufshcd_is_auto_hibern8_error() check in
ufshcd_uic_cmd_compl() ahead of the active_uic_cmd check so that
auto-hibern8 errors are recorded even when no UIC command is active.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index cc75384c65021..e15b08488187f 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -5928,6 +5928,9 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
>  	struct uic_command *cmd;
>  
>  	guard(spinlock_irqsave)(&hba->host->host_lock);
> +	if (ufshcd_is_auto_hibern8_error(hba, intr_status))
> +		hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);
> +
>  	cmd = hba->active_uic_cmd;
>  	if (!cmd) {
>  		dev_err(hba->dev,
> @@ -5935,9 +5938,6 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
>  		return retval;
>  	}
>  
> -	if (ufshcd_is_auto_hibern8_error(hba, intr_status))
> -		hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);
> -
>  	if (intr_status & UIC_COMMAND_COMPL) {
>  		/*
>  		 * Store the UIC command result in the lowest byte of

[Severity: Medium]
When ufshcd_is_auto_hibern8_error() returns true in ufshcd_uic_cmd_compl(),
execution still falls through after setting hba->errors:

drivers/ufs/core/ufshcd.c:ufshcd_uic_cmd_compl() {
	...
	if (ufshcd_is_auto_hibern8_error(hba, intr_status))
		hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);

	cmd = hba->active_uic_cmd;
	if (!cmd) {
		dev_err(hba->dev,
			"No active UIC command. Maybe a timeout occurred?\n");
		return retval;
	}
	...
	if (intr_status & UFSHCD_UIC_PWR_MASK && hba->uic_async_done) {
		cmd->cmd_active = false;
		complete(hba->uic_async_done);
		retval = IRQ_HANDLED;
	}
	...
}

In the normal auto-hibern8 case where no UIC command is active
(hba->active_uic_cmd is NULL), won't falling through to the !cmd check
always log a misleading "No active UIC command. Maybe a timeout occurred?"
error and return IRQ_NONE?

Also, if an asynchronous UIC command such as UIC_CMD_DME_SET is active with
hba->uic_async_done set when an auto-hibern8 error interrupt arrives,
UFSHCD_UIC_PWR_MASK includes UFSHCD_UIC_HIBERN8_MASK. Could falling through
with UFSHCD_UIC_HIBERN8_MASK still set in intr_status prematurely complete
hba->uic_async_done?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021051951.1687479-1-hy50.seo@samsung.com?part=1

[PATCH v1] scsi: ufs: Change the error check sequence for auto hibern8

[HOYOUNG SEO]

Auto-hibern8 enter and exit are performed by the host controller without
a UIC command, and the auto-hibern8 idle timer only runs while no UIC
command is outstanding. An auto-hibern8 error (IS.UHES or IS.UHXS) is
therefore usually reported while hba->active_uic_cmd is NULL.

Since commit fcd8b0450a9a ("scsi: ufs: core: Make ufshcd_uic_cmd_compl()
easier to analyze"), ufshcd_uic_cmd_compl() returns early when
hba->active_uic_cmd is NULL, before ufshcd_is_auto_hibern8_error() is
called. Such auto-hibern8 errors are no longer recorded in hba->errors,
so ufshcd_check_errors() is not called for them and the error handler is
not scheduled.

Check for auto-hibern8 errors before checking for an active UIC command,
as was done before that commit, but keep returning early when there is
no active UIC command so that a NULL cmd is never dereferenced. Only
report the missing UIC command if no auto-hibern8 error was found.

Fixes: fcd8b0450a9a ("scsi: ufs: core: Make ufshcd_uic_cmd_compl() easier to analyze")
---
Changes in v3:
 - Check auto-hibern8 errors before the NULL check again, so that errors
   reported while a non-hibernate UIC command is active are still
   detected (Sashiko review).
 - Return early when no UIC command is active instead of falling
   through, to avoid dereferencing a NULL cmd (Sashiko review).
 - Add Fixes tag, reword subject and commit message.

Changes in v2:
 - Check UFSHCD_UIC_HIBERN8_MASK for auto-hibern8 only when cmd is NULL.

v2: https://lore.kernel.org/linux-scsi/20261022074249.3098100-1-hy50.seo@samsung.com/

Signed-off-by: HOYOUNG SEO <hy50.seo@samsung.com>
---
 drivers/ufs/core/ufshcd.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index d085d394858f..4541776e8d38 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -5941,16 +5941,22 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
 	struct uic_command *cmd;
 
 	guard(spinlock_irqsave)(&hba->host->host_lock);
+
+	/*
+	 * Auto-hibern8 is performed without a UIC command, so check for
+	 * auto-hibern8 errors before checking for an active UIC command.
+	 */
+	if (ufshcd_is_auto_hibern8_error(hba, intr_status)) {
+		hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);
+
 	cmd = hba->active_uic_cmd;
 	if (!cmd) {
-		dev_err(hba->dev,
-			"No active UIC command. Maybe a timeout occurred?\n");
+		if (!(hba->errors & UFSHCD_UIC_HIBERN8_MASK))
+			dev_err(hba->dev,
+				"No active UIC command. Maybe a timeout occurred?\n");
 		return retval;
 	}
 
-	if (ufshcd_is_auto_hibern8_error(hba, intr_status))
-		hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);
-
 	if (intr_status & UIC_COMMAND_COMPL) {
 		/*
 		 * Store the UIC command result in the lowest byte of
-- 
2.34.1