mbox series

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

Message ID 20240912003102.3110110-1-bvanassche@acm.org
Headers show
Series Clean up the UFS driver UIC code | expand

Message

Bart Van Assche Sept. 12, 2024, 12:30 a.m. UTC 
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(-)

Comments

Peter Wang (王信友) Sept. 12, 2024, 1:27 p.m. UTC | #1 
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;
>  }
>
Bart Van Assche Sept. 12, 2024, 9:19 p.m. UTC | #2 
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.