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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
> > > > ---
> > > >    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