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-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
