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

[Laurentiu Tudor <laurentiu.tudor@nxp.com>]

On 02/03/2017 02:02 AM, Stuart Yoder wrote:
>
>
>> -----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.
>

Agree. I think I end up doing a double free here.

---
Thanks & Best Regards, Laurentiu