Message ID | d5a326b6c284f7389301b265716dc58fda492c98.1521358552.git.arvind.yadav.cs@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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 >
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 >>
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 >>> >
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 >>>>
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 >>>>> >
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
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:
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(-)