Re: [PATCH] ssb:: use put_device() if device_register fail
On Wednesday 07 March 2018 11:08 PM, Michael Büsch wrote: On Wed, 7 Mar 2018 22:46:14 +0530 arvindYwrote: diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c index 65420a9..c4449e0 100644 --- a/drivers/ssb/main.c +++ b/drivers/ssb/main.c @@ -521,6 +521,7 @@ static int ssb_devices_register(struct ssb_bus *bus) ssb_err("Could not register %s\n", dev_name(dev)); /* Set dev to NULL to not unregister * dev on error unwinding. */ + put_device(dev); sdev->dev = NULL; kfree(devwrap); goto error; I don't think this is correct. The dev structure is allocated as part of devwrap, which is freed here. Why do you think we need put_device here? Yes this patch is not correct, We must not use kfree() after you called device_register() (even if it was not successful!) -- see the comment for device_register(). I will delete kfree() and send updated patch. Is device_put() going to call ssb_release_dev() to free the structure? Can you please elaborate on why device_put() must be used? The comment is not really of any use here. put_device() will call kobject_put(). By calling this, The kobject core will automatically clean up all of the memory allocated with the kobject. Internally kobject_put() will call 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
Re: [PATCH] ssb:: use put_device() if device_register fail
On Wed, 7 Mar 2018 22:46:14 +0530 arvindYwrote: > >> diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c > >> index 65420a9..c4449e0 100644 > >> --- a/drivers/ssb/main.c > >> +++ b/drivers/ssb/main.c > >> @@ -521,6 +521,7 @@ static int ssb_devices_register(struct ssb_bus *bus) > >>ssb_err("Could not register %s\n", dev_name(dev)); > >>/* Set dev to NULL to not unregister > >> * dev on error unwinding. */ > >> + put_device(dev); > >>sdev->dev = NULL; > >>kfree(devwrap); > >>goto error; > > > > I don't think this is correct. > > The dev structure is allocated as part of devwrap, which is freed here. > > > > Why do you think we need put_device here? > > > Yes this patch is not correct, We must not use kfree() after you called > device_register() (even > if it was not successful!) -- see the comment for device_register(). > I will delete kfree() and send updated patch. Is device_put() going to call ssb_release_dev() to free the structure? Can you please elaborate on why device_put() must be used? The comment is not really of any use here. -- Michael pgp9IEPbSlMjr.pgp Description: OpenPGP digital signature
Re: [PATCH] ssb:: use put_device() if device_register fail
On Wednesday 07 March 2018 10:17 PM, Michael Büsch wrote: On Wed, 7 Mar 2018 15:31:30 +0530 Arvind Yadavwrote: if device_register() returned an error! Always use put_device() to give up the reference initialized. Signed-off-by: Arvind Yadav --- drivers/ssb/main.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c index 65420a9..c4449e0 100644 --- a/drivers/ssb/main.c +++ b/drivers/ssb/main.c @@ -521,6 +521,7 @@ static int ssb_devices_register(struct ssb_bus *bus) ssb_err("Could not register %s\n", dev_name(dev)); /* Set dev to NULL to not unregister * dev on error unwinding. */ + put_device(dev); sdev->dev = NULL; kfree(devwrap); goto error; I don't think this is correct. The dev structure is allocated as part of devwrap, which is freed here. Why do you think we need put_device here? Yes this patch is not correct, We must not use kfree() after you called device_register() (even if it was not successful!) -- see the comment for device_register(). I will delete kfree() and send updated patch. ~arvind
Re: [PATCH] ssb:: use put_device() if device_register fail
On Wed, 7 Mar 2018 15:31:30 +0530 Arvind Yadavwrote: > if device_register() returned an error! Always use put_device() > to give up the reference initialized. > > Signed-off-by: Arvind Yadav > --- > drivers/ssb/main.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c > index 65420a9..c4449e0 100644 > --- a/drivers/ssb/main.c > +++ b/drivers/ssb/main.c > @@ -521,6 +521,7 @@ static int ssb_devices_register(struct ssb_bus *bus) > ssb_err("Could not register %s\n", dev_name(dev)); > /* Set dev to NULL to not unregister >* dev on error unwinding. */ > + put_device(dev); > sdev->dev = NULL; > kfree(devwrap); > goto error; I don't think this is correct. The dev structure is allocated as part of devwrap, which is freed here. Why do you think we need put_device here? -- Michael pgp9lFUSVai7N.pgp Description: OpenPGP digital signature
[PATCH] ssb:: use put_device() if device_register fail
if device_register() returned an error! Always use put_device() to give up the reference initialized. Signed-off-by: Arvind Yadav--- drivers/ssb/main.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c index 65420a9..c4449e0 100644 --- a/drivers/ssb/main.c +++ b/drivers/ssb/main.c @@ -521,6 +521,7 @@ static int ssb_devices_register(struct ssb_bus *bus) ssb_err("Could not register %s\n", dev_name(dev)); /* Set dev to NULL to not unregister * dev on error unwinding. */ + put_device(dev); sdev->dev = NULL; kfree(devwrap); goto error; -- 1.9.1