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 <[email protected]>
---
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
> 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
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
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
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
Please add:
Fixes: 74e5e468b664 (scsi: ufs-bsg: Wake the device before sending raw upiu commands)
Reviewed-by: Avri Altman <[email protected]>
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
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