| Message ID | 20240912003102.3110110-4-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Clean up the UFS driver UIC code | expand |
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; > } >
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.
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; + 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; }
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(-)