[PATCH v4 0/4] Clean up the UFS driver UIC code

[PATCH v4 0/4] Clean up the UFS driver UIC code

[Bart Van Assche]

Hi Martin,

This patch series includes four patches that modify the UFS driver UIC
code without modifying the behavior of that code.

Please consider this patch series for the next merge window.

Thanks,

Bart.

Changes compared to v3 of this patch series:
 - Added a WARN_ON_ONCE() call.

Changes compared to v2 of this patch series:
 - Dropped patch "Change the approach for power change UIC commands".
 - Added patch "Make ufshcd_uic_cmd_compl() easier to analyze".

Changes compared to v1 of this patch series:
 - A patch that improves the struct ufs_hba documentation has been added.
 - Patch 2/2 has been split into two patches.
 - Instead of leaving the UIC completion interrupt enabled, disable it if
   UFSHCD_QUIRK_DISABLE_UIC_INTR_FOR_PWR_CMDS has been set.

Bart Van Assche (4):
  scsi: ufs: core: Improve the struct ufs_hba documentation
  scsi: ufs: core: Make ufshcd_uic_cmd_compl() easier to read
  scsi: ufs: core: Make ufshcd_uic_cmd_compl() easier to analyze
  scsi: ufs: core: Always initialize the UIC done completion

 drivers/ufs/core/ufshcd.c | 38 ++++++++++++++++++++------------------
 include/ufs/ufshcd.h      |  7 ++++---
 2 files changed, 24 insertions(+), 21 deletions(-)

[PATCH v4 1/4] scsi: ufs: core: Improve the struct ufs_hba documentation

[Bart Van Assche]

Make the role of the structure members related to UIC command processing
more clear.

