On 20-10-02 07:15, Sascha Hauer wrote:
> On Wed, Sep 30, 2020 at 10:47:09AM +0200, Marco Felsch wrote:
> > After checking the linux history I found no state where it was explicite
> > allowed to register a platform device twice. In the early days there was
> > an bug and in some cases linux did populated the same device again. This
> > was fixed in 2014 by commit c6e126de43e7 ("of: Keep track of populated
> > platform devices") and since then this wasn't possible anymore.
> >
> > The for_each_device() solution had two issues:
> > 1) We are looping over all devices each time
> > of_platform_device_create() gets called.
> > 2) The logic around 'if (!dev->resource)' then 'continue' seems to be
> > broken. This suggest that we can populate a device without
> > resource(s) first and adding resource(s) later which isn't the case.
> > Instead the current logic will populate the same device again but
> > this time (maybe) with resources. So the caller can add the same
> > device as many times as possible without resources.
> > 3) The device_node gets modified if the new device_node to add has the
> > same resources region.
> >
> > Add a device reference to the device_node to track an registered device
> > seems to:
> > 1) Simplify the code.
> > 2) Align the logic with the current linux state with the exception that
> > we are returning the already created device.
> > 3) Avoid looping over each device all the time.
> > 4) Fix the memory leak which can occure if of_platform_device_create()
> > gets called for the same device without any resources.
> >
> > I added the missing check for amba devices too while on it.
> >
> > Signed-off-by: Marco Felsch <m.felsch@xxxxxxxxxxxxxx>
> > ---
> > v2:
> > - new patch
> >
> > drivers/of/platform.c | 51 +++++++++++++++++--------------------------
> > include/of.h | 1 +
> > 2 files changed, 21 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index 21c7cce1a5..01de6f98af 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -101,11 +101,18 @@ struct device_d *of_platform_device_create(struct device_node *np,
> > struct device_d *dev;
> > struct resource *res = NULL, temp_res;
> > resource_size_t resinval;
> > - int i, j, ret, num_reg = 0, match;
> > + int i, ret, num_reg = 0;
> >
> > if (!of_device_is_available(np))
> > return NULL;
> >
> > + /*
> > + * Linux uses the OF_POPULATED flag to skip already populated/created
> > + * devices.
> > + */
> > + if (np->dev)
> > + return np->dev;
> > +
> > /* count the io resources */
> > if (of_can_translate_address(np))
> > while (of_address_to_resource(np, num_reg, &temp_res) == 0)
> > @@ -121,35 +128,6 @@ struct device_d *of_platform_device_create(struct device_node *np,
> > return NULL;
> > }
> > }
> > -
> > - /*
> > - * A device may already be registered as platform_device.
> > - * Instead of registering the same device again, just
> > - * add this node to the existing device.
> > - */
> > - for_each_device(dev) {
> > - if (!dev->resource)
> > - continue;
> > -
> > - for (i = 0, match = 0; i < num_reg; i++)
> > - for (j = 0; j < dev->num_resources; j++)
> > - if (dev->resource[j].start ==
> > - res[i].start &&
> > - dev->resource[j].end ==
> > - res[i].end) {
> > - match++;
> > - break;
> > - }
> > -
> > - /* check if all address resources match */
> > - if (match == num_reg) {
> > - debug("connecting %s to %s\n",
> > - np->name, dev_name(dev));
> > - dev->device_node = np;
> > - free(res);
> > - return dev;
> > - }
> > - }
>
> This code is for the case when platform code has registered a device
> using platform_device_register() which is also in the device tree. When
> the device tree gets populated afterwards then we would have two
> devices for the same resources.
Ah okay, thanks for the clarification :) I see now.
> The code you are removing here catches
> this case and instead of registering a second device for the same
> resources, the existing device only gets the pointer to the corresponding
> node.
Do you think that this use-case is still valid?
> That said we can probably remove this code which was useful in the early
> barebox device tree days, but the code you are adding doesn't replace
> it. This should really be two patches.
Make sense with your explanation.
Regards,
Marco
_______________________________________________
barebox mailing list
barebox@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
