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 - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

emaste edited reviewers, added: aryeeteygerald_rogers.com; removed: emaste.
  • 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
sys/dev/mgb/if_mgb.c
1417–1422 ↗(On Diff #56981)

no K&R in new code

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

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.