Messages in this thread | | | From | Mathieu Poirier <> | Date | Tue, 27 Mar 2018 11:13:41 -0600 | Subject | Re: [PATCH] coresight: use put_device() instead of kfree() |
| |
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 >>>>> >
| |