Details

Reviewers

emaste
gonzo
• hselasky

Commits

rS333713: Add driver for Microchip LAN78xx USB3-GigE controller

Summary

This is a driver for Microchip's LAN78xx family.

What's left to do:

RX/TX Checksum Offloading: most of the code that handles RX checksum offloading is there, it works with ICMP packets, but it seems to break TCP/UDP packets.
PHY Driver: as of now this driver is using the generic ukphy driver, but an additional lan78xxphy would be more optimal for the chip (I think).
Perfect Address Filtering and Hash Address Filtering: the code for this is provided, but I haven't tested whether the filtering actually works yet.
VLAN Tag Filtering: not done yet.
USB Interrupt Endpoints: they're enabled but not used yet, the driver polls on the MII status, similar to the if_smsc driver so usb interrupts were not really needed. It would be nice to have though.
Wake Up and Suspend: Linux seems to have some infrastructure for handling this, I saw it being used in their driver. It would be really nice to add wake up and suspend capability to FreeBSD as well.
Makefile: The current Makefile provided works for building the driver, but I believe in order to get this to be built with buildkernel, there needs to be some more changes made to some Makefiles in the upper directories.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

arshankhanifar_gmail.com created this revision.Apr 23 2018, 2:01 PM

Herald added a subscriber: imp. · View Herald TranscriptApr 23 2018, 2:01 PM

Please align the constants in the header file

sys/dev/usb/net/if_lan78xx.c
4–5	Let's collapse these to one line.

emaste added subscribers: manu, db.Apr 24 2018, 7:29 PM

style fixes, removed all the TODOs in the code, and added a REMAINING FEATURES section.

diff wrt the base of this project.

emaste added inline comments.Apr 27 2018, 5:43 PM

