linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] PCI: dwc: fix reference leak in pex_ep_event_pex_rst_deassert
@ 2020-11-02 14:30 Zhang Qilong
  2020-11-03  2:52 ` Vidya Sagar
  0 siblings, 1 reply; 7+ messages in thread
From: Zhang Qilong @ 2020-11-02 14:30 UTC (permalink / raw)
  To: lorenzo.pieralisi, robh, bhelgaas, thierry.reding, jonathanh
  Cc: linux-pci, linux-tegra

pm_runtime_get_sync will increment pm usage counter even it
failed. Forgetting to pm_runtime_put_noidle will result in
reference leak in pex_ep_event_pex_rst_deassert, so we should
fix it.

Fixes: c57247f940e8e ("PCI: tegra: Add support for PCIe endpoint mode in Tegra194")
Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
---
 drivers/pci/controller/dwc/pcie-tegra194.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
index f920e7efe118..936510b5c649 100644
--- a/drivers/pci/controller/dwc/pcie-tegra194.c
+++ b/drivers/pci/controller/dwc/pcie-tegra194.c
@@ -1662,6 +1662,7 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
 
 	ret = pm_runtime_get_sync(dev);
 	if (ret < 0) {
+		pm_runtime_put_noidle(dev);
 		dev_err(dev, "Failed to get runtime sync for PCIe dev: %d\n",
 			ret);
 		return;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] PCI: dwc: fix reference leak in pex_ep_event_pex_rst_deassert
  2020-11-02 14:30 [PATCH] PCI: dwc: fix reference leak in pex_ep_event_pex_rst_deassert Zhang Qilong
@ 2020-11-03  2:52 ` Vidya Sagar
  2020-11-03  3:14   ` 答复: " zhangqilong
  2021-01-25 19:30   ` Jon Hunter
  0 siblings, 2 replies; 7+ messages in thread
From: Vidya Sagar @ 2020-11-03  2:52 UTC (permalink / raw)
  To: Zhang Qilong, lorenzo.pieralisi, robh, bhelgaas, thierry.reding,
	jonathanh
  Cc: linux-pci, linux-tegra



