RE: 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: 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(>dev); >>>if (err) { >>>mutex_unlock(>lock); >>> -goto err_dev; >>> +put_device(>dev); >>> +goto err_out; >>>} >>> devfreq->trans_table =devm_kzalloc(>dev, >>> @@ -671,6 +672,7 @@ struct devfreq *devfreq_add_device(struct device >>> *dev, >>>mutex_unlock(_list_lock); >>> device_unregister(>dev); >>> +devfreq = NULL; >>>err_dev: >>>if (devfreq) >>>kfree(devfreq); >>> Ah.. this was critcal. Thanks! Acked-by: MyungJoo Ham>Reviewed-by: Chanwoo Choi Cheers, MyungJoo
RE: 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: 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(>dev); >>>if (err) { >>>mutex_unlock(>lock); >>> -goto err_dev; >>> +put_device(>dev); >>> +goto err_out; >>>} >>> devfreq->trans_table =devm_kzalloc(>dev, >>> @@ -671,6 +672,7 @@ struct devfreq *devfreq_add_device(struct device >>> *dev, >>>mutex_unlock(_list_lock); >>> device_unregister(>dev); >>> +devfreq = NULL; >>>err_dev: >>>if (devfreq) >>>kfree(devfreq); >>> Ah.. this was critcal. Thanks! Acked-by: MyungJoo Ham >Reviewed-by: Chanwoo Choi Cheers, MyungJoo
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>> --- >>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(>dev); >>if (err) { >>mutex_unlock(>lock); >> -goto err_dev; >> +put_device(>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(>dev, >> @@ -671,6 +672,7 @@ struct devfreq *devfreq_add_device(struct device >> *dev, >>mutex_unlock(_list_lock); >> device_unregister(>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(>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 >> >>err_dev: >>if (devfreq) >>kfree(devfreq); >> >>> ~arvind >>> >>> >>> >> > > > > -- Best Regards, Chanwoo Choi Samsung Electronics
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 >> --- >>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(>dev); >>if (err) { >>mutex_unlock(>lock); >> -goto err_dev; >> +put_device(>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(>dev, >> @@ -671,6 +672,7 @@ struct devfreq *devfreq_add_device(struct device >> *dev, >>mutex_unlock(_list_lock); >> device_unregister(>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(>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 >> >>err_dev: >>if (devfreq) >>kfree(devfreq); >> >>> ~arvind >>> >>> >>> >> > > > > -- Best Regards, Chanwoo Choi Samsung Electronics
Re: [PATCH] PM / devfreq: use put_device() instead of kfree()
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--- 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(>dev); if (err) { mutex_unlock(>lock); -goto err_dev; +put_device(>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(>dev, @@ -671,6 +672,7 @@ struct devfreq *devfreq_add_device(struct device *dev, mutex_unlock(_list_lock); device_unregister(>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(>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
Re: [PATCH] PM / devfreq: use put_device() instead of kfree()
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 --- 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(>dev); if (err) { mutex_unlock(>lock); -goto err_dev; +put_device(>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(>dev, @@ -671,6 +672,7 @@ struct devfreq *devfreq_add_device(struct device *dev, mutex_unlock(_list_lock); device_unregister(>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(>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
Re: [PATCH] PM / devfreq: use put_device() instead of kfree()
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--- 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(>dev); if (err) { mutex_unlock(>lock); -goto err_dev; +put_device(>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(>dev, @@ -671,6 +672,7 @@ struct devfreq *devfreq_add_device(struct device *dev, mutex_unlock(_list_lock); device_unregister(>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(>lock); kfree(devfreq); + devfreq = NULL; } >>> err_dev: if (devfreq) kfree(devfreq); >>> >> > > ~arvind > > > -- Best Regards, Chanwoo Choi Samsung Electronics
Re: [PATCH] PM / devfreq: use put_device() instead of kfree()
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 --- 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(>dev); if (err) { mutex_unlock(>lock); -goto err_dev; +put_device(>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(>dev, @@ -671,6 +672,7 @@ struct devfreq *devfreq_add_device(struct device *dev, mutex_unlock(_list_lock); device_unregister(>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(>lock); kfree(devfreq); + devfreq = NULL; } >>> err_dev: if (devfreq) kfree(devfreq); >>> >> > > ~arvind > > > -- Best Regards, Chanwoo Choi Samsung Electronics
Re: [PATCH] PM / devfreq: use put_device() instead of kfree()
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--- 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(>dev); if (err) { mutex_unlock(>lock); - goto err_dev; + put_device(>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. } devfreq->trans_table = devm_kzalloc(>dev, @@ -671,6 +672,7 @@ struct devfreq *devfreq_add_device(struct device *dev, mutex_unlock(_list_lock); device_unregister(>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. err_dev: if (devfreq) kfree(devfreq); ~arvind
Re: [PATCH] PM / devfreq: use put_device() instead of kfree()
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 --- 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(>dev); if (err) { mutex_unlock(>lock); - goto err_dev; + put_device(>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. } devfreq->trans_table = devm_kzalloc(>dev, @@ -671,6 +672,7 @@ struct devfreq *devfreq_add_device(struct device *dev, mutex_unlock(_list_lock); device_unregister(>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. err_dev: if (devfreq) kfree(devfreq); ~arvind
Re: [PATCH] PM / devfreq: use put_device() instead of kfree()
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>> --- >> 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(>dev); >> if (err) { >> mutex_unlock(>lock); >> -goto err_dev; >> +put_device(>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. > >> } >> >> devfreq->trans_table = devm_kzalloc(>dev, >> @@ -671,6 +672,7 @@ struct devfreq *devfreq_add_device(struct device *dev, >> mutex_unlock(_list_lock); >> >> device_unregister(>dev); >> +devfreq = NULL; > > It is wrong. If you initialize the devfreq as NULL, > never free the 'devfreq' instance. > >> err_dev: >> if (devfreq) >> kfree(devfreq); >> > > -- Best Regards, Chanwoo Choi Samsung Electronics
Re: [PATCH] PM / devfreq: use put_device() instead of kfree()
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 >> --- >> 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(>dev); >> if (err) { >> mutex_unlock(>lock); >> -goto err_dev; >> +put_device(>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. > >> } >> >> devfreq->trans_table = devm_kzalloc(>dev, >> @@ -671,6 +672,7 @@ struct devfreq *devfreq_add_device(struct device *dev, >> mutex_unlock(_list_lock); >> >> device_unregister(>dev); >> +devfreq = NULL; > > It is wrong. If you initialize the devfreq as NULL, > never free the 'devfreq' instance. > >> err_dev: >> if (devfreq) >> kfree(devfreq); >> > > -- Best Regards, Chanwoo Choi Samsung Electronics
Re: [PATCH] PM / devfreq: use put_device() instead of kfree()
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> --- > 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(>dev); > if (err) { > mutex_unlock(>lock); > - goto err_dev; > + put_device(>dev); > + goto err_out; why do you change the goto postion? err_out is correct to free the memory of devfreq instance. > } > > devfreq->trans_table = devm_kzalloc(>dev, > @@ -671,6 +672,7 @@ struct devfreq *devfreq_add_device(struct device *dev, > mutex_unlock(_list_lock); > > device_unregister(>dev); > + devfreq = NULL; It is wrong. If you initialize the devfreq as NULL, never free the 'devfreq' instance. > err_dev: > if (devfreq) > kfree(devfreq); > -- Best Regards, Chanwoo Choi Samsung Electronics
Re: [PATCH] PM / devfreq: use put_device() instead of kfree()
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 > --- > 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(>dev); > if (err) { > mutex_unlock(>lock); > - goto err_dev; > + put_device(>dev); > + goto err_out; why do you change the goto postion? err_out is correct to free the memory of devfreq instance. > } > > devfreq->trans_table = devm_kzalloc(>dev, > @@ -671,6 +672,7 @@ struct devfreq *devfreq_add_device(struct device *dev, > mutex_unlock(_list_lock); > > device_unregister(>dev); > + devfreq = NULL; It is wrong. If you initialize the devfreq as NULL, never free the 'devfreq' instance. > err_dev: > if (devfreq) > kfree(devfreq); > -- Best Regards, Chanwoo Choi Samsung Electronics
[PATCH] PM / devfreq: use put_device() instead of kfree()
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--- 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(>dev); if (err) { mutex_unlock(>lock); - goto err_dev; + put_device(>dev); + goto err_out; } devfreq->trans_table = devm_kzalloc(>dev, @@ -671,6 +672,7 @@ struct devfreq *devfreq_add_device(struct device *dev, mutex_unlock(_list_lock); device_unregister(>dev); + devfreq = NULL; err_dev: if (devfreq) kfree(devfreq); -- 2.7.4
[PATCH] PM / devfreq: use put_device() instead of kfree()
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 --- 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(>dev); if (err) { mutex_unlock(>lock); - goto err_dev; + put_device(>dev); + goto err_out; } devfreq->trans_table = devm_kzalloc(>dev, @@ -671,6 +672,7 @@ struct devfreq *devfreq_add_device(struct device *dev, mutex_unlock(_list_lock); device_unregister(>dev); + devfreq = NULL; err_dev: if (devfreq) kfree(devfreq); -- 2.7.4