New driver for Broadcom NetXtreme-C and NetXtreme-E devices

[shurd]

Summary

Supports 10/25/40/50Gbps devices in the "Cumulus" famuly.

Test Plan

Testing traffic passing on amd64 and a kernel universe

[shurd]

More cxgb problems - sorry guys.

[rpokala]

I'm entirely unqualified to comment on the actual driver, but I can nit-pick on some of the surrounding infrastructure.

share/man/man4/bnxt.4

94

"throug" -> "through"

100

"three positive integers," -> "three positive integers:"

108

"two positive integers," -> "two positive integers:"

115

Sentence fragment

127

The following
.Xr sysctl 8
variables can be changed at any time:

146

Please describe when the random value is generated - boot time? driver load time? One value for all bnxtX instances, or does each bnxtX have its own value?

151

Under what situation would this ever be written? The driver code has a minimum firmware API that it is written against, so why would/should a user ever change this value?

154

The following
.Xr sysctl 8
variables are read-only:

186

Since there are cases where the device may need to be reset, please document how to reset the device.

199

This should be 12.0, because that's what -HEAD is now. IFF it's MFCed into 11.0, then this should be updated at that time.

sys/boot/forth/loader.conf

315

Perhaps "Broadcom 57x0y PCI 10/25/40/50 Gigabit Ethernet" would be more accurate?

Or "Broadcom NetXtreme-C/NetXtreme-E", as in sys/conf/NOTES

sys/conf/NOTES

2121

You probably didn't mean to comment out cxgb. :-)

sys/conf/files

1186

It looks like this list is in alphabetical order, so this should go to the end.

1275

Same here - probably not intended.

sys/modules/Makefile

537

Why do you hate cxgb? ;-)

[gallatin]

Mostly looks great. A few general comments:

• I didn't see SIOCGI2C support (for reading sfp/sfp+/qsfp.. power/temp/etc). Adding this (in a future patch) would be nice
• "real" RSS support would be nice. With that said, I appreciate the hooks you have to change the keys, etc.
• Ravi had some good nit-picks.
• One additional nit is that I don't think that // style comments are technically allowd by style(9).

I don't see anything that I'd regard as a blocking issue though.

sys/dev/bnxt/bnxt_txrx.c

431

So it seems like we're not going to have options RSS support in the driver until this is figured out; that's kind of too bad

[shurd]

Incorporate feedback.

[shurd]

In D7551#157043, @gallatin wrote:

• I didn't see SIOCGI2C support (for reading sfp/sfp+/qsfp.. power/temp/etc). Adding this (in a future patch) would be nice

This should be trivial to add. I'm not sure how to test it though... is there a tool that uses it I can use?

• One additional nit is that I don't think that // style comments are technically allowd by style(9).

Fixed.

share/man/man4/bnxt.4

146

Fixed

"generated for each device during attach"

151

The minimum a driver is written against may be missing features provided in later API updates. For example, 1.2.2 doesn't support Qos. If you want to be warned if the version is too old for what you need, you could set this.

Alternatively, I could remove this sysctl.

186

I'm actually not certain if there's a way to issue a PCI reset to a device under FreeBSD. If not, a system reboot would likely be required. I could change this to say the system needs to be rebooted, but I'm not *positive* there's no way to do a PCI reset.

sys/conf/files

1275

Shouldn't have been part of the patch. Changes here removed.

sys/dev/bnxt/bnxt_txrx.c

431

Yeah, this is a matter of tracking down someone who knows how this field is populated and getting it documented.

sys/modules/Makefile

537

Heh, I was getting errors in my universe builds from it at one point then I forgot I had commented it out.

[rpokala]

share/man/man4/bnxt.4

151

If the firmware doesn't support a given feature, I would expect an attempt to enable or configure that feature to fail, loudly and informatively. IMHO, the version should be hardcoded to whatever firmware API version the driver was actually written against, and warnings should be unconditional if the firmware API doesn't support the attempted operation.

186

I think @jhb can give you more info about PCI resets, since he's done a bunch of work with the FreeBSD PCI stack.

[jhb]

share/man/man4/bnxt.4

186

Right now we do not provide a way to do a reset in the PCI bus layer (though I will probably be adding one "soon" as I need FLR for PCI pass through with bhyve). I've seen other drivers that want to do resets by doing a secondary bus reset on the parent end of a PCI-e link, so I'd like to have a generic pci_* method for that as well.

