Re: [PATCH v5 2/4] base/platform: Continue on insert_resource() error

[Rob Herring <robherring2@gmail.com>]

On Thu, Jun 4, 2015 at 2:54 AM, Grant Likely <grant.likely@linaro.org> wrote:
> On Tue, 26 May 2015 09:31:24 +0200
> , Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>  wrote:
>> insert_resource() can fail when the resource added  overlaps
>> (partially or fully) with another.
>>
>> Device tree and AMBA devices may contain resources that overlap, so they
>> could not call platform_device_add (see 02bbde7849e6 ('Revert "of:
>> use platform_device_add"'))"
>>
>> On the other hand, device trees are released using
>> platform_device_unregister(). This function calls platform_device_del(),
>> which calls release_resource(), that crashes when the resource has not
>> been added with with insert_resource. This was not an issue when the
>> device tree could not be modified online, but this is not the case
>> anymore.
>>
>> This patch let the flow continue when there is an insert error, after
>> notifying the user with a dev_err(). r->parent is set to NULL, so
>> platform_device_del() knows that the resource was not added, and
>> therefore it should not be released.
>>
>> Acked-by: Rob Herring <robh@kernel.org>
>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>> ---
>>  drivers/base/platform.c | 26 +++++++++++++++-----------
>>  1 file changed, 15 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>> index 46a56f694cec..5a29387e5ff6 100644
>> --- a/drivers/base/platform.c
>> +++ b/drivers/base/platform.c
>> @@ -332,7 +332,7 @@ int platform_device_add(struct platform_device *pdev)
>>                */
>>               ret = ida_simple_get(&platform_devid_ida, 0, 0, GFP_KERNEL);
>>               if (ret < 0)
>> -                     goto err_out;
>> +                     return ret;
>>               pdev->id = ret;
>>               pdev->id_auto = true;
>>               dev_set_name(&pdev->dev, "%s.%d.auto", pdev->name, pdev->id);
>> @@ -340,7 +340,7 @@ int platform_device_add(struct platform_device *pdev)
>>       }
>>
>>       for (i = 0; i < pdev->num_resources; i++) {
>> -             struct resource *p, *r = &pdev->resource[i];
>> +             struct resource *conflict, *p, *r = &pdev->resource[i];
>>               unsigned long type = resource_type(r);
>>
>>               if (r->name == NULL)
>> @@ -357,11 +357,14 @@ int platform_device_add(struct platform_device *pdev)
>>                               p = &ioport_resource;
>>               }
>>
>> -             if (insert_resource(p, r)) {
>> -                     dev_err(&pdev->dev, "failed to claim resource %d\n", i);
>> -                     ret = -EBUSY;
>> -                     goto failed;
>> -             }
>> +             conflict = insert_resource_conflict(p, r);
>> +             if (!conflict)
>> +                     continue;
>> +
>> +             dev_err(&pdev->dev,
>> +                     "ignoring resource %pR (conflicts with %s %pR)\n",
>> +                     r, conflict->name, conflict);
>> +             p->parent = NULL;
>
> I'm pretty sure this is going to break some platforms. I described it in
> my earlier email today, but I'll summarize here too since this is the
> latest patch set.
>
> Making this change allows the registration of devices to continue, but
> it will still break device drivers that do a request_resource() on a
> region that another device managed to claim with insert_resource(). The
> only way around this is to not do insert_resource() at all, or to remove
> the request_resource() from all drivers (not feasible).

Do we have some clue as to which platforms have problems? I seem to
recall some i.MX platform.

> I think we have to deal with it by making resource insertion optional.
> I'd like to make the default be to do the insertion, and be able to
> blacklist platforms that have issues.

Yes, otherwise we'll never put this issue to bed.

Rob