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

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

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20240821182923.145631-2-bvanassche@acm.org>
References: <20240821182923.145631-2-bvanassche@acm.org>
TO: Bart Van Assche <bvanassche@acm.org>
TO: "Martin K . Petersen" <martin.petersen@oracle.com>
CC: linux-scsi@vger.kernel.org
CC: Bart Van Assche <bvanassche@acm.org>
CC: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
CC: Peter Wang <peter.wang@mediatek.com>
CC: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
CC: Avri Altman <avri.altman@wdc.com>
CC: Bean Huo <beanhuo@micron.com>
CC: Andrew Halaney <ahalaney@redhat.com>

Hi Bart,

kernel test robot noticed the following build warnings:

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on jejb-scsi/for-next linus/master v6.11-rc4 next-20240823]
[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/Bart-Van-Assche/scsi-ufs-core-Make-ufshcd_uic_cmd_compl-easier-to-read/20240822-023058
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
patch link:    https://lore.kernel.org/r/20240821182923.145631-2-bvanassche%40acm.org
patch subject: [PATCH 1/2] scsi: ufs: core: Make ufshcd_uic_cmd_compl() easier to read
:::::: branch date: 3 days ago
:::::: commit date: 3 days ago
config: i386-randconfig-141-20240824 (https://download.01.org/0day-ci/archive/20240824/202408241736.p8Mxizp7-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202408241736.p8Mxizp7-lkp@intel.com/

New smatch warnings:
drivers/ufs/core/ufshcd.c:5484 ufshcd_uic_cmd_compl() error: we previously assumed 'cmd' could be null (see line 5474)

Old smatch warnings:
drivers/ufs/core/ufshcd.c:9226 ufshcd_setup_clocks() warn: 'clki->clk' from clk_prepare_enable() not released on lines: 9204.

vim +/cmd +5484 drivers/ufs/core/ufshcd.c

a45f937110fa6b drivers/scsi/ufs/ufshcd.c Can Guo               2021-05-24  5454  
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon         2013-06-26  5455  /**
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon         2013-06-26  5456   * ufshcd_uic_cmd_compl - handle completion of uic command
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon         2013-06-26  5457   * @hba: per adapter instance
53b3d9c3fdda94 drivers/scsi/ufs/ufshcd.c Seungwon Jeon         2013-08-31  5458   * @intr_status: interrupt status generated by the controller
9333d77573485c drivers/scsi/ufs/ufshcd.c Venkat Gopalakrishnan 2019-11-14  5459   *
3a17fefe0f1960 drivers/ufs/core/ufshcd.c Bart Van Assche       2023-07-27  5460   * Return:
9333d77573485c drivers/scsi/ufs/ufshcd.c Venkat Gopalakrishnan 2019-11-14  5461   *  IRQ_HANDLED - If interrupt is valid
9333d77573485c drivers/scsi/ufs/ufshcd.c Venkat Gopalakrishnan 2019-11-14  5462   *  IRQ_NONE    - If invalid interrupt
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon         2013-06-26  5463   */
9333d77573485c drivers/scsi/ufs/ufshcd.c Venkat Gopalakrishnan 2019-11-14  5464  static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon         2013-06-26  5465  {
9333d77573485c drivers/scsi/ufs/ufshcd.c Venkat Gopalakrishnan 2019-11-14  5466  	irqreturn_t retval = IRQ_NONE;
71779e69ba68be drivers/ufs/core/ufshcd.c Bart Van Assche       2024-08-21  5467  	struct uic_command *cmd;
9333d77573485c drivers/scsi/ufs/ufshcd.c Venkat Gopalakrishnan 2019-11-14  5468  
a45f937110fa6b drivers/scsi/ufs/ufshcd.c Can Guo               2021-05-24  5469  	spin_lock(hba->host->host_lock);
71779e69ba68be drivers/ufs/core/ufshcd.c Bart Van Assche       2024-08-21  5470  	cmd = hba->active_uic_cmd;
a45f937110fa6b drivers/scsi/ufs/ufshcd.c Can Guo               2021-05-24  5471  	if (ufshcd_is_auto_hibern8_error(hba, intr_status))
a45f937110fa6b drivers/scsi/ufs/ufshcd.c Can Guo               2021-05-24  5472  		hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);
a45f937110fa6b drivers/scsi/ufs/ufshcd.c Can Guo               2021-05-24  5473  
71779e69ba68be drivers/ufs/core/ufshcd.c Bart Van Assche       2024-08-21 @5474  	if (intr_status & UIC_COMMAND_COMPL && cmd) {
71779e69ba68be drivers/ufs/core/ufshcd.c Bart Van Assche       2024-08-21  5475  		cmd->argument2 |= ufshcd_get_uic_cmd_result(hba);
71779e69ba68be drivers/ufs/core/ufshcd.c Bart Van Assche       2024-08-21  5476  		cmd->argument3 = ufshcd_get_dme_attr_val(hba);
0f52fcb99ea273 drivers/scsi/ufs/ufshcd.c Can Guo               2020-11-02  5477  		if (!hba->uic_async_done)
71779e69ba68be drivers/ufs/core/ufshcd.c Bart Van Assche       2024-08-21  5478  			cmd->cmd_active = 0;
71779e69ba68be drivers/ufs/core/ufshcd.c Bart Van Assche       2024-08-21  5479  		complete(&cmd->done);
9333d77573485c drivers/scsi/ufs/ufshcd.c Venkat Gopalakrishnan 2019-11-14  5480  		retval = IRQ_HANDLED;
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon         2013-06-26  5481  	}
53b3d9c3fdda94 drivers/scsi/ufs/ufshcd.c Seungwon Jeon         2013-08-31  5482  
71779e69ba68be drivers/ufs/core/ufshcd.c Bart Van Assche       2024-08-21  5483  	if (intr_status & UFSHCD_UIC_PWR_MASK && hba->uic_async_done) {
71779e69ba68be drivers/ufs/core/ufshcd.c Bart Van Assche       2024-08-21 @5484  		cmd->cmd_active = 0;
57d104c153d3d6 drivers/scsi/ufs/ufshcd.c Subhash Jadavani      2014-09-25  5485  		complete(hba->uic_async_done);
9333d77573485c drivers/scsi/ufs/ufshcd.c Venkat Gopalakrishnan 2019-11-14  5486  		retval = IRQ_HANDLED;
9333d77573485c drivers/scsi/ufs/ufshcd.c Venkat Gopalakrishnan 2019-11-14  5487  	}
aa5c697988b4c7 drivers/scsi/ufs/ufshcd.c Stanley Chu           2020-06-15  5488  
aa5c697988b4c7 drivers/scsi/ufs/ufshcd.c Stanley Chu           2020-06-15  5489  	if (retval == IRQ_HANDLED)
71779e69ba68be drivers/ufs/core/ufshcd.c Bart Van Assche       2024-08-21  5490  		ufshcd_add_uic_command_trace(hba, cmd, UFS_CMD_COMP);
a45f937110fa6b drivers/scsi/ufs/ufshcd.c Can Guo               2021-05-24  5491  	spin_unlock(hba->host->host_lock);
9333d77573485c drivers/scsi/ufs/ufshcd.c Venkat Gopalakrishnan 2019-11-14  5492  	return retval;
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon         2013-06-26  5493  }
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon         2013-06-26  5494  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 0/2] Fix the UFS driver hibernation code

