Teach if_smsc to get MAC from bootargs.

[ronald_klop.ws]

Summary

Some Raspberty PIs pass smsc95xx.macaddr=XX:XX:XX:XX:XX:XX as bootargs.
Use this if no ethernet address is found in a EEPROM.
As last resort fall back to ether_gen_addr() instead of random MAC.

PR: 274092
Tested by: Patrick M. Hausen
MFC after: 1 month

[• karels]

sys/dev/usb/net/if_smsc.c

1569

I think this routine, and hence smsc_get_smsc95xx_macaddr, should be #ifdef FDT; otherwise the OF_* routines won't exist. FDT comes from opt_platform.h.

[ronald_klop.ws]

use ifdef FDT for optional code

[ronald_klop.ws]

fix some uncasted ue->ue_sc occurances

[• karels]

sys/dev/usb/net/if_smsc.c

1569

Is FDT defined, or do you need to include "opt_platform.h" at the top? Also, smsc_get_smsc95xx_macaddr() should be inside the ifdef FDT, or it will be unused.

[ronald_klop.ws]

ifdef FDT smsc_get_smsc95xx_macaddr(...) also

[• karels]

sys/dev/usb/net/if_smsc.c

1569

Oh, "opt_platform.h" is included after the standard includes; that's not the convention, but I wouldn't change it. The other comment still applies.

[ronald_klop.ws]

sys/dev/usb/net/if_smsc.c

1569

Is FDT defined, or do you need to include "opt_platform.h" at the top? Also, smsc_get_smsc95xx_macaddr() should be inside the ifdef FDT, or it will be unused.

opt_platform.h is included. FDT was already used for other parts of this file.
I have put the other method inside ifdef also.

[ronald_klop.ws]

sys/dev/usb/net/if_smsc.c

1569

Oh, "opt_platform.h" is included after the standard includes; that's not the convention, but I wouldn't change it. The other comment still applies.

Both comments should be handled now.

[zlei]

sys/net/if_ethersubr.c

1546

if_ethersubr.c is for upper protocol layer, I think it do not want look into lower level info such as device_t.

We may expose above ether_gen_addr_byname if desirable.

[• karels]

sys/net/if_ethersubr.c

1546

I'll second that. I think that ether_gen_addr should have taken a name rather than an ifp. It's too late to change that, but adding ether_gen_addr_byname provides the simplest interface. The device driver should always be able to produce the name.

[ronald_klop.ws]

remove ether_gen_addr_bydev

It is considered as being a layering violation.

[ronald_klop.ws]

sys/net/if_ethersubr.c

1546

if_ethersubr.c is for upper protocol layer, I think it do not want look into lower level info such as device_t.

We may expose above ether_gen_addr_byname if desirable.

Removed ether_gen_addr_bydev and used the *_byname directly. Ironicly this was my first implementation.

[ronald_klop.ws]

remove static as the method is now exposed outside of this file

[ronald_klop.ws]

add NULL check for result of strnstr(...)

[• karels]

I guess I hadn't done a pass looking for style(9) issues before; better late than never. I see some in original code too; I'd leave those alone, at least in this commit, unless they are intermixed.

Have you looked for a USB reviewer? I think there is a usb mailing list. But I'm a little less reluctant to approve now that I know the driver was already using the FDT.

sys/dev/usb/net/if_smsc.c

1553

A number of lines are over 80 characters, should be <= 80.

1556

Do you want this error message if p is null (no MAC address in bootargs)?

1557

return with value should have parens e.g. return (false); More of these below.

1577

More lines over 80 characters.

1663

Also over 80 characters.

sys/net/ethernet.h

451

This should be wrapped too.

sys/net/if_ethersubr.c

1541

Opening curly brace should be on a line by itself.

[imp]

sys/dev/usb/net/if_smsc.c

1553

Also don't Yoda speak: sscanf(...) != 6

1556

Here and below: I don't think you need a cast here for ue->ue_sc

1563

I think the %D freebsd extension would make this easier to read

1572

This is a bool

1574

Do you really need 2k? Is there a centralized constant for this?

[ronald_klop.ws]

follow the suggestions and style(9) the code

[ronald_klop.ws]

Thanks for all the feedback. I incorporated most of it and in two cases had a question about it.

sys/dev/usb/net/if_smsc.c

1556

Here and below: I don't think you need a cast here for ue->ue_sc

I got compilation errors if I removed the cast. Field ue_sc is void* in the struct.
Suggestions to do this better?

1572

This is a bool

To make this bool I needed to handle this in the calling method where the result was put in an 'int err'. I hope I did this in a proper way.

1574

Do you really need 2k? Is there a centralized constant for this?

The code now allocates the memory dynamically by using OF_getprop_alloc(). I found this in other drivers being used. The code might sleep according to the documentation. Is that a problem here?

sys/net/ethernet.h

451

This should be wrapped too.

This line is < 80 characters (79). Does it really need a wrap?

[imp]

sys/dev/usb/net/if_smsc.c

1556

It should have automatically coercesed from void * to struct smsc_softc... But if not, then you can ignore that bit.

1574

No. Not a problem. The probe / attach code is tolerant of sleeping.

[• karels]

sys/net/ethernet.h

451

79 is OK. Apparently Phabricator is doing something odd.

[ronald_klop.ws]

All actions are Done now,

[• karels]

@imp, do you care if the indents in the continuations are fixed? This file doesn't do them right (where they wrap at all). I guess it shouldn't require retesting though.

