| Message ID | 20221109025142.1565445-3-yangyingliang@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: sdio: fixes some leaks | expand |
On Wed, 9 Nov 2022 at 03:53, Yang Yingliang <yangyingliang@huawei.com> wrote: > > If device_add() returns error in sdio_add_func(), sdio function is not > presented, so the node refcount that hold in sdio_set_of_node() can not > be put in sdio_remove_func() which is called from error path. Fix this > by moving of_node_put() before present check in remove() function. > > Fixes: 25185f3f31c9 ("mmc: Add SDIO function devicetree subnode parsing") > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > --- > drivers/mmc/core/sdio_bus.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c > index babf21a0adeb..266639504a94 100644 > --- a/drivers/mmc/core/sdio_bus.c > +++ b/drivers/mmc/core/sdio_bus.c > @@ -377,11 +377,11 @@ int sdio_add_func(struct sdio_func *func) > */ > void sdio_remove_func(struct sdio_func *func) > { > + of_node_put(func->dev.of_node); > if (!sdio_func_present(func)) > return; > > device_del(&func->dev); > - of_node_put(func->dev.of_node); > put_device(&func->dev); Seems like we should call put_device() even if sdio_func_present() returns false, don't you think? In this way, the corresponding sdio_release_func() will help to manage the cleanup for us, so patch1 can be dropped. Or is there a problem with that? Kind regards Uffe
On 2022/11/9 20:27, Ulf Hansson wrote: > On Wed, 9 Nov 2022 at 03:53, Yang Yingliang <yangyingliang@huawei.com> wrote: >> If device_add() returns error in sdio_add_func(), sdio function is not >> presented, so the node refcount that hold in sdio_set_of_node() can not >> be put in sdio_remove_func() which is called from error path. Fix this >> by moving of_node_put() before present check in remove() function. >> >> Fixes: 25185f3f31c9 ("mmc: Add SDIO function devicetree subnode parsing") >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >> --- >> drivers/mmc/core/sdio_bus.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c >> index babf21a0adeb..266639504a94 100644 >> --- a/drivers/mmc/core/sdio_bus.c >> +++ b/drivers/mmc/core/sdio_bus.c >> @@ -377,11 +377,11 @@ int sdio_add_func(struct sdio_func *func) >> */ >> void sdio_remove_func(struct sdio_func *func) >> { >> + of_node_put(func->dev.of_node); >> if (!sdio_func_present(func)) >> return; >> >> device_del(&func->dev); >> - of_node_put(func->dev.of_node); >> put_device(&func->dev); > Seems like we should call put_device() even if sdio_func_present() > returns false, don't you think? > > In this way, the corresponding sdio_release_func() will help to manage In sdio_release_func(), sdio_free_fun_cis() is called, it put refcount of 'func->card->dev', but the refcount isn't get until sdio_init_func() is successful. In this way, it's no need to put refcount of 'func->card->dev', so we can not call sdio_release_func() in patch1, and patch1 is needed. Thanks, yang > the cleanup for us, so patch1 can be dropped. Or is there a problem > with that? > > Kind regards > Uffe > .
On Wed, 9 Nov 2022 at 14:23, Yang Yingliang <yangyingliang@huawei.com> wrote: > > > On 2022/11/9 20:27, Ulf Hansson wrote: > > On Wed, 9 Nov 2022 at 03:53, Yang Yingliang <yangyingliang@huawei.com> wrote: > >> If device_add() returns error in sdio_add_func(), sdio function is not > >> presented, so the node refcount that hold in sdio_set_of_node() can not > >> be put in sdio_remove_func() which is called from error path. Fix this > >> by moving of_node_put() before present check in remove() function. > >> > >> Fixes: 25185f3f31c9 ("mmc: Add SDIO function devicetree subnode parsing") > >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > >> --- > >> drivers/mmc/core/sdio_bus.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c > >> index babf21a0adeb..266639504a94 100644 > >> --- a/drivers/mmc/core/sdio_bus.c > >> +++ b/drivers/mmc/core/sdio_bus.c > >> @@ -377,11 +377,11 @@ int sdio_add_func(struct sdio_func *func) > >> */ > >> void sdio_remove_func(struct sdio_func *func) > >> { > >> + of_node_put(func->dev.of_node); > >> if (!sdio_func_present(func)) > >> return; > >> > >> device_del(&func->dev); > >> - of_node_put(func->dev.of_node); > >> put_device(&func->dev); > > Seems like we should call put_device() even if sdio_func_present() > > returns false, don't you think? > > > > In this way, the corresponding sdio_release_func() will help to manage > In sdio_release_func(), sdio_free_fun_cis() is called, it put refcount of > 'func->card->dev', but the refcount isn't get until sdio_init_func() > is successful. In this way, it's no need to put refcount of > 'func->card->dev', > so we can not call sdio_release_func() in patch1, and patch1 is needed. I see. However, in that case, it seems like we need to fix this slightly differently. In patch1, we should not do the kfree() thing immediately in the error patch, but rather rely on the reference counting (but in a more clever way). Kind regards Uffe
diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c index babf21a0adeb..266639504a94 100644 --- a/drivers/mmc/core/sdio_bus.c +++ b/drivers/mmc/core/sdio_bus.c @@ -377,11 +377,11 @@ int sdio_add_func(struct sdio_func *func) */ void sdio_remove_func(struct sdio_func *func) { + of_node_put(func->dev.of_node); if (!sdio_func_present(func)) return; device_del(&func->dev); - of_node_put(func->dev.of_node); put_device(&func->dev); }
If device_add() returns error in sdio_add_func(), sdio function is not presented, so the node refcount that hold in sdio_set_of_node() can not be put in sdio_remove_func() which is called from error path. Fix this by moving of_node_put() before present check in remove() function. Fixes: 25185f3f31c9 ("mmc: Add SDIO function devicetree subnode parsing") Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- drivers/mmc/core/sdio_bus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)