| Message ID | 39c4af723ea37cd05976a17cb9c1fbc975496ffd.1520592440.git.arvind.yadav.cs@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Boris Brezillon |
| Headers | show |
| Series | mtd: use put_device() if device_register fail | expand |
On Fri, 9 Mar 2018 16:20:48 +0530 Arvind Yadav <arvind.yadav.cs@gmail.com> wrote: > if device_register() returned an error! Always use put_device() > to give up the reference initialized. > > Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> > --- > drivers/mtd/mtdcore.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index 28553c8..4d77ca2 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -586,6 +586,7 @@ int add_mtd_device(struct mtd_info *mtd) > return 0; > > fail_added: > + put_device(&mtd->dev); Not sure this is a good idea: the put_device() call will trigger an mtd_devtype->release(), which will in turn call device_destroy() on something that does not exist yet. Not sure if this is a real problem, but it does not look like the right thing to do. > of_node_put(mtd_get_of_node(mtd)); You're referencing an object that is supposed to have been freed/released by the put_device() call. Again, it's not really a problem because in our case ->release() does not free the mtd object (as is usually done in other parts of the kernel), but it still looks wrong. It's probably better to move the of_node_put() and the below idr_remove() call in the ->release() hook if you want to use put_device(). > idr_remove(&mtd_idr, i); > fail_locked:
On Wednesday 14 March 2018 08:06 PM, Boris Brezillon wrote: > On Fri, 9 Mar 2018 16:20:48 +0530 > Arvind Yadav <arvind.yadav.cs@gmail.com> wrote: > >> if device_register() returned an error! Always use put_device() >> to give up the reference initialized. >> >> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> >> --- >> drivers/mtd/mtdcore.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c >> index 28553c8..4d77ca2 100644 >> --- a/drivers/mtd/mtdcore.c >> +++ b/drivers/mtd/mtdcore.c >> @@ -586,6 +586,7 @@ int add_mtd_device(struct mtd_info *mtd) >> return 0; >> >> fail_added: >> + put_device(&mtd->dev); > Not sure this is a good idea: the put_device() call will trigger > an mtd_devtype->release(), which will in turn call device_destroy() on > something that does not exist yet. Not sure if this is a real problem, > but it does not look like the right thing to do. > yes, you are correct. No need to call put_device(). which can cause other problem. >> of_node_put(mtd_get_of_node(mtd)); > You're referencing an object that is supposed to have been > freed/released by the put_device() call. Again, it's not really a > problem because in our case ->release() does not free the mtd object > (as is usually done in other parts of the kernel), but it still looks > wrong. It's probably better to move the of_node_put() and the below > idr_remove() call in the ->release() hook if you want to use > put_device(). > >> idr_remove(&mtd_idr, i); Sure, we can move put_device() below this. But need to check how we can add hook in release. > >> fail_locked: > > ~arvind
On 17/03/18 09:45, arvindY wrote: >>> of_node_put(mtd_get_of_node(mtd)); >> You're referencing an object that is supposed to have been >> freed/released by the put_device() call. Again, it's not really a >> problem because in our case ->release() does not free the mtd object >> (as is usually done in other parts of the kernel), but it still looks >> wrong. It's probably better to move the of_node_put() and the below >> idr_remove() call in the ->release() hook if you want to use >> put_device(). >> >>> idr_remove(&mtd_idr, i); > Sure, we can move put_device() below this. But need to check > how we can add hook in release. My guess is that you would need this: http://lists.infradead.org/pipermail/linux-mtd/2017-May/074373.html Martin >>> fail_locked: >> >> > ~arvind > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ >
Hi Arvind, On 19/03/18 17:56, arvindY wrote: > > > On Monday 19 March 2018 04:13 PM, Martin Habets wrote: >> On 17/03/18 09:45, arvindY wrote: >>>>> of_node_put(mtd_get_of_node(mtd)); >>>> You're referencing an object that is supposed to have been >>>> freed/released by the put_device() call. Again, it's not really a >>>> problem because in our case ->release() does not free the mtd object >>>> (as is usually done in other parts of the kernel), but it still looks >>>> wrong. It's probably better to move the of_node_put() and the below >>>> idr_remove() call in the ->release() hook if you want to use >>>> put_device(). >>>> >>>>> idr_remove(&mtd_idr, i); >>> Sure, we can move put_device() below this. But need to check >>> how we can add hook in release. >> My guess is that you would need this: >> http://lists.infradead.org/pipermail/linux-mtd/2017-May/074373.html > > we should not removes(device_destroy) a MTD device in > release. MTD device should be removes when > deleting(unregister) a MTD device. No, deleting an MTD device should only decrement a refcounter. At this point there can still be other processes with a /dev/mtd* device open. When there are no more users release gets called to remove it. > MTD device should decrement refcount of a node and > Remove MTD from IDR in dev->release(). You could be right about this, I'm not sure. My patch allows the caller to free the mtd_info memory. This is needed since the caller allocated the memory in the first place, and because the caller has no other of knowing that the last MTD user is gone. Martin
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 28553c8..4d77ca2 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -586,6 +586,7 @@ int add_mtd_device(struct mtd_info *mtd) return 0; fail_added: + put_device(&mtd->dev); of_node_put(mtd_get_of_node(mtd)); idr_remove(&mtd_idr, i); fail_locked:
if device_register() returned an error! Always use put_device() to give up the reference initialized. Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> --- drivers/mtd/mtdcore.c | 1 + 1 file changed, 1 insertion(+)