Created attachment 245237 [details] git diff to use ether_gen_addr Read: "Getting a stable MAC address for a RPI CM3+ with ue0 interface" https://lists.freebsd.org/archives/freebsd-arm/2023-September/003045.html
Created attachment 245239 [details] git diff to use ether_gen_addr
The patch looks good to me generally. I do not have RPI devices, has the patch been tested ?
Comment on attachment 245239 [details] git diff to use ether_gen_addr Why don't you just call ether_gen_addr(ue->ue_ifp, sc->sc_ue.ue_eaddr);
I'll be able to test tomorrow, 2023-09-27.
(In reply to Michael Tuexen from comment #3) Variable sc->sc_ue.ue_eaddr is a different type than what the function expects. uint8_t[] vs struct ether_addr
(In reply to Zhenlei Huang from comment #2) See the mail thread. Patrick M. Hausen has tested it 1 time and it crashed. Probably something simple I overlooked. He will try to get more output of the panic when he has time. More testing is always welcome. I don't have this hardware myself.
(In reply to Ronald Klop from comment #6) This is the mention of the testing. https://lists.freebsd.org/archives/freebsd-arm/2023-September/003111.html "The new kernel prints the "No MAC address found" message, then panics." More info about the panic is requested, but needs some time to provide.
(In reply to Ronald Klop from comment #5) I see. So ether_gen_addr(ue->ue_ifp, (struct ether_addr *)sc->sc_ue.ue_eaddr); would work.
(In reply to Michael Tuexen from comment #8) I'd prefer not to use such a cast; it assumes that struct ether_addr has only the 6-byte address. On the other hand, maybe ether_gen_addr() should be changed to use the raw pointer to the array. There aren't too many users of it yet.
(In reply to Mike Karels from comment #9) I was assuming that struct ether_addr is the structure used for holding an Ethernet address, which is a 6 byte entity. But if it is more generic, forget about the patch; I'm not a layer two expert. Sorry for the noise.
The definition in sys/net/ethernet.h looks alright: /* * Structure of a 48-bit Ethernet address. */ struct ether_addr { u_char octet[ETHER_ADDR_LEN]; } __packed; ETHER_ADDR_LEN is defined as 6.
Do understand that everybody only has limited time. Still any interest in testing this patch? I don't have the hardware myself.
I'll try the patch this weekend again and take a mobile phone movie of the kernel panic. Sorry, no serial console access or similar. I only have compute modules, no "real" Pi 3 or 3+.
BETA5, patch applied, here's two screenshots of the kernel panic: https://cloud.hausen.com/s/Hx2F2KqJBZ4NXpP https://cloud.hausen.com/s/CatjsJ8gyb4wg3J HTH, Patrick
(In reply to Patrick M. Hausen from comment #14) The panic error 1 indicates accessing invalid kernel address. > sys/vm/vm_param.h:#define KERN_INVALID_ADDRESS 1 I'm not sure how that happens. Can you please try the following: 1. Unplug USB ethernet 2. Boot the board 3. On boot (when have a login prompt) plug in the USB ethernet The screen shot helps some but it will helps a lot if you can share the backtrace or core dump (compile kernel with DEBUG options and install gdb package).
Created attachment 245577 [details] git diff with more debug output (In reply to Patrick M. Hausen from comment #14) Hi. Thanks for the screenshots. As noted in another comment they do not provide the details needed to debug this. If it is not possible to create a DEBUG kernel or some other way to get a core dump would you be able to try this patch for some extra information during the boot?
Unfortunately I cannot boot the board without USB Ethernet connected. It's a compute module placed into this board: https://www.waveshare.com/wiki/Compute_Module_PoE_Board Also although I tried for a long time I could not get serial console to work. So once the system crashes at boot and I lose SSH access all I can do is re-image. With a debug kernel will there be more output to get a screenshot with a camera? Or can I dd the entire EMMC of the module to a file and then extract information, somehow? Kind regards, Patrick For the moment I am satisfied to be able to use these tasks in Ansible to solve the matter: - name: Read serial number ansible.builtin.shell: | set -o pipefail && /bin/kenv | \ /usr/bin/awk '/smbios.system.serial/ {printf "%s:%s:%s\n", substr($0, length($0)-6, 2), substr($0, length($0)-4, 2), substr($0, length($0)-2, 2)}' register: mac_lower changed_when: false - name: Set MAC address ansible.builtin.copy: dest: "/etc/start_if.{{ network.wired.interface }}" owner: root group: wheel mode: '0644' content: | /sbin/ifconfig $1 down /sbin/ifconfig $1 link b8:27:eb:{{ mac_lower.stdout }} /sbin/ifconfig $1 up Anyway what would be the result if we get this to work? As I have been arguing on the mailinglist the RPi foundation clearly documents that we should use the device's serial number to generate the MAC and nothing else for all RPi 3 and older. Kind regards, Patrick
(In reply to Patrick M. Hausen from comment #17) > Anyway what would be the result if we get this to work? As I have been arguing on the mailinglist the RPi foundation clearly documents that we should use the device's serial number to generate the MAC and nothing else for all RPi 3 and older. The result would be that the MAC address is stable after FreeBSD is installed and a hostid is generated. However, you argue that the RPi Foundation documents should control FreeBSD's implementation choices, even for a device that is not RPi specific. It seems to me quite wrong to implement an RPi-specific method in a non-RPi-specific driver. And the RPi Foundation is not very cooperative with others attempting to support RPi with other software.
Oh, of course we should not perform the "serial number" magic on anything but a RPi 3(+) and older. In general there might not be a serial available on some particular device. Sorry if I seemed to propose that. If you deploy a bunch of Pis with RPi OS (Debian) in your company network and open your DHCP server overview page they will all show as "Raspberry Pi Foundation" + serial number, so if you took inventory in advance, you know which device is which etc. I am only arguing that FreeBSD on the Pi should not deliver a worse experience. On the Pi only. Maybe it's not worth the effort. Anyway I can continue to help debug this but I need some more guidance, please. So build a debug kernel, crash the system again, then what? I don't have access to the system other than raw media access to the EMMC flash.
(In reply to Patrick M. Hausen from comment #19) This driver has no idea whether it is on an RPi 3 or higher, or on an amd64. Should every USB Ethernet driver check for RPi? My DHCP server doesn't look up vendor info, but in this case, I would expect yours to find "FreeBSD Foundation" (the OUI used by ether_gen_addr).
No idea, honestly. How does RPi OS do it? If I remember correctly the serial is somehow passed as a parameter to the kernel by the bootloader. Using a FreeBSD specific vendor OUI and a stable MAC based on the hostid is probably best. Now, how can we proceed to get this working? I can try to find a "real Pi" instead of a compute module, serial console access, and an external USB Ethernet based on that chipset. Possibly we have some old hardware lying around somewhere at the office. I'll report back on the weekend.
(In reply to Patrick M. Hausen from comment #19) Please apply the patch "git diff with more debug output" I added today also. So apply both patches from this PR, build a new kernel and boot it. It should print a couple more lines of output which indicates better where it crashes. Please make a picture of that output.
(In reply to Patrick M. Hausen from comment #21) Hi. After we get the FreeBSD OUI working I have the following idea. In https://lists.freebsd.org/archives/freebsd-arm/2023-September/003066.html you posted that some bootargs are passed to the kernel. These bootargs contain "smsc95xx.macaddr=B8:27:EB:09:CB:7D". It would be nice if we could get these args in the driver and use it to set a MAC address.
https://cloud.hausen.com/s/WnSRKFnggyLLZyz https://cloud.hausen.com/s/3DHQ8FDrEpfMc9c
Created attachment 245603 [details] git diff using attach_post_sub (In reply to Patrick M. Hausen from comment #24) Thanks you for the testing. I think I found what is causing the crash. ue->ifp is NULL I made a new patch. Please undo the previous two and apply this one. git restore sys/dev/usb/net/if_smsc.c git restore sys/net/if_ethersubr.c <apply this patch> <build kernel> <boot> Please make a picture again about what happens. If this works I have a bonus for you. 🤩🤩🤩 With the patch applied change line 1657 in if_smsc.c from "#if 0" to "#if 1". <build kernel> <boot> This should give you the MAC address supplied by Raspberry Pi.
https://cloud.hausen.com/s/MbSfR559QPCTe8c And very early in the boot process something about smsc9...mac...something not found. Flashes very quickly, sorry, no picture.
(In reply to Patrick M. Hausen from comment #26) I once made a movie of a quick scrolling screen with my phone and it was possible to rewind/fast-forward. What kernel are you running? GENERIC or GENERIC-NODEBUG? Or custom kernel?
I do record movies and then take screenshots. Unfortunately that early message is all blurred. I am running releng/14.0 GENERIC.
Created attachment 245616 [details] use bootargs Mmm. I think I missed that I moved some code from a locked region to unlocked. (it used UE_LOCK(ue) in the usb_ethernet.c framework) New patch: don't try to use ether_gen_addr() as ifp == NULL but do parse bootargs now. This code does not use ifp. git restore sys/dev/usb/net/if_smsc.c git restore sys/net/if_ethersubr.c <apply new patch: if_smsc.c.bootargs.diff> <build kernel> <boot> 🤞 I really tested this code as much as possible. Hope it is correct enough.
I am familiar with git, thank you ;-) Compile underway ...
Great success! :-) smsc0 on uhub1 smsc0: <vendor 0x0424 product 0xec00, rev 2.00/2.00, addr 3> on usbus1 smsc0: unit=0 smsc0: /chosen found smsc0: bootargs: coherent_pool=1M snd_bcm2835.enable_headphones=0 snd_bcm2835.enable_hdmi=1 bcm2708_fb.fbwidth=656 bcm2708_fb.fbheight=416 bcm2708_fb.fbswap=1 smsc95xx.macaddr=B8:27:EB:2A:0C:1C vc_mem.mem_base=0x3ec00000 vc_mem.mem_size=0x40000000 console=ttyAMA0,115200 kgdboc=ttyAMA0,115200 console=tty1 root=/dev/mmcblk0p2 rootfstype=ext4 rootwait. smsc0: sizeof(*bootargs)=2048 334 smsc0: mac1=smsc95xx.macaddr=B8:27:EB:2A:0C:1C vc_mem.mem_base=0x3ec00000 vc_mem.mem_size=0x40000000 console=ttyAMA0,115200 kgdboc=ttyAMA0,115200 console=tty1 root=/dev/mmcblk0p2 rootfstype=ext4 rootwait. 16 mac=b8:27:eb:2a:c:1c. smsc0: Ethernet address found in bootargs b8:27:eb:2a:c:1c. smsc0: chip 0xec00, rev. 0002 miibus0: <MII bus> on smsc0 smscphy0: <SMC LAN8700 10/100 interface> PHY 1 on miibus0 smscphy0: 10baseT, 10baseT-FDX, 100baseTX, 100baseTX-FDX, auto ue0: <USB Ethernet> on smsc0 ue0: Ethernet address: b8:27:eb:2a:0c:1c ue0: link state changed to UP smsc0: chip 0xec00, rev. 0002 ue0: link state changed to DOWN ue0: link state changed to UP
Created attachment 246108 [details] use bootargs Cleaned up patch. Less verbose during boot. Could you please test this one more time so I'm sure no small bug got into it? I'm going to try to get this committed.
Compile started, results tomorrow.
---------- /usr/src/sys/dev/usb/net/if_smsc.c:1562:2: error: member reference base type 'void' is not a structure or union smsc_dbg_printf(ue->ue_sc, "bootargs mac=%x:%x:%x:%x:%x:%x.\n", ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /usr/src/sys/dev/usb/net/if_smsc.c:167:22: note: expanded from macro 'smsc_dbg_printf' device_printf((sc)->sc_ue.ue_dev, "debug: " fmt, ##args); \ ~~~~^ ~~~~~ 1 error generated. *** Error code 1 ---------- Patch applied cleanly against releng/14.0
Created attachment 246128 [details] use bootargs (2) That was exactly a cleanup line. Weird that I didn't see the compile error locally. Or would I have uploaded the wrong patch? Anyway... new patch. It compiles on my computer. Plus handled some feedback from the review D42463.
Working perfectly this time: --------- smsc0 on uhub1 smsc0: <vendor 0x0424 product 0xec00, rev 2.00/2.00, addr 3> on usbus1 smsc0: Ethernet address found in bootargs b8:27:eb:2a:c:1c. smsc0: chip 0xec00, rev. 0002 miibus0: <MII bus> on smsc0 smscphy0: <SMC LAN8700 10/100 interface> PHY 1 on miibus0 smscphy0: 10baseT, 10baseT-FDX, 100baseTX, 100baseTX-FDX, auto --------- root@pi8:~ # ifconfig ue0 ue0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500 options=80009<RXCSUM,VLAN_MTU,LINKSTATE> ether b8:27:eb:2a:0c:1c ---------
Any chance this will make it into releng/14.0 in time?
(In reply to Patrick M. Hausen from comment #37) No, it's too late.
Hi Patrick, May you please try the latest patch from https://reviews.freebsd.org/D42463 ? I'm interested in this part ``` smsc_dbg_printf(sc, "No MAC address found. Using ether_gen_addr().\n"); ether_gen_addr_byname(device_get_nameunit(ue->ue_dev), &eaddr); ``` You can test that part without boot args `smsc95xx.macaddr`.
How do I remove the boot arg? It's created automatically from the serial number or some such.
(In reply to Patrick M. Hausen from comment #40) I do not have any PI devices and unable to test the patch. The simplest way could be intentionally changing the definition of BOOTARGS_SMSC95XX to wrong. - #define BOOTARGS_SMSC95XX "smsc95xx.macaddr" + #define BOOTARGS_SMSC95XX "smsc95xx.macaddr.wrong"
Good idea, thanks. Leave the code unchanged as much as possible but make it take the other branch by introducing a "typo". I'll do that and report back.
Mmm, reading the code with the "smsc95xx.macaddr.wrong" typo idea I suspect this can give a NULL reference as strnstr can return NULL if the parameter is not found. + p = strnstr(bootargs, BOOTARGS_SMSC95XX, bootargs_len); + + if (6 != sscanf(p, BOOTARGS_SMSC95XX "=%x:%x:%x:%x:%x:%x%*c", I will change this to: if (p == NULL || 6 != sscanf(p, BOOTARGS_SMSC95XX "=%x:%x:%x:%x:%x:%x%*c",
I'll wait for your new patch, then.
Created attachment 246231 [details] use bootargs Added a NULL check for the result of strnstr. BTW: this patch is also on https://reviews.freebsd.org/D42463 below the button "Download Raw Diff".
With the latest patch applied and then manually changing "smsc95xx.macaddr" to "smsc95yy.macaddr" the system does not come back up again. So it's connect a monitor and shoot some video time again. Tomorrow or Tuesday. Kind regards, Patrick
Sorry - looks like a temporary glitch unrelated to this patch. After removing the USB disk I use for compiling and a powercycle I am back up and running. This is the dmesg: ------- smsc0 on uhub1 smsc0: <vendor 0x0424 product 0xec00, rev 2.00/2.00, addr 3> on usbus1 smsc0: error: invalid mac from bootargs '(null)'. smsc0: chip 0xec00, rev. 0002 miibus0: <MII bus> on smsc0 smscphy0: <SMC LAN8700 10/100 interface> PHY 1 on miibus0 smscphy0: 10baseT, 10baseT-FDX, 100baseTX, 100baseTX-FDX, auto ue0: <USB Ethernet> on smsc0 ue0: Ethernet address: 58:9c:fc:00:5d:3a smsc0: chip 0xec00, rev. 0002 ------- And this is the dmesg after another reboot: ------- smsc0 on uhub1 smsc0: <vendor 0x0424 product 0xec00, rev 2.00/2.00, addr 3> on usbus1 smsc0: error: invalid mac from bootargs '(null)'. smsc0: chip 0xec00, rev. 0002 miibus0: <MII bus> on smsc0 smscphy0: <SMC LAN8700 10/100 interface> PHY 1 on miibus0 smscphy0: 10baseT, 10baseT-FDX, 100baseTX, 100baseTX-FDX, auto ue0: <USB Ethernet> on smsc0 ue0: Ethernet address: 58:9c:fc:00:5d:3a smsc0: chip 0xec00, rev. 0002 ------- So it looks like we have a stable MAC address - woohoo! :-)
@Patrick Would you mind testing one more time? I changed some memory allocation to dynamic so would like to see that it does not crash. :-) Please download the diff from review D42463. All remarks in the review are done. So I hope I can commit it soon.
Comment on attachment 246231 [details] use bootargs patch is obsolete use the diff from review D42463
Of course, but tomorrow. Currently building and releasing Vagrant box updates for 14.0 ... :-)
Sorry folks, I was travelling over the weekend end promptly fell sick after returning on Monday. Latest patch still looks good: ------- smsc0 on uhub1 smsc0: <vendor 0x0424 product 0xec00, rev 2.00/2.00, addr 3> on usbus1 smsc0: MAC address found in bootargs b8:27:eb:2a:0c:1c. smsc0: chip 0xec00, rev. 0002 miibus0: <MII bus> on smsc0 smscphy0: <SMC LAN8700 10/100 interface> PHY 1 on miibus0 smscphy0: 10baseT, 10baseT-FDX, 100baseTX, 100baseTX-FDX, auto ue0: <USB Ethernet> on smsc0 ue0: Ethernet address: b8:27:eb:2a:0c:1c -------
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=3878bbf1bb9e68f8579b57cde7d4e5c77de93320 commit 3878bbf1bb9e68f8579b57cde7d4e5c77de93320 Author: Ronald Klop <ronald@FreeBSD.org> AuthorDate: 2023-11-04 14:14:00 +0000 Commit: Ronald Klop <ronald@FreeBSD.org> CommitDate: 2023-12-07 11:32:01 +0000 Teach if_smsc to get MAC from bootargs. Some Raspberry Pi pass smsc95xx.macaddr=XX:XX:XX:XX:XX:XX as bootargs. Use this if no ethernet address is found in an EEPROM. As last resort fall back to ether_gen_addr() instead of random MAC. PR: 274092 Reported by: Patrick M. Hausen (via ML) Reviewed by: imp, karels, zlei Tested by: Patrick M. Hausen Approved by: karels MFC after: 1 month Relnotes: yes Differential Revision: https://reviews.freebsd.org/D42463 sys/dev/usb/net/if_smsc.c | 86 +++++++++++++++++++++++++++++++++++++++++++++-- sys/net/ethernet.h | 1 + sys/net/if_ethersubr.c | 10 ++++-- 3 files changed, 92 insertions(+), 5 deletions(-)
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=8a0ee306227a17a998bdc7af2275fd94b9164342 commit 8a0ee306227a17a998bdc7af2275fd94b9164342 Author: Ronald Klop <ronald@FreeBSD.org> AuthorDate: 2023-12-07 16:46:58 +0000 Commit: Ronald Klop <ronald@FreeBSD.org> CommitDate: 2023-12-07 16:50:28 +0000 if_smsc: fix build on armv6 & armv7 compile error was: /usr/src/sys/dev/usb/net/if_smsc.c:1597:40: error: format specifies type 'unsigned long' but the argument has type 'ssize_t' (aka 'int') [-Werror,-Wformat] "failed alloc for bootargs (%lu)", len); ~~~ ^~~ %zd PR: 274092 Approved by: karels MFC after: 1 month sys/dev/usb/net/if_smsc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
A commit in branch stable/14 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=028e4c6548e44f3d37210f0087bafb29d49704be commit 028e4c6548e44f3d37210f0087bafb29d49704be Author: Ronald Klop <ronald@FreeBSD.org> AuthorDate: 2023-11-04 14:14:00 +0000 Commit: Ronald Klop <ronald@FreeBSD.org> CommitDate: 2023-12-28 14:40:31 +0000 Teach if_smsc to get MAC from bootargs. Some Raspberry Pi pass smsc95xx.macaddr=XX:XX:XX:XX:XX:XX as bootargs. Use this if no ethernet address is found in an EEPROM. As last resort fall back to ether_gen_addr() instead of random MAC. (cherry picked from commit 3878bbf1bb9e68f8579b57cde7d4e5c77de93320) if_smsc: fix build on armv6 & armv7 compile error was: /usr/src/sys/dev/usb/net/if_smsc.c:1597:40: error: format specifies type 'unsigned long' but the argument has type 'ssize_t' (aka 'int') [-Werror,-Wformat] "failed alloc for bootargs (%lu)", len); ~~~ ^~~ %zd (cherry picked from commit 8a0ee306227a17a998bdc7af2275fd94b9164342) PR: 274092 Reported by: Patrick M. Hausen (via ML) Reviewed by: imp, karels, zlei Tested by: Patrick M. Hausen Approved by: karels Relnotes: yes Differential Revision: https://reviews.freebsd.org/D42463 sys/dev/usb/net/if_smsc.c | 86 +++++++++++++++++++++++++++++++++++++++++++++-- sys/net/ethernet.h | 1 + sys/net/if_ethersubr.c | 10 ++++-- 3 files changed, 92 insertions(+), 5 deletions(-)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=3d96ee7c7dcc54995894985ef2beacc4c4f57956 commit 3d96ee7c7dcc54995894985ef2beacc4c4f57956 Author: Ronald Klop <ronald@FreeBSD.org> AuthorDate: 2023-11-04 14:14:00 +0000 Commit: Ronald Klop <ronald@FreeBSD.org> CommitDate: 2024-01-01 09:13:13 +0000 Teach if_smsc to get MAC from bootargs. Some Raspberry Pi pass smsc95xx.macaddr=XX:XX:XX:XX:XX:XX as bootargs. Use this if no ethernet address is found in an EEPROM. As last resort fall back to ether_gen_addr() instead of random MAC. (cherry picked from commit 3878bbf1bb9e68f8579b57cde7d4e5c77de93320) if_smsc: fix build on armv6 & armv7 compile error was: /usr/src/sys/dev/usb/net/if_smsc.c:1597:40: error: format specifies type 'unsigned long' but the argument has type 'ssize_t' (aka 'int') [-Werror,-Wformat] "failed alloc for bootargs (%lu)", len); ~~~ ^~~ %zd (cherry picked from commit 8a0ee306227a17a998bdc7af2275fd94b9164342) PR: 274092 Reported by: Patrick M. Hausen (via ML) Reviewed by: imp, karels, zlei Tested by: Patrick M. Hausen Approved by: karels Relnotes: yes Differential Revision: https://reviews.freebsd.org/D42463 sys/dev/usb/net/if_smsc.c | 86 +++++++++++++++++++++++++++++++++++++++++++++-- sys/net/ethernet.h | 1 + sys/net/if_ethersubr.c | 10 ++++-- 3 files changed, 92 insertions(+), 5 deletions(-)
Committed to all supported -STABLE branches. Upcoming 13.3 and 14.1 will contain this. Closing the issue. Have fun with it.