Page MenuHomeFreeBSD

[new driver] Microchips LAN743X driver for FreeBSD
ClosedPublic

Authored by emaste on Apr 26 2019, 10:33 PM.

Details

Summary

This driver adds support for LAN7430/LAN7431.

What's left to do:

  • Multi-buffer packets (RX and TX)
  • Multiple RX queues
  • Checksum offloading
  • Other H/W Supported functions
    • WoL
    • LSO
    • RSS
    • MAC Stats and debugging info
    • VLAN
    • Recieve Packet Filtering

Depends on D19946

Test Plan

Tested by pinging device.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

emaste commandeered this revision.May 2 2019, 7:28 PM
emaste edited reviewers, added: aryeeteygerald_rogers.com; removed: emaste.
emaste updated this revision to Diff 56981.May 2 2019, 7:31 PM
  • rebase on updated D19946 and now-committed D20142
  • pass newly-added IFLIB_DRIVER_MEDIA flag from D19946
  • apply some style(9)
  • add man page
linimon retitled this revision from Microchips LAN743X driver for FreeBSD to [new driver] Microchips LAN743X driver for FreeBSD.May 5 2019, 5:16 PM
emaste added inline comments.May 6 2019, 12:52 AM
sys/dev/mgb/if_mgb.c
1417–1422 ↗(On Diff #56981)

no K&R in new code

emaste updated this revision to Diff 57081.May 6 2019, 1:30 AM

some more style(9)

jhb added a subscriber: jhb.May 6 2019, 5:12 PM
jhb added inline comments.
sys/dev/mgb/if_mgb.c
73 ↗(On Diff #57081)

sys/param.h is always first and in this case should replace sys/types.h. Also, style is to sort headers and not really break them up into blocks like this.

202 ↗(On Diff #57081)

Not your fault, but device_register is horrible. It is completely iflib specific and shouldn't have the generic name it has. Also, it seems there should be a template iflib driver you can inherit from as a template that has the various device_foo methods mapped to iflib_device_foo so you don't have to duplicate all that.

230 ↗(On Diff #57081)

So why not use this always?

284 ↗(On Diff #57081)

Why are there two driver_t's and two devmethod arrays? Can't there just be one?

399 ↗(On Diff #57081)

Normally attach isn't really a hot path such that we sprinkle __builtin_expect(). In FreeBSD we only use that in places where it actually matters (and ideally with some kind of measurement to show that it matters)

639 ↗(On Diff #57081)

Much better to export these as sysctl nodes.

1218 ↗(On Diff #57081)

Having a constant for this register would be nice along with constants for the subfields, or at least a comment in the header explaining what the subfields are.

1236 ↗(On Diff #57081)

Having this be part of a 'alloc_regs' function is somewhat confusing. (pci_enable_busmaster means "I want permission to initiate DMA"). Most drivers just inline this into attach anyway without a subroutine. You could at least move pci_enable_busmaster directly into attach.

1254 ↗(On Diff #57081)

Most drivers don't bother doing this on detach.

sys/dev/mgb/if_mgb.h
259 ↗(On Diff #57081)

Is this even used? The C file uses a 'pci_vendor_info_t'?

263 ↗(On Diff #57081)

Did you mean to define a global variable in all the files that include this?

290 ↗(On Diff #57081)

Debugging leftover?

300 ↗(On Diff #57081)

These macros do seem to be a bit of an obscure way of dealing with a 'struct mgb_ring_info' defined in the #if 0'd comment.

emaste marked 4 inline comments as done.May 6 2019, 5:48 PM
emaste added inline comments.
sys/dev/mgb/if_mgb.h
259 ↗(On Diff #57081)

Appears completely unused.

263 ↗(On Diff #57081)

Almost certainly not :) That said only if_mgb.c will include if_mgb.h.

Aside, I noticed Linux drivers typically do not prefix register definitions etc. in their driver .h file, presumably because it will only be included by the driver itself.

emaste updated this revision to Diff 57093.May 6 2019, 5:50 PM
emaste marked 2 inline comments as done.

update some feedback from @jhb

sys/dev/mgb/if_mgb.c
284 ↗(On Diff #57081)

This is how all the other iflib drivers do it but I think they could be combined.

sys/dev/mgb/if_mgb.h
300 ↗(On Diff #57081)

Yes, all of these are obsolete.

They were defined prior to having multiple dma queues. If we revert back to a single dma'd block (which is preferable) then they might come in handy. I defined the macros because I don't think the struct would work quite the way we would want since it requires allocating extra space for the pointer.

emaste added a subscriber: marius.May 8 2019, 12:44 PM
emaste added a comment.Nov 6 2019, 7:46 PM

D21240 moved ETHER_IS_ZERO to ethernet.h, need to update this. Most likely I will commit the WIP (connected to the build or not) so that changes in the future can update this driver too.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 6 2019, 7:51 PM
This revision was automatically updated to reflect the committed changes.