| Message ID | 20240821182923.145631-2-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| 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>
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; > }
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.
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>
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(-)