| Message ID | 20240523002257.1068373-1-cw9316.lee@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | ufs:mcq:Fixing Error Output for ufshcd_try_to_abort_task in ufshcd_mcq_abort | expand |
On 5/22/24 17:22, Chanwoo Lee wrote: > An error unrelated to ufshcd_try_to_abort_task is being output and > can cause confusion. So, I modified it to output the result of abort > fail. This modification was similarly revised by referring to the > ufshcd_abort function. > > Signed-off-by: Chanwoo Lee <cw9316.lee@samsung.com> > --- > drivers/ufs/core/ufs-mcq.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c > index 005d63ab1f44..fc24d1af1fe8 100644 > --- a/drivers/ufs/core/ufs-mcq.c > +++ b/drivers/ufs/core/ufs-mcq.c > @@ -667,9 +667,11 @@ int ufshcd_mcq_abort(struct scsi_cmnd *cmd) > * in the completion queue either. Query the device to see if > * the command is being processed in the device. > */ > - if (ufshcd_try_to_abort_task(hba, tag)) { > + err = ufshcd_try_to_abort_task(hba, tag); > + if (err) { > dev_err(hba->dev, "%s: device abort failed %d\n", __func__, err); > lrbp->req_abort_skip = true; > + err = FAILED; > goto out; > } Why does the word "Fixing" occur in the title of this patch? I think that this patch does not affect the value returned by ufshcd_mcq_abort(). From the start of that function: int err = FAILED; Thanks, Bart.
On 5/23/24 22:43, Bart Van Assche wrote: >On 5/22/24 17:22, Chanwoo Lee wrote: >> An error unrelated to ufshcd_try_to_abort_task is being output and >> can cause confusion. So, I modified it to output the result of abort >> fail. This modification was similarly revised by referring to the >> ufshcd_abort function. >> >> Signed-off-by: Chanwoo Lee <cw9316.lee@samsung.com> >> --- >> drivers/ufs/core/ufs-mcq.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c >> index 005d63ab1f44..fc24d1af1fe8 100644 >> --- a/drivers/ufs/core/ufs-mcq.c >> +++ b/drivers/ufs/core/ufs-mcq.c >> @@ -667,9 +667,11 @@ int ufshcd_mcq_abort(struct scsi_cmnd *cmd) >> * in the completion queue either. Query the device to see if >> * the command is being processed in the device. >> */ >> - if (ufshcd_try_to_abort_task(hba, tag)) { >> + err = ufshcd_try_to_abort_task(hba, tag); >> + if (err) { >> dev_err(hba->dev, "%s: device abort failed %d\n", __func__, err); >> lrbp->req_abort_skip = true; >> + err = FAILED; >> goto out; >> } > >Why does the word "Fixing" occur in the title of this patch? I think >that this patch does not affect the value returned by >ufshcd_mcq_abort(). From the start of that function: > >int err = FAILED; > >Thanks, > >Bart. I thought this patch would be appropriate to "fix" the following log. * dev_err(hba->dev, "%s: device abort failed %d\n", __func__, err); If "Fixing" is not appropriate, could you suggest another word? Thanks, Chanwoo Lee.
On 5/23/24 16:56, Chanwoo Lee wrote: > I thought this patch would be appropriate to "fix" the following log. > * dev_err(hba->dev, "%s: device abort failed %d\n", __func__, err); > If "Fixing" is not appropriate, could you suggest another word? That's something I had not noticed. This is indeed a bug fix. Please add a "Fixes:" tag as is expected for bug fixes. BTW, I think that ufshcd_mcq_abort() can be improved significantly. How about reworking that function as follows before the bug reported in this patch is fixed? - Remove the local variable 'err' (and reintroduce that variable in your patch). - Change all 'goto out' statements into 'return FAILED'. - Add 'return SUCCESS' at the end. I expect that this change will make that function easier to read and to maintain. Thanks, Bart.
On 5/24/24 12:08, Bart Van Assche wrote: >On 5/23/24 16:56, Chanwoo Lee wrote: >> I thought this patch would be appropriate to "fix" the following log. >> * dev_err(hba->dev, "%s: device abort failed %d\n", __func__, err); >> If "Fixing" is not appropriate, could you suggest another word? > >That's something I had not noticed. This is indeed a bug fix. Please add >a "Fixes:" tag as is expected for bug fixes. > >BTW, I think that ufshcd_mcq_abort() can be improved significantly. How >about reworking that function as follows before the bug reported in this >patch is fixed? >- Remove the local variable 'err' (and reintroduce that variable in your >patch). >- Change all 'goto out' statements into 'return FAILED'. >- Add 'return SUCCESS' at the end. > >I expect that this change will make that function easier to read and to >maintain. > >Thanks, > >Bart. Thank you for the good suggestion. I will create a new patch and reply with v2. Thanks, Chanwoo Lee.
diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c index 005d63ab1f44..fc24d1af1fe8 100644 --- a/drivers/ufs/core/ufs-mcq.c +++ b/drivers/ufs/core/ufs-mcq.c @@ -667,9 +667,11 @@ int ufshcd_mcq_abort(struct scsi_cmnd *cmd) * in the completion queue either. Query the device to see if * the command is being processed in the device. */ - if (ufshcd_try_to_abort_task(hba, tag)) { + err = ufshcd_try_to_abort_task(hba, tag); + if (err) { dev_err(hba->dev, "%s: device abort failed %d\n", __func__, err); lrbp->req_abort_skip = true; + err = FAILED; goto out; }
An error unrelated to ufshcd_try_to_abort_task is being output and can cause confusion. So, I modified it to output the result of abort fail. This modification was similarly revised by referring to the ufshcd_abort function. Signed-off-by: Chanwoo Lee <cw9316.lee@samsung.com> --- drivers/ufs/core/ufs-mcq.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)