[U-Boot] Patch: Fix device enumeration through API.

The one-line patch below fixes device enumeration through the U-Boot API.
Device enumeration crashes when the system in question doesn't have any RAM mapped to address zero (I discovered this on a BeagleBone board), since the enumeration calls get_dev with a NULL ifname sometimes which then gets passed down to strncmp().
This fix simply ensures that get_dev returns NULL when invoked with a NULL ifname.
This could also be fixed by reworking the device enumeration to never call get_dev with a NULL argument, but that's a much more extensive change. (get_dev is called from several places and the code is driven by a list that's constructed in a way that naturally leaves lots of NULLs.)
Cheers,
Tim Kientzle
diff --git a/disk/part.c b/disk/part.c index f07a17f..1a82539 100644 --- a/disk/part.c +++ b/disk/part.c @@ -84,7 +84,7 @@ block_dev_desc_t *get_dev(char* ifname, int dev) #ifdef CONFIG_NEEDS_MANUAL_RELOC name += gd->reloc_off; #endif - while (drvr->name) { + while (ifname && drvr->name) { name = drvr->name; reloc_get_dev = drvr->get_dev; #ifdef CONFIG_NEEDS_MANUAL_RELOC

Hi Tim,
Thanks for the patch. Please see some comments below.
On Tue, 21 Feb 2012 22:34:35 -0800 Tim Kientzle kientzle@freebsd.org wrote:
The one-line patch below fixes device enumeration through the U-Boot API.
Device enumeration crashes when the system in question doesn't have any RAM mapped to address zero (I discovered this on a BeagleBone board), since the enumeration calls get_dev with a NULL ifname sometimes which then gets passed down to strncmp().
This fix simply ensures that get_dev returns NULL when invoked with a NULL ifname.
This could also be fixed by reworking the device enumeration to never call get_dev with a NULL argument, but that's a much more extensive change. (get_dev is called from several places and the code is driven by a list that's constructed in a way that naturally leaves lots of NULLs.)
Cheers,
Tim Kientzle
diff --git a/disk/part.c b/disk/part.c index f07a17f..1a82539 100644 --- a/disk/part.c +++ b/disk/part.c @@ -84,7 +84,7 @@ block_dev_desc_t *get_dev(char* ifname, int dev) #ifdef CONFIG_NEEDS_MANUAL_RELOC name += gd->reloc_off; #endif
- while (drvr->name) {
- while (ifname && drvr->name) { name = drvr->name; reloc_get_dev = drvr->get_dev;
I would prefer just checking for ifname == NULL at the top of the function and not on each loop iteration. Also please add your Signed-off-by when submitting patches.
Thanks, Anatolij

Hello, Anatolij,
Thank you for your response. Modified patch below:
On Mar 26, 2012, at 4:06 AM, Anatolij Gustschin wrote:
Hi Tim,
Thanks for the patch. Please see some comments below.
On Tue, 21 Feb 2012 22:34:35 -0800 Tim Kientzle kientzle@freebsd.org wrote:
The one-line patch below fixes device enumeration through the U-Boot API.
Device enumeration crashes when the system in question doesn't have any RAM mapped to address zero (I discovered this on a BeagleBone board), since the enumeration calls get_dev with a NULL ifname sometimes which then gets passed down to strncmp().
This fix simply ensures that get_dev returns NULL when invoked with a NULL ifname.
This could also be fixed by reworking the device enumeration to never call get_dev with a NULL argument, but that's a much more extensive change. (get_dev is called from several places and the code is driven by a list that's constructed in a way that naturally leaves lots of NULLs.)
Cheers,
Tim Kientzle
diff --git a/disk/part.c b/disk/part.c index f07a17f..1a82539 100644 --- a/disk/part.c +++ b/disk/part.c @@ -84,7 +84,7 @@ block_dev_desc_t *get_dev(char* ifname, int dev) #ifdef CONFIG_NEEDS_MANUAL_RELOC name += gd->reloc_off; #endif
- while (drvr->name) {
- while (ifname && drvr->name) { name = drvr->name; reloc_get_dev = drvr->get_dev;
I would prefer just checking for ifname == NULL at the top of the function and not on each loop iteration. Also please add your Signed-off-by when submitting patches.
Signed-off-by: Tim Kientzle kientzle@freebsd.org
diff --git a/disk/part.c b/disk/part.c index f07a17f..35a2def 100644 --- a/disk/part.c +++ b/disk/part.c @@ -80,6 +80,9 @@ block_dev_desc_t *get_dev(char* ifname, int dev) block_dev_desc_t* (*reloc_get_dev)(int dev); char *name;
+ if (ifname == NULL) + return NULL; + name = drvr->name; #ifdef CONFIG_NEEDS_MANUAL_RELOC name += gd->reloc_off;

Dear Tim Kientzle,
please do not top post / full quote. Especially not when submitting patches.
In message 2A2CB860-6BB6-46C5-B508-F7F97B1EB930@freebsd.org you wrote:
Hello, Anatolij,
Thank you for your response. Modified patch below:
On Mar 26, 2012, at 4:06 AM, Anatolij Gustschin wrote:
Hi Tim,
Thanks for the patch. Please see some comments below.
On Tue, 21 Feb 2012 22:34:35 -0800 Tim Kientzle kientzle@freebsd.org wrote:
The one-line patch below fixes device enumeration through the
...
All this would become the commit message - this doesn't work. Any such comments belong to the comment section, i. e. below the "---" line which would bepresentif you generated your patches using "git format- patch".
Best regards,
Wolfgang Denk

From: Tim Kientzle kientzle@freebsd.org
The patch below fixes device enumeration through the U-Boot API.
Device enumeration crashes when the system in question doesn't have any RAM mapped to address zero (I discovered this on a BeagleBone board), since the enumeration calls get_dev with a NULL ifname sometimes which then gets passed down to strncmp().
This fix simply ensures that get_dev returns NULL when invoked with a NULL ifname.
Signed-off-by: Tim Kientzle kientzle@freebsd.org Signed-off-by: Anatolij Gustschin agust@denx.de --- v2: - resend with fixed whitespace errors and properly added commit log.
I've queued this patch in my staging branch. Thanks!
Anatolij
disk/part.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/disk/part.c b/disk/part.c index f07a17f..8ca5d4b 100644 --- a/disk/part.c +++ b/disk/part.c @@ -80,6 +80,9 @@ block_dev_desc_t *get_dev(char* ifname, int dev) block_dev_desc_t* (*reloc_get_dev)(int dev); char *name;
+ if (!ifname) + return NULL; + name = drvr->name; #ifdef CONFIG_NEEDS_MANUAL_RELOC name += gd->reloc_off;
participants (3)
-
Anatolij Gustschin
-
Tim Kientzle
-
Wolfgang Denk