[new driver] Microchips LAN743X driver for FreeBSD

[emaste]

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.

[aryeeteygerald_rogers.com]

Link to Git repository

[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

[emaste]

sys/dev/mgb/if_mgb.c

1417–1422 ↗

(On Diff #56981)

no K&R in new code

[emaste]

some more style(9)

[jhb]

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]

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]

update some feedback from @jhb

[aryeeteygerald_rogers.com]

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]

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.