On 11/2/2020 8:00 PM, Zhang Qilong wrote:
> External email: Use caution opening links or attachments
> 
> 
> pm_runtime_get_sync will increment pm usage counter even it
> failed. Forgetting to pm_runtime_put_noidle will result in
> reference leak in pex_ep_event_pex_rst_deassert, so we should
> fix it.
> 
> Fixes: c57247f940e8e ("PCI: tegra: Add support for PCIe endpoint mode in Tegra194")
> Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
> ---
>   drivers/pci/controller/dwc/pcie-tegra194.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index f920e7efe118..936510b5c649 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -1662,6 +1662,7 @@ static void pex_ep_event_pex_rst_deassert(struct tegra_pcie_dw *pcie)
> 
>          ret = pm_runtime_get_sync(dev);
>          if (ret < 0) {
> +               pm_runtime_put_noidle(dev);
Why can't we call pm_runtime_put_sync(dev) as that is what is being 
called in failure cases anyway further down in this API?

>                  dev_err(dev, "Failed to get runtime sync for PCIe dev: %d\n",
>                          ret);
>                  return;
> --
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* 答复: [PATCH] PCI: dwc: fix reference leak in pex_ep_event_pex_rst_deassert
  2020-11-03  2:52 ` Vidya Sagar
@ 2020-11-03  3:14   ` zhangqilong
  2021-01-25 16:34     ` Lorenzo Pieralisi
  2021-01-25 19:30   ` Jon Hunter
  1 sibling, 1 reply; 7+ messages in thread
From: zhangqilong @ 2020-11-03  3:14 UTC (permalink / raw)
  To: Vidya Sagar, lorenzo.pieralisi, robh, bhelgaas, thierry.reding,
	jonathanh
  Cc: linux-pci, linux-tegra

Hi

> 
> 
> On 11/2/2020 8:00 PM, Zhang Qilong wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > pm_runtime_get_sync will increment pm usage counter even it failed.
> > Forgetting to pm_runtime_put_noidle will result in reference leak in
> > pex_ep_event_pex_rst_deassert, so we should fix it.
> >
> > Fixes: c57247f940e8e ("PCI: tegra: Add support for PCIe endpoint mode
> > in Tegra194")
> > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
> > ---
> >   drivers/pci/controller/dwc/pcie-tegra194.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
> > b/drivers/pci/controller/dwc/pcie-tegra194.c
> > index f920e7efe118..936510b5c649 100644
> > --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> > @@ -1662,6 +1662,7 @@ static void pex_ep_event_pex_rst_deassert(struct
> > tegra_pcie_dw *pcie)
> >
> >          ret = pm_runtime_get_sync(dev);
> >          if (ret < 0) {
> > +               pm_runtime_put_noidle(dev);
> Why can't we call pm_runtime_put_sync(dev) as that is what is being called in
> failure cases anyway further down in this API?
> 
Both of the two functions are OK, the difference is that, when pm_runtime_put_sync failed(runtime of the device has error, the device is in Inaccessible state or other error state...), it only increase the usage count of the power, and do nothing else. We merely need call pm_runtime_put_noidle to decrease the usage count. If we call pm_runtime_put_sync to reset it, it will notify device bus type if the device can be suspended, and that is meanless when pm_runtime_put_sync failed.

Thanks, best wish!

Zhang Qilong
> >                  dev_err(dev, "Failed to get runtime sync for PCIe
> dev: %d\n",
> >                          ret);
> >                  return;
> > --
> > 2.17.1
> >

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: 答复: [PATCH] PCI: dwc: fix reference leak in pex_ep_event_pex_rst_deassert
  2020-11-03  3:14   ` 答复: " zhangqilong
@ 2021-01-25 16:34     ` Lorenzo Pieralisi
  2021-01-27  6:19       ` Vidya Sagar
  0 siblings, 1 reply; 7+ messages in thread
From: Lorenzo Pieralisi @ 2021-01-25 16:34 UTC (permalink / raw)
  To: zhangqilong, vidyas
  Cc: robh, bhelgaas, thierry.reding, jonathanh, linux-pci, linux-tegra

On Tue, Nov 03, 2020 at 03:14:21AM +0000, zhangqilong wrote:
> Hi
> 
> > 
> > 
> > On 11/2/2020 8:00 PM, Zhang Qilong wrote:
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > pm_runtime_get_sync will increment pm usage counter even it failed.
> > > Forgetting to pm_runtime_put_noidle will result in reference leak in
> > > pex_ep_event_pex_rst_deassert, so we should fix it.
> > >
> > > Fixes: c57247f940e8e ("PCI: tegra: Add support for PCIe endpoint mode
> > > in Tegra194")
> > > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
> > > ---
> > >   drivers/pci/controller/dwc/pcie-tegra194.c | 1 +
> > >   1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
> > > b/drivers/pci/controller/dwc/pcie-tegra194.c
> > > index f920e7efe118..936510b5c649 100644
> > > --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> > > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> > > @@ -1662,6 +1662,7 @@ static void pex_ep_event_pex_rst_deassert(struct
> > > tegra_pcie_dw *pcie)
> > >
> > >          ret = pm_runtime_get_sync(dev);
> > >          if (ret < 0) {
> > > +               pm_runtime_put_noidle(dev);
> > Why can't we call pm_runtime_put_sync(dev) as that is what is being called in
> > failure cases anyway further down in this API?
> > 
> Both of the two functions are OK, the difference is that, when pm_runtime_put_sync failed(runtime of the device has error, the device is in Inaccessible state or other error state...), it only increase the usage count of the power, and do nothing else. We merely need call pm_runtime_put_noidle to decrease the usage count. If we call pm_runtime_put_sync to reset it, it will notify device bus type if the device can be suspended, and that is meanless when pm_runtime_put_sync failed.

I don't understand. Vidya any feedback ?

Lorenzo

> Thanks, best wish!
> 
> Zhang Qilong
> > >                  dev_err(dev, "Failed to get runtime sync for PCIe
> > dev: %d\n",
> > >                          ret);
> > >                  return;
> > > --
> > > 2.17.1
> > >

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] PCI: dwc: fix reference leak in pex_ep_event_pex_rst_deassert
  2020-11-03  2:52 ` Vidya Sagar
  2020-11-03  3:14   ` 答复: " zhangqilong
@ 2021-01-25 19:30   ` Jon Hunter
  2021-03-23 10:24     ` Lorenzo Pieralisi
  1 sibling, 1 reply; 7+ messages in thread
From: Jon Hunter @ 2021-01-25 19:30 UTC (permalink / raw)
  To: Vidya Sagar, Zhang Qilong, lorenzo.pieralisi, robh, bhelgaas,
	thierry.reding
  Cc: linux-pci, linux-tegra


On 03/11/2020 02:52, Vidya Sagar wrote:
> 
> 
> On 11/2/2020 8:00 PM, Zhang Qilong wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> pm_runtime_get_sync will increment pm usage counter even it
>> failed. Forgetting to pm_runtime_put_noidle will result in
>> reference leak in pex_ep_event_pex_rst_deassert, so we should
>> fix it.
>>
>> Fixes: c57247f940e8e ("PCI: tegra: Add support for PCIe endpoint mode
>> in Tegra194")
>> Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-tegra194.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
>> b/drivers/pci/controller/dwc/pcie-tegra194.c
>> index f920e7efe118..936510b5c649 100644
>> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
>> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
>> @@ -1662,6 +1662,7 @@ static void pex_ep_event_pex_rst_deassert(struct
>> tegra_pcie_dw *pcie)
>>
>>          ret = pm_runtime_get_sync(dev);
>>          if (ret < 0) {
>> +               pm_runtime_put_noidle(dev);
> Why can't we call pm_runtime_put_sync(dev) as that is what is being
> called in failure cases anyway further down in this API?


Simply because this is a failure case where the get_sync did not
complete. So this change is correct, however, now we have
pm_runtime_resume_and_get(), it is better/simpler just to replace the
pm_runtime_get_sync with pm_runtime_resume_and_get.

Cheers
Jon

-- 
nvpublic

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: 答复: [PATCH] PCI: dwc: fix reference leak in pex_ep_event_pex_rst_deassert
  2021-01-25 16:34     ` Lorenzo Pieralisi
@ 2021-01-27  6:19       ` Vidya Sagar
  0 siblings, 0 replies; 7+ messages in thread
From: Vidya Sagar @ 2021-01-27  6:19 UTC (permalink / raw)
  To: Lorenzo Pieralisi, zhangqilong
  Cc: robh, bhelgaas, thierry.reding, jonathanh, linux-pci, linux-tegra



On 1/25/2021 10:04 PM, Lorenzo Pieralisi wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Tue, Nov 03, 2020 at 03:14:21AM +0000, zhangqilong wrote:
>> Hi
>>
>>>
>>>
>>> On 11/2/2020 8:00 PM, Zhang Qilong wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> pm_runtime_get_sync will increment pm usage counter even it failed.
>>>> Forgetting to pm_runtime_put_noidle will result in reference leak in
>>>> pex_ep_event_pex_rst_deassert, so we should fix it.
>>>>
>>>> Fixes: c57247f940e8e ("PCI: tegra: Add support for PCIe endpoint mode
>>>> in Tegra194")
>>>> Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
>>>> ---
>>>>    drivers/pci/controller/dwc/pcie-tegra194.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
>>>> b/drivers/pci/controller/dwc/pcie-tegra194.c
>>>> index f920e7efe118..936510b5c649 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
>>>> @@ -1662,6 +1662,7 @@ static void pex_ep_event_pex_rst_deassert(struct
>>>> tegra_pcie_dw *pcie)
>>>>
>>>>           ret = pm_runtime_get_sync(dev);
>>>>           if (ret < 0) {
>>>> +               pm_runtime_put_noidle(dev);
>>> Why can't we call pm_runtime_put_sync(dev) as that is what is being called in
>>> failure cases anyway further down in this API?
>>>
>> Both of the two functions are OK, the difference is that, when pm_runtime_put_sync failed(runtime of the device has error, the device is in Inaccessible state or other error state...), it only increase the usage count of the power, and do nothing else. We merely need call pm_runtime_put_noidle to decrease the usage count. If we call pm_runtime_put_sync to reset it, it will notify device bus type if the device can be suspended, and that is meanless when pm_runtime_put_sync failed.
> 
> I don't understand. Vidya any feedback ?
If I understand this correctly, we are assuming that nothing would have 
happened apart from just counter increment and hence don't want to run 
the idle check by calling pm_runtime_put_noidle(). On a different note, 
what is it that we are going to lose if we let the idle check also to 
run i.e. by calling pm_runtime_put_sync()?

Thanks,
Vidya Sagar
> 
> Lorenzo
> 
>> Thanks, best wish!
>>
>> Zhang Qilong
>>>>                   dev_err(dev, "Failed to get runtime sync for PCIe
>>> dev: %d\n",
>>>>                           ret);
>>>>                   return;
>>>> --
>>>> 2.17.1
>>>>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] PCI: dwc: fix reference leak in pex_ep_event_pex_rst_deassert
  2021-01-25 19:30   ` Jon Hunter
@ 2021-03-23 10:24     ` Lorenzo Pieralisi
  0 siblings, 0 replies; 7+ messages in thread
From: Lorenzo Pieralisi @ 2021-03-23 10:24 UTC (permalink / raw)
  To: Jon Hunter
  Cc: Vidya Sagar, Zhang Qilong, robh, bhelgaas, thierry.reding,
	linux-pci, linux-tegra

On Mon, Jan 25, 2021 at 07:30:13PM +0000, Jon Hunter wrote:
> 
> On 03/11/2020 02:52, Vidya Sagar wrote:
> > 
> > 
> > On 11/2/2020 8:00 PM, Zhang Qilong wrote:
> >> External email: Use caution opening links or attachments
> >>
> >>
> >> pm_runtime_get_sync will increment pm usage counter even it
> >> failed. Forgetting to pm_runtime_put_noidle will result in
> >> reference leak in pex_ep_event_pex_rst_deassert, so we should
> >> fix it.
> >>
> >> Fixes: c57247f940e8e ("PCI: tegra: Add support for PCIe endpoint mode
> >> in Tegra194")
> >> Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
> >> ---
> >>   drivers/pci/controller/dwc/pcie-tegra194.c | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c
> >> b/drivers/pci/controller/dwc/pcie-tegra194.c
> >> index f920e7efe118..936510b5c649 100644
> >> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> >> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> >> @@ -1662,6 +1662,7 @@ static void pex_ep_event_pex_rst_deassert(struct
> >> tegra_pcie_dw *pcie)
> >>
> >>          ret = pm_runtime_get_sync(dev);
> >>          if (ret < 0) {
> >> +               pm_runtime_put_noidle(dev);
> > Why can't we call pm_runtime_put_sync(dev) as that is what is being
> > called in failure cases anyway further down in this API?
> 
> 
> Simply because this is a failure case where the get_sync did not
> complete. So this change is correct, however, now we have
> pm_runtime_resume_and_get(), it is better/simpler just to replace the
> pm_runtime_get_sync with pm_runtime_resume_and_get.

I think this patch slipped through the cracks, should I update the patch
myself with the suggestion above and merge it or you guys prefer sending
it ?

Thanks,
Lorenzo

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-03-23 10:25 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-02 14:30 [PATCH] PCI: dwc: fix reference leak in pex_ep_event_pex_rst_deassert Zhang Qilong
2020-11-03  2:52 ` Vidya Sagar
2020-11-03  3:14   ` 答复: " zhangqilong
2021-01-25 16:34     ` Lorenzo Pieralisi
2021-01-27  6:19       ` Vidya Sagar
2021-01-25 19:30   ` Jon Hunter
2021-03-23 10:24     ` Lorenzo Pieralisi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).