| Message ID | 1620791837-16138-1-git-send-email-zou_wei@huawei.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [-next] pwm: img: Fix PM reference leak in img_pwm_enable() | expand |
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
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
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.
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
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
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 >
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;
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(-)