sys/dev/usb/net/if_smsc.c

1556

In theory continuation lines should just have an extra 4 spaces of indent, per style(9).

[imp]

I'm good with the imperfect indentation

[zlei]

Looks good to me.

sys/dev/usb/net/if_smsc.c

1547

1555

To avoid bad MAC address, although unlikely.

[zlei]

sys/dev/usb/net/if_smsc.c

1548

1555

1557

Parameters in bootargs are separated by spaces.

Prevent invalid MAC address with remainders such as smsc95xx.macaddr=00:11:22:33:44:55f or
smsc95xx.macaddr=00:11:22:33:44:55f foo=bar

[zlei]

sys/dev/usb/net/if_smsc.c

1555

To avoid bad MAC address, although unlikely.

I'm eating my words. It is not unlikely. It is from bootargs and provided by user.

[• karels]

sys/dev/usb/net/if_smsc.c

1555

I don't think malformed bootargs are likely, they are from the firmware on the Raspberry Pi.

[zlei]

sys/dev/usb/net/if_smsc.c

1555

I don't think malformed bootargs are likely, they are from the firmware on the Raspberry Pi.

From [1] and search result [2] [3] of 'smsc95xx.macaddr' via Google then I guess the Raspberry Pi firmware (uboot) reads the parameter 'smsc95xx.macaddr' from /boot/cmdline.txt.

Since /boot/cmdline.txt is editable by user then I can conclude that may potentially result in bad MAC address.

I do not know (and do not have Pi to test) if the firmware will sanity check the parameters ( it does not scale so probably not ).

Correct me if wrong.

To avoid bad MAC address, although unlikely.

I'm eating my words. It is not unlikely. It is from bootargs and provided by user.

I'd revise this to To avoid possible / potential bad MAC address.

1. https://www.raspberrypi.com/documentation/computers/config_txt.html
2. https://forums.raspberrypi.com//viewtopic.php?f=72&t=143011
3. https://freebsd-arm.freebsd.narkive.com/joXO4IKH/how-to-change-mac-address-on-rpi-b

[• karels]

Ronald, I think you can commit this. You can put "Approved: karels", although that's implied by the reviewers listing.

sys/dev/usb/net/if_smsc.c

1555

Replying to the comment about cmdline.txt: there is no cmdline.txt on the images we ship, and I have never heard of one being used. I don't know if it is supported by the BSD boot path. The common case is certainly that the firmware is generating the bootparam.

The review is accepted, so I assume it can be committed.

[zlei]

sys/dev/usb/net/if_smsc.c

1555

The review is accepted, so I assume it can be committed.

Yes, it can be landed as is.

As for the sanity check for the parameters, it can be fixed separately if we really reach that case.

[ronald_klop.ws]

I will work on committing this soon. A bit busy with work and the rest of life. :-)

sys/dev/usb/net/if_smsc.c

1555

I don't think malformed bootargs are likely, they are from the firmware on the Raspberry Pi.

From [1] and search result [2] [3] of 'smsc95xx.macaddr' via Google then I guess the Raspberry Pi firmware (uboot) reads the parameter 'smsc95xx.macaddr' from /boot/cmdline.txt.

Since /boot/cmdline.txt is editable by user then I can conclude that may potentially result in bad MAC address.

I do not know (and do not have Pi to test) if the firmware will sanity check the parameters ( it does not scale so probably not ).

Correct me if wrong.

To avoid bad MAC address, although unlikely.

I'm eating my words. It is not unlikely. It is from bootargs and provided by user.

I'd revise this to To avoid possible / potential bad MAC address.

1. https://www.raspberrypi.com/documentation/computers/config_txt.html
2. https://forums.raspberrypi.com//viewtopic.php?f=72&t=143011
3. https://freebsd-arm.freebsd.narkive.com/joXO4IKH/how-to-change-mac-address-on-rpi-b

Users can change the bootargs so care during parsing should be taken. But the default for bootargs is a string coming from RPI firmware containing a hardcoded 'smsc95xx.macaddr'. This value does not come from config.txt in the cases we are observing while developing this patch.

1557

Parameters in bootargs are separated by spaces.

Prevent invalid MAC address with remainders such as smsc95xx.macaddr=00:11:22:33:44:55f or
smsc95xx.macaddr=00:11:22:33:44:55f foo=bar

I tested this some time ago and again this morning. Your example will give the macaddr ending on 5f instead of 55. Both results are a valid MAC and I don't see a reason why choosing 55 over 5f would be better. Can you enlighten me?
NB: using non-hex characters will fail the scanf so that case is handled too.

[zlei]

sys/dev/usb/net/if_smsc.c

1557

Parameters in bootargs are separated by spaces.

Prevent invalid MAC address with remainders such as smsc95xx.macaddr=00:11:22:33:44:55f or
smsc95xx.macaddr=00:11:22:33:44:55f foo=bar

I tested this some time ago and again this morning. Your example will give the macaddr ending on 5f instead of 55. Both results are a valid MAC and I don't see a reason why choosing 55 over 5f would be better. Can you enlighten me?

00:11:22:33:44:55f is bad MAC address, so either result 5f or 55 by sscanf() should be buggy implementation.

@karels
The common case is certainly that the firmware is generating the bootparam.

As for the sanity check for the parameters, it can be fixed separately if we really reach that case.

So I am OK to commit it right now.

NB: using non-hex characters will fail the scanf so that case is handled too.