| Message ID | d8d8708d2283349c2c4f3e9f3c9fd5f47126d2bc.1522410143.git.arvind.yadav.cs@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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. > } > > 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. > err_dev: > if (devfreq) > kfree(devfreq); >
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. > >> } >> >> 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. > >> err_dev: >> if (devfreq) >> kfree(devfreq); >> > >
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. > >>> } >>> >>> 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. >> >>> err_dev: >>> if (devfreq) >>> kfree(devfreq); >>> >> > ~arvind
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);
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(-)