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

[PATCH v3 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 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 v3 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 v3 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 v3 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..bd4ce3395972 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 (!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;
 }
 

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

[Peter Wang (王信友)]

On Wed, 2024-09-11 at 17: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>
> ---
>  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..bd4ce3395972 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 (!cmd)
> +		goto unlock;
> +
> 

Hi Bart,

Could add a warning line in this case? 	
WARN_ON(!cmd);
I'm worried that if this situation occurs, we won't have 
enough information to debug.

Thanks
Peter


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

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

[Bart Van Assche]

On 9/12/24 6:27 AM, Peter Wang (王信友) wrote:
> On Wed, 2024-09-11 at 17:30 -0700, Bart Van Assche wrote:
>> @@ -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 (!cmd)
>> +		goto unlock;
>> +
> 
> Could add a warning line in this case? 	
> WARN_ON(!cmd);
> I'm worried that if this situation occurs, we won't have
> enough information to debug.

I will do that.

Thanks,

Bart.

[PATCH v3 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 bd4ce3395972..753480a1c74e 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",