Reviewed-by: Peter Wang <peter.wang@mediatek.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/ufs/ufshcd.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index a43b14276bc3..85933775c9f3 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -868,9 +868,10 @@ enum ufshcd_mcq_opr {
  * @tmf_tag_set: TMF tag set.
  * @tmf_queue: Used to allocate TMF tags.
  * @tmf_rqs: array with pointers to TMF requests while these are in progress.
- * @active_uic_cmd: handle of active UIC command
- * @uic_cmd_mutex: mutex for UIC command
- * @uic_async_done: completion used during UIC processing
+ * @active_uic_cmd: pointer to active UIC command.
+ * @uic_cmd_mutex: mutex used for serializing UIC command processing.
+ * @uic_async_done: completion used to wait for power mode or hibernation state
+ *	changes.
  * @ufshcd_state: UFSHCD state
  * @eh_flags: Error handling flags
  * @intr_mask: Interrupt Mask Bits

[PATCH v4 2/4] scsi: ufs: core: Make ufshcd_uic_cmd_compl() easier to read

[Bart Van Assche]

Introduce a local variable for the expression hba->active_uic_cmd.
Remove superfluous parentheses. No functionality has been changed.

Reviewed-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Peter Wang <peter.wang@mediatek.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 8ea5a82503a9..134cba0ff512 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -5458,31 +5458,30 @@ static bool ufshcd_is_auto_hibern8_error(struct ufs_hba *hba,
 static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
 {
 	irqreturn_t retval = IRQ_NONE;
+	struct uic_command *cmd;
 
 	spin_lock(hba->host->host_lock);
+	cmd = hba->active_uic_cmd;
 	if (ufshcd_is_auto_hibern8_error(hba, intr_status))
 		hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);
 
-	if ((intr_status & UIC_COMMAND_COMPL) && hba->active_uic_cmd) {
-		hba->active_uic_cmd->argument2 |=
-			ufshcd_get_uic_cmd_result(hba);
-		hba->active_uic_cmd->argument3 =
-			ufshcd_get_dme_attr_val(hba);
+	if (intr_status & UIC_COMMAND_COMPL && cmd) {
+		cmd->argument2 |= ufshcd_get_uic_cmd_result(hba);
+		cmd->argument3 = ufshcd_get_dme_attr_val(hba);
 		if (!hba->uic_async_done)
-			hba->active_uic_cmd->cmd_active = 0;
-		complete(&hba->active_uic_cmd->done);
+			cmd->cmd_active = 0;
+		complete(&cmd->done);
 		retval = IRQ_HANDLED;
 	}
 
-	if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done) {
-		hba->active_uic_cmd->cmd_active = 0;
+	if (intr_status & UFSHCD_UIC_PWR_MASK && hba->uic_async_done) {
+		cmd->cmd_active = 0;
 		complete(hba->uic_async_done);
 		retval = IRQ_HANDLED;
 	}
 
 	if (retval == IRQ_HANDLED)
-		ufshcd_add_uic_command_trace(hba, hba->active_uic_cmd,
-					     UFS_CMD_COMP);
+		ufshcd_add_uic_command_trace(hba, cmd, UFS_CMD_COMP);
 	spin_unlock(hba->host->host_lock);
 	return retval;
 }

[PATCH v4 3/4] scsi: ufs: core: Make ufshcd_uic_cmd_compl() easier to analyze

[Bart Van Assche]

In ufshcd_uic_cmd_compl(), there is code that dereferences 'cmd' with
and without checking the 'cmd' pointer. This confuses static source code
analyzers like Coverity and sparse. Since none of the code in
ufshcd_uic_cmd_compl() can do anything useful if 'cmd' is NULL, move the
'cmd' test near the start of this function.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 134cba0ff512..d047ccba8b84 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -5462,10 +5462,13 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
 
 	spin_lock(hba->host->host_lock);
 	cmd = hba->active_uic_cmd;
+	if (WARN_ON_ONCE(!cmd))
+		goto unlock;
+
 	if (ufshcd_is_auto_hibern8_error(hba, intr_status))
 		hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);
 
-	if (intr_status & UIC_COMMAND_COMPL && cmd) {
+	if (intr_status & UIC_COMMAND_COMPL) {
 		cmd->argument2 |= ufshcd_get_uic_cmd_result(hba);
 		cmd->argument3 = ufshcd_get_dme_attr_val(hba);
 		if (!hba->uic_async_done)
@@ -5482,7 +5485,10 @@ static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
 
 	if (retval == IRQ_HANDLED)
 		ufshcd_add_uic_command_trace(hba, cmd, UFS_CMD_COMP);
+
+unlock:
 	spin_unlock(hba->host->host_lock);
+
 	return retval;
 }
 

[PATCH v4 4/4] scsi: ufs: core: Always initialize the UIC done completion

[Bart Van Assche]

Simplify __ufshcd_send_uic_cmd() by always initializing the
uic_cmd::done completion. This is fine since the time required to
initialize a completion is small compared to the time required to
process an UIC command.

Reviewed-by: Peter Wang <peter.wang@mediatek.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index d047ccba8b84..c881d1fea968 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2537,13 +2537,11 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
  * __ufshcd_send_uic_cmd - Send UIC commands and retrieve the result
  * @hba: per adapter instance
  * @uic_cmd: UIC command
- * @completion: initialize the completion only if this is set to true
  *
  * Return: 0 only if success.
  */
 static int
-__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd,
-		      bool completion)
+__ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
 {
 	lockdep_assert_held(&hba->uic_cmd_mutex);
 
@@ -2553,8 +2551,7 @@ __ufshcd_send_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd,
 		return -EIO;
 	}
 
-	if (completion)
-		init_completion(&uic_cmd->done);
+	init_completion(&uic_cmd->done);
 
 	uic_cmd->cmd_active = 1;
 	ufshcd_dispatch_uic_cmd(hba, uic_cmd);
@@ -2580,7 +2577,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);
 
-	ret = __ufshcd_send_uic_cmd(hba, uic_cmd, true);
+	ret = __ufshcd_send_uic_cmd(hba, uic_cmd);
 	if (!ret)
 		ret = ufshcd_wait_for_uic_cmd(hba, uic_cmd);
 
@@ -4270,7 +4267,7 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
 		reenable_intr = true;
 	}
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	ret = __ufshcd_send_uic_cmd(hba, cmd, false);
+	ret = __ufshcd_send_uic_cmd(hba, cmd);
 	if (ret) {
 		dev_err(hba->dev,
 			"pwr ctrl cmd 0x%x with mode 0x%x uic error %d\n",

Re: [PATCH v4 3/4] scsi: ufs: core: Make ufshcd_uic_cmd_compl() easier to analyze

[Peter Wang (王信友)]

On Thu, 2024-09-12 at 15:30 -0700, Bart Van Assche wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  In ufshcd_uic_cmd_compl(), there is code that dereferences 'cmd'
> with
> and without checking the 'cmd' pointer. This confuses static source
> code
> analyzers like Coverity and sparse. Since none of the code in
> ufshcd_uic_cmd_compl() can do anything useful if 'cmd' is NULL, move
> the
> 'cmd' test near the start of this function.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> 
> 

Reviewed-by: Peter Wang <peter.wang@mediatek.com>

Re: [PATCH v4 1/4] scsi: ufs: core: Improve the struct ufs_hba documentation

[Bao D. Nguyen]

On 9/12/2024 3:30 PM, Bart Van Assche wrote:
> Make the role of the structure members related to UIC command processing
> more clear.
> 
> Reviewed-by: Peter Wang <peter.wang@mediatek.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>

Re: [PATCH v4 2/4] scsi: ufs: core: Make ufshcd_uic_cmd_compl() easier to read

[Bao D. Nguyen]

On 9/12/2024 3:30 PM, Bart Van Assche wrote:
> Introduce a local variable for the expression hba->active_uic_cmd.
> Remove superfluous parentheses. No functionality has been changed.
> 
> Reviewed-by: Bean Huo <beanhuo@micron.com>
> Reviewed-by: Peter Wang <peter.wang@mediatek.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>

Re: [PATCH v4 3/4] scsi: ufs: core: Make ufshcd_uic_cmd_compl() easier to analyze

[Bao D. Nguyen]

On 9/12/2024 3:30 PM, Bart Van Assche wrote:
> In ufshcd_uic_cmd_compl(), there is code that dereferences 'cmd' with
> and without checking the 'cmd' pointer. This confuses static source code
> analyzers like Coverity and sparse. Since none of the code in
> ufshcd_uic_cmd_compl() can do anything useful if 'cmd' is NULL, move the
> 'cmd' test near the start of this function.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>


>   	if (ufshcd_is_auto_hibern8_error(hba, intr_status))
>   		hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);

Hi Bart, while you are at it, there are extra parenthesis here too.

Reviewed-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>

Re: [PATCH v4 3/4] scsi: ufs: core: Make ufshcd_uic_cmd_compl() easier to analyze

[Bart Van Assche]

On 9/13/24 12:31 AM, Bao D. Nguyen wrote:
> On 9/12/2024 3:30 PM, Bart Van Assche wrote:
>>       if (ufshcd_is_auto_hibern8_error(hba, intr_status))
>>           hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);
> 
> Hi Bart, while you are at it, there are extra parenthesis here too.

Agreed, but since of the one-change-per-patch rule I'd like to keep that
change out of this patch.

Thanks,

Bart.

Re: [PATCH v4 0/4] Clean up the UFS driver UIC code

[Martin K. Petersen]

Bart,

> This patch series includes four patches that modify the UFS driver UIC
> code without modifying the behavior of that code.

Applied to 6.13/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v4 0/4] Clean up the UFS driver UIC code

[Martin K. Petersen]

On Thu, 12 Sep 2024 15:30:01 -0700, Bart Van Assche wrote:

> This patch series includes four patches that modify the UFS driver UIC
> code without modifying the behavior of that code.
> 
> Please consider this patch series for the next merge window.
> 
> Thanks,
> 
> [...]

Applied to 6.13/scsi-queue, thanks!

[1/4] scsi: ufs: core: Improve the struct ufs_hba documentation
      https://git.kernel.org/mkp/scsi/c/22fbabe82cea
[2/4] scsi: ufs: core: Make ufshcd_uic_cmd_compl() easier to read
      https://git.kernel.org/mkp/scsi/c/e31931d646d3
[3/4] scsi: ufs: core: Make ufshcd_uic_cmd_compl() easier to analyze
      https://git.kernel.org/mkp/scsi/c/fcd8b0450a9a
[4/4] scsi: ufs: core: Always initialize the UIC done completion
      https://git.kernel.org/mkp/scsi/c/b1e8c53749ad

-- 
Martin K. Petersen	Oracle Linux Engineering