| Message ID | 1577192466-20762-2-git-send-email-stanley.chu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | scsi: ufs: use existed well-defined functions | expand |
On 2019-12-24 05:01, Stanley Chu wrote: > Currently UFS driver has ufshcd_scsi_block_requests() with > reference counter mechanism to avoid possible racing of blocking and > unblocking requests flow. Unify all users in UFS driver to use the > same function. > > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> > --- > drivers/scsi/ufs/ufshcd.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index ed02a70..b6b9665 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -5177,7 +5177,7 @@ static void ufshcd_exception_event_handler(struct work_struct *work) > hba = container_of(work, struct ufs_hba, eeh_work); > > pm_runtime_get_sync(hba->dev); > - scsi_block_requests(hba->host); > + 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", > @@ -5191,7 +5191,7 @@ static void ufshcd_exception_event_handler(struct work_struct *work) > ufshcd_bkops_exception_event_handler(hba); > > out: > - scsi_unblock_requests(hba->host); > + ufshcd_scsi_unblock_requests(hba); > pm_runtime_put_sync(hba->dev); > return; > } Hi Stanley, From the SCSI core: void scsi_block_requests(struct Scsi_Host *shost) { shost->host_self_blocked = 1; } In other words, neither scsi_block_requests() nor ufshcd_scsi_block_requests() wait for ongoing ufshcd_queuecommand() calls to finish. Is it required to wait for these calls to finish before exceptions are handled? If not, can the scsi_block_requests() and scsi_unblock_requests() calls be left out? If it is required to wait for ongoing ufshcd_queuecommand() calls to finish then I think the scsi_block_requests() and scsi_unblock_requests() will have to be changed into something else. Thanks, Bart.
Hi Bart, > Hi Stanley, > > From the SCSI core: > > void scsi_block_requests(struct Scsi_Host *shost) > { > shost->host_self_blocked = 1; > } > > In other words, neither scsi_block_requests() nor > ufshcd_scsi_block_requests() wait for ongoing ufshcd_queuecommand() > calls to finish. Is it required to wait for these calls to finish before > exceptions are handled? If not, can the scsi_block_requests() and > scsi_unblock_requests() calls be left out? If it is required to wait for > ongoing ufshcd_queuecommand() calls to finish then I think the > scsi_block_requests() and scsi_unblock_requests() will have to be > changed into something else. ASFAIK, ufshcd_exception_event_handler() is not required to wait for ongoing ufshcd_queuecommand() calls to finish. The scsi_block_requests() call here is trying to increase successful rate of requests sent by ufshcd_exception_event_handler() because timeout may happen if device is too busy to handle those requests. Blocking any future incoming requests can help. As time goes by, actually current UFS driver allows more waiting time by below changes for ufshcd_exception_event_handler(), and thus the successful rate shall be raised much nowadays. - Enlarge QUERY_REQ_TIMEOUT time from 100 ms to 1.5 seconds - Allow retry if query requests are timed out Therefore, the scsi_block_requests() call is actually a "helper" to help ufshcd_exception_event_handler() successful. I think it could be better kept to make UFS device recover its performance as soon as possible. > > Thanks, > > Bart. Thanks, Stanley
On 2019-12-24 21:01, Stanley Chu wrote: > Currently UFS driver has ufshcd_scsi_block_requests() with > reference counter mechanism to avoid possible racing of blocking and > unblocking requests flow. Unify all users in UFS driver to use the > same function. > > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> > --- > drivers/scsi/ufs/ufshcd.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index ed02a70..b6b9665 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -5177,7 +5177,7 @@ static void > ufshcd_exception_event_handler(struct work_struct *work) > hba = container_of(work, struct ufs_hba, eeh_work); > > pm_runtime_get_sync(hba->dev); > - scsi_block_requests(hba->host); > + 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", > @@ -5191,7 +5191,7 @@ static void > ufshcd_exception_event_handler(struct work_struct *work) > ufshcd_bkops_exception_event_handler(hba); > > out: > - scsi_unblock_requests(hba->host); > + ufshcd_scsi_unblock_requests(hba); > pm_runtime_put_sync(hba->dev); > return; > } Reviewed-by: Can Guo <cang@codeaurora.org>
On 12/24/19 5:01 AM, Stanley Chu wrote: > Currently UFS driver has ufshcd_scsi_block_requests() with > reference counter mechanism to avoid possible racing of blocking and > unblocking requests flow. Unify all users in UFS driver to use the > same function. Reviewed-by: Bart Van Assche <bvanassche@acm.org>
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index ed02a70..b6b9665 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -5177,7 +5177,7 @@ static void ufshcd_exception_event_handler(struct work_struct *work) hba = container_of(work, struct ufs_hba, eeh_work); pm_runtime_get_sync(hba->dev); - scsi_block_requests(hba->host); + 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", @@ -5191,7 +5191,7 @@ static void ufshcd_exception_event_handler(struct work_struct *work) ufshcd_bkops_exception_event_handler(hba); out: - scsi_unblock_requests(hba->host); + ufshcd_scsi_unblock_requests(hba); pm_runtime_put_sync(hba->dev); return; }
Currently UFS driver has ufshcd_scsi_block_requests() with reference counter mechanism to avoid possible racing of blocking and unblocking requests flow. Unify all users in UFS driver to use the same function. Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> --- drivers/scsi/ufs/ufshcd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)