[PATCH] scsi: ufs-bsg: Fix runtime PM imbalance on error

[PATCH] scsi: ufs-bsg: Fix runtime PM imbalance on error

[Dinghao Liu]

When ufs_bsg_alloc_desc_buffer() returns an error code,
a pairing runtime PM usage counter decrement is needed
to keep the counter balanced.

Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
---
 drivers/scsi/ufs/ufs_bsg.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
index 53dd87628cbe..516a7f573942 100644
--- a/drivers/scsi/ufs/ufs_bsg.c
+++ b/drivers/scsi/ufs/ufs_bsg.c
@@ -106,8 +106,10 @@ static int ufs_bsg_request(struct bsg_job *job)
 		desc_op = bsg_request->upiu_req.qr.opcode;
 		ret = ufs_bsg_alloc_desc_buffer(hba, job, &desc_buff,
 						&desc_len, desc_op);
-		if (ret)
+		if (ret) {
+			pm_runtime_put_sync(hba->dev);
 			goto out;
+		}
 
 		/* fall through */
 	case UPIU_TRANSACTION_NOP_OUT:
-- 
2.17.1

RE: [EXT] [PATCH] scsi: ufs-bsg: Fix runtime PM imbalance on error

[Bean Huo (beanhuo)]

>  1 file changed, 3 insertions(+), 1 deletion(-)
Hi, Dinghao
> 
> diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c index
> 53dd87628cbe..516a7f573942 100644
> --- a/drivers/scsi/ufs/ufs_bsg.c
> +++ b/drivers/scsi/ufs/ufs_bsg.c
> @@ -106,8 +106,10 @@ static int ufs_bsg_request(struct bsg_job *job)
>  		desc_op = bsg_request->upiu_req.qr.opcode;
>  		ret = ufs_bsg_alloc_desc_buffer(hba, job, &desc_buff,
>  						&desc_len, desc_op);
> -		if (ret)
> +		if (ret) {
> +			pm_runtime_put_sync(hba->dev);

No  need to add pm_runtime_put_sync() here, you can change "goto out" to "break",
Or move original pm_runtime_put_sync() to after goto label.

>  			goto out;
> +		}
> 
>  		/* fall through */
>  	case UPIU_TRANSACTION_NOP_OUT:
> --
> 2.17.1

Re: RE: [EXT] [PATCH] scsi: ufs-bsg: Fix runtime PM imbalance on error

[dinghao.liu]

Hi, Bean

Thank you for your advice! Moving original pm_runtime_put_sync() 
to after "out" label will influence an error path branched from 
ups_bsg_verify_query_size(). So I think changing "goto out" to
"break" is a good idea. But in this case we may execute an extra
sg_copy_from_buffer() and an extra kfree() compared with unpatched
version. Does this matter?

Regards,
Dinghao

> >  1 file changed, 3 insertions(+), 1 deletion(-)
> Hi, Dinghao
> > 
> > diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c index
> > 53dd87628cbe..516a7f573942 100644
> > --- a/drivers/scsi/ufs/ufs_bsg.c
> > +++ b/drivers/scsi/ufs/ufs_bsg.c
> > @@ -106,8 +106,10 @@ static int ufs_bsg_request(struct bsg_job *job)
> >  		desc_op = bsg_request->upiu_req.qr.opcode;
> >  		ret = ufs_bsg_alloc_desc_buffer(hba, job, &desc_buff,
> >  						&desc_len, desc_op);
> > -		if (ret)
> > +		if (ret) {
> > +			pm_runtime_put_sync(hba->dev);
> 
> No  need to add pm_runtime_put_sync() here, you can change "goto out" to "break",
> Or move original pm_runtime_put_sync() to after goto label.
> 
> >  			goto out;
> > +		}
> > 
> >  		/* fall through */
> >  	case UPIU_TRANSACTION_NOP_OUT:
> > --
> > 2.17.1

RE: RE: [EXT] [PATCH] scsi: ufs-bsg: Fix runtime PM imbalance on error

[Bean Huo (beanhuo)]

Hi, Dinghao

> Thank you for your advice! Moving original pm_runtime_put_sync() to after
> "out" label will influence an error path branched from
> ups_bsg_verify_query_size(). So I think changing "goto out" to "break" is a good
> idea. But in this case we may execute an extra
> sg_copy_from_buffer() and an extra kfree() compared with unpatched version.
> Does this matter?
> 
What do you mean " unpatched version "? 

