* [U-Boot] [PATCH] dm: fdtdec: Check full path for alias
@ 2016-04-14 13:02 Michal Simek
2016-04-14 15:24 ` Stephen Warren
2016-04-20 14:41 ` Simon Glass
0 siblings, 2 replies; 5+ messages in thread
From: Michal Simek @ 2016-04-14 13:02 UTC (permalink / raw)
To: u-boot
Fix fdtdec_get_alias_seq() which is not checking full path to certain
node and it incorrectly provides incorrect seq number.
Checking full path ensure that if alias is present correct seq number is
return.
This problem was found on ZynqMP zcu102 where are several I2C muxes
where the first bus name is i2c at 0 and for following muxes incorrect seq
numbers are return.
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
I am not sure if there is any macro for max alias length which I can use
instead of new macro.
Before:
ZynqMP> i2c bus
Bus 0: i2c at ff020000
20: gpio at 20, offset len 1, flags 0
21: gpio at 21, offset len 1, flags 0
75: i2cswitch at 75, offset len 1, flags 0
Bus 750: i2c at 0
Bus 751: i2c at 1
Bus 752: i2c at 2
Bus 1: i2c at ff030000
74: i2cswitch at 74, offset len 1, flags 0
75: i2cswitch at 75, offset len 1, flags 0
Bus 750: i2c at 0
Bus 751: i2c at 1
Bus 752: i2c at 2
Bus 1743: i2c at 3
Bus 1744: i2c at 4
Bus 750: i2c at 0
Bus 751: i2c at 1
Bus 752: i2c at 2
Bus 1743: i2c at 3
Bus 1744: i2c at 4
Bus 1755: i2c at 5
Bus 1756: i2c at 6
Bus 1757: i2c at 7
After:
ZynqMP> i2c bus
Bus 0: i2c at ff020000
20: gpio at 20, offset len 1, flags 0
21: gpio at 21, offset len 1, flags 0
75: i2cswitch at 75, offset len 1, flags 0
Bus 750: i2c at 0
Bus 751: i2c at 1
Bus 752: i2c at 2
Bus 1: i2c at ff030000
74: i2cswitch at 74, offset len 1, flags 0
75: i2cswitch at 75, offset len 1, flags 0
Bus 1740: i2c at 0
Bus 1741: i2c at 1
Bus 1742: i2c at 2
Bus 1743: i2c at 3
Bus 1744: i2c at 4
Bus 1750: i2c at 0
Bus 1751: i2c at 1
Bus 1752: i2c at 2
Bus 1753: i2c at 3
Bus 1754: i2c at 4
Bus 1755: i2c at 5
Bus 1756: i2c at 6
Bus 1757: i2c at 7
Which reflects my aliases
i2c0 = &i2c0;
i2c750 = &i2c0_75_0;
i2c751 = &i2c0_75_1;
i2c752 = &i2c0_75_2;
i2c1 = &i2c1;
i2c1740 = &i2c1_74_0;
i2c1741 = &i2c1_74_1;
i2c1742 = &i2c1_74_2;
i2c1743 = &i2c1_74_3;
i2c1744 = &i2c1_74_4;
i2c1750 = &i2c1_75_0;
i2c1751 = &i2c1_75_1;
i2c1752 = &i2c1_75_2;
i2c1753 = &i2c1_75_3;
i2c1754 = &i2c1_75_4;
i2c1755 = &i2c1_75_5;
i2c1756 = &i2c1_75_6;
i2c1757 = &i2c1_75_7;
---
lib/fdtdec.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 70acc29c924d..79efcf0cd6b0 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -506,6 +506,8 @@ int fdtdec_add_aliases_for_id(const void *blob, const char *name,
return num_found;
}
+#define FDT_ALIAS_PATH_LEN 64
+
int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
int *seqp)
{
@@ -514,9 +516,13 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
int find_namelen;
int prop_offset;
int aliases;
+ char buf[FDT_ALIAS_PATH_LEN];
+
+ fdt_get_path(blob, offset, buf, sizeof(buf));
find_name = fdt_get_name(blob, offset, &find_namelen);
- debug("Looking for '%s' at %d, name %s\n", base, offset, find_name);
+ debug("Looking for '%s' at %d, name %s, buf %s\n",
+ base, offset, find_name, buf);
aliases = fdt_path_offset(blob, "/aliases");
for (prop_offset = fdt_first_property_offset(blob, aliases);
@@ -533,6 +539,13 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
strncmp(name, base, base_len))
continue;
+ if (len >= FDT_ALIAS_PATH_LEN)
+ printf("Too long path in aliases node\n");
+
+ /* Check full path first not to point to incorrect alias */
+ if (strncmp(prop, buf, min(len, FDT_ALIAS_PATH_LEN)))
+ continue;
+
slash = strrchr(prop, '/');
if (strcmp(slash + 1, find_name))
continue;
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] dm: fdtdec: Check full path for alias
2016-04-14 13:02 [U-Boot] [PATCH] dm: fdtdec: Check full path for alias Michal Simek
@ 2016-04-14 15:24 ` Stephen Warren
2016-04-20 14:41 ` Simon Glass
1 sibling, 0 replies; 5+ messages in thread
From: Stephen Warren @ 2016-04-14 15:24 UTC (permalink / raw)
To: u-boot
On 04/14/2016 07:02 AM, Michal Simek wrote:
> Fix fdtdec_get_alias_seq() which is not checking full path to certain
> node and it incorrectly provides incorrect seq number.
> Checking full path ensure that if alias is present correct seq number is
> return.
> This problem was found on ZynqMP zcu102 where are several I2C muxes
> where the first bus name is i2c at 0 and for following muxes incorrect seq
> numbers are return.
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> I am not sure if there is any macro for max alias length which I can use
> instead of new macro.
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index 70acc29c924d..79efcf0cd6b0 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -506,6 +506,8 @@ int fdtdec_add_aliases_for_id(const void *blob, const char *name,
> return num_found;
> }
>
> +#define FDT_ALIAS_PATH_LEN 64
> +
> int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
> int *seqp)
> {
> @@ -514,9 +516,13 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
> int find_namelen;
> int prop_offset;
> int aliases;
> + char buf[FDT_ALIAS_PATH_LEN];
> +
> + fdt_get_path(blob, offset, buf, sizeof(buf));
Rather the getting the path of the object, and then comparing it against
the alias, you could check each component separately, and hence not need
to ever generate the full concatenated path. That would be a lot slower
though.
Another thought is a variant of fdt_get_path() which also returns the
required length of the path, so you could do something like:
buf = malloc(DEFAULT_LEN);
ret = fdt_get_path(blob, offset, buf, DEFAULT_LEN, &required_len);
if (ret == -ENOSPC)
buf = realloc(buf, required_len);
ret = fdt_get_path(blob, offset, buf, required_len, &required_len);
}
if (ret)
return ret;
> @@ -533,6 +539,13 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
> + /* Check full path first not to point to incorrect alias */
> + if (strncmp(prop, buf, min(len, FDT_ALIAS_PATH_LEN)))
> + continue;
> +
> slash = strrchr(prop, '/');
> if (strcmp(slash + 1, find_name))
> continue;
I would expect the existing check to completely replace the original
one. IIUC, the comparison is required to be via the full path, and the
existing code is simply a bug for simplicity.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] dm: fdtdec: Check full path for alias
2016-04-14 13:02 [U-Boot] [PATCH] dm: fdtdec: Check full path for alias Michal Simek
2016-04-14 15:24 ` Stephen Warren
@ 2016-04-20 14:41 ` Simon Glass
2016-04-21 4:47 ` Michal Simek
1 sibling, 1 reply; 5+ messages in thread
From: Simon Glass @ 2016-04-20 14:41 UTC (permalink / raw)
To: u-boot
Hi Michal,
On 14 April 2016 at 07:02, Michal Simek <michal.simek@xilinx.com> wrote:
> Fix fdtdec_get_alias_seq() which is not checking full path to certain
> node and it incorrectly provides incorrect seq number.
> Checking full path ensure that if alias is present correct seq number is
> return.
> This problem was found on ZynqMP zcu102 where are several I2C muxes
> where the first bus name is i2c at 0 and for following muxes incorrect seq
> numbers are return.
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
> I am not sure if there is any macro for max alias length which I can use
> instead of new macro.
>
> Before:
> ZynqMP> i2c bus
> Bus 0: i2c at ff020000
> 20: gpio at 20, offset len 1, flags 0
> 21: gpio at 21, offset len 1, flags 0
> 75: i2cswitch at 75, offset len 1, flags 0
> Bus 750: i2c at 0
> Bus 751: i2c at 1
> Bus 752: i2c at 2
> Bus 1: i2c at ff030000
> 74: i2cswitch at 74, offset len 1, flags 0
> 75: i2cswitch at 75, offset len 1, flags 0
> Bus 750: i2c at 0
Sorry I'm a bit confused by this. Do you have two i2c at 0 buses?
> Bus 751: i2c at 1
> Bus 752: i2c at 2
> Bus 1743: i2c at 3
> Bus 1744: i2c at 4
> Bus 750: i2c at 0
> Bus 751: i2c at 1
> Bus 752: i2c at 2
> Bus 1743: i2c at 3
> Bus 1744: i2c at 4
> Bus 1755: i2c at 5
> Bus 1756: i2c at 6
> Bus 1757: i2c at 7
>
> After:
> ZynqMP> i2c bus
> Bus 0: i2c at ff020000
> 20: gpio at 20, offset len 1, flags 0
> 21: gpio at 21, offset len 1, flags 0
> 75: i2cswitch at 75, offset len 1, flags 0
> Bus 750: i2c at 0
> Bus 751: i2c at 1
> Bus 752: i2c at 2
> Bus 1: i2c at ff030000
> 74: i2cswitch at 74, offset len 1, flags 0
> 75: i2cswitch at 75, offset len 1, flags 0
> Bus 1740: i2c at 0
> Bus 1741: i2c at 1
> Bus 1742: i2c at 2
> Bus 1743: i2c at 3
> Bus 1744: i2c at 4
> Bus 1750: i2c at 0
> Bus 1751: i2c at 1
> Bus 1752: i2c at 2
> Bus 1753: i2c at 3
> Bus 1754: i2c at 4
> Bus 1755: i2c at 5
> Bus 1756: i2c at 6
> Bus 1757: i2c at 7
>
> Which reflects my aliases
> i2c0 = &i2c0;
> i2c750 = &i2c0_75_0;
> i2c751 = &i2c0_75_1;
> i2c752 = &i2c0_75_2;
> i2c1 = &i2c1;
> i2c1740 = &i2c1_74_0;
> i2c1741 = &i2c1_74_1;
> i2c1742 = &i2c1_74_2;
> i2c1743 = &i2c1_74_3;
> i2c1744 = &i2c1_74_4;
> i2c1750 = &i2c1_75_0;
> i2c1751 = &i2c1_75_1;
> i2c1752 = &i2c1_75_2;
> i2c1753 = &i2c1_75_3;
> i2c1754 = &i2c1_75_4;
> i2c1755 = &i2c1_75_5;
> i2c1756 = &i2c1_75_6;
> i2c1757 = &i2c1_75_7;
I only see one in you aliases above.
>
> ---
> lib/fdtdec.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index 70acc29c924d..79efcf0cd6b0 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -506,6 +506,8 @@ int fdtdec_add_aliases_for_id(const void *blob, const char *name,
> return num_found;
> }
>
> +#define FDT_ALIAS_PATH_LEN 64
> +
> int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
> int *seqp)
> {
> @@ -514,9 +516,13 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
> int find_namelen;
> int prop_offset;
> int aliases;
> + char buf[FDT_ALIAS_PATH_LEN];
> +
> + fdt_get_path(blob, offset, buf, sizeof(buf));
Eek! That is pretty expensive.
>
> find_name = fdt_get_name(blob, offset, &find_namelen);
> - debug("Looking for '%s' at %d, name %s\n", base, offset, find_name);
> + debug("Looking for '%s' at %d, name %s, buf %s\n",
> + base, offset, find_name, buf);
>
> aliases = fdt_path_offset(blob, "/aliases");
> for (prop_offset = fdt_first_property_offset(blob, aliases);
> @@ -533,6 +539,13 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
> strncmp(name, base, base_len))
> continue;
>
> + if (len >= FDT_ALIAS_PATH_LEN)
> + printf("Too long path in aliases node\n");
Can you return an error and make it debug() here?
> +
> + /* Check full path first not to point to incorrect alias */
> + if (strncmp(prop, buf, min(len, FDT_ALIAS_PATH_LEN)))
> + continue;
> +
> slash = strrchr(prop, '/');
> if (strcmp(slash + 1, find_name))
> continue;
> --
> 1.9.1
>
Regards,
Simon
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] dm: fdtdec: Check full path for alias
2016-04-20 14:41 ` Simon Glass
@ 2016-04-21 4:47 ` Michal Simek
2016-05-01 18:54 ` Simon Glass
0 siblings, 1 reply; 5+ messages in thread
From: Michal Simek @ 2016-04-21 4:47 UTC (permalink / raw)
To: u-boot
Hi Simon,
On 20.4.2016 16:41, Simon Glass wrote:
> Hi Michal,
>
> On 14 April 2016 at 07:02, Michal Simek <michal.simek@xilinx.com> wrote:
>> Fix fdtdec_get_alias_seq() which is not checking full path to certain
>> node and it incorrectly provides incorrect seq number.
>> Checking full path ensure that if alias is present correct seq number is
>> return.
>> This problem was found on ZynqMP zcu102 where are several I2C muxes
>> where the first bus name is i2c at 0 and for following muxes incorrect seq
>> numbers are return.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>> I am not sure if there is any macro for max alias length which I can use
>> instead of new macro.
>>
>> Before:
>> ZynqMP> i2c bus
>> Bus 0: i2c at ff020000
>> 20: gpio at 20, offset len 1, flags 0
>> 21: gpio at 21, offset len 1, flags 0
>> 75: i2cswitch at 75, offset len 1, flags 0
>> Bus 750: i2c at 0
>> Bus 751: i2c at 1
>> Bus 752: i2c at 2
>> Bus 1: i2c at ff030000
>> 74: i2cswitch at 74, offset len 1, flags 0
>> 75: i2cswitch at 75, offset len 1, flags 0
>> Bus 750: i2c at 0
>
> Sorry I'm a bit confused by this. Do you have two i2c at 0 buses?
Yes there are two i2c IP cores. It means 2 main i2c busses.
Here is full i2c description
arch/arm/dts/zynqmp-zcu102.dts
I have pushed my hacky/testing tree here with these changes.
u-boot-microblaze.git xnext/i2c
>
>> Bus 751: i2c at 1
>> Bus 752: i2c at 2
>> Bus 1743: i2c at 3
>> Bus 1744: i2c at 4
>> Bus 750: i2c at 0
>> Bus 751: i2c at 1
>> Bus 752: i2c at 2
>> Bus 1743: i2c at 3
>> Bus 1744: i2c at 4
>> Bus 1755: i2c at 5
>> Bus 1756: i2c at 6
>> Bus 1757: i2c at 7
>>
>> After:
>> ZynqMP> i2c bus
>> Bus 0: i2c at ff020000
>> 20: gpio at 20, offset len 1, flags 0
>> 21: gpio at 21, offset len 1, flags 0
>> 75: i2cswitch at 75, offset len 1, flags 0
>> Bus 750: i2c at 0
>> Bus 751: i2c at 1
>> Bus 752: i2c at 2
>> Bus 1: i2c at ff030000
>> 74: i2cswitch at 74, offset len 1, flags 0
>> 75: i2cswitch at 75, offset len 1, flags 0
>> Bus 1740: i2c at 0
>> Bus 1741: i2c at 1
>> Bus 1742: i2c at 2
>> Bus 1743: i2c at 3
>> Bus 1744: i2c at 4
>> Bus 1750: i2c at 0
>> Bus 1751: i2c at 1
>> Bus 1752: i2c at 2
>> Bus 1753: i2c at 3
>> Bus 1754: i2c at 4
>> Bus 1755: i2c at 5
>> Bus 1756: i2c at 6
>> Bus 1757: i2c at 7
>>
>> Which reflects my aliases
>> i2c0 = &i2c0;
>> i2c750 = &i2c0_75_0;
>> i2c751 = &i2c0_75_1;
>> i2c752 = &i2c0_75_2;
>> i2c1 = &i2c1;
>> i2c1740 = &i2c1_74_0;
>> i2c1741 = &i2c1_74_1;
>> i2c1742 = &i2c1_74_2;
>> i2c1743 = &i2c1_74_3;
>> i2c1744 = &i2c1_74_4;
>> i2c1750 = &i2c1_75_0;
>> i2c1751 = &i2c1_75_1;
>> i2c1752 = &i2c1_75_2;
>> i2c1753 = &i2c1_75_3;
>> i2c1754 = &i2c1_75_4;
>> i2c1755 = &i2c1_75_5;
>> i2c1756 = &i2c1_75_6;
>> i2c1757 = &i2c1_75_7;
>
> I only see one in you aliases above.
i2c0 and i2c1 are that main one.
>
>>
>> ---
>> lib/fdtdec.c | 15 ++++++++++++++-
>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
>> index 70acc29c924d..79efcf0cd6b0 100644
>> --- a/lib/fdtdec.c
>> +++ b/lib/fdtdec.c
>> @@ -506,6 +506,8 @@ int fdtdec_add_aliases_for_id(const void *blob, const char *name,
>> return num_found;
>> }
>>
>> +#define FDT_ALIAS_PATH_LEN 64
>> +
>> int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
>> int *seqp)
>> {
>> @@ -514,9 +516,13 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
>> int find_namelen;
>> int prop_offset;
>> int aliases;
>> + char buf[FDT_ALIAS_PATH_LEN];
>> +
>> + fdt_get_path(blob, offset, buf, sizeof(buf));
>
> Eek! That is pretty expensive.
How to do it better?
>
>>
>> find_name = fdt_get_name(blob, offset, &find_namelen);
>> - debug("Looking for '%s' at %d, name %s\n", base, offset, find_name);
>> + debug("Looking for '%s' at %d, name %s, buf %s\n",
>> + base, offset, find_name, buf);
>>
>> aliases = fdt_path_offset(blob, "/aliases");
>> for (prop_offset = fdt_first_property_offset(blob, aliases);
>> @@ -533,6 +539,13 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
>> strncmp(name, base, base_len))
>> continue;
>>
>> + if (len >= FDT_ALIAS_PATH_LEN)
>> + printf("Too long path in aliases node\n");
>
> Can you return an error and make it debug() here?
Error code?
>
>> +
>> + /* Check full path first not to point to incorrect alias */
>> + if (strncmp(prop, buf, min(len, FDT_ALIAS_PATH_LEN)))
>> + continue;
>> +
>> slash = strrchr(prop, '/');
>> if (strcmp(slash + 1, find_name))
>> continue;
>> --
>> 1.9.1
>>
Thanks,
Michal
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] dm: fdtdec: Check full path for alias
2016-04-21 4:47 ` Michal Simek
@ 2016-05-01 18:54 ` Simon Glass
0 siblings, 0 replies; 5+ messages in thread
From: Simon Glass @ 2016-05-01 18:54 UTC (permalink / raw)
To: u-boot
Hi Michal,
On 20 April 2016 at 22:47, Michal Simek <michal.simek@xilinx.com> wrote:
> Hi Simon,
>
> On 20.4.2016 16:41, Simon Glass wrote:
>> Hi Michal,
>>
>> On 14 April 2016 at 07:02, Michal Simek <michal.simek@xilinx.com> wrote:
>>> Fix fdtdec_get_alias_seq() which is not checking full path to certain
>>> node and it incorrectly provides incorrect seq number.
>>> Checking full path ensure that if alias is present correct seq number is
>>> return.
>>> This problem was found on ZynqMP zcu102 where are several I2C muxes
>>> where the first bus name is i2c at 0 and for following muxes incorrect seq
>>> numbers are return.
>>>
>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>> ---
>>>
>>> I am not sure if there is any macro for max alias length which I can use
>>> instead of new macro.
>>>
>>> Before:
>>> ZynqMP> i2c bus
>>> Bus 0: i2c at ff020000
>>> 20: gpio at 20, offset len 1, flags 0
>>> 21: gpio at 21, offset len 1, flags 0
>>> 75: i2cswitch at 75, offset len 1, flags 0
>>> Bus 750: i2c at 0
>>> Bus 751: i2c at 1
>>> Bus 752: i2c at 2
>>> Bus 1: i2c at ff030000
>>> 74: i2cswitch at 74, offset len 1, flags 0
>>> 75: i2cswitch at 75, offset len 1, flags 0
>>> Bus 750: i2c at 0
>>
>> Sorry I'm a bit confused by this. Do you have two i2c at 0 buses?
>
> Yes there are two i2c IP cores. It means 2 main i2c busses.
>
> Here is full i2c description
> arch/arm/dts/zynqmp-zcu102.dts
>
> I have pushed my hacky/testing tree here with these changes.
> u-boot-microblaze.git xnext/i2c
>
>>
>>> Bus 751: i2c at 1
>>> Bus 752: i2c at 2
>>> Bus 1743: i2c at 3
>>> Bus 1744: i2c at 4
>>> Bus 750: i2c at 0
>>> Bus 751: i2c at 1
>>> Bus 752: i2c at 2
>>> Bus 1743: i2c at 3
>>> Bus 1744: i2c at 4
>>> Bus 1755: i2c at 5
>>> Bus 1756: i2c at 6
>>> Bus 1757: i2c at 7
>>>
>>> After:
>>> ZynqMP> i2c bus
>>> Bus 0: i2c at ff020000
>>> 20: gpio at 20, offset len 1, flags 0
>>> 21: gpio at 21, offset len 1, flags 0
>>> 75: i2cswitch at 75, offset len 1, flags 0
>>> Bus 750: i2c at 0
>>> Bus 751: i2c at 1
>>> Bus 752: i2c at 2
>>> Bus 1: i2c at ff030000
>>> 74: i2cswitch at 74, offset len 1, flags 0
>>> 75: i2cswitch at 75, offset len 1, flags 0
>>> Bus 1740: i2c at 0
>>> Bus 1741: i2c at 1
>>> Bus 1742: i2c at 2
>>> Bus 1743: i2c at 3
>>> Bus 1744: i2c at 4
>>> Bus 1750: i2c at 0
>>> Bus 1751: i2c at 1
>>> Bus 1752: i2c at 2
>>> Bus 1753: i2c at 3
>>> Bus 1754: i2c at 4
>>> Bus 1755: i2c at 5
>>> Bus 1756: i2c at 6
>>> Bus 1757: i2c at 7
>>>
>>> Which reflects my aliases
>>> i2c0 = &i2c0;
>>> i2c750 = &i2c0_75_0;
>>> i2c751 = &i2c0_75_1;
>>> i2c752 = &i2c0_75_2;
>>> i2c1 = &i2c1;
>>> i2c1740 = &i2c1_74_0;
>>> i2c1741 = &i2c1_74_1;
>>> i2c1742 = &i2c1_74_2;
>>> i2c1743 = &i2c1_74_3;
>>> i2c1744 = &i2c1_74_4;
>>> i2c1750 = &i2c1_75_0;
>>> i2c1751 = &i2c1_75_1;
>>> i2c1752 = &i2c1_75_2;
>>> i2c1753 = &i2c1_75_3;
>>> i2c1754 = &i2c1_75_4;
>>> i2c1755 = &i2c1_75_5;
>>> i2c1756 = &i2c1_75_6;
>>> i2c1757 = &i2c1_75_7;
>>
>> I only see one in you aliases above.
>
> i2c0 and i2c1 are that main one.
>
>
>>
>>>
>>> ---
>>> lib/fdtdec.c | 15 ++++++++++++++-
>>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
>>> index 70acc29c924d..79efcf0cd6b0 100644
>>> --- a/lib/fdtdec.c
>>> +++ b/lib/fdtdec.c
>>> @@ -506,6 +506,8 @@ int fdtdec_add_aliases_for_id(const void *blob, const char *name,
>>> return num_found;
>>> }
>>>
>>> +#define FDT_ALIAS_PATH_LEN 64
>>> +
>>> int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
>>> int *seqp)
>>> {
>>> @@ -514,9 +516,13 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
>>> int find_namelen;
>>> int prop_offset;
>>> int aliases;
>>> + char buf[FDT_ALIAS_PATH_LEN];
>>> +
>>> + fdt_get_path(blob, offset, buf, sizeof(buf));
>>
>> Eek! That is pretty expensive.
>
> How to do it better?
This is so confusing. It would help to have the 'dm tree' output.
From what I can see i2c1_74_1 is the problem. It is supposed to get a
sequence number of 1741 but is detected as 1, since the understore
before '1' stops trailing_strtol() from moving left to see additional
digits.
So when the algorithm looks at the i2c1_74_1 alias, and sees that it
points to /path/to/i2c1_74_1, it decides that the sequence number for
this is 1, and returns that.
This is not what you want, since you already have another i2c1 alias,
which needs to get that sequence number.
Your solution relies on skipping that answer and finding another one.
But for the skipping the function would still returns 1 in *seqp.
Wouldn't it be better to adjust the function to detect 1741 in this
case? This would involve adding something like
trailing_strto_skip_underscoresl() to skip underscores.
Whatever we do here, we need a unit test to cover the cases (could be
added to test/dm/core.c with a few strange i2c nodes added to
test.dts), and the header file comment needs to be expanded a little.
BTW at some point I think we will want to introduce a little more
device tree support in driver model. For example, we might want to add
a table of aliases built up in dm_scan_fdt_node() or similar. We might
want a table of phandles. I haven't looked into it in detail, but this
fdtdec function predates driver model and I suspect we can create
something more efficient now.
>
>>
>>>
>>> find_name = fdt_get_name(blob, offset, &find_namelen);
>>> - debug("Looking for '%s' at %d, name %s\n", base, offset, find_name);
>>> + debug("Looking for '%s' at %d, name %s, buf %s\n",
>>> + base, offset, find_name, buf);
>>>
>>> aliases = fdt_path_offset(blob, "/aliases");
>>> for (prop_offset = fdt_first_property_offset(blob, aliases);
>>> @@ -533,6 +539,13 @@ int fdtdec_get_alias_seq(const void *blob, const char *base, int offset,
>>> strncmp(name, base, base_len))
>>> continue;
>>>
>>> + if (len >= FDT_ALIAS_PATH_LEN)
>>> + printf("Too long path in aliases node\n");
>>
>> Can you return an error and make it debug() here?
>
> Error code?
>
>>
>>> +
>>> + /* Check full path first not to point to incorrect alias */
>>> + if (strncmp(prop, buf, min(len, FDT_ALIAS_PATH_LEN)))
>>> + continue;
>>> +
>>> slash = strrchr(prop, '/');
>>> if (strcmp(slash + 1, find_name))
>>> continue;
>>> --
>>> 1.9.1
>>>
>
> Thanks,
> Michal
>
>
Regards,
Simon
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-05-01 18:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-14 13:02 [U-Boot] [PATCH] dm: fdtdec: Check full path for alias Michal Simek
2016-04-14 15:24 ` Stephen Warren
2016-04-20 14:41 ` Simon Glass
2016-04-21 4:47 ` Michal Simek
2016-05-01 18:54 ` Simon Glass
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.