[U-Boot] [PATCH v2] sun8i: On H3 Use words 1-3 instead of just word 3 from the SID

[U-Boot] [PATCH v2] sun8i: On H3 Use words 1-3 instead of just word 3 from the SID

[Hans de Goede]

It seems that bytes 13-14 of the SID / bytes 1-2 from word 3 of the SID
are always 0 on H3 making it a poor candidate to use as source for the
serialnr / mac-address, and the other non constant words (1 and 2) also
have quite a few bits which are the same for some boards,

This commits switches to using words 1 - 3 ex-or-ed together to get a
more unique value for the mac-address / serialnr.

Cc: Chen-Yu Tsai <wens@csie.org>
Cc: Corentin LABBE <clabbe.montjoie@gmail.com>
Cc: Amit Singh Tomar <amittomer25@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-ex-or all 3 words together instead of picking a different word
---
 board/sunxi/board.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index ef3fe26..e0734fb 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -623,6 +623,21 @@ static void setup_environment(const void *fdt)
 
 	ret = sunxi_get_sid(sid);
 	if (ret == 0 && sid[0] != 0 && sid[3] != 0) {
+#ifdef CONFIG_MACH_SUN8I_H3
+		/*
+		 * The single words 1 - 3 of the SID have quite a few bits
+		 * which are the same on many models on the H3, so we ex-or
+		 * them together to get a bit more unique value.
+		 *
+		 * Note we only do this on the H3 as we cannot change the
+		 * algorithm on other SoCs since those have been using
+		 * fixed mac-addresses based on only using word 3 for a
+		 * a long time and changing a fixed mac-address with an
+		 * u-boot update is not good.
+		 */
+		sid[3] ^= sid[1] ^ sid[2];
+#endif
+
 		/* Ensure the NIC specific bytes of the mac are not all 0 */
 		if ((sid[3] & 0xffffff) == 0)
 			sid[3] |= 0x800000;
-- 
2.7.4

[U-Boot] [PATCH v2] sun8i: On H3 Use words 1-3 instead of just word 3 from the SID

[Ian Campbell]

On Fri, 2016-07-29 at 11:49 +0200, Hans de Goede wrote:
> It seems that bytes 13-14 of the SID / bytes 1-2 from word 3 of the
> SID
> are always 0 on H3 making it a poor candidate to use as source for
> the
> serialnr / mac-address, and the other non constant words (1 and 2)
> also
> have quite a few bits which are the same for some boards,
> 
> This commits switches to using words 1 - 3 ex-or-ed together to get a
> more unique value for the mac-address / serialnr.
> 
> Cc: Chen-Yu Tsai <wens@csie.org>
> Cc: Corentin LABBE <clabbe.montjoie@gmail.com>
> Cc: Amit Singh Tomar <amittomer25@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -ex-or all 3 words together instead of picking a different word
> ---
> ?board/sunxi/board.c | 15 +++++++++++++++
> ?1 file changed, 15 insertions(+)
> 
> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> index ef3fe26..e0734fb 100644
> --- a/board/sunxi/board.c
> +++ b/board/sunxi/board.c
> @@ -623,6 +623,21 @@ static void setup_environment(const void *fdt)
> ?
> ?	ret = sunxi_get_sid(sid);
> ?	if (ret == 0 && sid[0] != 0 && sid[3] != 0) {

Could/should this check be removed now? Or should sid[1]/sid[2] be
included?

> +#ifdef CONFIG_MACH_SUN8I_H3
> > +		/*
> > +		?* The single words 1 - 3 of the SID have quite a few bits
> > +		?* which are the same on many models on the H3, so we ex-or
> > +		?* them together to get a bit more unique value.
> > +		?*
> > +		?* Note we only do this on the H3 as we cannot change the
> > +		?* algorithm on other SoCs since those have been using
> > +		?* fixed mac-addresses based on only using word 3 for a
> > +		?* a long time and changing a fixed mac-address with an
> > > +		?* u-boot update is not good.

I wonder if we should invert this, i.e. make it not use this new method
on all existing models. That would mean that all future models will
automatically use this new (IMHO better) scheme instead of requiring it
to be manually added each time (which we are sure to forget to do).

Main downside is the ifdef is going to be pretty long, but at least it
would be static. Maybe doing it with select in Kconfig (i.e. select
SUNXI_LEGACY_MAC_SID_THINGY on the existing platforms) would be a more
tollerable approach?

> > > > > > +		?*/
> > +		sid[3] ^= sid[1] ^ sid[2];
> +#endif
> +
> > ?		/* Ensure the NIC specific bytes of the mac are not all 0 */
> ?		if ((sid[3] & 0xffffff) == 0)
> ?			sid[3] |= 0x800000;

[U-Boot] [PATCH v2] sun8i: On H3 Use words 1-3 instead of just word 3 from the SID

[Siarhei Siamashka]

On Fri, 29 Jul 2016 11:49:45 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> It seems that bytes 13-14 of the SID / bytes 1-2 from word 3 of the SID
> are always 0 on H3 making it a poor candidate to use as source for the
> serialnr / mac-address, and the other non constant words (1 and 2) also
> have quite a few bits which are the same for some boards,
> 
> This commits switches to using words 1 - 3 ex-or-ed together to get a
> more unique value for the mac-address / serialnr.
> 
> Cc: Chen-Yu Tsai <wens@csie.org>
> Cc: Corentin LABBE <clabbe.montjoie@gmail.com>
> Cc: Amit Singh Tomar <amittomer25@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -ex-or all 3 words together instead of picking a different word
> ---
>  board/sunxi/board.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> index ef3fe26..e0734fb 100644
> --- a/board/sunxi/board.c
> +++ b/board/sunxi/board.c
> @@ -623,6 +623,21 @@ static void setup_environment(const void *fdt)
>  
>  	ret = sunxi_get_sid(sid);
>  	if (ret == 0 && sid[0] != 0 && sid[3] != 0) {
> +#ifdef CONFIG_MACH_SUN8I_H3
> +		/*
> +		 * The single words 1 - 3 of the SID have quite a few bits
> +		 * which are the same on many models on the H3, so we ex-or
> +		 * them together to get a bit more unique value.
> +		 *
> +		 * Note we only do this on the H3 as we cannot change the
> +		 * algorithm on other SoCs since those have been using
> +		 * fixed mac-addresses based on only using word 3 for a
> +		 * a long time and changing a fixed mac-address with an
> +		 * u-boot update is not good.
> +		 */
> +		sid[3] ^= sid[1] ^ sid[2];
> +#endif
> +
>  		/* Ensure the NIC specific bytes of the mac are not all 0 */
>  		if ((sid[3] & 0xffffff) == 0)
>  			sid[3] |= 0x800000;

Using XOR is not a good idea, see my reply in the v1 patch discussion:
http://lists.denx.de/pipermail/u-boot/2016-July/262298.html

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [PATCH v2] sun8i: On H3 Use words 1-3 instead of just word 3 from the SID

[Hans de Goede]

Hi,

On 29-07-16 12:28, Ian Campbell wrote:
> On Fri, 2016-07-29 at 11:49 +0200, Hans de Goede wrote:
>> It seems that bytes 13-14 of the SID / bytes 1-2 from word 3 of the
>> SID
>> are always 0 on H3 making it a poor candidate to use as source for
>> the
>> serialnr / mac-address, and the other non constant words (1 and 2)
>> also
>> have quite a few bits which are the same for some boards,
>>
>> This commits switches to using words 1 - 3 ex-or-ed together to get a
>> more unique value for the mac-address / serialnr.
>>
>> Cc: Chen-Yu Tsai <wens@csie.org>
>> Cc: Corentin LABBE <clabbe.montjoie@gmail.com>
>> Cc: Amit Singh Tomar <amittomer25@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -ex-or all 3 words together instead of picking a different word
>> ---
>>  board/sunxi/board.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
>> index ef3fe26..e0734fb 100644
>> --- a/board/sunxi/board.c
>> +++ b/board/sunxi/board.c
>> @@ -623,6 +623,21 @@ static void setup_environment(const void *fdt)
>>
>>  	ret = sunxi_get_sid(sid);
>>  	if (ret == 0 && sid[0] != 0 && sid[3] != 0) {
>
> Could/should this check be removed now? Or should sid[1]/sid[2] be
> included?

Good point, the check is there because some A10 / A20 boards have an
all 0 sid, now that we make sure that the NIC specific bytes of the
mac are not all 0, only checking for sid[0] != 0 should be enough
to capture non initialized sid-s.

>> +#ifdef CONFIG_MACH_SUN8I_H3
>>> +		/*
>>> +		 * The single words 1 - 3 of the SID have quite a few bits
>>> +		 * which are the same on many models on the H3, so we ex-or
>>> +		 * them together to get a bit more unique value.
>>> +		 *
>>> +		 * Note we only do this on the H3 as we cannot change the
>>> +		 * algorithm on other SoCs since those have been using
>>> +		 * fixed mac-addresses based on only using word 3 for a
>>> +		 * a long time and changing a fixed mac-address with an
>>>> +		 * u-boot update is not good.
>
> I wonder if we should invert this, i.e. make it not use this new method
> on all existing models. That would mean that all future models will
> automatically use this new (IMHO better) scheme instead of requiring it
> to be manually added each time (which we are sure to forget to do).

Good point, I'll fix this and the above check for v3.

Regards,

Hans