| Message ID | 5AD0160E.7050601@samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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. > >>>>> err_dev: >>>>> if (devfreq) >>>>> kfree(devfreq); >>>>> >> ~arvind >> >> >> >
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 >>> >>> >>> >> > > > >
>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: >>>> On Friday 13 April 2018 06:43 AM, Chanwoo Choi wrote: >>>>> On 2018년 04월 13일 10:03, Chanwoo Choi wrote: [] >>>>>>> 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; >>>>>>> } >>>>>>> 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; >>>>>>> err_dev: >>>>>>> if (devfreq) >>>>>>> kfree(devfreq); >>>>>>> Ah.. this was critcal. Thanks! Acked-by: MyungJoo Ham <myungjoo.ham@samsung.com> >Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> Cheers, MyungJoo
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; }