sys/dev/usb/usbdevs
297 ↗	(On Diff #41932)	These are ordered by ID it seems

Turns out SMC2 is the old Microchip. So I used that instead. We need to update the vendor description though! SMC2 doesn't represent microchip, which is the current company that makes micro things. :P

reverted it back to use SMC2 as the vendor id.

emaste added inline comments.Apr 27 2018, 7:38 PM

sys/dev/usb/net/if_lan78xx.c
5–6	Collapse to one line. `* Copyright (C) 2012 Ben Gray <bgray@freebsd.org>.`
39	`Microchip LAN78XX USB-Ethernet driver` and link should probably go to https://www.microchip.com/wwwproducts/en/LAN7800 (i.e., to the direct product page).
44	I'm not sure that's true - it's based on a number of different things, but is probably most closely based on the if_smsc driver?
48	probably "UNIMPLEMENTED FEATURES" or such
57	`Pi` Since the R-Pi3B+ is the primary use case for this driver right now we should mention what happens today (i.e., do we chose a random address? should the user provide one manually? etc.)
104	This seems like a strange `#define`
160	/* * Multi-line comments should be * like this. / i.e. full sentences and begin and end are the `/` and `*/` on their own.
203	Don't need the `#if` guards, all supported versions of FreeBSD are > 1000000.
276	Remove these blank lines.
sys/dev/usb/net/if_lan78xxreg.h
30	Ethernet
32	probably 'taken' rather than 'brought'
33	maybe `from the Linux driver (lan78xx.h)` so there's a direct reference to the filename
sys/modules/usb/lan78xx/Makefile
5	old (c) statement. probably we should just omit the copyright and license from the makefile.

andreast added a subscriber: andreast.Apr 29 2018, 3:48 PM

andreast added inline comments.

sys/dev/usb/net/if_lan78xxreg.h
126	Should be AN78XX_E2P_CMD_ADDR_MASK ?

arshankhanifar_gmail.com marked 14 inline comments as done.Apr 30 2018, 2:49 PM

arshankhanifar_gmail.com added inline comments.

sys/dev/usb/net/if_lan78xx.c
104	in the `if_smsc` driver this is USB_DEBUG_VAR. I think there are tunables that enable debug mode for the driver.
104	I changed it to `USB_DEBUG_VAR` to be the same as `if_smsc` driver
sys/dev/usb/net/if_lan78xxreg.h
126	yeah I must've made a mistake when fixing the tabs. I'll run a build before every commit to catch mistakes like this from now on.

more style fixes, typo fixes, and comment fixes

arshankhanifar_gmail.com marked 3 inline comments as done.Apr 30 2018, 2:51 PM

More fixes, informally discussed with @emaste

andreast added inline comments.Apr 30 2018, 6:48 PM

sys/modules/usb/lan78xx/Makefile
36	Is this lan78phy.c needed, if so, where is it?

removed incomplete PHY driver from makefile.

arshankhanifar_gmail.com marked an inline comment as done.Apr 30 2018, 7:37 PM

arshankhanifar_gmail.com added inline comments.

sys/modules/usb/lan78xx/Makefile
36	I was working on a PHY driver in parallel to this. Currently this driver resolves to `ukphy` driver. I've removed it. Thanks for pointing this out.

arshankhanifar_gmail.com marked 2 inline comments as done.Apr 30 2018, 7:38 PM

removed the FDT includes, as they're for reading the MAC address from RPIs and are not implemented yet.

Added a few subscribers with USB network experience

This is the glue needed to build as a ko. If you add it to the KERNCONF file like 'device if_lan78xx # PI3b+ USB NIC' then you build it inline.

Index: sys/conf/files

sys/conf/files (revision 333092)

+++ sys/conf/files (working copy)
@@ -3317,6 +3317,7 @@
dev/usb/net/if_cue.c optional cue
dev/usb/net/if_ipheth.c optional ipheth
dev/usb/net/if_kue.c optional kue
+dev/usb/net/if_lan78xx.c optional if_lan78xx
dev/usb/net/if_mos.c optional mos
dev/usb/net/if_rue.c optional rue
dev/usb/net/if_smsc.c optional smsc

Index: sys/modules/usb/Makefile

sys/modules/usb/Makefile (revision 333092)

+++ sys/modules/usb/Makefile (working copy)
@@ -47,7 +47,7 @@
SUBDIR += ${_dwc_otg} ehci ${_musb} ohci uhci xhci ${_uss820dci} ${_at91dci} \

	  ${_atmegadci} ${_avr32dci} ${_rsu} ${_rsufw} ${_saf1761otg}

SUBDIR += ${_rum} ${_run} ${_runfw} ${_uath} upgt usie ural ${_zyd} ${_urtw}
-SUBDIR += atp cfumass uhid ukbd ums udbp ufm uep wmt wsp ugold uled
+SUBDIR += atp cfumass lan78xx uhid ukbd ums udbp ufm uep wmt wsp ugold uled
SUBDIR += ucom u3g uark ubsa ubser uchcom ucycom ufoma uftdi ugensa uipaq ulpt \

	  umct umcs umodem umoscom uplcom uslcom uvisor uvscom

SUBDIR += udl

lan78xx isn't a very FreeBSD-like device name: It doesn't start with 'u' like most other usb lan drivers and we usually don't name them so directly. History has shown that this driver will either (a) be used for a lan79<mumble> part or the lan7850, for example, will need a new driver. You don't have to rename all the variables / functions, but please consider selecting a different name,

In D15168#321360, @imp wrote:

(a) be used for a lan79<mumble>

That's already the case - we developed this driver to support the USB-Ethernet on the Raspberry Pi 3B+ which has a LAN7515.

Driver seems to follow the yellow brick road of the existing USB ethernet drivers. Some comments added.

sys/dev/usb/net/if_lan78xx.c
996	FreeBSD style: switch<space>()...{ case XXX: }
1182	There should be a check for every usbd_copy_out() that the offset is valid and contains at least XXX bytes of data.
1522	Empty lines??

This revision is now accepted and ready to land.Apr 30 2018, 8:57 PM

arshankhanifar_gmail.com marked 3 inline comments as done.May 1 2018, 12:41 AM

arshankhanifar_gmail.com added inline comments.

sys/dev/usb/net/if_lan78xx.c
1182	Each rx_cmd_* is 4 bytes. How can I check whether they contain 4 bytes of data? What if their value is intentionally zero?

Added error checking for offset in mii read callback.

This revision now requires review to proceed.May 1 2018, 12:42 AM

emaste added inline comments.May 1 2018, 2:41 AM

sys/dev/usb/net/if_lan78xx.c

996

i.e.

switch (usbd_get_speed(sc->sc_ue.ue_udev)) {
case USB_SPEED_SUPER:        
        burst_cap = LAN78XX_DEFAULT_BURST_CAP_SIZE/LAN78XX_SS_USB_PKT_SIZE;
        break;
case USB_SPEED_HIGH:        
        burst_cap = LAN78XX_DEFAULT_BURST_CAP_SIZE/LAN78XX_HS_USB_PKT_SIZE;
        break;
default:
        burst_cap = LAN78XX_DEFAULT_BURST_CAP_SIZE/LAN78XX_FS_USB_PKT_SIZE;
        break;
}

• hselasky added inline comments.May 1 2018, 8:52 AM

sys/dev/usb/net/if_lan78xx.c
1182	You need to do something like this: if ((off + 4) > actlen) goto tr_setup;

more accurate error checking, style fixes.

What is the preferred name for this driver? I basically used LAN78XX because that was the naming scheme linux went with. It's kind of confusing as this driver is used for also LAN7515 on raspberry pi's. My suggestion is: if_umicro.c since it's for microchip, and it starts with a u like other FreeBSD's usb-to-ethernet drivers. However, that naming doesn't have any info about the model.

Looking at existing examples,

File	Devices
if_aue	ADMtek AN986 Pegasus
if_axe	ASIX AX88x7x/760
if_axge	ASIX AX88178A/AX88179 GigE
if_cdce	USB Communication Device Class
if_cue	Computer Access Technology Corporation (CATC) USB-EL1210A
if_ipheth	Apple iPhone / iPad tethered Ethernet
if_kue	Kawasaki LSI KL5KUSB101B
if_mos	Moschip MCS7730/MCS7830/MCS7832
if_rue	RealTek RTL8150
if_smsc	SMSC LAN9xxx
if_udav	Davicom DM9601
if_ure	RealTek RTL8152
if_urndis	Remote NDIS
if_usie	No man page

So some ideas:

if_mchpge (MCHP = Microchip short form / stock ticker)
if_muge (Microchip USB Gigabit Ethernet)
if_mlge (Microchip LANxxxx Gigabit Ethernet)
if_mluge (Microchip LANxxxx USB Gigabit Ethernet)

After a brief IRC discussion it sounds like if_muge is the preferred option.
Aside, this exercise highlights that we need a man page for the new driver too.

emaste added inline comments.May 1 2018, 11:26 PM

sys/dev/usb/net/if_lan78xx.c
1416	These all should be /* * This form. */
1440	overly long line
1454	strange indentation

I guess muggle is right out :)

