| Message ID | 20241016211154.2425403-5-bvanassche@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | UFS driver fixes and cleanups | expand |
> Use blk_mq_quiesce_tagset() / blk_mq_unquiesce_tagset() instead of > scsi_block_requests() / scsi_unblock_requests() because the former wait for > ongoing SCSI command dispatching to finish while the latter do not. > > Fixes: 2e3611e9546c ("scsi: ufs: fix exception event handling") I think that when Maya introduced the scsi_block_requests calls (2018), the block tagset quiesce api wasn't available yet (2022). Thanks, Avri > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/ufs/core/ufshcd.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index > 76884df580c3..ff1b0af74041 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -6195,7 +6195,7 @@ static void ufshcd_exception_event_handler(struct > work_struct *work) > u32 status = 0; > hba = container_of(work, struct ufs_hba, eeh_work); > > - ufshcd_scsi_block_requests(hba); > + blk_mq_quiesce_tagset(&hba->host->tag_set); > err = ufshcd_get_ee_status(hba, &status); > if (err) { > dev_err(hba->dev, "%s: failed to get exception status %d\n", @@ - > 6213,7 +6213,7 @@ static void ufshcd_exception_event_handler(struct > work_struct *work) > > ufs_debugfs_exception_event(hba, status); > out: > - ufshcd_scsi_unblock_requests(hba); > + blk_mq_unquiesce_tagset(&hba->host->tag_set); > } > > /* Complete requests that have door-bell cleared */
On 10/17/24 10:50 PM, Avri Altman wrote: >> Use blk_mq_quiesce_tagset() / blk_mq_unquiesce_tagset() instead of >> scsi_block_requests() / scsi_unblock_requests() because the former wait for >> ongoing SCSI command dispatching to finish while the latter do not. >> >> Fixes: 2e3611e9546c ("scsi: ufs: fix exception event handling") > > I think that when Maya introduced the scsi_block_requests calls (2018), > the block tagset quiesce api wasn't available yet (2022). Hi Avri, Do you perhaps want me to integrate that information in the patch description? Thanks, Bart.
> On 10/17/24 10:50 PM, Avri Altman wrote: > >> Use blk_mq_quiesce_tagset() / blk_mq_unquiesce_tagset() instead of > >> scsi_block_requests() / scsi_unblock_requests() because the former > >> wait for ongoing SCSI command dispatching to finish while the latter do > not. > >> > >> Fixes: 2e3611e9546c ("scsi: ufs: fix exception event handling") > > > > I think that when Maya introduced the scsi_block_requests calls > > (2018), the block tagset quiesce api wasn't available yet (2022). > > Hi Avri, > > Do you perhaps want me to integrate that information in the patch > description? No. But the Fixes tag seems strange, isn't it? Thanks, Avri > > Thanks, > > Bart.
On 10/18/24 10:25 AM, Avri Altman wrote: >> On 10/17/24 10:50 PM, Avri Altman wrote: >>>> Use blk_mq_quiesce_tagset() / blk_mq_unquiesce_tagset() instead of >>>> scsi_block_requests() / scsi_unblock_requests() because the former >>>> wait for ongoing SCSI command dispatching to finish while the latter do >> not. >>>> >>>> Fixes: 2e3611e9546c ("scsi: ufs: fix exception event handling") >> > >>> I think that when Maya introduced the scsi_block_requests calls >>> (2018), the block tagset quiesce api wasn't available yet (2022). >> >> Hi Avri, >> >> Do you perhaps want me to integrate that information in the patch >> description? > > No. But the Fixes tag seems strange, isn't it? Right, that patch did not introduce the issue that ufshcd_exception_event_handler() doesn't wait for ongoing SCSI command dispatching calls. Let me look up which patch introduced that issue. Thanks, Bart.
On 10/18/24 10:25 AM, Avri Altman wrote:
> No. But the Fixes tag seems strange, isn't it?
How about replacing the entire patch with the patch below?
Thanks,
Bart.
scsi: ufs: core: Simplify ufshcd_exception_event_handler()
The ufshcd_scsi_block_requests() and ufshcd_scsi_unblock_requests()
calls were introduced in ufshcd_exception_event_handler() to prevent
that querying the exception event information would time out. Commit
10fe5888a40e ("scsi: ufs: increase the scsi query response timeout")
increased the timeout for querying exception information from 30 ms to
1.5 s and thereby eliminated the risk that a timeout would happen.
Hence, the calls to block and unblock SCSI requests are superfluous.
Remove these calls.
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 76884df580c3..2fde1b0a6086 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -6195,12 +6195,11 @@ static void
ufshcd_exception_event_handler(struct work_struct *work)
u32 status = 0;
hba = container_of(work, struct ufs_hba, eeh_work);
- ufshcd_scsi_block_requests(hba);
err = ufshcd_get_ee_status(hba, &status);
if (err) {
dev_err(hba->dev, "%s: failed to get exception status %d\n",
__func__, err);
- goto out;
+ return;
}
trace_ufshcd_exception_event(dev_name(hba->dev), status);
@@ -6212,8 +6211,6 @@ static void ufshcd_exception_event_handler(struct
work_struct *work)
ufshcd_temp_exception_event_handler(hba, status);
ufs_debugfs_exception_event(hba, status);
-out:
- ufshcd_scsi_unblock_requests(hba);
}
/* Complete requests that have door-bell cleared */
> > On 10/18/24 10:25 AM, Avri Altman wrote: > > No. But the Fixes tag seems strange, isn't it? > > How about replacing the entire patch with the patch below? Right. Looks like it was introduced in the first place when the query timeout was 100ms: "..When trying to check the exception event status (for finding the cause for the exception event), the device may be busy with additional SCSI commands handling and may not respond within the 100ms timeout..." Looks good to me. I can also test it on Sunday. Thanks, Avri > > Thanks, > > Bart. > > > scsi: ufs: core: Simplify ufshcd_exception_event_handler() > > The ufshcd_scsi_block_requests() and ufshcd_scsi_unblock_requests() calls > were introduced in ufshcd_exception_event_handler() to prevent that > querying the exception event information would time out. Commit > 10fe5888a40e ("scsi: ufs: increase the scsi query response timeout") > increased the timeout for querying exception information from 30 ms to > 1.5 s and thereby eliminated the risk that a timeout would happen. > Hence, the calls to block and unblock SCSI requests are superfluous. > Remove these calls. > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index > 76884df580c3..2fde1b0a6086 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -6195,12 +6195,11 @@ static void > ufshcd_exception_event_handler(struct work_struct *work) > u32 status = 0; > hba = container_of(work, struct ufs_hba, eeh_work); > > - ufshcd_scsi_block_requests(hba); > err = ufshcd_get_ee_status(hba, &status); > if (err) { > dev_err(hba->dev, "%s: failed to get exception status %d\n", > __func__, err); > - goto out; > + return; > } > > trace_ufshcd_exception_event(dev_name(hba->dev), status); @@ - > 6212,8 +6211,6 @@ static void ufshcd_exception_event_handler(struct > work_struct *work) > ufshcd_temp_exception_event_handler(hba, status); > > ufs_debugfs_exception_event(hba, status); > -out: > - ufshcd_scsi_unblock_requests(hba); > } > > /* Complete requests that have door-bell cleared */
> > scsi: ufs: core: Simplify ufshcd_exception_event_handler() > > > > The ufshcd_scsi_block_requests() and ufshcd_scsi_unblock_requests() > > calls were introduced in ufshcd_exception_event_handler() to prevent > > that querying the exception event information would time out. Commit > > 10fe5888a40e ("scsi: ufs: increase the scsi query response timeout") > > increased the timeout for querying exception information from 30 ms to > > 1.5 s and thereby eliminated the risk that a timeout would happen. > > Hence, the calls to block and unblock SCSI requests are superfluous. > > Remove these calls. > > > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > > index > > 76884df580c3..2fde1b0a6086 100644 > > --- a/drivers/ufs/core/ufshcd.c > > +++ b/drivers/ufs/core/ufshcd.c > > @@ -6195,12 +6195,11 @@ static void > > ufshcd_exception_event_handler(struct work_struct *work) > > u32 status = 0; > > hba = container_of(work, struct ufs_hba, eeh_work); > > > > - ufshcd_scsi_block_requests(hba); > > err = ufshcd_get_ee_status(hba, &status); > > if (err) { > > dev_err(hba->dev, "%s: failed to get exception status %d\n", > > __func__, err); > > - goto out; > > + return; > > } > > > > trace_ufshcd_exception_event(dev_name(hba->dev), status); @@ - > > 6212,8 +6211,6 @@ static void ufshcd_exception_event_handler(struct > > work_struct *work) > > ufshcd_temp_exception_event_handler(hba, status); > > > > ufs_debugfs_exception_event(hba, status); > > -out: > > - ufshcd_scsi_unblock_requests(hba); > > } > > > > /* Complete requests that have door-bell cleared */ Tested-by: Avri Altman <avri.altman@wdc.com>
On Fri, 2024-10-18 at 12:35 -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 10/18/24 10:25 AM, Avri Altman wrote: > > No. But the Fixes tag seems strange, isn't it? > > How about replacing the entire patch with the patch below? > > Thanks, > > Bart. > > > scsi: ufs: core: Simplify ufshcd_exception_event_handler() > > The ufshcd_scsi_block_requests() and ufshcd_scsi_unblock_requests() > calls were introduced in ufshcd_exception_event_handler() to prevent > that querying the exception event information would time out. Commit > 10fe5888a40e ("scsi: ufs: increase the scsi query response timeout") > increased the timeout for querying exception information from 30 ms > to > 1.5 s and thereby eliminated the risk that a timeout would happen. > Hence, the calls to block and unblock SCSI requests are superfluous. > Remove these calls. > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index 76884df580c3..2fde1b0a6086 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -6195,12 +6195,11 @@ static void > ufshcd_exception_event_handler(struct work_struct *work) > u32 status = 0; > hba = container_of(work, struct ufs_hba, eeh_work); > > -ufshcd_scsi_block_requests(hba); > err = ufshcd_get_ee_status(hba, &status); > if (err) { > dev_err(hba->dev, "%s: failed to get exception status %d\n", > __func__, err); > -goto out; > +return; > } > > trace_ufshcd_exception_event(dev_name(hba->dev), status); > @@ -6212,8 +6211,6 @@ static void > ufshcd_exception_event_handler(struct > work_struct *work) > ufshcd_temp_exception_event_handler(hba, status); > > ufs_debugfs_exception_event(hba, status); > -out: > -ufshcd_scsi_unblock_requests(hba); > } > > /* Complete requests that have door-bell cleared */ > Hi Bart, This patch looks good to me as well. Thanks Peter
On 10/21/24 1:59 AM, Peter Wang (王信友) wrote:
> This patch looks good to me as well.
Does this count as a Reviewed-by?
Thanks,
Bart.
On Mon, 2024-10-21 at 16:25 -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 10/21/24 1:59 AM, Peter Wang (王信友) wrote: > > This patch looks good to me as well. > > Does this count as a Reviewed-by? > > Thanks, > > Bart. > Hi Bart, Yes, I also think that an extra loop shouldn't have too much impact. It's just to ensure that you haven't overlooked this aspect or that others might have different opinions. Reviewed-by: Peter Wang <peter.wang@mediatek.com> Thanks Peter
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 76884df580c3..ff1b0af74041 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -6195,7 +6195,7 @@ static void ufshcd_exception_event_handler(struct work_struct *work) u32 status = 0; hba = container_of(work, struct ufs_hba, eeh_work); - ufshcd_scsi_block_requests(hba); + blk_mq_quiesce_tagset(&hba->host->tag_set); err = ufshcd_get_ee_status(hba, &status); if (err) { dev_err(hba->dev, "%s: failed to get exception status %d\n", @@ -6213,7 +6213,7 @@ static void ufshcd_exception_event_handler(struct work_struct *work) ufs_debugfs_exception_event(hba, status); out: - ufshcd_scsi_unblock_requests(hba); + blk_mq_unquiesce_tagset(&hba->host->tag_set); } /* Complete requests that have door-bell cleared */
Use blk_mq_quiesce_tagset() / blk_mq_unquiesce_tagset() instead of scsi_block_requests() / scsi_unblock_requests() because the former wait for ongoing SCSI command dispatching to finish while the latter do not. Fixes: 2e3611e9546c ("scsi: ufs: fix exception event handling") Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/ufs/core/ufshcd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)