Messages in this thread | | | Date | Fri, 13 Apr 2018 11:43:03 +0900 | From | Chanwoo Choi <> | Subject | Re: [PATCH] PM / devfreq: use put_device() instead of kfree() |
| |
On 2018년 04월 13일 11:37, arvindY wrote: > > > On Friday 13 April 2018 07:59 AM, Chanwoo Choi wrote: >> On 2018년 04월 13일 11:15, arvindY wrote: >>> Hi Chanwoo, >>> >>> On Friday 13 April 2018 06:43 AM, Chanwoo Choi wrote: >>>> On 2018년 04월 13일 10:03, Chanwoo Choi wrote: >>>>> Hi, >>>>> >>>>> I'm sorry for the late reply. >>>>> >>>>> On 2018년 03월 30일 20:44, Arvind Yadav wrote: >>>>>> Never directly free @dev after calling device_register() or >>>>>> device_unregister(), even if device_register() returned an error. >>>>>> Always use put_device() to give up the reference initialized. >>>>>> >>>>>> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> >>>>>> --- >>>>>> drivers/devfreq/devfreq.c | 4 +++- >>>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>>>>> index fe2af6a..a225b94 100644 >>>>>> --- a/drivers/devfreq/devfreq.c >>>>>> +++ b/drivers/devfreq/devfreq.c >>>>>> @@ -625,7 +625,8 @@ struct devfreq *devfreq_add_device(struct device *dev, >>>>>> err = device_register(&devfreq->dev); >>>>>> if (err) { >>>>>> mutex_unlock(&devfreq->lock); >>>>>> - goto err_dev; >>>>>> + put_device(&devfreq->dev); >>>>>> + goto err_out; >>>>> why do you change the goto postion? >>>>> err_out is correct to free the memory of devfreq instance. >>>> Sorry. err_dev is correct instead of err_out. >>> If you will see the comment for device_register(drivers/base/core.c) >>> there is mentioned that 'NOTE: _Never_ directly free @dev >>> after calling this function, even if it returned an error! >>> Always use put_device() to give up the reference initialized >>> in this function instead. Here put_device() will decrement >>> the last reference and then free the memory by calling dev->release. >>> Internally put_device() -> kobject_put() -> kobject_cleanup() which >>> is responsible to call 'dev -> release' and also free other kobject resources. >>> >>> We are releasing devfreq in devfreq_dev_release(). So no need >>> to call kfree() again. It'll be redundant. 'err_out' is correct. >> You're right. err_out is correct. >> put_device() -> dev->release() -> devfreq_dev_release() -> kfree(devfreq) >> >>>>>> } >>>>>> devfreq->trans_table = devm_kzalloc(&devfreq->dev, >>>>>> @@ -671,6 +672,7 @@ struct devfreq *devfreq_add_device(struct device *dev, >>>>>> mutex_unlock(&devfreq_list_lock); >>>>>> device_unregister(&devfreq->dev); >>>>>> + devfreq = NULL; >>>>> It is wrong. If you initialize the devfreq as NULL, >>>>> never free the 'devfreq' instance. >>> No need to release memory after device_unregister(). >>> driver core will take care of this. It will release memory >>> So no need to call free again. >> If you have to initialize the devfreq instance as NULL, >> I think that you better to init in the devfreq_dev_release() >> as following: >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index fe2af6aa88fc..8c52a13d3887 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -543,6 +543,7 @@ static void devfreq_dev_release(struct device *dev) >> mutex_destroy(&devfreq->lock); >> kfree(devfreq); >> + devfreq = NULL; >> } > > Yes, You are right. I will share a update patch. > Thanks for reviewing.
It is my mistake. 'devfreq' is local variable in the devfreq_dev_release(0. Even if initializes 'devfreq = NULL' in the devfreq_dev_release(), it cannot initialize the 'devfreq' local variable in the devfreq_add_device().
I think that your original patch is good. Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>> >>>>>> err_dev: >>>>>> if (devfreq) >>>>>> kfree(devfreq); >>>>>> >>> ~arvind >>> >>> >>> >> > > > >
-- Best Regards, Chanwoo Choi Samsung Electronics
| |