On Thu, Jun 4, 2015 at 2:54 AM, Grant Likely <grant.likely@xxxxxxxxxx> wrote:
> On Tue, 26 May 2015 09:31:24 +0200
> , Ricardo Ribalda Delgado <ricardo.ribalda@xxxxxxxxx>
> 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@xxxxxxxxxx>
>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@xxxxxxxxx>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
