Details

Reviewers

imp
• karels
zlei

Group Reviewers

network

Commits

rG3d96ee7c7dcc: Teach if_smsc to get MAC from bootargs.
rG028e4c6548e4: Teach if_smsc to get MAC from bootargs.
rG3878bbf1bb9e: Teach if_smsc to get MAC from bootargs.

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

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 54426
Build 51316: arc lint + arc unit

Event Timeline

ronald_klop.ws created this revision.Nov 4 2023, 2:23 PM

Herald added subscribers: glebius, melifaro, ae, imp. · View Herald TranscriptNov 4 2023, 2:23 PM

ronald_klop.ws requested review of this revision.Nov 4 2023, 2:23 PM

Harbormaster completed remote builds in B54278: Diff 129714.Nov 4 2023, 2:23 PM

• karels added a subscriber: • karels.Nov 4 2023, 3:56 PM

• karels added inline comments.

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.

use ifdef FDT for optional code

Harbormaster completed remote builds in B54283: Diff 129725.Nov 4 2023, 7:09 PM

fix some uncasted ue->ue_sc occurances

Harbormaster completed remote builds in B54284: Diff 129732.Nov 4 2023, 7:32 PM

• karels added inline comments.Nov 4 2023, 8:13 PM

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.

ifdef FDT smsc_get_smsc95xx_macaddr(...) also

Harbormaster completed remote builds in B54285: Diff 129733.Nov 4 2023, 8:32 PM

• karels added inline comments.Nov 4 2023, 8:32 PM

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 marked 2 inline comments as done.Nov 4 2023, 8:33 PM

ronald_klop.ws added inline comments.

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 marked an inline comment as done.Nov 4 2023, 8:35 PM

ronald_klop.ws added inline comments.

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 added a reviewer: network.Nov 5 2023, 2:57 AM

zlei added a subscriber: zlei.Nov 5 2023, 3:02 AM

zlei added inline comments.

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 added inline comments.Nov 5 2023, 4:18 AM

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.

remove ether_gen_addr_bydev

It is considered as being a layering violation.

Harbormaster completed remote builds in B54288: Diff 129740.Nov 5 2023, 7:49 AM

ronald_klop.ws marked 2 inline comments as done.Nov 5 2023, 7:51 AM

ronald_klop.ws added inline comments.

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.

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

Harbormaster completed remote builds in B54289: Diff 129741.Nov 5 2023, 8:01 AM

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

Harbormaster completed remote builds in B54368: Diff 129936.Nov 10 2023, 11:28 AM

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 added inline comments.Nov 14 2023, 4:02 PM

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?

follow the suggestions and style(9) the code

Harbormaster completed remote builds in B54426: Diff 130089.Nov 14 2023, 6:43 PM

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 added inline comments.Nov 14 2023, 6:55 PM

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.

ronald_klop.ws marked 5 inline comments as done.Nov 14 2023, 7:03 PM

• karels added inline comments.Nov 14 2023, 7:44 PM

sys/net/ethernet.h
451	79 is OK. Apparently Phabricator is doing something odd.

All actions are Done now,

@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).

I'm good with the imperfect indentation

imp accepted this revision.Nov 14 2023, 8:54 PM

This revision is now accepted and ready to land.Nov 14 2023, 8:54 PM

• karels accepted this revision.Nov 14 2023, 9:50 PM

• karels added a reviewer: • karels.

Looks good to me.

sys/dev/usb/net/if_smsc.c
1547
1555	To avoid bad MAC address, although unlikely.

zlei added inline comments.Nov 15 2023, 3:36 AM

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 added inline comments.Nov 15 2023, 4:02 AM

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 added inline comments.Nov 15 2023, 4:50 AM

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 added inline comments.Nov 15 2023, 7:42 AM

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`. https://www.raspberrypi.com/documentation/computers/config_txt.html https://forums.raspberrypi.com//viewtopic.php?f=72&t=143011 https://freebsd-arm.freebsd.narkive.com/joXO4IKH/how-to-change-mac-address-on-rpi-b

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 added inline comments.Nov 30 2023, 1:10 AM

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.

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`. https://www.raspberrypi.com/documentation/computers/config_txt.html https://forums.raspberrypi.com//viewtopic.php?f=72&t=143011 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 added inline comments.Nov 30 2023, 2:58 PM

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.