[PATCH 1/2] scsi: ufs: Unlock on a couple error paths

[PATCH 1/2] scsi: ufs: Unlock on a couple error paths

[Dan Carpenter]

We introduced a few new error paths, but we can't return directly, we
first have to unlock "hba->clk_scaling_lock" first.

Fixes: a276c19e3e98 ("scsi: ufs: Avoid busy-waiting by eliminating tag conflicts")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/scsi/ufs/ufshcd.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index e5cd57e68c46..bf981f0ea74c 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2617,8 +2617,10 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 	 * the maximum wait time is bounded by SCSI request timeout.
 	 */
 	req = blk_get_request(q, REQ_OP_DRV_OUT, 0);
-	if (IS_ERR(req))
-		return PTR_ERR(req);
+	if (IS_ERR(req)) {
+		err = PTR_ERR(req);
+		goto out_unlock;
+	}
 	tag = req->tag;
 	WARN_ON_ONCE(!ufshcd_valid_tag(hba, tag));
 
@@ -2646,6 +2648,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 
 out_put_tag:
 	blk_put_request(req);
+out_unlock:
 	up_read(&hba->clk_scaling_lock);
 	return err;
 }
@@ -5854,8 +5857,10 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 	down_read(&hba->clk_scaling_lock);
 
 	req = blk_get_request(q, REQ_OP_DRV_OUT, 0);
-	if (IS_ERR(req))
-		return PTR_ERR(req);
+	if (IS_ERR(req)) {
+		err = PTR_ERR(req);
+		goto out_unlock;
+	}
 	tag = req->tag;
 	WARN_ON_ONCE(!ufshcd_valid_tag(hba, tag));
 
@@ -5934,6 +5939,7 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 	}
 
 	blk_put_request(req);
+out_unlock:
 	up_read(&hba->clk_scaling_lock);
 	return err;
 }
-- 
2.11.0

[PATCH 2/2] scsi: ufs: Simplify a condition

[Dan Carpenter]

We know that "check_for_bkops" is non-zero on this side of the ||
because it was checked on the other side.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.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 bf981f0ea74c..c299c5feaf1a 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7684,8 +7684,7 @@ static int ufshcd_link_state_transition(struct ufs_hba *hba,
 	 * turning off the link would also turn off the device.
 	 */
 	else if ((req_link_state == UIC_LINK_OFF_STATE) &&
-		   (!check_for_bkops || (check_for_bkops &&
-		    !hba->auto_bkops_enabled))) {
+		 (!check_for_bkops || !hba->auto_bkops_enabled)) {
 		/*
 		 * Let's make sure that link is in low power mode, we are doing
 		 * this currently by putting the link in Hibern8. Otherway to
-- 
2.11.0

Re: [PATCH 2/2] scsi: ufs: Simplify a condition

[Bart Van Assche]

On 12/13/19 5:49 AM, Dan Carpenter wrote:
> We know that "check_for_bkops" is non-zero on this side of the ||
> because it was checked on the other side.

How about also removing the superfluous parentheses? Anyway:

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

Re: [PATCH 2/2] scsi: ufs: Simplify a condition

[Dan Carpenter]

On Fri, Dec 13, 2019 at 01:05:55PM -0700, Bart Van Assche wrote:
> On 12/13/19 5:49 AM, Dan Carpenter wrote:
> > We know that "check_for_bkops" is non-zero on this side of the ||
> > because it was checked on the other side.
> 
> How about also removing the superfluous parentheses? Anyway:
> 

Around "(req_link_state == UIC_LINK_OFF_STATE)"?  I considered it but
some people like them...

regards,
dan carpenter

RE: [PATCH 2/2] scsi: ufs: Simplify a condition

[Avri Altman]

 
> 
> We know that "check_for_bkops" is non-zero on this side of the || because it
> was checked on the other side.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Avri Altman <avri.altman@wdc.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
> bf981f0ea74c..c299c5feaf1a 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -7684,8 +7684,7 @@ static int ufshcd_link_state_transition(struct ufs_hba
> *hba,
>          * turning off the link would also turn off the device.
>          */
>         else if ((req_link_state == UIC_LINK_OFF_STATE) &&
> -                  (!check_for_bkops || (check_for_bkops &&
> -                   !hba->auto_bkops_enabled))) {
> +                (!check_for_bkops || !hba->auto_bkops_enabled)) {
>                 /*
>                  * Let's make sure that link is in low power mode, we are doing
>                  * this currently by putting the link in Hibern8. Otherway to
> --
> 2.11.0

Re: [PATCH 1/2] scsi: ufs: Unlock on a couple error paths

[Bart Van Assche]

On 12/13/19 5:48 AM, Dan Carpenter wrote:
> We introduced a few new error paths, but we can't return directly, we
> first have to unlock "hba->clk_scaling_lock" first.

This may have escaped from my attention due to having swapped the order 
of the patch that removed that locking and the patch this patch is a fix 
for. Anyway, thanks for this patch.

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

Re: [PATCH 1/2] scsi: ufs: Unlock on a couple error paths

[Martin K. Petersen]

Dan,

> We introduced a few new error paths, but we can't return directly, we
> first have to unlock "hba->clk_scaling_lock" first.

Applied #1 + #2 to 5.6/scsi-queue. Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering