On 02/03/2017 02:02 AM, Stuart Yoder wrote: > > >> -----Original Message----- >> From: laurentiu.tu...@nxp.com [mailto:laurentiu.tu...@nxp.com] >> Sent: Wednesday, February 01, 2017 5:43 AM >> To: gre...@linuxfoundation.org >> Cc: de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org; ag...@suse.de; >> a...@arndb.de; Ioana >> Ciornei <ioana.cior...@nxp.com>; Ruxandra Ioana Radulescu >> <ruxandra.radule...@nxp.com>; Bharat Bhushan >> <bharat.bhus...@nxp.com>; Stuart Yoder <stuart.yo...@nxp.com>; Catalin >> Horghidan >> <catalin.horghi...@nxp.com>; Leo Li <leoyang...@nxp.com>; Roy Pledge >> <roy.ple...@nxp.com>; Laurentiu >> Tudor <laurentiu.tu...@nxp.com> >> Subject: [PATCH 4/9] staging: fsl-mc: don't use devres api for refcounted >> objects >> >> From: Laurentiu Tudor <laurentiu.tu...@nxp.com> >> >> Mixing two memory management systems, in this case >> managed device resource api and refcounted objects >> is a bad idea. Lifetime of an object is controlled >> by its refcount so allocating it with other apis >> that have their own lifetime control is not ok. >> Drop devm_*() apis in favor of plain allocations. >> >> While at it, let's drop the slab cache for objects >> until we actually have proof that it improves >> performance. This allows for some code cleanup. > > Those 2 changes (dropping devm_* apis and slab cache > changes) are 2 orthogonal things, right? It would > be better to split those into separate patches. Mixing > them makes it harder to review. > >> Signed-off-by: Laurentiu Tudor <laurentiu.tu...@nxp.com> >> --- >> drivers/staging/fsl-mc/bus/fsl-mc-bus.c | 43 >> +++++---------------------------- >> 1 file changed, 6 insertions(+), 37 deletions(-) >> >> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >> b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >> index 6601bde..c493427 100644 >> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c >> @@ -27,8 +27,6 @@ >> #include "fsl-mc-private.h" >> #include "dprc-cmd.h" >> >> -static struct kmem_cache *mc_dev_cache; >> - >> /** >> * Default DMA mask for devices on a fsl-mc bus >> */ >> @@ -422,17 +420,12 @@ bool fsl_mc_is_root_dprc(struct device *dev) >> static void fsl_mc_device_release(struct device *dev) >> { >> struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev); >> - struct fsl_mc_bus *mc_bus = NULL; >> >> kfree(mc_dev->regions); >> - >> - if (strcmp(mc_dev->obj_desc.type, "dprc") == 0) >> - mc_bus = to_fsl_mc_bus(mc_dev); >> - >> - if (mc_bus) >> - devm_kfree(mc_dev->dev.parent, mc_bus); >> + if (!strcmp(mc_dev->obj_desc.type, "dprc")) >> + kfree(to_fsl_mc_bus(mc_dev)); >> else >> - kmem_cache_free(mc_dev_cache, mc_dev); >> + kfree(mc_dev); >> } >> >> /** >> @@ -457,7 +450,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc, >> /* >> * Allocate an MC bus device object: >> */ >> - mc_bus = devm_kzalloc(parent_dev, sizeof(*mc_bus), GFP_KERNEL); >> + mc_bus = kzalloc(sizeof(*mc_bus), GFP_KERNEL); >> if (!mc_bus) >> return -ENOMEM; >> >> @@ -466,7 +459,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc, >> /* >> * Allocate a regular fsl_mc_device object: >> */ >> - mc_dev = kmem_cache_zalloc(mc_dev_cache, GFP_KERNEL); >> + mc_dev = kzalloc(sizeof(*mc_dev), GFP_KERNEL); >> if (!mc_dev) >> return -ENOMEM; >> } >> @@ -561,10 +554,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc, >> >> error_cleanup_dev: >> kfree(mc_dev->regions); >> - if (mc_bus) >> - devm_kfree(parent_dev, mc_bus); >> - else >> - kmem_cache_free(mc_dev_cache, mc_dev); >> + kfree(mc_bus ? (void *)mc_bus : (void *)mc_dev); >> >> return error; >> } >> @@ -578,23 +568,11 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc, >> */ >> void fsl_mc_device_remove(struct fsl_mc_device *mc_dev) >> { >> - struct fsl_mc_bus *mc_bus = NULL; >> - >> - kfree(mc_dev->regions); >> - >> /* >> * The device-specific remove callback will get invoked by device_del() >> */ >> device_del(&mc_dev->dev); >> put_device(&mc_dev->dev); >> - >> - if (strcmp(mc_dev->obj_desc.type, "dprc") == 0) >> - mc_bus = to_fsl_mc_bus(mc_dev); >> - >> - if (mc_bus) >> - devm_kfree(mc_dev->dev.parent, mc_bus); >> - else >> - kmem_cache_free(mc_dev_cache, mc_dev); >> } > > The above changes in fsl_mc_device_remove(), I think > actually belong in patch #3 of this series. >
Agree. I think I end up doing a double free here. --- Thanks & Best Regards, Laurentiu _______________________________________________ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
