linux-kernel - RE: [PATCH 4/9] staging: fsl-mc: don't use devres api for refcounted objects

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <VI1PR0401MB2638AD1CA95E430AA25F346B8D4F0@VI1PR0401MB2638.eurprd04.prod.outlook.com>
Date:   Fri, 3 Feb 2017 00:02:55 +0000
From:   Stuart Yoder <stuart.yoder@....com>
To:     Laurentiu Tudor <laurentiu.tudor@....com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC:     "devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "agraf@...e.de" <agraf@...e.de>, "arnd@...db.de" <arnd@...db.de>,
        "Ioana Ciornei" <ioana.ciornei@....com>,
        Ruxandra Ioana Radulescu <ruxandra.radulescu@....com>,
        Bharat Bhushan <bharat.bhushan@....com>,
        Catalin Horghidan <catalin.horghidan@....com>,
        Leo Li <leoyang.li@....com>, Roy Pledge <roy.pledge@....com>,
        Laurentiu Tudor <laurentiu.tudor@....com>
Subject: RE: [PATCH 4/9] staging: fsl-mc: don't use devres api for refcounted
 objects



> -----Original Message-----
> From: laurentiu.tudor@....com [mailto:laurentiu.tudor@....com]
> Sent: Wednesday, February 01, 2017 5:43 AM
> To: gregkh@...uxfoundation.org
> Cc: devel@...verdev.osuosl.org; linux-kernel@...r.kernel.org; agraf@...e.de; arnd@...db.de; Ioana
> Ciornei <ioana.ciornei@....com>; Ruxandra Ioana Radulescu <ruxandra.radulescu@....com>; Bharat Bhushan
> <bharat.bhushan@....com>; Stuart Yoder <stuart.yoder@....com>; Catalin Horghidan
> <catalin.horghidan@....com>; Leo Li <leoyang.li@....com>; Roy Pledge <roy.pledge@....com>; Laurentiu
> Tudor <laurentiu.tudor@....com>
> Subject: [PATCH 4/9] staging: fsl-mc: don't use devres api for refcounted objects
> 
> From: Laurentiu Tudor <laurentiu.tudor@....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@....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