[U-Boot] [PATCH 0/4] dm: a collection of minor fixes in Driver-Model

[U-Boot] [PATCH 0/4] dm: a collection of minor fixes in Driver-Model

[Masahiro Yamada]

Masahiro Yamada (4):
  dm: fix comments
  dm: do not check the existence of uclass operation
  dm: do not check dm_root before searching in uclass_root
  dm: simplify the loop in lists_driver_lookup_name()

 drivers/core/lists.c  | 9 +--------
 drivers/core/uclass.c | 6 ------
 include/dm/lists.h    | 4 ++--
 include/dm/platdata.h | 2 +-
 4 files changed, 4 insertions(+), 17 deletions(-)

-- 
1.9.1

[U-Boot] [PATCH 1/4] dm: fix comments

[Masahiro Yamada]

The struct udevice stands for a device, not a driver.
The driver_info.name is a driver's name, which is referenced
to bind devices.

Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
---

 include/dm/lists.h    | 4 ++--
 include/dm/platdata.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/dm/lists.h b/include/dm/lists.h
index 2356895..704e33e 100644
--- a/include/dm/lists.h
+++ b/include/dm/lists.h
@@ -38,7 +38,7 @@ struct uclass_driver *lists_uclass_lookup(enum uclass_id id);
  * This searches the U_BOOT_DEVICE() structures and creates new devices for
  * each one. The devices will have @parent as their parent.
  *
- * @parent: parent driver (root)
+ * @parent: parent device (root)
  * @early_only: If true, bind only drivers with the DM_INIT_F flag. If false
  * bind all drivers.
  */
@@ -50,7 +50,7 @@ int lists_bind_drivers(struct udevice *parent, bool pre_reloc_only);
  * This creates a new device bound to the given device tree node, with
  * @parent as its parent.
  *
- * @parent: parent driver (root)
+ * @parent: parent device (root)
  * @blob: device tree blob
  * @offset: offset of this device tree node
  * @devp: if non-NULL, returns a pointer to the bound device
diff --git a/include/dm/platdata.h b/include/dm/platdata.h
index 2bc8b14..7716f19 100644
--- a/include/dm/platdata.h
+++ b/include/dm/platdata.h
@@ -14,7 +14,7 @@
 /**
  * struct driver_info - Information required to instantiate a device
  *
- * @name:	Device name
+ * @name:	Driver name
  * @platdata:	Driver-specific platform data
  */
 struct driver_info {
-- 
1.9.1

[U-Boot] [PATCH 2/4] dm: do not check the existence of uclass operation

[Masahiro Yamada]

The function uclass_add() checks uc_drv->ops as follows:

        if (uc_drv->ops) {
                dm_warn("No ops for uclass id %d\n", id);
                return -EINVAL;
        }

It seems odd because it warns "No ops" when uc_drv->ops has
non-NULL pointer.  (Looks opposite.)

Anyway, most of UCLASS_DRIVER entries have no .ops member.
This check makes no sense.

Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
---

 drivers/core/uclass.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
index 61ca17e..901b06e 100644
--- a/drivers/core/uclass.c
+++ b/drivers/core/uclass.c
@@ -60,10 +60,6 @@ static int uclass_add(enum uclass_id id, struct uclass **ucp)
 			id);
 		return -ENOENT;
 	}
-	if (uc_drv->ops) {
-		dm_warn("No ops for uclass id %d\n", id);
-		return -EINVAL;
-	}
 	uc = calloc(1, sizeof(*uc));
 	if (!uc)
 		return -ENOMEM;
-- 
1.9.1

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

[Masahiro Yamada]

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@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
-- 
1.9.1

[U-Boot] [PATCH 4/4] dm: simplify the loop in lists_driver_lookup_name()

[Masahiro Yamada]

        if (strncmp(name, entry->name, len))
                continue;

        /* Full match */
        if (len == strlen(entry->name))
                return entry;

is equivalent to:

        if (!strcmp(name, entry->name))
                return entry;

The latter is simpler.

Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
---

 drivers/core/lists.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/core/lists.c b/drivers/core/lists.c
index 699f94b..3a1ea85 100644
--- a/drivers/core/lists.c
+++ b/drivers/core/lists.c
@@ -24,19 +24,12 @@ struct driver *lists_driver_lookup_name(const char *name)
 		ll_entry_start(struct driver, driver);
 	const int n_ents = ll_entry_count(struct driver, driver);
 	struct driver *entry;