I'd actually like to have a 'devctl reset' command, but I'm far less certain of how that should work for a device for which a driver is attached. Some drivers probably can tolerate a reset if they are notified in some way. Initially though I just want to provide pci_* methods to do standard PCI resets that drivers can at least use.

[shurd]

Update manpage to indicate a PCI device reset is required to recover from
HWRM timeouts, and mention that at present a system reboot is required to
do a PCI device reset.

[wblock]

share/man/man4/bnxt.4

93

s/The following/These/
(see below)

94

Please keep macros like this on their own lines:

​.Xr loader.conf 5
or through the use of

95

As above:

.Xr kenv 1 .
These are provided by the

96

Same deal:

.Xr iflib 9
framework, and may be better documented there.

Also, s/may/might/

99

Please start new sentences on a new line.
Both of these sentences start with "This", which is indefinite and can make it hard to tell which thing is being talked about. Better to replace the second one with something more definite, like "The value is a comma..."

101

"respectively" might not be needed, particularly if the output labels the values.

103

Does not make it clear whether this is
4*(receivering+aggregationring)
or
(4*receivering)+aggregationring

104

"should" usually means a recommendation. Maybe better as:

zero means to use the default.

107

Again, the repeated "This". Seeing now that these are items in a table list, I suggest that such tables usually read better when the "this" is removed from the description. Also, please start new sentences on new lines.

It Va dev.bnxt.X.iflib.override_ntxds
Override the number of TX descriptors for each queue.
This is a comma...

111

See above about rewriting this to avoid "should".

114

Maybe

When set, allows...

115

Remove "this":

If not set, ...

118

Passive -> active. Also, please start new sentences on new lines.

Set the number of RX queues.

119

Dangling... and kind of hard to reword because the meaning is a little unclear. Maybe

the number of cores on the socket connected to the controller.

120

Missing period?

122

s/Sets/Set/

123

The "in" here was "on" above, but rearrange and clarify.

126

s/The following/These/

131

passive->active

s/This requires/Require/

132

Start new sentences on new lines.

134

Missing period?

136

When non-zero, the NIC strips VLAN tags on receive.

137

Missing period?

139

Remove "this", passive -> active:

Enable buffering rather than dropping frames when there are no available

140

Dangling:

host RX buffers for DMA.

141

Missing period?

143

"A" is not needed, just start with "Comma".
New sentence to new line.

144

Missing period?

146

Might be better without "The".

150

New sentence to new line.

153

Missing period?

156

No, please avoid "the following", because it's bloated and overused and usually misused. Just "These" is almost always better.

161

s/The current/Current/
New sentence to new line.
Quoted value is probably better with markup, although it is too early in the morning to think which would be best. Maybe .Va.

162

"via" usually means a route, ifconfig name is a command (which I am unsure about for markup, but have seen .Cm misused for it)... Also, new sentence to new line, and "This sysctl" is redundant:

this can be changed with
.Cm ifconfig name .
Allows correlating an

165

"Various" is not really useful here. "part" can also be misunderstood to mean "a section of". How about:

Information about the NVRAM device which contains the firmware.

167

As above, "various" is not needed. The second sentence that tells the reader the next thing is, well, following, can be replaced entirely with a colon. Some people would put a dash in Version-related.

Version related information about the device and firmware:
.It ...

170

s/The supported/Supported/

172

s/The HWRM/HWRM/

174

s/Various per-queue/Per-queue/

176

s/The number/Number/
New sentence to new line.
s/may seem/might seem/

177

s/this count/the count/

178

s/resons/reasons/
s/ethernet/Ethernet/ (in my opinion, but depends on the rest of the document)

184

"May" is generally for permission, as in "it is allowed for there to be an API mismatch". "might" is usually better for possibility, like here.

[shurd]

Integrate manpoage feedback

share/man/man4/bnxt.4

101

The output does not label them, it's just a simple comma separated list.

103

Changed to "the size of the aggregation ring plus four times the size of the receive ring".

[wblock]

Man page looks okay to me. Worth running igor -R bnxt.4 | less -RS and mandoc -Tlint bnxt4 on it, just in case. Please remember to update .Dd also. Thanks!

[shurd]

Accept