[Bart Van Assche]

Hi Martin,

While testing the UFS driver we noticed that the UFS host controller does not
always enter hibernation when it should enter hibernation. This patch series
fixes this. Please consider this patch series for the next merge window.

Thanks,

Bart.

Bart Van Assche (2):
  scsi: ufs: core: Make ufshcd_uic_cmd_compl() easier to read
  scsi: ufs: core: Fix the code for entering hibernation

 drivers/ufs/core/ufshcd.c | 37 +++++++++++++------------------------
 include/ufs/ufshcd.h      |  7 ++++---
 2 files changed, 17 insertions(+), 27 deletions(-)

[PATCH 1/2] 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.

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 0dd26059f5d7..d0ae6e50becc 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -5464,31 +5464,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;
 }

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

[Bean Huo]

On Wed, 2024-08-21 at 11:29 -0700, Bart Van Assche wrote:
> Introduce a local variable for the expression hba->active_uic_cmd.
> Remove superfluous parentheses.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Bean Huo <beanhuo@micron.com>

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

[Peter Wang (王信友)]

On Wed, 2024-08-21 at 11:29 -0700, Bart Van Assche wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Introduce a local variable for the expression hba->active_uic_cmd.
> Remove superfluous parentheses.
> 
> 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 0dd26059f5d7..d0ae6e50becc 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -5464,31 +5464,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) {
> 

Hi Bart,

in C language, when conditional expressions become complex, 
using parentheses can help improve the readability of the 
code and clearly specify the precedence of operators. 
This is very important because different operators, 
such as logical operators && and ||, comparison operators 
==, !=, <, >, etc., have different levels of precedence. 
Without using parentheses to clarify,  it could lead to 
unexpected results.

For example, consider the following complex conditional expression:

if (a == b && c > d || e < f && g != h)

While this conditional expression is valid, its order of 
operations might not be immediately clear. Using parentheses 
can help others reading the code (including your future self) 
to understand your intent more easily:

if ((a == b && c > d) || (e < f && g != h))

In this example, the parentheses clearly indicate that the 
&& operations are to be evaluated first, followed by the || 
operation. This avoids confusion caused by operator precedence 
and makes the logic of the code more explicit. Therefore, 
it is a good practice to use parentheses when dealing with 
complex conditional expressions.

Thanks.
Peter



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

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

[Bart Van Assche]

On 8/21/24 10:34 PM, Peter Wang (王信友) wrote:
> For example, consider the following complex conditional expression:
> 
> if (a == b && c > d || e < f && g != h)

Hi Peter,

As you may know, the above is not allowed in Linux kernel code. I'm
not sure this has been written down anywhere formally. The C compilers
supported by the Linux kernel emit a warning about expressions like the
above and Linux kernel code should be warning-free.

I agree with you that code readability is important. However, I think
that it's important for Linux kernel developers to make themselves
familiar with expressions like (a & b && ...) since this style is common
in the Linux kernel. Do you agree that the data below shows that not
surrounding binary and expressions with parentheses is common in Linux
kernel code?

$ git grep -nHE 'if.*&&[^()&]*&[^&]|if.*[^&]&[^&()]*&&' | wc -l
    2548

Thanks,

Bart.

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

[Peter Wang (王信友)]

On Thu, 2024-08-22 at 10:02 -0700, Bart Van Assche wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 8/21/24 10:34 PM, Peter Wang (王信友) wrote:
> > For example, consider the following complex conditional expression:
> > 
> > if (a == b && c > d || e < f && g != h)
> 
> Hi Peter,
> 
> As you may know, the above is not allowed in Linux kernel code. I'm
> not sure this has been written down anywhere formally. The C
> compilers
> supported by the Linux kernel emit a warning about expressions like
> the
> above and Linux kernel code should be warning-free.
> 
> I agree with you that code readability is important. However, I think
> that it's important for Linux kernel developers to make themselves
> familiar with expressions like (a & b && ...) since this style is
> common
> in the Linux kernel. Do you agree that the data below shows that not
> surrounding binary and expressions with parentheses is common in
> Linux
> kernel code?
> 
> $ git grep -nHE 'if.*&&[^()&]*&[^&]|if.*[^&]&[^&()]*&&' | wc -l
>     2548
> 
> Thanks,
> 
> Bart.


Hi Bart,

I agree that it's important for Linux kernel developers to make
themselves familiar with expressions like (a & b && ...)
I have no further comments.

Thanks
Peter

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

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

[Dan Carpenter]

Hi Bart,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bart-Van-Assche/scsi-ufs-core-Make-ufshcd_uic_cmd_compl-easier-to-read/20240822-023058
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
patch link:    https://lore.kernel.org/r/20240821182923.145631-2-bvanassche%40acm.org
patch subject: [PATCH 1/2] scsi: ufs: core: Make ufshcd_uic_cmd_compl() easier to read
config: i386-randconfig-141-20240824 (https://download.01.org/0day-ci/archive/20240824/202408241736.p8Mxizp7-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202408241736.p8Mxizp7-lkp@intel.com/

New smatch warnings:
drivers/ufs/core/ufshcd.c:5484 ufshcd_uic_cmd_compl() error: we previously assumed 'cmd' could be null (see line 5474)

vim +/cmd +5484 drivers/ufs/core/ufshcd.c

9333d77573485c drivers/scsi/ufs/ufshcd.c Venkat Gopalakrishnan 2019-11-14  5464  static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon         2013-06-26  5465  {
9333d77573485c drivers/scsi/ufs/ufshcd.c Venkat Gopalakrishnan 2019-11-14  5466  	irqreturn_t retval = IRQ_NONE;
71779e69ba68be drivers/ufs/core/ufshcd.c Bart Van Assche       2024-08-21  5467  	struct uic_command *cmd;
9333d77573485c drivers/scsi/ufs/ufshcd.c Venkat Gopalakrishnan 2019-11-14  5468  
a45f937110fa6b drivers/scsi/ufs/ufshcd.c Can Guo               2021-05-24  5469  	spin_lock(hba->host->host_lock);
71779e69ba68be drivers/ufs/core/ufshcd.c Bart Van Assche       2024-08-21  5470  	cmd = hba->active_uic_cmd;
a45f937110fa6b drivers/scsi/ufs/ufshcd.c Can Guo               2021-05-24  5471  	if (ufshcd_is_auto_hibern8_error(hba, intr_status))
a45f937110fa6b drivers/scsi/ufs/ufshcd.c Can Guo               2021-05-24  5472  		hba->errors |= (UFSHCD_UIC_HIBERN8_MASK & intr_status);
a45f937110fa6b drivers/scsi/ufs/ufshcd.c Can Guo               2021-05-24  5473  
71779e69ba68be drivers/ufs/core/ufshcd.c Bart Van Assche       2024-08-21 @5474  	if (intr_status & UIC_COMMAND_COMPL && cmd) {
                                                                                                                               ^^^
cmd checked for NULL here

71779e69ba68be drivers/ufs/core/ufshcd.c Bart Van Assche       2024-08-21  5475  		cmd->argument2 |= ufshcd_get_uic_cmd_result(hba);
71779e69ba68be drivers/ufs/core/ufshcd.c Bart Van Assche       2024-08-21  5476  		cmd->argument3 = ufshcd_get_dme_attr_val(hba);
0f52fcb99ea273 drivers/scsi/ufs/ufshcd.c Can Guo               2020-11-02  5477  		if (!hba->uic_async_done)
71779e69ba68be drivers/ufs/core/ufshcd.c Bart Van Assche       2024-08-21  5478  			cmd->cmd_active = 0;
71779e69ba68be drivers/ufs/core/ufshcd.c Bart Van Assche       2024-08-21  5479  		complete(&cmd->done);
9333d77573485c drivers/scsi/ufs/ufshcd.c Venkat Gopalakrishnan 2019-11-14  5480  		retval = IRQ_HANDLED;
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon         2013-06-26  5481  	}
53b3d9c3fdda94 drivers/scsi/ufs/ufshcd.c Seungwon Jeon         2013-08-31  5482  
71779e69ba68be drivers/ufs/core/ufshcd.c Bart Van Assche       2024-08-21  5483  	if (intr_status & UFSHCD_UIC_PWR_MASK && hba->uic_async_done) {
71779e69ba68be drivers/ufs/core/ufshcd.c Bart Van Assche       2024-08-21 @5484  		cmd->cmd_active = 0;

Not checked here.  It could be be that when UFSHCD_UIC_PWR_MASK is set that
means cmd is valid?

57d104c153d3d6 drivers/scsi/ufs/ufshcd.c Subhash Jadavani      2014-09-25  5485  		complete(hba->uic_async_done);
9333d77573485c drivers/scsi/ufs/ufshcd.c Venkat Gopalakrishnan 2019-11-14  5486  		retval = IRQ_HANDLED;
9333d77573485c drivers/scsi/ufs/ufshcd.c Venkat Gopalakrishnan 2019-11-14  5487  	}
aa5c697988b4c7 drivers/scsi/ufs/ufshcd.c Stanley Chu           2020-06-15  5488  
aa5c697988b4c7 drivers/scsi/ufs/ufshcd.c Stanley Chu           2020-06-15  5489  	if (retval == IRQ_HANDLED)
71779e69ba68be drivers/ufs/core/ufshcd.c Bart Van Assche       2024-08-21  5490  		ufshcd_add_uic_command_trace(hba, cmd, UFS_CMD_COMP);
                                                                                                                                  ^^^
cmd is not checked for NULL inside this function but it's not dereferenced on
every path either.

a45f937110fa6b drivers/scsi/ufs/ufshcd.c Can Guo               2021-05-24  5491  	spin_unlock(hba->host->host_lock);
9333d77573485c drivers/scsi/ufs/ufshcd.c Venkat Gopalakrishnan 2019-11-14  5492  	return retval;
6ccf44fe4cd7c4 drivers/scsi/ufs/ufshcd.c Seungwon Jeon         2013-06-26  5493  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Bart Van Assche]

On 8/25/24 11:25 PM, Dan Carpenter wrote:
> New smatch warnings:
> drivers/ufs/core/ufshcd.c:5484 ufshcd_uic_cmd_compl() error: we previously assumed 'cmd' could be null (see line 5474)

This smatch warning is a false positive. There are multiple code blocks
in this functions that are guarded by if-statements. The two code blocks
this warning applies to are mutually exclusive. Hence, the 'cmd' check
from one block should not be used to draw conclusions about other code
blocks. I will consider to introduce the 'else' keyword to suppress this
false positive.

Thanks,

Bart.

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

[Dan Carpenter]

On Mon, Aug 26, 2024 at 11:05:47AM -0700, Bart Van Assche wrote:
> On 8/25/24 11:25 PM, Dan Carpenter wrote:
> > New smatch warnings:
> > drivers/ufs/core/ufshcd.c:5484 ufshcd_uic_cmd_compl() error: we previously assumed 'cmd' could be null (see line 5474)
> 
> This smatch warning is a false positive. There are multiple code blocks
> in this functions that are guarded by if-statements. The two code blocks
> this warning applies to are mutually exclusive. Hence, the 'cmd' check
> from one block should not be used to draw conclusions about other code
> blocks. I will consider to introduce the 'else' keyword to suppress this
> false positive.

I thought that might be the case, but I wasn't sure.

If it's a false positive, then you can just ignore it.  All old warnings are
false positives.  People can find this thread if they have questions.

regards,
dan carpenter