Never directly free @dev after calling device_register(), even
if it returned an error. Always use put_device() to give up the
reference initialized.
Signed-off-by: Arvind Yadav <[email protected]>
---
drivers/hwtracing/coresight/coresight.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 389c4ba..132dfbc 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -1026,8 +1026,10 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
dev_set_name(&csdev->dev, "%s", desc->pdata->name);
ret = device_register(&csdev->dev);
- if (ret)
- goto err_device_register;
+ if (ret) {
+ put_device(&csdev->dev);
+ goto err_kzalloc_csdev;
+ }
mutex_lock(&coresight_mutex);
@@ -1038,8 +1040,6 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
return csdev;
-err_device_register:
- kfree(conns);
err_kzalloc_conns:
kfree(refcnts);
err_kzalloc_refcnts:
--
2.7.4
drivers/hwtracing/coresight/coresight.c
On 18 March 2018 at 01:38, Arvind Yadav <[email protected]> wrote:
> Never directly free @dev after calling device_register(), even
> if it returned an error. Always use put_device() to give up the
> reference initialized.
>
> Signed-off-by: Arvind Yadav <[email protected]>
> ---
> drivers/hwtracing/coresight/coresight.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index 389c4ba..132dfbc 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -1026,8 +1026,10 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
> dev_set_name(&csdev->dev, "%s", desc->pdata->name);
>
> ret = device_register(&csdev->dev);
> - if (ret)
> - goto err_device_register;
> + if (ret) {
> + put_device(&csdev->dev);
> + goto err_kzalloc_csdev;
> + }
>
> mutex_lock(&coresight_mutex);
>
> @@ -1038,8 +1040,6 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>
> return csdev;
>
> -err_device_register:
> - kfree(conns);
Apologies for the late reply, I was travelling.
I suggest to simply replace kfree() with put_device() in order to
concentrate the error handling code in the same area and make sure
that memory allocated for @conns and @refcnts is freed.
Thanks,
Mathieu
> err_kzalloc_conns:
> kfree(refcnts);
> err_kzalloc_refcnts:
> --
> 2.7.4
>
On Tuesday 27 March 2018 03:16 AM, Mathieu Poirier wrote:
> drivers/hwtracing/coresight/coresight.c
> On 18 March 2018 at 01:38, Arvind Yadav <[email protected]> wrote:
>> Never directly free @dev after calling device_register(), even
>> if it returned an error. Always use put_device() to give up the
>> reference initialized.
>>
>> Signed-off-by: Arvind Yadav <[email protected]>
>> ---
>> drivers/hwtracing/coresight/coresight.c | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
>> index 389c4ba..132dfbc 100644
>> --- a/drivers/hwtracing/coresight/coresight.c
>> +++ b/drivers/hwtracing/coresight/coresight.c
>> @@ -1026,8 +1026,10 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>> dev_set_name(&csdev->dev, "%s", desc->pdata->name);
>>
>> ret = device_register(&csdev->dev);
>> - if (ret)
>> - goto err_device_register;
>> + if (ret) {
>> + put_device(&csdev->dev);
>> + goto err_kzalloc_csdev;
>> + }
>>
>> mutex_lock(&coresight_mutex);
>>
>> @@ -1038,8 +1040,6 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>>
>> return csdev;
>>
>> -err_device_register:
>> - kfree(conns);
> Apologies for the late reply, I was travelling.
>
> I suggest to simply replace kfree() with put_device() in order to
> concentrate the error handling code in the same area and make sure
> that memory allocated for @conns and @refcnts is freed.
>
> Thanks,
> Mathieu
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 should always avoid kfree() if device_register()
returned an error. Otherwise it'll not do clean up of other
kobject resources.
~arvind
>> err_kzalloc_conns:
>> kfree(refcnts);
>> err_kzalloc_refcnts:
>> --
>> 2.7.4
>>
On 26 March 2018 at 20:30, arvindY <[email protected]> wrote:
>
>
> On Tuesday 27 March 2018 03:16 AM, Mathieu Poirier wrote:
>>
>> drivers/hwtracing/coresight/coresight.c
>> On 18 March 2018 at 01:38, Arvind Yadav <[email protected]> wrote:
>>>
>>> Never directly free @dev after calling device_register(), even
>>> if it returned an error. Always use put_device() to give up the
>>> reference initialized.
>>>
>>> Signed-off-by: Arvind Yadav <[email protected]>
>>> ---
>>> drivers/hwtracing/coresight/coresight.c | 8 ++++----
>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight.c
>>> b/drivers/hwtracing/coresight/coresight.c
>>> index 389c4ba..132dfbc 100644
>>> --- a/drivers/hwtracing/coresight/coresight.c
>>> +++ b/drivers/hwtracing/coresight/coresight.c
>>> @@ -1026,8 +1026,10 @@ struct coresight_device *coresight_register(struct
>>> coresight_desc *desc)
>>> dev_set_name(&csdev->dev, "%s", desc->pdata->name);
>>>
>>> ret = device_register(&csdev->dev);
>>> - if (ret)
>>> - goto err_device_register;
>>> + if (ret) {
>>> + put_device(&csdev->dev);
>>> + goto err_kzalloc_csdev;
>>> + }
>>>
>>> mutex_lock(&coresight_mutex);
>>>
>>> @@ -1038,8 +1040,6 @@ struct coresight_device *coresight_register(struct
>>> coresight_desc *desc)
>>>
>>> return csdev;
>>>
>>> -err_device_register:
>>> - kfree(conns);
>>
>> Apologies for the late reply, I was travelling.
>>
>> I suggest to simply replace kfree() with put_device() in order to
>> concentrate the error handling code in the same area and make sure
>> that memory allocated for @conns and @refcnts is freed.
>>
>> Thanks,
>> Mathieu
>
>
> 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.
I have read the notice and in full agreement with the put_device() part.
> 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.
Memory would be automatically freed if it would have been allocated
with the devm_XYZ() helpers but it is not the case here. As such it
has to be freed explicitly. Reading your patch again (and the jetlag
somewhat fading) I think you've done the right thing except for the
"goto" that should still point to "err_device_register".
>we should always avoid kfree() if device_register()
> returned an error. Otherwise it'll not do clean up of other
> kobject resources.
>
> ~arvind
>>>
>>> err_kzalloc_conns:
>>> kfree(refcnts);
>>> err_kzalloc_refcnts:
>>> --
>>> 2.7.4
>>>
>
On Tuesday 27 March 2018 09:37 PM, Mathieu Poirier wrote:
> On 26 March 2018 at 20:30, arvindY <[email protected]> wrote:
>>
>> On Tuesday 27 March 2018 03:16 AM, Mathieu Poirier wrote:
>>> drivers/hwtracing/coresight/coresight.c
>>> On 18 March 2018 at 01:38, Arvind Yadav <[email protected]> wrote:
>>>> Never directly free @dev after calling device_register(), even
>>>> if it returned an error. Always use put_device() to give up the
>>>> reference initialized.
>>>>
>>>> Signed-off-by: Arvind Yadav <[email protected]>
>>>> ---
>>>> drivers/hwtracing/coresight/coresight.c | 8 ++++----
>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight.c
>>>> b/drivers/hwtracing/coresight/coresight.c
>>>> index 389c4ba..132dfbc 100644
>>>> --- a/drivers/hwtracing/coresight/coresight.c
>>>> +++ b/drivers/hwtracing/coresight/coresight.c
>>>> @@ -1026,8 +1026,10 @@ struct coresight_device *coresight_register(struct
>>>> coresight_desc *desc)
>>>> dev_set_name(&csdev->dev, "%s", desc->pdata->name);
>>>>
>>>> ret = device_register(&csdev->dev);
>>>> - if (ret)
>>>> - goto err_device_register;
>>>> + if (ret) {
>>>> + put_device(&csdev->dev);
>>>> + goto err_kzalloc_csdev;
>>>> + }
>>>>
>>>> mutex_lock(&coresight_mutex);
>>>>
>>>> @@ -1038,8 +1040,6 @@ struct coresight_device *coresight_register(struct
>>>> coresight_desc *desc)
>>>>
>>>> return csdev;
>>>>
>>>> -err_device_register:
>>>> - kfree(conns);
>>> Apologies for the late reply, I was travelling.
>>>
>>> I suggest to simply replace kfree() with put_device() in order to
>>> concentrate the error handling code in the same area and make sure
>>> that memory allocated for @conns and @refcnts is freed.
>>>
>>> Thanks,
>>> Mathieu
>>
>> 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.
> I have read the notice and in full agreement with the put_device() part.
>
>> 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.
> Memory would be automatically freed if it would have been allocated
> with the devm_XYZ() helpers but it is not the case here. As such it
> has to be freed explicitly. Reading your patch again (and the jetlag
> somewhat fading) I think you've done the right thing except for the
> "goto" that should still point to "err_device_register".
>
Take rest :)
'goto' should not point to "err_device_register" because
put_device() will call 'dev->release' which is coresight_device_release().
It's release @conns, @refcnt and @csdev . If we will keep same 'goto
then kfree() will be redundant for all.
>> we should always avoid kfree() if device_register()
>> returned an error. Otherwise it'll not do clean up of other
>> kobject resources.
>>
>> ~arvind
>>>> err_kzalloc_conns:
>>>> kfree(refcnts);
>>>> err_kzalloc_refcnts:
>>>> --
>>>> 2.7.4
>>>>
On 27 March 2018 at 10:28, arvindY <[email protected]> wrote:
>
>
> On Tuesday 27 March 2018 09:37 PM, Mathieu Poirier wrote:
>>
>> On 26 March 2018 at 20:30, arvindY <[email protected]> wrote:
>>>
>>>
>>> On Tuesday 27 March 2018 03:16 AM, Mathieu Poirier wrote:
>>>>
>>>> drivers/hwtracing/coresight/coresight.c
>>>> On 18 March 2018 at 01:38, Arvind Yadav <[email protected]>
>>>> wrote:
>>>>>
>>>>> Never directly free @dev after calling device_register(), even
>>>>> if it returned an error. Always use put_device() to give up the
>>>>> reference initialized.
>>>>>
>>>>> Signed-off-by: Arvind Yadav <[email protected]>
>>>>> ---
>>>>> drivers/hwtracing/coresight/coresight.c | 8 ++++----
>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/hwtracing/coresight/coresight.c
>>>>> b/drivers/hwtracing/coresight/coresight.c
>>>>> index 389c4ba..132dfbc 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight.c
>>>>> @@ -1026,8 +1026,10 @@ struct coresight_device
>>>>> *coresight_register(struct
>>>>> coresight_desc *desc)
>>>>> dev_set_name(&csdev->dev, "%s", desc->pdata->name);
>>>>>
>>>>> ret = device_register(&csdev->dev);
>>>>> - if (ret)
>>>>> - goto err_device_register;
>>>>> + if (ret) {
>>>>> + put_device(&csdev->dev);
>>>>> + goto err_kzalloc_csdev;
>>>>> + }
>>>>>
>>>>> mutex_lock(&coresight_mutex);
>>>>>
>>>>> @@ -1038,8 +1040,6 @@ struct coresight_device
>>>>> *coresight_register(struct
>>>>> coresight_desc *desc)
>>>>>
>>>>> return csdev;
>>>>>
>>>>> -err_device_register:
>>>>> - kfree(conns);
>>>>
>>>> Apologies for the late reply, I was travelling.
>>>>
>>>> I suggest to simply replace kfree() with put_device() in order to
>>>> concentrate the error handling code in the same area and make sure
>>>> that memory allocated for @conns and @refcnts is freed.
>>>>
>>>> Thanks,
>>>> Mathieu
>>>
>>>
>>> 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.
>>
>> I have read the notice and in full agreement with the put_device() part.
>>
>>> 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.
>>
>> Memory would be automatically freed if it would have been allocated
>> with the devm_XYZ() helpers but it is not the case here. As such it
>> has to be freed explicitly. Reading your patch again (and the jetlag
>> somewhat fading) I think you've done the right thing except for the
>> "goto" that should still point to "err_device_register".
>>
>
> Take rest :)
> 'goto' should not point to "err_device_register" because
> put_device() will call 'dev->release' which is coresight_device_release().
> It's release @conns, @refcnt and @csdev . If we will keep same 'goto
> then kfree() will be redundant for all.
You are correct - your patch does exactly what should be done.
>
>
>>> we should always avoid kfree() if device_register()
>>> returned an error. Otherwise it'll not do clean up of other
>>> kobject resources.
>>>
>>> ~arvind
>>>>>
>>>>> err_kzalloc_conns:
>>>>> kfree(refcnts);
>>>>> err_kzalloc_refcnts:
>>>>> --
>>>>> 2.7.4
>>>>>
>
On 18 March 2018 at 01:38, Arvind Yadav <[email protected]> wrote:
> Never directly free @dev after calling device_register(), even
> if it returned an error. Always use put_device() to give up the
> reference initialized.
>
> Signed-off-by: Arvind Yadav <[email protected]>
> ---
> drivers/hwtracing/coresight/coresight.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index 389c4ba..132dfbc 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -1026,8 +1026,10 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
> dev_set_name(&csdev->dev, "%s", desc->pdata->name);
>
> ret = device_register(&csdev->dev);
> - if (ret)
> - goto err_device_register;
> + if (ret) {
> + put_device(&csdev->dev);
> + goto err_kzalloc_csdev;
> + }
>
> mutex_lock(&coresight_mutex);
>
> @@ -1038,8 +1040,6 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>
> return csdev;
>
> -err_device_register:
> - kfree(conns);
> err_kzalloc_conns:
> kfree(refcnts);
> err_kzalloc_refcnts:
> --
> 2.7.4
>
Applied - thanks
Mathieu