[U-Boot] [PATCH 0/4] dm: a collection of minor fixes in Driver-Model
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(-)
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 {
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
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!
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;
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
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!
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@chromium.org): Optimise this, perhaps moving the found * node to the start of the list, or creating a linear array mapping
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@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
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@chromium.org): Optimise this, perhaps moving the found * node to the start of the list, or creating a linear array mappingThis 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.
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@chromium.org): Optimise this, perhaps moving the found * node to the start of the list, or creating a linear array mappingThis 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
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@chromium.org): Optimise this, perhaps moving the found * node to the start of the list, or creating a linear array mappingThis 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
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@chromium.org): Optimise this, perhaps moving the found * node to the start of the list, or creating a linear array mappingThis 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
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; }
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
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
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
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.
participants (4)
-
Igor Grinberg -
Masahiro YAMADA -
Masahiro Yamada
-
Simon Glass