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

[Masahiro Yamada]

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.


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.


Best Regards
Masahiro Yamada