| Message ID | 20200522045932.31795-1-dinghao.liu@zju.edu.cn (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | a1e17eb03e69bb61bd1b1a14610436b7b9be12d9 |
| Headers | show |
| Series | scsi: ufs-bsg: Fix runtime PM imbalance on error | expand |
> 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 <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:
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
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
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:
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(-)