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(-)