Re: [PATCH v5 7/7] ufs: core: Add error handling for MCQ mode

[Bao D. Nguyen]

On 5/19/2023 4:03 PM, Bart Van Assche wrote:
> On 5/11/23 23:28, Bao D. Nguyen wrote:
> > @@ -6378,18 +6407,36 @@ static bool ufshcd_abort_all(struct ufs_hba *hba)
> >       bool needs_reset = false;
> >       int tag, ret;
> > -    /* Clear pending transfer requests */
> > -    for_each_set_bit(tag, &hba->outstanding_reqs, hba->nutrs) {
> > -        ret = ufshcd_try_to_abort_task(hba, tag);
> > -        dev_err(hba->dev, "Aborting tag %d / CDB %#02x %s\n", tag,
> > -            hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1,
> > -            ret ? "failed" : "succeeded");
> > -        if (ret) {
> > -            needs_reset = true;
> > -            goto out;
> > +    if (is_mcq_enabled(hba)) {
> > +        struct ufshcd_lrb *lrbp;
> > +        int tag;
> > +
> > +        for (tag = 0; tag < hba->nutrs; tag++) {
> > +            lrbp = &hba->lrb[tag];
> > +            if (!ufshcd_cmd_inflight(lrbp->cmd))
> > +                continue;
> > +            ret = ufshcd_try_to_abort_task(hba, tag);
> > +            dev_err(hba->dev, "Aborting tag %d / CDB %#02x %s\n", tag,
> > +                hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1,
> > +                ret ? "failed" : "succeeded");
> > +            if (ret) {
> > +                needs_reset = true;
> > +                goto out;
> > +            }
> > +        }
> > +    } else {
> > +        /* Clear pending transfer requests */
> > +        for_each_set_bit(tag, &hba->outstanding_reqs, hba->nutrs) {
> > +            ret = ufshcd_try_to_abort_task(hba, tag);
> > +            dev_err(hba->dev, "Aborting tag %d / CDB %#02x %s\n", tag,
> > +                hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1,
> > +                ret ? "failed" : "succeeded");
> > +            if (ret) {
> > +                needs_reset = true;
> > +                goto out;
> > +            }
> >           }
> >       }
>
> Please move the ufshcd_cmd_inflight() check into ufshcd_try_to_abort_task()
> such that the same code path can be used for MCQ and legacy mode.
Hi Bart,
Because the ufshcd_try_to_abort_task() is shared by sdb and mcq modes, I 
feel a bit uncomfortable using the new function ufshcd_cmd_inflight() in 
ufshcd_try_to_abort_task(). In this patch series, I am trying to avoid 
changing the sdb error handling logic as much as possible; only add 
error handling support for mcq mode. If you feel there is a very good 
benefit in making the change, I would give it a try. Otherwise, I would 
prefer not touching sdb error handling code that has been working well. 
Please let me know.

Thanks,
Bao

> Thanks,
>
> Bart.