Messages in this thread | | | Subject | Re: [PATCH -next] pwm: img: Fix PM reference leak in img_pwm_enable() | From | Samuel Zou <> | Date | Tue, 29 Jun 2021 11:23:41 +0800 |
| |
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 >
| |