-	int len;
 
 	if (!drv || !n_ents)
 		return NULL;
 
-	len = strlen(name);
-
 	for (entry = drv; entry != drv + n_ents; entry++) {
-		if (strncmp(name, entry->name, len))
-			continue;
-
-		/* Full match */
-		if (len == strlen(entry->name))
+		if (!strcmp(name, entry->name))
 			return entry;
 	}
 
-- 
1.9.1

[U-Boot] [PATCH 1/4] dm: fix comments

[Simon Glass]

On 28 September 2014 07:52, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>
> The struct udevice stands for a device, not a driver.
> The driver_info.name is a driver's name, which is referenced
> to bind devices.
>
> Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 2/4] dm: do not check the existence of uclass operation

[Simon Glass]

On 28 September 2014 07:52, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
> The function uclass_add() checks uc_drv->ops as follows:
>
>         if (uc_drv->ops) {
>                 dm_warn("No ops for uclass id %d\n", id);
>                 return -EINVAL;
>         }
>
> It seems odd because it warns "No ops" when uc_drv->ops has
> non-NULL pointer.  (Looks opposite.)
>
> Anyway, most of UCLASS_DRIVER entries have no .ops member.
> This check makes no sense.
>
> Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>

Acked-by: Simon Glass <sjg@chromium.org>

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

[Simon Glass]

Hi Masahiro,

On 28 September 2014 07:52, Masahiro Yamada <yamada.m@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@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.

Regards,
Simon

[U-Boot] [PATCH 4/4] dm: simplify the loop in lists_driver_lookup_name()

[Simon Glass]

Hi Masahiro,

On 28 September 2014 07:52, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>         if (strncmp(name, entry->name, len))
>                 continue;
>
>         /* Full match */
>         if (len == strlen(entry->name))
>                 return entry;
>
> is equivalent to:
>
>         if (!strcmp(name, entry->name))
>                 return entry;
>
> The latter is simpler.
>
> Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
> ---
>
>  drivers/core/lists.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/core/lists.c b/drivers/core/lists.c
> index 699f94b..3a1ea85 100644
> --- a/drivers/core/lists.c
> +++ b/drivers/core/lists.c
> @@ -24,19 +24,12 @@ struct driver *lists_driver_lookup_name(const char *name)
>                 ll_entry_start(struct driver, driver);
>         const int n_ents = ll_entry_count(struct driver, driver);
>         struct driver *entry;
> -       int len;
>
>         if (!drv || !n_ents)
>                 return NULL;
>
> -       len = strlen(name);
> -
>         for (entry = drv; entry != drv + n_ents; entry++) {
> -               if (strncmp(name, entry->name, len))
> -                       continue;
> -
> -               /* Full match */
> -               if (len == strlen(entry->name))
> +               if (!strcmp(name, entry->name))
>                         return entry;
>         }

The discussion on this was here:


>
> --
> 1.9.1
>

[U-Boot] [PATCH 4/4] dm: simplify the loop in lists_driver_lookup_name()

[Simon Glass]

Hi Masahiro,

On 28 September 2014 07:52, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>         if (strncmp(name, entry->name, len))
>                 continue;
>
>         /* Full match */
>         if (len == strlen(entry->name))
>                 return entry;
>
> is equivalent to:
>
>         if (!strcmp(name, entry->name))
>                 return entry;
>
> The latter is simpler.
>
> Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
> ---
>
>  drivers/core/lists.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/core/lists.c b/drivers/core/lists.c
> index 699f94b..3a1ea85 100644
> --- a/drivers/core/lists.c
> +++ b/drivers/core/lists.c
> @@ -24,19 +24,12 @@ struct driver *lists_driver_lookup_name(const char *name)
>                 ll_entry_start(struct driver, driver);
>         const int n_ents = ll_entry_count(struct driver, driver);
>         struct driver *entry;
> -       int len;
>
>         if (!drv || !n_ents)
>                 return NULL;
>
> -       len = strlen(name);
> -
>         for (entry = drv; entry != drv + n_ents; entry++) {
> -               if (strncmp(name, entry->name, len))
> -                       continue;
> -
> -               /* Full match */
> -               if (len == strlen(entry->name))
> +               if (!strcmp(name, entry->name))
>                         return entry;
>         }

The discussion on this was here:

http://lists.denx.de/pipermail/u-boot/2014-September/189336.html

