* Re: [PATCH -next] pwm: img: Fix PM reference leak in img_pwm_enable() [not found] <1620791837-16138-1-git-send-email-zou_wei@huawei.com> @ 2021-05-12 4:52 ` Uwe Kleine-König 2021-06-25 17:33 ` Uwe Kleine-König 2021-06-25 17:45 ` Rafael J. Wysocki 0 siblings, 2 replies; 6+ messages in thread From: Uwe Kleine-König @ 2021-05-12 4:52 UTC (permalink / raw) To: Zou Wei Cc: thierry.reding, lee.jones, linux-pwm, linux-kernel, Rafael J. Wysocki, Kevin Hilman, Ulf Hansson, linux-pm [-- Attachment #1: Type: text/plain, Size: 1659 bytes --] Hello, On Wed, May 12, 2021 at 11:57:17AM +0800, Zou Wei wrote: > pm_runtime_get_sync will increment pm usage counter even it failed. > Forgetting to putting operation will result in reference leak here. > Fix it by replacing it with pm_runtime_resume_and_get to keep usage > counter balanced. > > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: Zou Wei <zou_wei@huawei.com> > --- > drivers/pwm/pwm-img.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pwm/pwm-img.c b/drivers/pwm/pwm-img.c > index cc37054..11b16ec 100644 > --- a/drivers/pwm/pwm-img.c > +++ b/drivers/pwm/pwm-img.c > @@ -156,7 +156,7 @@ static int img_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > struct img_pwm_chip *pwm_chip = to_img_pwm_chip(chip); > int ret; > > - ret = pm_runtime_get_sync(chip->dev); > + ret = pm_runtime_resume_and_get(chip->dev); > if (ret < 0) > return ret; This patch looks right with my limited understanding of pm_runtime. A similar issue in this driver was fixed in commit ca162ce98110 ("pwm: img: Call pm_runtime_put() in pm_runtime_get_sync() failed case") where (even though the commit log talks about pm_runtime_put()) a call to pm_runtime_put_autosuspend() was added in the error path. I added the PM guys to Cc, maybe they can advise about the right thing to do here. Does it make sense to use the same idiom in both img_pwm_enable() and img_pwm_config()? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] pwm: img: Fix PM reference leak in img_pwm_enable() 2021-05-12 4:52 ` [PATCH -next] pwm: img: Fix PM reference leak in img_pwm_enable() Uwe Kleine-König @ 2021-06-25 17:33 ` Uwe Kleine-König 2021-06-25 17:45 ` Rafael J. Wysocki 1 sibling, 0 replies; 6+ messages in thread From: Uwe Kleine-König @ 2021-06-25 17:33 UTC (permalink / raw) To: Rafael J. Wysocki, Kevin Hilman, Ulf Hansson, linux-pm Cc: Zou Wei, thierry.reding, lee.jones, linux-pwm, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1870 bytes --] Hello Rafael, Kevin and Ulf, On Wed, May 12, 2021 at 06:52:22AM +0200, Uwe Kleine-König wrote: > Hello, > > On Wed, May 12, 2021 at 11:57:17AM +0800, Zou Wei wrote: > > pm_runtime_get_sync will increment pm usage counter even it failed. > > Forgetting to putting operation will result in reference leak here. > > Fix it by replacing it with pm_runtime_resume_and_get to keep usage > > counter balanced. > > > > Reported-by: Hulk Robot <hulkci@huawei.com> > > Signed-off-by: Zou Wei <zou_wei@huawei.com> > > --- > > drivers/pwm/pwm-img.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/pwm/pwm-img.c b/drivers/pwm/pwm-img.c > > index cc37054..11b16ec 100644 > > --- a/drivers/pwm/pwm-img.c > > +++ b/drivers/pwm/pwm-img.c > > @@ -156,7 +156,7 @@ static int img_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > > struct img_pwm_chip *pwm_chip = to_img_pwm_chip(chip); > > int ret; > > > > - ret = pm_runtime_get_sync(chip->dev); > > + ret = pm_runtime_resume_and_get(chip->dev); > > if (ret < 0) > > return ret; > > This patch looks right with my limited understanding of pm_runtime. A > similar issue in this driver was fixed in commit > > ca162ce98110 ("pwm: img: Call pm_runtime_put() in pm_runtime_get_sync() failed case") > > where (even though the commit log talks about pm_runtime_put()) a call > to pm_runtime_put_autosuspend() was added in the error path. > > I added the PM guys to Cc, maybe they can advise about the right thing > to do here. Does it make sense to use the same idiom in both > img_pwm_enable() and img_pwm_config()? Can you give some feedback here? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] pwm: img: Fix PM reference leak in img_pwm_enable() 2021-05-12 4:52 ` [PATCH -next] pwm: img: Fix PM reference leak in img_pwm_enable() Uwe Kleine-König 2021-06-25 17:33 ` Uwe Kleine-König @ 2021-06-25 17:45 ` Rafael J. Wysocki 2021-06-28 6:38 ` Uwe Kleine-König 1 sibling, 1 reply; 6+ messages in thread From: Rafael J. Wysocki @ 2021-06-25 17:45 UTC (permalink / raw) To: Uwe Kleine-König Cc: Zou Wei, Thierry Reding, Lee Jones, Linux PWM List, Linux Kernel Mailing List, Rafael J. Wysocki, Kevin Hilman, Ulf Hansson, Linux PM On Wed, May 12, 2021 at 6:52 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello, > > On Wed, May 12, 2021 at 11:57:17AM +0800, Zou Wei wrote: > > pm_runtime_get_sync will increment pm usage counter even it failed. > > Forgetting to putting operation will result in reference leak here. > > Fix it by replacing it with pm_runtime_resume_and_get to keep usage > > counter balanced. > > > > Reported-by: Hulk Robot <hulkci@huawei.com> > > Signed-off-by: Zou Wei <zou_wei@huawei.com> > > --- > > drivers/pwm/pwm-img.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/pwm/pwm-img.c b/drivers/pwm/pwm-img.c > > index cc37054..11b16ec 100644 > > --- a/drivers/pwm/pwm-img.c > > +++ b/drivers/pwm/pwm-img.c > > @@ -156,7 +156,7 @@ static int img_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > > struct img_pwm_chip *pwm_chip = to_img_pwm_chip(chip); > > int ret; > > > > - ret = pm_runtime_get_sync(chip->dev); > > + ret = pm_runtime_resume_and_get(chip->dev); > > if (ret < 0) > > return ret; > > This patch looks right with my limited understanding of pm_runtime. A > similar issue in this driver was fixed in commit > > ca162ce98110 ("pwm: img: Call pm_runtime_put() in pm_runtime_get_sync() failed case") > > where (even though the commit log talks about pm_runtime_put()) a call > to pm_runtime_put_autosuspend() was added in the error path. > > I added the PM guys to Cc, maybe they can advise about the right thing > to do here. Does it make sense to use the same idiom in both > img_pwm_enable() and img_pwm_config()? I think so. And calling pm_runtime_put_autosuspend() in the img_pwm_enable() error path would work too. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] pwm: img: Fix PM reference leak in img_pwm_enable() 2021-06-25 17:45 ` Rafael J. Wysocki @ 2021-06-28 6:38 ` Uwe Kleine-König 2021-06-28 17:01 ` Uwe Kleine-König 0 siblings, 1 reply; 6+ messages in thread From: Uwe Kleine-König @ 2021-06-28 6:38 UTC (permalink / raw) To: Zou Wei Cc: Thierry Reding, Lee Jones, Linux PWM List, Linux Kernel Mailing List, Rafael J. Wysocki, Kevin Hilman, Ulf Hansson, Linux PM, Rafael J. Wysocki [-- Attachment #1: Type: text/plain, Size: 2187 bytes --] Hello Zou, On Fri, Jun 25, 2021 at 07:45:14PM +0200, Rafael J. Wysocki wrote: > On Wed, May 12, 2021 at 6:52 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Wed, May 12, 2021 at 11:57:17AM +0800, Zou Wei wrote: > > > pm_runtime_get_sync will increment pm usage counter even it failed. > > > Forgetting to putting operation will result in reference leak here. > > > Fix it by replacing it with pm_runtime_resume_and_get to keep usage > > > counter balanced. > > > > > > Reported-by: Hulk Robot <hulkci@huawei.com> > > > Signed-off-by: Zou Wei <zou_wei@huawei.com> > > > --- > > > drivers/pwm/pwm-img.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/pwm/pwm-img.c b/drivers/pwm/pwm-img.c > > > index cc37054..11b16ec 100644 > > > --- a/drivers/pwm/pwm-img.c > > > +++ b/drivers/pwm/pwm-img.c > > > @@ -156,7 +156,7 @@ static int img_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > > > struct img_pwm_chip *pwm_chip = to_img_pwm_chip(chip); > > > int ret; > > > > > > - ret = pm_runtime_get_sync(chip->dev); > > > + ret = pm_runtime_resume_and_get(chip->dev); > > > if (ret < 0) > > > return ret; > > > > This patch looks right with my limited understanding of pm_runtime. A > > similar issue in this driver was fixed in commit > > > > ca162ce98110 ("pwm: img: Call pm_runtime_put() in pm_runtime_get_sync() failed case") > > > > where (even though the commit log talks about pm_runtime_put()) a call > > to pm_runtime_put_autosuspend() was added in the error path. > > > > I added the PM guys to Cc, maybe they can advise about the right thing > > to do here. Does it make sense to use the same idiom in both > > img_pwm_enable() and img_pwm_config()? > > I think so. > > And calling pm_runtime_put_autosuspend() in the img_pwm_enable() error > path would work too. Do you care to clean this up accordingly and send a new patch? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] pwm: img: Fix PM reference leak in img_pwm_enable() 2021-06-28 6:38 ` Uwe Kleine-König @ 2021-06-28 17:01 ` Uwe Kleine-König 2021-06-29 3:23 ` Samuel Zou 0 siblings, 1 reply; 6+ messages in thread From: Uwe Kleine-König @ 2021-06-28 17:01 UTC (permalink / raw) To: Zou Wei Cc: Thierry Reding, Lee Jones, Linux PWM List, Linux Kernel Mailing List, Rafael J. Wysocki, Kevin Hilman, Ulf Hansson, Linux PM, Rafael J. Wysocki [-- Attachment #1: Type: text/plain, Size: 2546 bytes --] Hello Zou, On Mon, Jun 28, 2021 at 08:38:39AM +0200, Uwe Kleine-König wrote: > On Fri, Jun 25, 2021 at 07:45:14PM +0200, Rafael J. Wysocki wrote: > > On Wed, May 12, 2021 at 6:52 AM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > On Wed, May 12, 2021 at 11:57:17AM +0800, Zou Wei wrote: > > > > pm_runtime_get_sync will increment pm usage counter even it failed. > > > > Forgetting to putting operation will result in reference leak here. > > > > Fix it by replacing it with pm_runtime_resume_and_get to keep usage > > > > counter balanced. > > > > > > > > Reported-by: Hulk Robot <hulkci@huawei.com> > > > > Signed-off-by: Zou Wei <zou_wei@huawei.com> > > > > --- > > > > drivers/pwm/pwm-img.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/pwm/pwm-img.c b/drivers/pwm/pwm-img.c > > > > index cc37054..11b16ec 100644 > > > > --- a/drivers/pwm/pwm-img.c > > > > +++ b/drivers/pwm/pwm-img.c > > > > @@ -156,7 +156,7 @@ static int img_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > > > > struct img_pwm_chip *pwm_chip = to_img_pwm_chip(chip); > > > > int ret; > > > > > > > > - ret = pm_runtime_get_sync(chip->dev); > > > > + ret = pm_runtime_resume_and_get(chip->dev); > > > > if (ret < 0) > > > > return ret; > > > > > > This patch looks right with my limited understanding of pm_runtime. A > > > similar issue in this driver was fixed in commit > > > > > > ca162ce98110 ("pwm: img: Call pm_runtime_put() in pm_runtime_get_sync() failed case") > > > > > > where (even though the commit log talks about pm_runtime_put()) a call > > > to pm_runtime_put_autosuspend() was added in the error path. > > > > > > I added the PM guys to Cc, maybe they can advise about the right thing > > > to do here. Does it make sense to use the same idiom in both > > > img_pwm_enable() and img_pwm_config()? > > > > I think so. > > > > And calling pm_runtime_put_autosuspend() in the img_pwm_enable() error > > path would work too. > > Do you care to clean this up accordingly and send a new patch? Note that Thierry applied your initial patch regardless of the inconsistency. Still I'd like to see this done in a consistent way. Do you care to follow up with a patch that unifies the behaviour? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] pwm: img: Fix PM reference leak in img_pwm_enable() 2021-06-28 17:01 ` Uwe Kleine-König @ 2021-06-29 3:23 ` Samuel Zou 0 siblings, 0 replies; 6+ messages in thread From: Samuel Zou @ 2021-06-29 3:23 UTC (permalink / raw) To: Uwe Kleine-König Cc: Thierry Reding, Lee Jones, Linux PWM List, Linux Kernel Mailing List, Rafael J. Wysocki, Kevin Hilman, Ulf Hansson, Linux PM, Rafael J. Wysocki Hi Uwe, Sorry for the delayed reply. Thanks for all the review,. To keep the consistency, it's better to clean this up accordingly, and I will send a new patch soon. On 2021/6/29 1:01, Uwe Kleine-König wrote: > Hello Zou, > On Mon, Jun 28, 2021 at 08:38:39AM +0200, Uwe Kleine-König wrote: >> On Fri, Jun 25, 2021 at 07:45:14PM +0200, Rafael J. Wysocki wrote: >>> On Wed, May 12, 2021 at 6:52 AM Uwe Kleine-König >>> <u.kleine-koenig@pengutronix.de> wrote: >>>> On Wed, May 12, 2021 at 11:57:17AM +0800, Zou Wei wrote: >>>>> pm_runtime_get_sync will increment pm usage counter even it failed. >>>>> Forgetting to putting operation will result in reference leak here. >>>>> Fix it by replacing it with pm_runtime_resume_and_get to keep usage >>>>> counter balanced. >>>>> >>>>> Reported-by: Hulk Robot <hulkci@huawei.com> >>>>> Signed-off-by: Zou Wei <zou_wei@huawei.com> >>>>> --- >>>>> drivers/pwm/pwm-img.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/pwm/pwm-img.c b/drivers/pwm/pwm-img.c >>>>> index cc37054..11b16ec 100644 >>>>> --- a/drivers/pwm/pwm-img.c >>>>> +++ b/drivers/pwm/pwm-img.c >>>>> @@ -156,7 +156,7 @@ static int img_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) >>>>> struct img_pwm_chip *pwm_chip = to_img_pwm_chip(chip); >>>>> int ret; >>>>> >>>>> - ret = pm_runtime_get_sync(chip->dev); >>>>> + ret = pm_runtime_resume_and_get(chip->dev); >>>>> if (ret < 0) >>>>> return ret; >>>> >>>> This patch looks right with my limited understanding of pm_runtime. A >>>> similar issue in this driver was fixed in commit >>>> >>>> ca162ce98110 ("pwm: img: Call pm_runtime_put() in pm_runtime_get_sync() failed case") >>>> >>>> where (even though the commit log talks about pm_runtime_put()) a call >>>> to pm_runtime_put_autosuspend() was added in the error path. >>>> >>>> I added the PM guys to Cc, maybe they can advise about the right thing >>>> to do here. Does it make sense to use the same idiom in both >>>> img_pwm_enable() and img_pwm_config()? >>> >>> I think so. >>> >>> And calling pm_runtime_put_autosuspend() in the img_pwm_enable() error >>> path would work too. >> >> Do you care to clean this up accordingly and send a new patch? > > Note that Thierry applied your initial patch regardless of the > inconsistency. Still I'd like to see this done in a consistent way. Do > you care to follow up with a patch that unifies the behaviour? > > Best regards > Uwe > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-06-29 3:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1620791837-16138-1-git-send-email-zou_wei@huawei.com>
2021-05-12 4:52 ` [PATCH -next] pwm: img: Fix PM reference leak in img_pwm_enable() Uwe Kleine-König
2021-06-25 17:33 ` Uwe Kleine-König
2021-06-25 17:45 ` Rafael J. Wysocki
2021-06-28 6:38 ` Uwe Kleine-König
2021-06-28 17:01 ` Uwe Kleine-König
2021-06-29 3:23 ` Samuel Zou
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).