From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: Ulf Hansson <ulf.hansson@linaro.org>
Cc: rjw@rjwysocki.net, heiko@sntech.de, lukasz.luba@arm.com,
linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
Daniel Lezcano <daniel.lezcano@kernel.org>,
"Rafael J. Wysocki" <rafael@kernel.org>
Subject: Re: [PATCH v1 3/7] powercap/dtpm: Fixup kfree for virtual node
Date: Thu, 17 Feb 2022 14:54:24 +0100 [thread overview]
Message-ID: <cfbaefa5-fc7b-bd0f-e4ed-8f046de2a7cf@linaro.org> (raw)
In-Reply-To: <CAPDyKFr8Ycr2cbiD5MM9FSPc1qea+Yp9=cottcGAo7HmFR9Eaw@mail.gmail.com>
On 17/02/2022 14:17, Ulf Hansson wrote:
> On Wed, 16 Feb 2022 at 19:10, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> On 16/02/2022 17:22, Ulf Hansson wrote:
>>> On Sun, 30 Jan 2022 at 22:02, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> When the node is virtual there is no release function associated which
>>>> can free the memory.
>>>>
>>>> Free the memory when no 'ops' exists.
>>>>
>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>> ---
>>>> drivers/powercap/dtpm.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
>>>> index 0b0121c37a1b..7bddd25a6767 100644
>>>> --- a/drivers/powercap/dtpm.c
>>>> +++ b/drivers/powercap/dtpm.c
>>>> @@ -181,12 +181,12 @@ int dtpm_release_zone(struct powercap_zone *pcz)
>>>>
>>>> if (dtpm->ops)
>>>> dtpm->ops->release(dtpm);
>>>> + else
>>>> + kfree(dtpm);
>>>>
>>>
>>> This doesn't look correct. Below you check dtpm against "root", which
>>> may be after its memory has been freed.
>>>
>>> If the ->release() function should be responsible for freeing the
>>> dtpm, it needs to be called after the check below.
>>
>> It is harmless, 'root' is not dereferenced but used as an ID
>>
>> Moreover, in the patch 5/7 it is moved out this function.
>
> Right. It just looks a bit odd here.
>
>>
>>
>>>> if (root == dtpm)
>>>> root = NULL;
>>>>
>>>> - kfree(dtpm);
>
> So then why doesn't this kfree do the job already?
>
> kfree(NULL) works fine, if dtpm->ops->release(dtpm) already freed the data.
The description is confusing.
Actually, there is a double kfree. When there is a ops->release, the
kfree is done there and again a few lines after.
The issue was introduced with the change where dtpm had a private data
field to store the backend specific structure and was converted to a
backend specific structure containing a dtpm node [1].
So this function was calling release from the dtpm backend which was
freeing the specific data in the dtpm->private and then here was freeing
the dtpm. Now, the backend frees the structure which contains the dtpm
structure, so when returning from ops->release(), dtpm is already free.
I should change the description and add a Fixes tag to the change
described above.
[1]
https://lore.kernel.org/r/20210312130411.29833-4-daniel.lezcano@linaro.org
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
next prev parent reply other threads:[~2022-02-17 13:54 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-30 21:02 [PATCH v1 1/7] powercap/dtpm: Change locking scheme Daniel Lezcano
2022-01-30 21:02 ` [PATCH v1 2/7] powercap/dtpm_cpu: Reset per_cpu variable in the release function Daniel Lezcano
2022-02-16 16:24 ` Ulf Hansson
2022-01-30 21:02 ` [PATCH v1 3/7] powercap/dtpm: Fixup kfree for virtual node Daniel Lezcano
2022-02-16 16:22 ` Ulf Hansson
2022-02-16 18:10 ` Daniel Lezcano
2022-02-17 13:17 ` Ulf Hansson
2022-02-17 13:54 ` Daniel Lezcano [this message]
2022-02-17 15:45 ` Ulf Hansson
2022-02-18 13:17 ` Daniel Lezcano
2022-02-22 15:55 ` Ulf Hansson
2022-02-22 15:59 ` Daniel Lezcano
2022-01-30 21:02 ` [PATCH v1 4/7] powercap/dtpm: Destroy hierarchy function Daniel Lezcano
2022-02-16 16:31 ` Ulf Hansson
2022-02-16 19:25 ` Daniel Lezcano
2022-02-17 13:12 ` Ulf Hansson
2022-02-17 13:17 ` Ulf Hansson
2022-01-30 21:02 ` [PATCH v1 5/7] powercap/dtpm: Move the 'root' reset place Daniel Lezcano
2022-02-17 13:19 ` Ulf Hansson
2022-01-30 21:02 ` [PATCH v1 6/7] powercap/dtpm/dtpm_cpu: Add exit function Daniel Lezcano
2022-02-17 13:20 ` Ulf Hansson
2022-01-30 21:02 ` [PATCH v1 7/7] dtpm/soc/rk3399: Add the ability to unload the module Daniel Lezcano
2022-01-30 21:02 ` Daniel Lezcano
2022-01-30 21:02 ` Daniel Lezcano
2022-02-17 13:21 ` Ulf Hansson
2022-02-17 13:21 ` Ulf Hansson
2022-02-17 13:21 ` Ulf Hansson
2022-02-16 16:24 ` [PATCH v1 1/7] powercap/dtpm: Change locking scheme Ulf Hansson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cfbaefa5-fc7b-bd0f-e4ed-8f046de2a7cf@linaro.org \
--to=daniel.lezcano@linaro.org \
--cc=daniel.lezcano@kernel.org \
--cc=heiko@sntech.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=lukasz.luba@arm.com \
--cc=rafael@kernel.org \
--cc=rjw@rjwysocki.net \
--cc=ulf.hansson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.