I don't see a lot of value in the extra code, so I think we should
take this patch. If it is found to be a problem, we can go back to the
defensive code and add a test case so it is clear what exactly we are
defending against.

Acked-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

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

[Masahiro YAMADA]

Simon,


2014-09-29 0:17 GMT+09:00 Simon Glass <sjg@chromium.org>:
> Hi Masahiro,
>
> On 28 September 2014 07:52, Masahiro Yamada <yamada.m@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@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.

This function, anyway, returns NULL.
The behavior is still the same, I think.


-- 
Best Regards
Masahiro Yamada

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

[Simon Glass]

Hi Masahiro,

On 28 September 2014 09:54, Masahiro YAMADA <yamada.m@jp.panasonic.com> wrote:
> Simon,
>
>
> 2014-09-29 0:17 GMT+09:00 Simon Glass <sjg@chromium.org>:
>> Hi Masahiro,
>>
>> On 28 September 2014 07:52, Masahiro Yamada <yamada.m@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@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()).

Regards,
Simon

[U-Boot] [PATCH 4/4] dm: simplify the loop in lists_driver_lookup_name()

[Igor Grinberg]

Hi Masahiro, Simon,

On 09/28/14 18:22, Simon Glass wrote:
> Hi Masahiro,
> 
> On 28 September 2014 07:52, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>>         if (strncmp(name, entry->name, len))
>>                 continue;
>>
>>         /* Full match */
>>         if (len == strlen(entry->name))
>>                 return entry;
>>
>> is equivalent to:
>>
>>         if (!strcmp(name, entry->name))
>>                 return entry;
>>
>> The latter is simpler.
>>
>> Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
>> ---
>>
>>  drivers/core/lists.c | 9 +--------
>>  1 file changed, 1 insertion(+), 8 deletions(-)
>>
>> diff --git a/drivers/core/lists.c b/drivers/core/lists.c
>> index 699f94b..3a1ea85 100644
>> --- a/drivers/core/lists.c
>> +++ b/drivers/core/lists.c
>> @@ -24,19 +24,12 @@ struct driver *lists_driver_lookup_name(const char *name)
>>                 ll_entry_start(struct driver, driver);
>>         const int n_ents = ll_entry_count(struct driver, driver);
>>         struct driver *entry;
>> -       int len;
>>
>>         if (!drv || !n_ents)
>>                 return NULL;
>>
>> -       len = strlen(name);
>> -
>>         for (entry = drv; entry != drv + n_ents; entry++) {
>> -               if (strncmp(name, entry->name, len))
>> -                       continue;
>> -
>> -               /* Full match */
>> -               if (len == strlen(entry->name))
>> +               if (!strcmp(name, entry->name))
>>                         return entry;
>>         }
> 
> The discussion on this was here:
> 
> http://lists.denx.de/pipermail/u-boot/2014-September/189336.html
> 
> I don't see a lot of value in the extra code, so I think we should
> take this patch. If it is found to be a problem, we can go back to the
> defensive code and add a test case so it is clear what exactly we are
> defending against.
> 
> Acked-by: Simon Glass <sjg@chromium.org>
> 

Following the discussion referenced above, we have here a classic
case of C language strings problem.
One can dig about it all over the Internet (for example here [1]).
I don't think we will invent a solution for that problem here.
Also we are not about to abandon the C language...
So, unless someone comes out with a real case to solve, I think
we should merge this.

Acked-by: Igor Grinberg <grinberg@compulab.co.il>

[1] http://www.sei.cmu.edu/library/abstracts/news-at-sei/feature120061.cfm



-- 
Regards,
Igor.

