| Message ID | 20240821182923.145631-2-bvanassche@acm.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix the UFS driver hibernation code | expand |
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>
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 }
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.
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
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; }
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(-)