[U-Boot] [PATCH 3/4] dm: do not check dm_root before searching in uclass_root

[Simon Glass]

Hi Masahiro,

On 28 September 2014 20:39, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
> Hi Simon,
>
>
> On Sun, 28 Sep 2014 10:18:59 -0600
> Simon Glass <sjg at chromium.org> wrote:
>
>> Hi Masahiro,
>>
>> On 28 September 2014 09:54, Masahiro YAMADA <yamada.m at jp.panasonic.com> wrote:
>> > Simon,
>> >
>> >
>> > 2014-09-29 0:17 GMT+09:00 Simon Glass <sjg at chromium.org>:
>> >> Hi Masahiro,
>> >>
>> >> On 28 September 2014 07:52, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
>> >>> The function uclass_find() looks for a uclass in the linked
>> >>> list of gd->uclass_root; gd->dm_root has nothing to do with
>> >>> gd->uclass_root.  Remove this confusing code.
>> >>>
>> >>> Signed-off-by: Masahiro Yamada <yamada.m at jp.panasonic.com>
>> >>> ---
>> >>>
>> >>>  drivers/core/uclass.c | 2 --
>> >>>  1 file changed, 2 deletions(-)
>> >>>
>> >>> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
>> >>> index 901b06e..74df613 100644
>> >>> --- a/drivers/core/uclass.c
>> >>> +++ b/drivers/core/uclass.c
>> >>> @@ -23,8 +23,6 @@ struct uclass *uclass_find(enum uclass_id key)
>> >>>  {
>> >>>         struct uclass *uc;
>> >>>
>> >>> -       if (!gd->dm_root)
>> >>> -               return NULL;
>> >>>         /*
>> >>>          * TODO(sjg at chromium.org): Optimise this, perhaps moving the found
>> >>>          * node to the start of the list, or creating a linear array mapping
>> >>
>> >> This came in in commit:
>> >>
>> >>  c910e2e dm: Avoid accessing uclasses before they are ready
>> >>
>> >> Please see that (and the test that was added) for an explanation.
>> >
>> >
>> > Commit c910e2e says:
>> >
>> > dm: Avoid accessing uclasses before they are ready
>> >
>> > Don't allow access to uclasses before they have been initialised.
>> >
>> >
>> >
>> > I still don't get it.
>> > The log did not help me because it is not saying 'why'.
>> >
>> > What kind problem would happen if this check was dropped?
>> >
>> > gd->dm_root is set when the root device is bound.
>> > At the first call of uclass_find(), it is true gd->dm_root is NULL,
>> > but gd->uclass_root is also empty.
>>
>> But 'empty' means the list is empty. For it to be empty it must be
>> initialised. But if you call the function before this list init
>> happens, then it will just crash.
>>
>> We can use dm_root as an indicator that init has happened.
>>
>> >
>> > This function, anyway, returns NULL.
>> > The behavior is still the same, I think.
>>
>> You can try this by removing that code and seeing what the test does
>> (dm_test_uclass_before_ready()).
>>
>
>
>
> So, you are checking gd->dm_root
> to make sure gd->uclass_root has been initialized.
>
> Understood what you are trying to do, but it is unfortunate.
>

Kind of. Actually we are allowed to add uclasses before gd->dm_root is
initialised. The way it works at the moment is:

- we create a root uclass
- we create a root device
- gd->dm_root is initialised

So you can see that dm_root is set up after the first uclass.

My check was just to make sure that nothing tried to look up uclasses
too early. It's not a very good check. It would be better to have a
separate flag but I feel I have added enough flags already. I don't
think this is a big problem but we can always adjust it if we need to.

>
> OK, let's see what will happen if uclass_get() is called before dm_init().
>
>
> It is true uclass_find() will return NULL safely,
> but uclass_add() will crash, I think.
>
> It looks like uclass_add() is trying to add a created uclass
> to the uninitialized gd->uclass_root.
>
> here,
>         list_add(&uc->sibling_node, &DM_UCLASS_ROOT_NON_CONST);
>
> I think it is as dangeous as searching something in an uninitialized list.
>
>
> Unfortunately, we cannot use LIST_HEAD(uclass_root)
> because it is in the global data.

Bear in mind that drive rmodel is set up very very early, so we mostly
need the driver model code to guard against itself, while it sets
things up.

Regards,
Simon