[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@chromium.org> wrote:

> Hi Masahiro,
> 
> On 28 September 2014 09:54, Masahiro YAMADA <yamada.m@jp.panasonic.com> wrote:
> > Simon,
> >
> >
> > 2014-09-29 0:17 GMT+09:00 Simon Glass <sjg@chromium.org>:
> >> Hi Masahiro,
> >>
> >> On 28 September 2014 07:52, Masahiro Yamada <yamada.m@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@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

[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@jp.panasonic.com> wrote:
> Hi Simon,
>
>
> On Sun, 28 Sep 2014 10:18:59 -0600
> Simon Glass <sjg@chromium.org> wrote:
>
>> Hi Masahiro,
>>
>> On 28 September 2014 09:54, Masahiro YAMADA <yamada.m@jp.panasonic.com> wrote:
>> > Simon,
>> >
>> >
>> > 2014-09-29 0:17 GMT+09:00 Simon Glass <sjg@chromium.org>:
>> >> Hi Masahiro,
>> >>
>> >> On 28 September 2014 07:52, Masahiro Yamada <yamada.m@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@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

[U-Boot] [PATCH 1/4] dm: fix comments

[Simon Glass]

On 28 September 2014 09:13, Simon Glass <sjg@chromium.org> wrote:
> On 28 September 2014 07:52, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>>
>> The struct udevice stands for a device, not a driver.
>> The driver_info.name is a driver's name, which is referenced
>> to bind devices.
>>
>> Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
>
> Acked-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-dm/next, thanks!

[U-Boot] [PATCH 2/4] dm: do not check the existence of uclass operation

[Simon Glass]

On 28 September 2014 09:15, Simon Glass <sjg@chromium.org> wrote:
> On 28 September 2014 07:52, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>> The function uclass_add() checks uc_drv->ops as follows:
>>
>>         if (uc_drv->ops) {
>>                 dm_warn("No ops for uclass id %d\n", id);
>>                 return -EINVAL;
>>         }
>>
>> It seems odd because it warns "No ops" when uc_drv->ops has
>> non-NULL pointer.  (Looks opposite.)
>>
>> Anyway, most of UCLASS_DRIVER entries have no .ops member.
>> This check makes no sense.
>>
>> Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
>
> Acked-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-dm/next, thanks!

[U-Boot] [PATCH 4/4] dm: simplify the loop in lists_driver_lookup_name()

[Simon Glass]

On 28 September 2014 12:49, Igor Grinberg <grinberg@compulab.co.il> wrote:
> Hi Masahiro, Simon,
>
> On 09/28/14 18:22, Simon Glass wrote:
>> Hi Masahiro,
>>
>> On 28 September 2014 07:52, Masahiro Yamada <yamada.m@jp.panasonic.com> wrote:
>>>         if (strncmp(name, entry->name, len))
>>>                 continue;
>>>
>>>         /* Full match */
>>>         if (len == strlen(entry->name))
>>>                 return entry;
>>>
>>> is equivalent to:
>>>
>>>         if (!strcmp(name, entry->name))
>>>                 return entry;
>>>
>>> The latter is simpler.
>>>
>>> Signed-off-by: Masahiro Yamada <yamada.m@jp.panasonic.com>
>>> ---
>>>
>>>  drivers/core/lists.c | 9 +--------
>>>  1 file changed, 1 insertion(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/core/lists.c b/drivers/core/lists.c
>>> index 699f94b..3a1ea85 100644
>>> --- a/drivers/core/lists.c
>>> +++ b/drivers/core/lists.c
>>> @@ -24,19 +24,12 @@ struct driver *lists_driver_lookup_name(const char *name)
>>>                 ll_entry_start(struct driver, driver);
>>>         const int n_ents = ll_entry_count(struct driver, driver);
>>>         struct driver *entry;
>>> -       int len;
>>>
>>>         if (!drv || !n_ents)
>>>                 return NULL;
>>>
>>> -       len = strlen(name);
>>> -
>>>         for (entry = drv; entry != drv + n_ents; entry++) {
>>> -               if (strncmp(name, entry->name, len))
>>> -                       continue;
>>> -
>>> -               /* Full match */
>>> -               if (len == strlen(entry->name))
>>> +               if (!strcmp(name, entry->name))
>>>                         return entry;
>>>         }
>>
>> The discussion on this was here:
>>
>> http://lists.denx.de/pipermail/u-boot/2014-September/189336.html
>>
>> I don't see a lot of value in the extra code, so I think we should
>> take this patch. If it is found to be a problem, we can go back to the
>> defensive code and add a test case so it is clear what exactly we are
>> defending against.
>>
>> Acked-by: Simon Glass <sjg@chromium.org>
>>
>
> Following the discussion referenced above, we have here a classic
> case of C language strings problem.
> One can dig about it all over the Internet (for example here [1]).
> I don't think we will invent a solution for that problem here.
> Also we are not about to abandon the C language...
> So, unless someone comes out with a real case to solve, I think
> we should merge this.
>
> Acked-by: Igor Grinberg <grinberg@compulab.co.il>

Applied to u-boot-dm/next, thanks!

>
> [1] http://www.sei.cmu.edu/library/abstracts/news-at-sei/feature120061.cfm
>
>
>
> --
> Regards,
> Igor.