| Message ID | 1599099873-32579-2-git-send-email-cang@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] scsi: ufs: Abort tasks before clear them from doorbell | expand |
I can't reconcile this hunk: On Wed, 2020-09-02 at 19:24 -0700, Can Guo wrote: > @@ -6504,6 +6505,80 @@ static void ufshcd_set_req_abort_skip(struct > ufs_hba *hba, unsigned long bitmap) > * issued. To avoid that, first issue UFS_QUERY_TASK to check if the > command is > * really issued and then try to abort it. > * > + * Returns zero on success, non-zero on failure > + */ > +static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag) > +{ > + struct ufshcd_lrb *lrbp = &hba->lrb[tag]; > + int err = 0; > + int poll_cnt; > + u8 resp = 0xF; > + u32 reg; > + > + for (poll_cnt = 100; poll_cnt; poll_cnt--) { > + err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp- > >task_tag, > + UFS_QUERY_TASK, &resp); > + if (!err && resp == > UPIU_TASK_MANAGEMENT_FUNC_SUCCEEDED) { > + /* cmd pending in the device */ > + dev_err(hba->dev, "%s: cmd pending in the > device. tag = %d\n", > + __func__, tag); > + break; > + } else if (!err && resp == > UPIU_TASK_MANAGEMENT_FUNC_COMPL) { > + /* > + * cmd not pending in the device, check if > it is > + * in transition. > + */ > + dev_err(hba->dev, "%s: cmd at tag %d not > pending in the device.\n", > + __func__, tag); > + reg = ufshcd_readl(hba, > REG_UTP_TRANSFER_REQ_DOOR_BELL); > + if (reg & (1 << tag)) { > + /* sleep for max. 200us to stabilize > */ > + usleep_range(100, 200); > + continue; > + } > + /* command completed already */ > + dev_err(hba->dev, "%s: cmd at tag %d > successfully cleared from DB.\n", > + __func__, tag); > + goto out; > + } else { > + dev_err(hba->dev, > + "%s: no response from device. tag = > %d, err %d\n", > + __func__, tag, err); > + if (!err) > + err = resp; /* service response > error */ > + goto out; > + } > + } > + > + if (!poll_cnt) { > + err = -EBUSY; > + goto out; > + } > + > + err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag, > + UFS_ABORT_TASK, &resp); > + if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) { > + if (!err) { > + err = resp; /* service response error */ > + dev_err(hba->dev, "%s: issued. tag = %d, err > %d\n", > + __func__, tag, err); > + } > + goto out; > + } > + > + err = ufshcd_clear_cmd(hba, tag); > + if (err) > + dev_err(hba->dev, "%s: Failed clearing cmd at tag > %d, err %d\n", > + __func__, tag, err); > + > +out: > + return err; > +} > + > +/** > + * ufshcd_abort - scsi host template eh_abort_handler callback > + * @cmd: SCSI command pointer > + * > * Returns SUCCESS/FAILED > */ > static int ufshcd_abort(struct scsi_cmnd *cmd) > @@ -6513,8 +6588,6 @@ static int ufshcd_abort(struct scsi_cmnd *cmd) > unsigned long flags; > unsigned int tag; > int err = 0; > - int poll_cnt; > - u8 resp = 0xF; > struct ufshcd_lrb *lrbp; > u32 reg; > > @@ -6583,63 +6656,9 @@ static int ufshcd_abort(struct scsi_cmnd *cmd) > goto out; > } > > - for (poll_cnt = 100; poll_cnt; poll_cnt--) { > - err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp- > >task_tag, > - UFS_QUERY_TASK, &resp); > - if (!err && resp == > UPIU_TASK_MANAGEMENT_FUNC_SUCCEEDED) { > - /* cmd pending in the device */ > - dev_err(hba->dev, "%s: cmd pending in the > device. tag = %d\n", > - __func__, tag); > - break; > - } else if (!err && resp == > UPIU_TASK_MANAGEMENT_FUNC_COMPL) { > - /* > - * cmd not pending in the device, check if > it is > - * in transition. > - */ > - dev_err(hba->dev, "%s: cmd at tag %d not > pending in the device.\n", > - __func__, tag); > - reg = ufshcd_readl(hba, > REG_UTP_TRANSFER_REQ_DOOR_BELL); > - if (reg & (1 << tag)) { > - /* sleep for max. 200us to stabilize > */ > - usleep_range(100, 200); > - continue; > - } > - /* command completed already */ > - dev_err(hba->dev, "%s: cmd at tag %d > successfully cleared from DB.\n", > - __func__, tag); > - goto out; > - } else { > - dev_err(hba->dev, > - "%s: no response from device. tag = > %d, err %d\n", > - __func__, tag, err); > - if (!err) > - err = resp; /* service response > error */ > - goto out; > - } > - } > - > - if (!poll_cnt) { > - err = -EBUSY; > - goto out; > - } > - > - err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag, > - UFS_ABORT_TASK, &resp); > - if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) { > - if (!err) { > - err = resp; /* service response error */ > - dev_err(hba->dev, "%s: issued. tag = %d, err > %d\n", > - __func__, tag, err); > - } > - goto out; > - } > - > - err = ufshcd_clear_cmd(hba, tag); > - if (err) { > - dev_err(hba->dev, "%s: Failed clearing cmd at tag > %d, err %d\n", > - __func__, tag, err); > + err = ufshcd_try_to_abort_task(hba, tag); > + if (err) > goto out; > - } > > spin_lock_irqsave(host->host_lock, flags); > __ufshcd_transfer_req_compl(hba, (1UL << tag)); With the change in this fix: commit b10178ee7fa88b68a9e8adc06534d2605cb0ec23 Author: Stanley Chu <stanley.chu@mediatek.com> Date: Tue Aug 11 16:18:58 2020 +0200 scsi: ufs: Clean up completed request without interrupt notification It looks like there have to be two separate error returns from your new ufshcd_try_to_abort_function() so it knows to continue with usfhcd_transfer_req_complete(), or the whole function needs to be refactored, but if this goes upstream as is it looks like it will eliminate the bug fix. James
Can and Stanley,
> I can't reconcile this hunk:
Please provide a resolution for these conflicting commits in fixes and
queue:
307348f6ab14 scsi: ufs: Abort tasks before clearing them from doorbell
b10178ee7fa8 scsi: ufs: Clean up completed request without interrupt
notification
Thanks!
Hi Martin, Can, On Wed, 2020-09-09 at 22:32 -0400, Martin K. Petersen wrote: > Can and Stanley, > > > I can't reconcile this hunk: > > Please provide a resolution for these conflicting commits in fixes and > queue: > > 307348f6ab14 scsi: ufs: Abort tasks before clearing them from doorbell > > b10178ee7fa8 scsi: ufs: Clean up completed request without interrupt > notification > Can's patch has considered my fix in the new flow. To be more clear, for the fixing case in my patch, ufshcd_try_to_abort_task() will return 0 (err = 0) and finally the target tag can be completed and cleared by __ufshcd_transfer_req_compl() in Can's new flow. Thus I think the resolution can just using the code in Can's patch. Can, please correct me if I was wrong. Thanks, Stanley Chu > Thanks! >
On Thu, 2020-09-10 at 10:48 +0800, Stanley Chu wrote: > Hi Martin, Can, > > On Wed, 2020-09-09 at 22:32 -0400, Martin K. Petersen wrote: > > Can and Stanley, > > > > > I can't reconcile this hunk: > > > > Please provide a resolution for these conflicting commits in fixes > > and > > queue: > > > > 307348f6ab14 scsi: ufs: Abort tasks before clearing them from > > doorbell > > > > b10178ee7fa8 scsi: ufs: Clean up completed request without > > interrupt > > notification > > > > Can's patch has considered my fix in the new flow. > > To be more clear, for the fixing case in my patch, > ufshcd_try_to_abort_task() will return 0 (err = 0) and finally the > target tag can be completed and cleared by > __ufshcd_transfer_req_compl() > in Can's new flow. > > Thus I think the resolution can just using the code in Can's patch. > > Can, please correct me if I was wrong. Well, that really doesn't make for an easy merge. The resolution I took is below. James --- commit 5399a4aa684d491c35a386effe385c06b41398fa Merge: 59958f7a956b 8c6572356646 Author: James Bottomley <James.Bottomley@HansenPartnership.com> Date: Wed Sep 9 23:12:52 2020 -0700 Merge branch 'misc' into for-next Conflicts: drivers/scsi/ufs/ufshcd.c drivers/scsi/ufs/ufshcd.h diff --cc drivers/scsi/ufs/ufshcd.c index 34e1ab407b05,05716f62febe..49478c8a601f --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@@ -6574,84 -6739,22 +6736,25 @@@ static int ufshcd_abort(struct scsi_cmn } hba->req_abort_count++; - /* Skip task abort in case previous aborts failed and report failure */ - if (lrbp->req_abort_skip) { - err = -EIO; - goto out; + if (!(reg & (1 << tag))) { + dev_err(hba->dev, + "%s: cmd was completed, but without a notifying intr, tag = %d", + __func__, tag); + goto cleanup; } - err = ufshcd_try_to_abort_task(hba, tag); - if (err) - goto out; - - spin_lock_irqsave(host->host_lock, flags); - __ufshcd_transfer_req_compl(hba, (1UL << tag)); - spin_unlock_irqrestore(host->host_lock, flags); + /* Skip task abort in case previous aborts failed and report failure */ - if (lrbp->req_abort_skip) { ++ if (lrbp->req_abort_skip) + err = -EIO; - goto out; - } - - for (poll_cnt = 100; poll_cnt; poll_cnt--) { - err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag, - UFS_QUERY_TASK, &resp); - if (!err && resp == UPIU_TASK_MANAGEMENT_FUNC_SUCCEEDED) { - /* cmd pending in the device */ - dev_err(hba->dev, "%s: cmd pending in the device. tag = %d\n", - __func__, tag); - break; - } else if (!err && resp == UPIU_TASK_MANAGEMENT_FUNC_COMPL) { - /* - * cmd not pending in the device, check if it is - * in transition. - */ - dev_err(hba->dev, "%s: cmd at tag %d not pending in the device.\n", - __func__, tag); - reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL); - if (reg & (1 << tag)) { - /* sleep for max. 200us to stabilize */ - usleep_range(100, 200); - continue; - } - /* command completed already */ - dev_err(hba->dev, "%s: cmd at tag %d successfully cleared from DB.\n", - __func__, tag); - goto cleanup; - } else { - dev_err(hba->dev, - "%s: no response from device. tag = %d, err %d\n", - __func__, tag, err); - if (!err) - err = resp; /* service response error */ - goto out; - } - } - - if (!poll_cnt) { - err = -EBUSY; - goto out; - } - - err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag, - UFS_ABORT_TASK, &resp); - if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) { - if (!err) { - err = resp; /* service response error */ - dev_err(hba->dev, "%s: issued. tag = %d, err %d\n", - __func__, tag, err); - } - goto out; - } - - err = ufshcd_clear_cmd(hba, tag); - if (err) { - dev_err(hba->dev, "%s: Failed clearing cmd at tag %d, err %d\n", - __func__, tag, err); - goto out; - } ++ else ++ err = ufshcd_try_to_abort_task(hba, tag); -out: + if (!err) { +cleanup: - spin_lock_irqsave(host->host_lock, flags); - __ufshcd_transfer_req_compl(hba, (1UL << tag)); - spin_unlock_irqrestore(host->host_lock, flags); - ++ spin_lock_irqsave(host->host_lock, flags); ++ __ufshcd_transfer_req_compl(hba, (1UL << tag)); ++ spin_unlock_irqrestore(host->host_lock, flags); +out: - if (!err) { err = SUCCESS; } else { dev_err(hba->dev, "%s: failed with err %d\n", __func__, err); diff --cc drivers/scsi/ufs/ufshcd.h index b5b2761456fb,8011fdc89fb1..6663325ed8a0 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@@ -531,11 -531,10 +531,16 @@@ enum ufshcd_quirks */ UFSHCD_QUIRK_BROKEN_OCS_FATAL_ERROR = 1 << 10, + /* + * This quirk needs to be enabled if the host controller has + * auto-hibernate capability but it doesn't work. + */ + UFSHCD_QUIRK_BROKEN_AUTO_HIBERN8 = 1 << 11, ++ + /* + * This quirk needs to disable manual flush for write booster + */ - UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL = 1 << 11, ++ UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL = 1 << 12, }; enum ufshcd_caps {
Hi James, On Wed, 2020-09-09 at 23:18 -0700, James Bottomley wrote: > On Thu, 2020-09-10 at 10:48 +0800, Stanley Chu wrote: > > Hi Martin, Can, > > > > On Wed, 2020-09-09 at 22:32 -0400, Martin K. Petersen wrote: > > > Can and Stanley, > > > > > > > I can't reconcile this hunk: > > > > > > Please provide a resolution for these conflicting commits in fixes > > > and > > > queue: > > > > > > 307348f6ab14 scsi: ufs: Abort tasks before clearing them from > > > doorbell > > > > > > b10178ee7fa8 scsi: ufs: Clean up completed request without > > > interrupt > > > notification > > > > > > > Can's patch has considered my fix in the new flow. > > > > To be more clear, for the fixing case in my patch, > > ufshcd_try_to_abort_task() will return 0 (err = 0) and finally the > > target tag can be completed and cleared by > > __ufshcd_transfer_req_compl() > > in Can's new flow. > > > > Thus I think the resolution can just using the code in Can's patch. > > > > Can, please correct me if I was wrong. > > Well, that really doesn't make for an easy merge. The resolution I took > is below. > > James > > --- > > commit 5399a4aa684d491c35a386effe385c06b41398fa > Merge: 59958f7a956b 8c6572356646 > Author: James Bottomley <James.Bottomley@HansenPartnership.com> > Date: Wed Sep 9 23:12:52 2020 -0700 > > Merge branch 'misc' into for-next > > Conflicts: > drivers/scsi/ufs/ufshcd.c > drivers/scsi/ufs/ufshcd.h > > diff --cc drivers/scsi/ufs/ufshcd.c > index 34e1ab407b05,05716f62febe..49478c8a601f > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@@ -6574,84 -6739,22 +6736,25 @@@ static int ufshcd_abort(struct scsi_cmn > } > hba->req_abort_count++; > > - /* Skip task abort in case previous aborts failed and report failure */ > - if (lrbp->req_abort_skip) { > - err = -EIO; > - goto out; > + if (!(reg & (1 << tag))) { > + dev_err(hba->dev, > + "%s: cmd was completed, but without a notifying intr, tag = %d", > + __func__, tag); > + goto cleanup; > } > > - err = ufshcd_try_to_abort_task(hba, tag); > - if (err) > - goto out; > - > - spin_lock_irqsave(host->host_lock, flags); > - __ufshcd_transfer_req_compl(hba, (1UL << tag)); > - spin_unlock_irqrestore(host->host_lock, flags); > + /* Skip task abort in case previous aborts failed and report failure */ > - if (lrbp->req_abort_skip) { > ++ if (lrbp->req_abort_skip) > + err = -EIO; > - goto out; > - } > - > - for (poll_cnt = 100; poll_cnt; poll_cnt--) { > - err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag, > - UFS_QUERY_TASK, &resp); > - if (!err && resp == UPIU_TASK_MANAGEMENT_FUNC_SUCCEEDED) { > - /* cmd pending in the device */ > - dev_err(hba->dev, "%s: cmd pending in the device. tag = %d\n", > - __func__, tag); > - break; > - } else if (!err && resp == UPIU_TASK_MANAGEMENT_FUNC_COMPL) { > - /* > - * cmd not pending in the device, check if it is > - * in transition. > - */ > - dev_err(hba->dev, "%s: cmd at tag %d not pending in the device.\n", > - __func__, tag); > - reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL); > - if (reg & (1 << tag)) { > - /* sleep for max. 200us to stabilize */ > - usleep_range(100, 200); > - continue; > - } > - /* command completed already */ > - dev_err(hba->dev, "%s: cmd at tag %d successfully cleared from DB.\n", > - __func__, tag); > - goto cleanup; > - } else { > - dev_err(hba->dev, > - "%s: no response from device. tag = %d, err %d\n", > - __func__, tag, err); > - if (!err) > - err = resp; /* service response error */ > - goto out; > - } > - } > - > - if (!poll_cnt) { > - err = -EBUSY; > - goto out; > - } > - > - err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag, > - UFS_ABORT_TASK, &resp); > - if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) { > - if (!err) { > - err = resp; /* service response error */ > - dev_err(hba->dev, "%s: issued. tag = %d, err %d\n", > - __func__, tag, err); > - } > - goto out; > - } > - > - err = ufshcd_clear_cmd(hba, tag); > - if (err) { > - dev_err(hba->dev, "%s: Failed clearing cmd at tag %d, err %d\n", > - __func__, tag, err); > - goto out; > - } > ++ else > ++ err = ufshcd_try_to_abort_task(hba, tag); > > -out: > + if (!err) { > +cleanup: Yeah, considering Bean Huo's patch "scsi: ufs: No need to send Abort Task if the task in DB was cleared", "cleanup" label shall be added back here. So your resolution looks good to me. Thanks so much : ) Stanley Chu > - spin_lock_irqsave(host->host_lock, flags); > - __ufshcd_transfer_req_compl(hba, (1UL << tag)); > - spin_unlock_irqrestore(host->host_lock, flags); > - > ++ spin_lock_irqsave(host->host_lock, flags); > ++ __ufshcd_transfer_req_compl(hba, (1UL << tag)); > ++ spin_unlock_irqrestore(host->host_lock, flags); > +out: > - if (!err) { > err = SUCCESS; > } else { > dev_err(hba->dev, "%s: failed with err %d\n", __func__, err); > diff --cc drivers/scsi/ufs/ufshcd.h > index b5b2761456fb,8011fdc89fb1..6663325ed8a0 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@@ -531,11 -531,10 +531,16 @@@ enum ufshcd_quirks > */ > UFSHCD_QUIRK_BROKEN_OCS_FATAL_ERROR = 1 << 10, > > + /* > + * This quirk needs to be enabled if the host controller has > + * auto-hibernate capability but it doesn't work. > + */ > + UFSHCD_QUIRK_BROKEN_AUTO_HIBERN8 = 1 << 11, > ++ > + /* > + * This quirk needs to disable manual flush for write booster > + */ > - UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL = 1 << 11, > ++ UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL = 1 << 12, > }; > > enum ufshcd_caps {
On Thu, 2020-09-10 at 16:18 +0800, Stanley Chu wrote: [...] > > + if (!err) { > > +cleanup: > > Yeah, considering Bean Huo's patch "scsi: ufs: No need to send Abort > Task if the task in DB was cleared", "cleanup" label shall be added > back here. > > So your resolution looks good to me. > > Thanks so much : ) You're welcome ... but just remember I have to explain this to Linus when the merge window opens. It would be a lot easier if this hadn't happened so please don't make it any worse ... James
Hi James and Stanley, On 2020-09-11 00:09, James Bottomley wrote: > On Thu, 2020-09-10 at 16:18 +0800, Stanley Chu wrote: > [...] >> > + if (!err) { >> > +cleanup: >> >> Yeah, considering Bean Huo's patch "scsi: ufs: No need to send Abort >> Task if the task in DB was cleared", "cleanup" label shall be added >> back here. >> >> So your resolution looks good to me. >> >> Thanks so much : ) > > You're welcome ... but just remember I have to explain this to Linus > when the merge window opens. It would be a lot easier if this hadn't > happened so please don't make it any worse ... > > James Sorry that my changes got you confused and thank you for help resolve the conflicts. My change ("scsi: ufs: Abort tasks before clearing them from doorbell") is to serve my fixes to ufs error recovery which only got picked up on scsi-queue-5.10. So I checked out to scsi-queue-5.10 and made my changes on the tip of scsi-queue-5.10, below 2 changes were not even present in scsi-queue-5.10 back that time. scsi: ufs: Clean up completed request without interrupt notification scsi: ufs: No need to send Abort Task if the task in DB was cleared Is there anything wrong with my work flow above? Please let me know the right process so that I can avoid such conflicts in my next changes, which also touch the func ufshcd_abort(). Thanks! Regards, Can Guo.
On Fri, 2020-09-11 at 02:16 +0000, Can Guo wrote: > > > > > > So your resolution looks good to me. > > > > > > Thanks so much : ) > > > > You're welcome ... but just remember I have to explain this to > > Linus > > when the merge window opens. It would be a lot easier if this > > hadn't > > happened so please don't make it any worse ... > > > > James > > Sorry that my changes got you confused and thank you for help > resolve > the > conflicts. My change ("scsi: ufs: Abort tasks before clearing them > from > doorbell") is to serve my fixes to ufs error recovery which only got > picked > up on scsi-queue-5.10. So I checked out to scsi-queue-5.10 and made > my > changes on the tip of scsi-queue-5.10, below 2 changes were not even > present in scsi-queue-5.10 back that time. I mentioned here https://patchwork.kernel.org/patch/11734713/ this change (scsi: ufs: Abort tasks before clearing them from doorbell) has conflicts with the scsi-fixes branch. I don't know which branch is the main branch we should focus on. Bean
On 2020-09-11 17:09, Bean Huo wrote: > On Fri, 2020-09-11 at 02:16 +0000, Can Guo wrote: >> > > >> > > So your resolution looks good to me. >> > > >> > > Thanks so much : ) >> > >> > You're welcome ... but just remember I have to explain this to >> > Linus >> > when the merge window opens. It would be a lot easier if this >> > hadn't >> > happened so please don't make it any worse ... >> > >> > James >> >> Sorry that my changes got you confused and thank you for help >> resolve >> the >> conflicts. My change ("scsi: ufs: Abort tasks before clearing them >> from >> doorbell") is to serve my fixes to ufs error recovery which only got >> picked >> up on scsi-queue-5.10. So I checked out to scsi-queue-5.10 and made >> my >> changes on the tip of scsi-queue-5.10, below 2 changes were not even >> present in scsi-queue-5.10 back that time. > > I mentioned here https://patchwork.kernel.org/patch/11734713/ > > this change (scsi: ufs: Abort tasks before clearing them from doorbell) > has conflicts with the scsi-fixes branch. I don't know which branch is > the main branch we should focus on. > > > Bean Yeah, I know that one, but I was not even working on scsi-fixes branch at that time. Now I have two more fixes to ufshcd_abort(), not sure which branch I should work on, so asking the same here. Regards, Can Guo.
Hi Bean, On 2020-09-11 17:09, Bean Huo wrote: > On Fri, 2020-09-11 at 02:16 +0000, Can Guo wrote: >> > > >> > > So your resolution looks good to me. >> > > >> > > Thanks so much : ) >> > >> > You're welcome ... but just remember I have to explain this to >> > Linus >> > when the merge window opens. It would be a lot easier if this >> > hadn't >> > happened so please don't make it any worse ... >> > >> > James >> >> Sorry that my changes got you confused and thank you for help >> resolve >> the >> conflicts. My change ("scsi: ufs: Abort tasks before clearing them >> from >> doorbell") is to serve my fixes to ufs error recovery which only got >> picked >> up on scsi-queue-5.10. So I checked out to scsi-queue-5.10 and made >> my >> changes on the tip of scsi-queue-5.10, below 2 changes were not even >> present in scsi-queue-5.10 back that time. > > I mentioned here https://patchwork.kernel.org/patch/11734713/ Do you know when can this change be picked up in scsi-queue-5.10? If I push my fixes to ufshcd_abort() on scsi-queue-5.10, they will run into conflicts with this one again, right? How should I move forward now? Thanks. Regards, Can Guo. > > this change (scsi: ufs: Abort tasks before clearing them from doorbell) > has conflicts with the scsi-fixes branch. I don't know which branch is > the main branch we should focus on. > > > Bean
Can, > Do you know when can this change be picked up in scsi-queue-5.10? > If I push my fixes to ufshcd_abort() on scsi-queue-5.10, they will > run into conflicts with this one again, right? How should I move > forward now? You should be able to use 5.10/scsi-queue as baseline now. For 5.11 I think I'll do a separate branch for UFS.
On 2020-09-16 04:21, Martin K. Petersen wrote: > Can, > >> Do you know when can this change be picked up in scsi-queue-5.10? >> If I push my fixes to ufshcd_abort() on scsi-queue-5.10, they will >> run into conflicts with this one again, right? How should I move >> forward now? > > You should be able to use 5.10/scsi-queue as baseline now. > > For 5.11 I think I'll do a separate branch for UFS. Thanks for the information. Regards, Can Guo.
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 06e2439..72afe12 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -238,6 +238,7 @@ static int ufshcd_setup_hba_vreg(struct ufs_hba *hba, bool on); static int ufshcd_setup_vreg(struct ufs_hba *hba, bool on); static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba, struct ufs_vreg *vreg); +static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag); static int ufshcd_wb_buf_flush_enable(struct ufs_hba *hba); static int ufshcd_wb_buf_flush_disable(struct ufs_hba *hba); static int ufshcd_wb_ctrl(struct ufs_hba *hba, bool enable); @@ -5666,8 +5667,8 @@ static void ufshcd_err_handler(struct work_struct *work) { struct ufs_hba *hba; unsigned long flags; - u32 err_xfer = 0; - u32 err_tm = 0; + bool err_xfer = false; + bool err_tm = false; int err = 0; int tag; bool needs_reset = false; @@ -5743,7 +5744,7 @@ static void ufshcd_err_handler(struct work_struct *work) spin_unlock_irqrestore(hba->host->host_lock, flags); /* Clear pending transfer requests */ for_each_set_bit(tag, &hba->outstanding_reqs, hba->nutrs) { - if (ufshcd_clear_cmd(hba, tag)) { + if (ufshcd_try_to_abort_task(hba, tag)) { err_xfer = true; goto lock_skip_pending_xfer_clear; } @@ -6495,7 +6496,7 @@ static void ufshcd_set_req_abort_skip(struct ufs_hba *hba, unsigned long bitmap) } /** - * ufshcd_abort - abort a specific command + * ufshcd_try_to_abort_task - abort a specific task * @cmd: SCSI command pointer * * Abort the pending command in device by sending UFS_ABORT_TASK task management @@ -6504,6 +6505,80 @@ static void ufshcd_set_req_abort_skip(struct ufs_hba *hba, unsigned long bitmap) * issued. To avoid that, first issue UFS_QUERY_TASK to check if the command is * really issued and then try to abort it. * + * Returns zero on success, non-zero on failure + */ +static int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag) +{ + struct ufshcd_lrb *lrbp = &hba->lrb[tag]; + int err = 0; + int poll_cnt; + u8 resp = 0xF; + u32 reg; + + for (poll_cnt = 100; poll_cnt; poll_cnt--) { + err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag, + UFS_QUERY_TASK, &resp); + if (!err && resp == UPIU_TASK_MANAGEMENT_FUNC_SUCCEEDED) { + /* cmd pending in the device */ + dev_err(hba->dev, "%s: cmd pending in the device. tag = %d\n", + __func__, tag); + break; + } else if (!err && resp == UPIU_TASK_MANAGEMENT_FUNC_COMPL) { + /* + * cmd not pending in the device, check if it is + * in transition. + */ + dev_err(hba->dev, "%s: cmd at tag %d not pending in the device.\n", + __func__, tag); + reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL); + if (reg & (1 << tag)) { + /* sleep for max. 200us to stabilize */ + usleep_range(100, 200); + continue; + } + /* command completed already */ + dev_err(hba->dev, "%s: cmd at tag %d successfully cleared from DB.\n", + __func__, tag); + goto out; + } else { + dev_err(hba->dev, + "%s: no response from device. tag = %d, err %d\n", + __func__, tag, err); + if (!err) + err = resp; /* service response error */ + goto out; + } + } + + if (!poll_cnt) { + err = -EBUSY; + goto out; + } + + err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag, + UFS_ABORT_TASK, &resp); + if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) { + if (!err) { + err = resp; /* service response error */ + dev_err(hba->dev, "%s: issued. tag = %d, err %d\n", + __func__, tag, err); + } + goto out; + } + + err = ufshcd_clear_cmd(hba, tag); + if (err) + dev_err(hba->dev, "%s: Failed clearing cmd at tag %d, err %d\n", + __func__, tag, err); + +out: + return err; +} + +/** + * ufshcd_abort - scsi host template eh_abort_handler callback + * @cmd: SCSI command pointer + * * Returns SUCCESS/FAILED */ static int ufshcd_abort(struct scsi_cmnd *cmd) @@ -6513,8 +6588,6 @@ static int ufshcd_abort(struct scsi_cmnd *cmd) unsigned long flags; unsigned int tag; int err = 0; - int poll_cnt; - u8 resp = 0xF; struct ufshcd_lrb *lrbp; u32 reg; @@ -6583,63 +6656,9 @@ static int ufshcd_abort(struct scsi_cmnd *cmd) goto out; } - for (poll_cnt = 100; poll_cnt; poll_cnt--) { - err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag, - UFS_QUERY_TASK, &resp); - if (!err && resp == UPIU_TASK_MANAGEMENT_FUNC_SUCCEEDED) { - /* cmd pending in the device */ - dev_err(hba->dev, "%s: cmd pending in the device. tag = %d\n", - __func__, tag); - break; - } else if (!err && resp == UPIU_TASK_MANAGEMENT_FUNC_COMPL) { - /* - * cmd not pending in the device, check if it is - * in transition. - */ - dev_err(hba->dev, "%s: cmd at tag %d not pending in the device.\n", - __func__, tag); - reg = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL); - if (reg & (1 << tag)) { - /* sleep for max. 200us to stabilize */ - usleep_range(100, 200); - continue; - } - /* command completed already */ - dev_err(hba->dev, "%s: cmd at tag %d successfully cleared from DB.\n", - __func__, tag); - goto out; - } else { - dev_err(hba->dev, - "%s: no response from device. tag = %d, err %d\n", - __func__, tag, err); - if (!err) - err = resp; /* service response error */ - goto out; - } - } - - if (!poll_cnt) { - err = -EBUSY; - goto out; - } - - err = ufshcd_issue_tm_cmd(hba, lrbp->lun, lrbp->task_tag, - UFS_ABORT_TASK, &resp); - if (err || resp != UPIU_TASK_MANAGEMENT_FUNC_COMPL) { - if (!err) { - err = resp; /* service response error */ - dev_err(hba->dev, "%s: issued. tag = %d, err %d\n", - __func__, tag, err); - } - goto out; - } - - err = ufshcd_clear_cmd(hba, tag); - if (err) { - dev_err(hba->dev, "%s: Failed clearing cmd at tag %d, err %d\n", - __func__, tag, err); + err = ufshcd_try_to_abort_task(hba, tag); + if (err) goto out; - } spin_lock_irqsave(host->host_lock, flags); __ufshcd_transfer_req_compl(hba, (1UL << tag));