changed the file name, and variables. build passes.

Looks good to me.

This revision is now accepted and ready to land.May 8 2018, 8:15 AM

I added the pid in rS333489, as well as the pid for the LAN7801 (which Microchip sent me after the co-op term ended). I'll commit the driver with LAN7800 support only, then see if the 7801 "just works" or if we need additional changes.

style(9) issue - should not have trailing whitespace (tabs or spaces) at the end of the line
I will clean these up before commit

yeah I must've forgotten to do that sorry. Thanks!

sys/dev/usb/net/if_lan78xx.c
1454	Sorry I can't see it. Where?

Closed by commit rS333713: Add driver for Microchip LAN78xx USB3-GigE controller (authored by emaste). · Explain WhyMay 17 2018, 2:05 PM

This revision was automatically updated to reflect the committed changes.

Microchip's LAN78XX driver for FreeBSD
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Index: sys/conf/files

Index: sys/modules/usb/Makefile

Revision Contents
Changeset List

Diff 41761

sys/dev/usb/net/if_lan78xx.c

sys/dev/usb/net/if_lan78xxreg.h

sys/modules/usb/lan78xx/Makefile

Microchip's LAN78XX driver for FreeBSDClosedPublicActions

Details

Diff Detail

Event Timeline

Index: sys/conf/files

Index: sys/modules/usb/Makefile

Revision ContentsChangeset List

Diff 41761

sys/dev/usb/net/if_lan78xx.c

sys/dev/usb/net/if_lan78xxreg.h

sys/modules/usb/lan78xx/Makefile

Microchip's LAN78XX driver for FreeBSD
ClosedPublic
Actions

Revision Contents
Changeset List