[PATCH v1 0/2] scsi: ufs: use existed well-defined functions

[PATCH v1 0/2] scsi: ufs: use existed well-defined functions

[Stanley Chu]

Hi,

This patchset fixes two small place to use existed well-defined functions to replace legacy statements.

Stanley Chu (2):
  scsi: ufs: unify scsi_block_requests usage
  scsi: ufs: use ufshcd_vops_dbg_register_dump for vendor specific
    dumps

 drivers/scsi/ufs/ufshcd.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

-- 
1.7.9.5

[PATCH v1 1/2] scsi: ufs: unify scsi_block_requests usage

[Stanley Chu]

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;
 }
-- 
1.7.9.5

Re: [PATCH v1 1/2] scsi: ufs: unify scsi_block_requests usage

[Bart Van Assche]

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.

Re: [PATCH v1 1/2] scsi: ufs: unify scsi_block_requests usage

[Stanley Chu]

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

Re: [PATCH v1 1/2] scsi: ufs: unify scsi_block_requests usage

[Can Guo]

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>

Re: [PATCH v1 1/2] scsi: ufs: unify scsi_block_requests usage

[Bart Van Assche]

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>

[PATCH v1 2/2] scsi: ufs: use ufshcd_vops_dbg_register_dump for vendor specific dumps

[Stanley Chu]

We already have ufshcd_vops_dbg_register_dump() thus all
"hba->vops->dbg_register_dump" references can be replaced by it.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index b6b9665..1ac9272 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -428,8 +428,7 @@ static void ufshcd_print_host_regs(struct ufs_hba *hba)
 
 	ufshcd_print_clk_freqs(hba);
 
-	if (hba->vops && hba->vops->dbg_register_dump)
-		hba->vops->dbg_register_dump(hba);
+	ufshcd_vops_dbg_register_dump(hba);
 }
 
 static
-- 
1.7.9.5

Re: [PATCH v1 2/2] scsi: ufs: use ufshcd_vops_dbg_register_dump for vendor specific dumps

[Bart Van Assche]

On 2019-12-24 05:01, Stanley Chu wrote:
> We already have ufshcd_vops_dbg_register_dump() thus all
> "hba->vops->dbg_register_dump" references can be replaced by it.
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> ---
>  drivers/scsi/ufs/ufshcd.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index b6b9665..1ac9272 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -428,8 +428,7 @@ static void ufshcd_print_host_regs(struct ufs_hba *hba)
>  
>  	ufshcd_print_clk_freqs(hba);
>  
> -	if (hba->vops && hba->vops->dbg_register_dump)
> -		hba->vops->dbg_register_dump(hba);
> +	ufshcd_vops_dbg_register_dump(hba);
>  }

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v1 2/2] scsi: ufs: use ufshcd_vops_dbg_register_dump for vendor specific dumps

[Can Guo]

On 2019-12-24 21:01, Stanley Chu wrote:
> We already have ufshcd_vops_dbg_register_dump() thus all
> "hba->vops->dbg_register_dump" references can be replaced by it.
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> ---
>  drivers/scsi/ufs/ufshcd.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index b6b9665..1ac9272 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -428,8 +428,7 @@ static void ufshcd_print_host_regs(struct ufs_hba 
> *hba)
> 
>  	ufshcd_print_clk_freqs(hba);
> 
> -	if (hba->vops && hba->vops->dbg_register_dump)
> -		hba->vops->dbg_register_dump(hba);
> +	ufshcd_vops_dbg_register_dump(hba);
>  }
> 
>  static

Reviewed-by: Can Guo <cang@codeaurora.org>

Re: [PATCH v1 0/2] scsi: ufs: use existed well-defined functions

[Alim Akhtar]

Hi Stanley

On Tue, Dec 24, 2019 at 6:31 PM Stanley Chu <stanley.chu@mediatek.com> wrote:
>
> Hi,
>
> This patchset fixes two small place to use existed well-defined functions to replace legacy statements.
>
> Stanley Chu (2):
>   scsi: ufs: unify scsi_block_requests usage
>   scsi: ufs: use ufshcd_vops_dbg_register_dump for vendor specific
>     dumps
>
>  drivers/scsi/ufs/ufshcd.c |    7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> --
For this series,
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>

> 1.7.9.5



-- 
Regards,
Alim

Re: [PATCH v1 0/2] scsi: ufs: use existed well-defined functions

[Martin K. Petersen]

Stanley,

> This patchset fixes two small place to use existed well-defined
> functions to replace legacy statements.

Applied to 5.6/scsi-queue, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering