[PATCH] coresight: use put_device() instead of kfree()

[PATCH] coresight: use put_device() instead of kfree()

[Arvind Yadav]

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 <arvind.yadav.cs@gmail.com>
---
 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

Re: [PATCH] coresight: use put_device() instead of kfree()

[Mathieu Poirier]

drivers/hwtracing/coresight/coresight.c
On 18 March 2018 at 01:38, Arvind Yadav <arvind.yadav.cs@gmail.com> 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 <arvind.yadav.cs@gmail.com>
> ---
>  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
>

Re: [PATCH] coresight: use put_device() instead of kfree()

[arvindY]

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 <arvind.yadav.cs@gmail.com> 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 <arvind.yadav.cs@gmail.com>
>> ---
>>   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
>>

Re: [PATCH] coresight: use put_device() instead of kfree()

[Mathieu Poirier]

On 26 March 2018 at 20:30, arvindY <arvind.yadav.cs@gmail.com> 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 <arvind.yadav.cs@gmail.com> 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 <arvind.yadav.cs@gmail.com>
>>> ---
>>>   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
>>>
>

Re: [PATCH] coresight: use put_device() instead of kfree()

[arvindY]

On Tuesday 27 March 2018 09:37 PM, Mathieu Poirier wrote:
> On 26 March 2018 at 20:30, arvindY <arvind.yadav.cs@gmail.com> 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 <arvind.yadav.cs@gmail.com> 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 <arvind.yadav.cs@gmail.com>
>>>> ---
>>>>    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
>>>>

Re: [PATCH] coresight: use put_device() instead of kfree()

[Mathieu Poirier]

On 27 March 2018 at 10:28, arvindY <arvind.yadav.cs@gmail.com> wrote:
>
>
> On Tuesday 27 March 2018 09:37 PM, Mathieu Poirier wrote:
>>
>> On 26 March 2018 at 20:30, arvindY <arvind.yadav.cs@gmail.com> 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 <arvind.yadav.cs@gmail.com>
>>>> 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 <arvind.yadav.cs@gmail.com>
>>>>> ---
>>>>>    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
>>>>>
>

Re: [PATCH] coresight: use put_device() instead of kfree()

[Mathieu Poirier]

On 18 March 2018 at 01:38, Arvind Yadav <arvind.yadav.cs@gmail.com> 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 <arvind.yadav.cs@gmail.com>
> ---
>  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