I see, below goto will bypass sg_copy_from_buffer() and an extra kfree()
In case ufs_bsg_alloc_desc_buffer() fails. 

Bean

Re: RE: RE: [EXT] [PATCH] scsi: ufs-bsg: Fix runtime PM imbalance on error

[dinghao.liu]

> Hi, Dinghao
> 
> > Thank you for your advice! Moving original pm_runtime_put_sync() to after
> > "out" label will influence an error path branched from
> > ups_bsg_verify_query_size(). So I think changing "goto out" to "break" is a good
> > idea. But in this case we may execute an extra
> > sg_copy_from_buffer() and an extra kfree() compared with unpatched version.
> > Does this matter?
> > 
> What do you mean " unpatched version "? 
>
> I see, below goto will bypass sg_copy_from_buffer() and an extra kfree()
> In case ufs_bsg_alloc_desc_buffer() fails. 
> 

That's exactly what I want to express. If using "break" is OK I will send
a new patch to fix this problem.

> Bean
> 

Regaeds,
Dinghao

Re: [PATCH] scsi: ufs-bsg: Fix runtime PM imbalance on error

[Martin K. Petersen]

Avri: Please review!

> When ufs_bsg_alloc_desc_buffer() returns an error code,
> a pairing runtime PM usage counter decrement is needed
> to keep the counter balanced.
>
> Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
> ---
>  drivers/scsi/ufs/ufs_bsg.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
> index 53dd87628cbe..516a7f573942 100644
> --- a/drivers/scsi/ufs/ufs_bsg.c
> +++ b/drivers/scsi/ufs/ufs_bsg.c
> @@ -106,8 +106,10 @@ static int ufs_bsg_request(struct bsg_job *job)
>  		desc_op = bsg_request->upiu_req.qr.opcode;
>  		ret = ufs_bsg_alloc_desc_buffer(hba, job, &desc_buff,
>  						&desc_len, desc_op);
> -		if (ret)
> +		if (ret) {
> +			pm_runtime_put_sync(hba->dev);
>  			goto out;
> +		}
>  
>  		/* fall through */
>  	case UPIU_TRANSACTION_NOP_OUT:

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH] scsi: ufs-bsg: Fix runtime PM imbalance on error

[Avri Altman]

Hi,
 
> Avri: Please review!
> 
> > When ufs_bsg_alloc_desc_buffer() returns an error code,
> > a pairing runtime PM usage counter decrement is needed
> > to keep the counter balanced.
> >
> > Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
Please add:
Fixes: 74e5e468b664 (scsi: ufs-bsg: Wake the device before sending raw upiu commands)

Reviewed-by: Avri Altman <avri.altman@wdc.com>

Thanks,
Avri

> > ---
> >  drivers/scsi/ufs/ufs_bsg.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
> > index 53dd87628cbe..516a7f573942 100644
> > --- a/drivers/scsi/ufs/ufs_bsg.c
> > +++ b/drivers/scsi/ufs/ufs_bsg.c
> > @@ -106,8 +106,10 @@ static int ufs_bsg_request(struct bsg_job *job)
> >               desc_op = bsg_request->upiu_req.qr.opcode;
> >               ret = ufs_bsg_alloc_desc_buffer(hba, job, &desc_buff,
> >                                               &desc_len, desc_op);
> > -             if (ret)
> > +             if (ret) {
> > +                     pm_runtime_put_sync(hba->dev);
> >                       goto out;
> > +             }
> >
> >               /* fall through */
> >       case UPIU_TRANSACTION_NOP_OUT:
> 
> --
> Martin K. Petersen      Oracle Linux Engineering

Re: [PATCH] scsi: ufs-bsg: Fix runtime PM imbalance on error

[Martin K. Petersen]

On Fri, 22 May 2020 12:59:29 +0800, Dinghao Liu wrote:

> When ufs_bsg_alloc_desc_buffer() returns an error code,
> a pairing runtime PM usage counter decrement is needed
> to keep the counter balanced.

Applied to 5.8/scsi-fixes, thanks!

[1/1] scsi: ufs-bsg: Fix runtime PM imbalance on error
      https://git.kernel.org/mkp/scsi/c/a1e17eb03e69

-- 
Martin K. Petersen	Oracle Linux Engineering