Messages in this thread | | | From | Stuart Yoder <> | Subject | RE: [PATCH 4/9] staging: fsl-mc: don't use devres api for refcounted objects | Date | Fri, 3 Feb 2017 00:02:55 +0000 |
| |
> -----Original Message----- > From: laurentiu.tudor@nxp.com [mailto:laurentiu.tudor@nxp.com] > Sent: Wednesday, February 01, 2017 5:43 AM > To: gregkh@linuxfoundation.org > Cc: devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org; agraf@suse.de; arnd@arndb.de; Ioana > Ciornei <ioana.ciornei@nxp.com>; Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>; Bharat Bhushan > <bharat.bhushan@nxp.com>; Stuart Yoder <stuart.yoder@nxp.com>; Catalin Horghidan > <catalin.horghidan@nxp.com>; Leo Li <leoyang.li@nxp.com>; Roy Pledge <roy.pledge@nxp.com>; Laurentiu > Tudor <laurentiu.tudor@nxp.com> > Subject: [PATCH 4/9] staging: fsl-mc: don't use devres api for refcounted objects > > From: Laurentiu Tudor <laurentiu.tudor@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.tudor@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.
Stuart
| |