Re: [PATCH] omap: Properly handle resources for omap_devices

[Pantelis Antoniou]

Hi,

On Jan 7, 2013, at 10:05 PM, Tony Lindgren wrote:

> Hi,
> 
> * Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx> [130103 14:34]:
>> omap_device relies on the platform notifier callbacks managing resources
>> behind the scenes. The resources were not properly linked causing crashes
>> when removing the device.
>> 
>> Rework the resource modification code so that linking is performed properly,
>> and make sure that no resources that have no parent (which can happen for DMA
>> & IRQ resources) are ever left for cleanup by the core resource layer.
> 
> Sounds like a fix, but it's hard to figure out from the patch which parts
> are the fix. Can you please split this patch into two, first move the
> code around with no functional changes, and then a second patch to fix
> the issue?
> 
> Thanks,
> 
> Tony

OK Tony, will do.

Regards

-- Pantelis

> 
>> Signed-off-by: Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx>
>> ---
>> arch/arm/mach-omap2/omap_device.c | 232 ++++++++++++++++++++++++--------------
>> 1 file changed, 148 insertions(+), 84 deletions(-)
>> 
>> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
>> index e065daa..9f8dba1 100644
>> --- a/arch/arm/mach-omap2/omap_device.c
>> +++ b/arch/arm/mach-omap2/omap_device.c
>> @@ -494,30 +494,156 @@ static int omap_device_fill_resources(struct omap_device *od,
>> }
>> 
>> /**
>> - * _od_fill_dma_resources - fill in array of struct resource with dma resources
>> + * omap_device_fixup_resources - Fix platform device resources
>>  * @od: struct omap_device *
>> - * @res: pointer to an array of struct resource to be filled in
>> - *
>> - * Populate one or more empty struct resource pointed to by @res with
>> - * the dma resource data for this omap_device @od.  Used by
>> - * omap_device_alloc() after calling omap_device_count_resources().
>> - *
>> - * Ideally this function would not be needed at all.  If we have
>> - * mechanism to get dma resources from DT.
>>  *
>> - * Returns 0.
>> + * Fixup the platform device resources so that the resources
>> + * from the hwmods are included for.
>>  */
>> -static int _od_fill_dma_resources(struct omap_device *od,
>> -				      struct resource *res)
>> +static int omap_device_fixup_resources(struct omap_device *od)
>> {
>> -	int i, r;
>> +	struct platform_device *pdev = od->pdev;
>> +	int i, j, ret, res_count;
>> +	struct resource *res, *r, *rnew, *rn;
>> +	unsigned long type;
>> 
>> -	for (i = 0; i < od->hwmods_cnt; i++) {
>> -		r = omap_hwmod_fill_dma_resources(od->hwmods[i], res);
>> -		res += r;
>> +	/*
>> +	 * DT Boot:
>> +	 *   OF framework will construct the resource structure (currently
>> +	 *   does for MEM & IRQ resource) and we should respect/use these
>> +	 *   resources, killing hwmod dependency.
>> +	 *   If pdev->num_resources > 0, we assume that MEM & IRQ resources
>> +	 *   have been allocated by OF layer already (through DTB).
>> +	 *
>> +	 * Non-DT Boot:
>> +	 *   Here, pdev->num_resources = 0, and we should get all the
>> +	 *   resources from hwmod.
>> +	 *
>> +	 * TODO: Once DMA resource is available from OF layer, we should
>> +	 *   kill filling any resources from hwmod.
>> +	 */
>> +
>> +	/* count number of resources hwmod provides */
>> +	res_count = omap_device_count_resources(od, IORESOURCE_IRQ |
>> +					IORESOURCE_DMA | IORESOURCE_MEM);
>> +
>> +	/* if no resources from hwmod, we're done already */
>> +	if (res_count == 0)
>> +		return 0;
>> +
>> +	/* Allocate resources memory to account for all hwmod resources */
>> +	res = kzalloc(sizeof(struct resource) * res_count, GFP_KERNEL);
>> +	if (!res) {
>> +		ret = -ENOMEM;
>> +		goto fail_no_res;
>> 	}
>> 
>> +	/* fill all the resources */
>> +	ret = omap_device_fill_resources(od, res);
>> +	if (ret != 0)
>> +		goto fail_no_fill;
>> +
>> +	/*
>> +	 * If pdev->num_resources > 0, then assume that,
>> +	 * MEM and IRQ resources will only come from DT and only
>> +	 * fill DMA resource from hwmod layer.
>> +	 */
>> +	if (pdev->num_resources > 0) {
>> +
>> +		dev_dbg(&pdev->dev, "%s(): resources allocated %d hwmod #%d\n",
>> +			__func__, pdev->num_resources, res_count);
>> +
>> +		/* find number of resources needing to be inserted */
>> +		for (i = 0, j = 0, r = res; i < res_count; i++, r++) {
>> +			type = resource_type(r);
>> +			if (type == IORESOURCE_DMA)
>> +				j++;
>> +		}
>> +
>> +		/* no need to insert anything, just return */
>> +		if (j == 0) {
>> +			kfree(res);
>> +			return 0;
>> +		}
>> +
>> +		/* we need to insert j additional resources */
>> +		rnew = kzalloc(sizeof(*rnew) *
>> +				(pdev->num_resources + j), GFP_KERNEL);
>> +		if (rnew == NULL)
>> +			goto fail_no_rnew;
>> +
>> +		/*
>> +		 * Unlink any resources from any lists.
>> +		 * This is important since the copying destroys any
>> +		 * linkage
>> +		 */
>> +		for (i = 0, r = pdev->resource;
>> +				i < pdev->num_resources; i++, r++) {
>> +
>> +			if (!r->parent)
>> +				continue;
>> +
>> +			dev_dbg(&pdev->dev,
>> +					"Releasing resource %p\n", r);
>> +			release_resource(r);
>> +			r->parent = NULL;
>> +			r->sibling = NULL;
>> +			r->child = NULL;
>> +		}
>> +
>> +		memcpy(rnew, pdev->resource,
>> +				sizeof(*rnew) * pdev->num_resources);
>> +
>> +		/* now append the resources from the hwmods */
>> +		rn = rnew + pdev->num_resources;
>> +		for (i = 0, r = res; i < res_count; i++, r++) {
>> +
>> +			type = resource_type(r);
>> +			if (type != IORESOURCE_DMA)
>> +				continue;
>> +
>> +			/* append the hwmod resource */
>> +			memcpy(rn, r, sizeof(*r));
>> +
>> +			/* make sure these are zeroed out */
>> +			rn->parent = NULL;
>> +			rn->child = NULL;
>> +			rn->sibling = NULL;
>> +
>> +			rn++;
>> +		}
>> +		kfree(res);	/* we don't need res anymore */
>> +
>> +		/* this is our new resource table */
>> +		res = rnew;
>> +		res_count = j;
>> +
>> +	} else {
>> +		dev_dbg(&pdev->dev, "%s(): using resources from hwmod %d\n",
>> +			__func__, res_count);
>> +	}
>> +
>> +	ret = platform_device_add_resources(pdev, res, res_count);
>> +	kfree(res);
>> +
>> +	/* failed to add the resources? */
>> +	if (ret != 0)
>> +		return ret;
>> +
>> +	/* finally link all the resources again */
>> +	ret = platform_device_link_resources(pdev);
>> +	if (ret != 0)
>> +		return ret;
>> +
>> 	return 0;
>> +
>> +fail_no_rnew:
>> +	/* nothing */
>> +fail_no_fill:
>> +	kfree(res);
>> +
>> +fail_no_res:
>> +	return ret;
>> }
>> 
>> /**
>> @@ -541,9 +667,8 @@ struct omap_device *omap_device_alloc(struct platform_device *pdev,
>> {
>> 	int ret = -ENOMEM;
>> 	struct omap_device *od;
>> -	struct resource *res = NULL;
>> -	int i, res_count;
>> 	struct omap_hwmod **hwmods;
>> +	int i;
>> 
>> 	od = kzalloc(sizeof(struct omap_device), GFP_KERNEL);
>> 	if (!od) {
>> @@ -553,79 +678,18 @@ struct omap_device *omap_device_alloc(struct platform_device *pdev,
>> 	od->hwmods_cnt = oh_cnt;
>> 
>> 	hwmods = kmemdup(ohs, sizeof(struct omap_hwmod *) * oh_cnt, GFP_KERNEL);
>> -	if (!hwmods)
>> +	if (!hwmods) {
>> +		ret = -ENOMEM;
>> 		goto oda_exit2;
>> +	}
>> 
>> 	od->hwmods = hwmods;
>> 	od->pdev = pdev;
>> 
>> -	/*
>> -	 * Non-DT Boot:
>> -	 *   Here, pdev->num_resources = 0, and we should get all the
>> -	 *   resources from hwmod.
>> -	 *
>> -	 * DT Boot:
>> -	 *   OF framework will construct the resource structure (currently
>> -	 *   does for MEM & IRQ resource) and we should respect/use these
>> -	 *   resources, killing hwmod dependency.
>> -	 *   If pdev->num_resources > 0, we assume that MEM & IRQ resources
>> -	 *   have been allocated by OF layer already (through DTB).
>> -	 *   As preparation for the future we examine the OF provided resources
>> -	 *   to see if we have DMA resources provided already. In this case
>> -	 *   there is no need to update the resources for the device, we use the
>> -	 *   OF provided ones.
>> -	 *
>> -	 * TODO: Once DMA resource is available from OF layer, we should
>> -	 *   kill filling any resources from hwmod.
>> -	 */
>> -	if (!pdev->num_resources) {
>> -		/* Count all resources for the device */
>> -		res_count = omap_device_count_resources(od, IORESOURCE_IRQ |
>> -							    IORESOURCE_DMA |
>> -							    IORESOURCE_MEM);
>> -	} else {
>> -		/* Take a look if we already have DMA resource via DT */
>> -		for (i = 0; i < pdev->num_resources; i++) {
>> -			struct resource *r = &pdev->resource[i];
>> -
>> -			/* We have it, no need to touch the resources */
>> -			if (r->flags == IORESOURCE_DMA)
>> -				goto have_everything;
>> -		}
>> -		/* Count only DMA resources for the device */
>> -		res_count = omap_device_count_resources(od, IORESOURCE_DMA);
>> -		/* The device has no DMA resource, no need for update */
>> -		if (!res_count)
>> -			goto have_everything;
>> -
>> -		res_count += pdev->num_resources;
>> -	}
>> -
>> -	/* Allocate resources memory to account for new resources */
>> -	res = kzalloc(sizeof(struct resource) * res_count, GFP_KERNEL);
>> -	if (!res)
>> -		goto oda_exit3;
>> -
>> -	if (!pdev->num_resources) {
>> -		dev_dbg(&pdev->dev, "%s: using %d resources from hwmod\n",
>> -			__func__, res_count);
>> -		omap_device_fill_resources(od, res);
>> -	} else {
>> -		dev_dbg(&pdev->dev,
>> -			"%s: appending %d DMA resources from hwmod\n",
>> -			__func__, res_count - pdev->num_resources);
>> -		memcpy(res, pdev->resource,
>> -		       sizeof(struct resource) * pdev->num_resources);
>> -		_od_fill_dma_resources(od, &res[pdev->num_resources]);
>> -	}
>> -
>> -	ret = platform_device_add_resources(pdev, res, res_count);
>> -	kfree(res);
>> -
>> -	if (ret)
>> +	ret = omap_device_fixup_resources(od);
>> +	if (ret != 0)
>> 		goto oda_exit3;
>> 
>> -have_everything:
>> 	if (!pm_lats) {
>> 		pm_lats = omap_default_latency;
>> 		pm_lats_cnt = ARRAY_SIZE(omap_default_latency);
>> -- 
>> 1.7.12
>> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/