NetFPGA SUME Reference NIC (RIFFA DMA) device driver

[denissal]

Summary

This project enables NetFPGA SUME 4x10 Gbps FPGA board to work as a NIC on FreeBSD by creating a driver based on the existing Linux driver for the 'Reference NIC' design using the RIFFA DMA engine from the private NetFPGA-SUME-live repository.

[zec]

sys/dev/sume/if_sume.c

1007 ↗

(On Diff #75843)

not needed here

[rgrimes]

share/man/man4/sume.4

1 ↗

(On Diff #75843)

This file is missing copyright, license and $FreeBSD$.

31 ↗

(On Diff #75843)

man pages usually have a line break at full stops per style guide (full stop == period), This is done to ease future edits.

38 ↗

(On Diff #75843)

"me" should probably not be in a man page.

59 ↗

(On Diff #75843)

Double use of original does not read well here. I would just drop the first one, as it is implied by the second one.

sys/dev/sume/adapter.h

7 ↗

(On Diff #75843)

Factoring out of "All rights reserved" leaves it ambiguous to whom has or has not asserted it. Have all 3 of these? Has only Denis? Technically none of these people need to assert it as this clause is no longer needed on copyrights asserted after August 23, 2000, but one must obtain a copyright holders permission to remove it from There copyright. A more precise way to write the above copyright is:

* Copyright (c) 2015 University of Cambridge  All rights reserved.
* Copyright (c) 2015 Bjoern A. Zeeb  All rights reserved.
* Copyright (c) 2020 Denis Salopek  All rights reserved.

14 ↗

(On Diff #75843)

Could the Grant clause be moved to after the copyright/license clauses so that it does not interfere with the normal flow of those sections?

[denissal]

Updated files as per reviewers' notes.

[rgrimes]

share/man/man4/sume.4

5 ↗

(On Diff #75868)

Since you just wrote this man page how is it possible to have a copyright from Cambridge or BZ, and especially from 2015? Or was some of this text from a linux file?

95 ↗

(On Diff #75868)

Change in tense from past to present (was in last sentence, is in this sentence), the "is" should be changed to a "was".

31 ↗

(On Diff #75843)

man pages usually have a line break at full stops per style guide (full stop == period), This is done to ease future edits.

This has not been done, the text is still flowed to fill margins, which is not the normal form for man pages.

[zec]

share/man/man4/sume.4

6 ↗

(On Diff #75868)

The "All rights reserved" formula is obsolete and can and could be ommited, especially in freshly created files

29–34 ↗

(On Diff #75868)

Neither Stanford nor UCAM have authored this manpage, so this paragraph should be removed.

sys/dev/sume/adapter.h

7 ↗

(On Diff #75843)

The UCAM was not even mentioned as copyright holder in the original Bjoern's Linux driver, so Denis should remove it here and elsewhere.

14 ↗

(On Diff #75843)

Certainly not without a permission from Cambridge people. We already spent a month waiting for a permission from them to (re)use Bjoern's Linux source from their private repo, and in particular to remove an additional proprietary licensing clause that was attached to the file. To proceed with your proposal we should ask UCAM again, but IMO it's not worth it, and we couldn't blame them if they would decline (possibly due to contractual obligations to their sponsors). Therefore, Denis pls. revert this.

sys/dev/sume/if_sume.c

28–34 ↗

(On Diff #75868)

Again, pls. revert, as we have no permission for this change from UCAM.

[rgrimes]

sys/dev/sume/adapter.h

14 ↗

(On Diff #75843)

I see nothing in any part of the grant statement, or in the license that says one word about position or for that mater even appearance of this text in the file. Normally these grants require the author to attach this text to the work, without care to location, and they have not protected there text with anything that says someone can not remove it at a future date for that mater, though I am NOT proposing that be done. Who listed did work under that grant and can speak to the text of the grant agreement?

[zec]

sys/dev/sume/adapter.h

176 ↗

(On Diff #75868)

ifp is set once and never used: prune it.

179 ↗

(On Diff #75868)

riffa_channel is set only once to SUME_RIFFA_CHANNEL_DATA, so de facto is used as constant, hence prune it.

192 ↗

(On Diff #75868)

num_chnls is unused, prune it.

196 ↗

(On Diff #75868)

should be *ifp[SUME_NPORTS];

197 ↗

(On Diff #75868)

should be moved to the top of the structure: fields should be sorted by order of use, because prefetchers work they way forwards, not backwards; and because style(9) says so.

sys/dev/sume/if_sume.c

759–760 ↗

(On Diff #75868)

Since nf_priv->riffa_channel is always CHANNEL_DATA, this KASSERT would always trigger. Did you ever test this with options INVARIANTS?

831–832 ↗

(On Diff #75868)

Since nf_priv->riffa_channel is always CHANNEL_DATA, this KASSERT would always trigger. Did you ever test this with options INVARIANTS?

1042 ↗

(On Diff #75868)

s/nf/sume/, or even better, with SUME_ETH_DEVICE_NAME.

1288 ↗

(On Diff #75868)

Where does the constant of -3 come from?

1344 ↗

(On Diff #75868)

s/nf/sume/, or even better, with SUME_ETH_DEVICE_NAME.

[zec]

sys/dev/sume/if_sume.c

121 ↗

(On Diff #75868)

Description of item (10) extends past 80 columns for no good reason, pls. trim.

[zec]

sys/dev/sume/if_sume.c

243 ↗

(On Diff #75868)

Remove redundant parentheses.

419 ↗

(On Diff #75868)

Fix indentation (s/tab/4 spaces/).

508–511 ↗

(On Diff #75868)

Convert this to a single-line comment.

1209 ↗

(On Diff #75868)

Add driver or interface name to the message.

1334–1336 ↗

(On Diff #75868)

Variable declarations in the middle of function body are discouraged, and #defines as well: pls. move those at the top of the function / file.

[zec]

sys/dev/sume/if_sume.c

455–456 ↗

(On Diff #75868)

Remove redundant parentheses.

647–670 ↗

(On Diff #75868)

Remove redundant parentheses.

672–685 ↗

(On Diff #75868)

Remove redundant parentheses.

1051 ↗

(On Diff #75868)

Remove redundant parentheses.

1405–1406 ↗

(On Diff #75868)

Remove redundant parentheses.

[denissal]

Regarding the licensing issue: I wanted to contact the NetFPGA community just in case and got a response (in the private github repository https://github.com/NetFPGA/NetFPGA-SUME-live/issues/55) :
"quoting a colleague and reflecting my own thoughts…. my intuition is
that if we leave the ACKs between the copyrights and the license
header, they are more likely to be maintained by people copying and
pasting. In the middle is the conventional place to put sponsor ACKs in
my experience, although I’m not sure if that’s the reason (it’s what
I’ve seen in FreeBSD and elsewhere). But maybe there are good reasons
to move it ..?"

So it seems to me it is best that the grant stays in the middle.

[rgrimes]

sys/dev/sume/adapter.h

14 ↗

(On Diff #75843)

I retract my request to move the grant clause.

[denissal]

share/man/man4/sume.4

31 ↗

(On Diff #75843)

Is this fixed now?

[zec]

sys/dev/sume/if_sume.c

343 ↗

(On Diff #76085)

Change the channel iterator name from i to something more intuitive, such as ch, chan, or channel.

352–354 ↗

(On Diff #76085)

Avoid spamming the console with function names (func) here and elsewhere when having the function name in dmesg is not absolutely required to find the offending place in the code.

409–411 ↗

(On Diff #76085)

Without logging the channel number here we don't know whether we have a problem with data or register transfers.

531–533 ↗

(On Diff #76085)

Without logging the channel number here we don't know whether we have a problem with data or register transfers.

552–555 ↗

(On Diff #76085)

This comment could be merged into the comment a few lines above for brevity.

583–584 ↗

(On Diff #76085)

Function name logging here is unnecessary, and inconsistent with other debug messages in the same function, pls. clean up the rest of uninformative func instances.

[rgrimes]

share/man/man4/sume.4

31 ↗

(On Diff #75843)

Not really, but this is not a blocking issue.

[zec]

rgrimes: me being essentially a manpage rookie as well, unfortunately I can't provide an advice to Denis on how proceed from here. None of the lines fill entirely or extend past 80 columns, and all sentences are terminated by line breaks. What else should we improve re. formatting? As non-native English speakers, we can also try to improve text composition and flow a bit, but I guess this is not what you had in mind?

[bz]

share/man/man4/sume.4

64 ↗

(On Diff #76085)

s/Cambridge/the NetFPGA project/

sys/dev/sume/adapter.h

35 ↗

(On Diff #76085)

I'll leave the comment for the sake of leaving it: the reason I have not suggested changing this to the FreeBSD OUI using the dynamic (random) allocator is that some of the NetFPGA infrastructure might still have that hardcoded elsewhere and we'd probably want things to work there (e.g, test suites or others). Not sure to which extend this is still true but I'd leave it for now.

sys/modules/Makefile

372 ↗

(On Diff #76085)

Here we compile it for all architectures; someone probably needs to check this actually does compile on all architectures before committing.

[bcr]

In D26074#581749, @zec wrote:

rgrimes: me being essentially a manpage rookie as well, unfortunately I can't provide an advice to Denis on how proceed from here. None of the lines fill entirely or extend past 80 columns, and all sentences are terminated by line breaks. What else should we improve re. formatting? As non-native English speakers, we can also try to improve text composition and flow a bit, but I guess this is not what you had in mind?

Have you run textproc/igor on the man page? It will give you a few hints on which lines you can make improvements. Also, running "mandoc -Tlint" on the man page will give you warnings about mandoc formatting.

[zec]

sys/dev/sume/if_sume.c

612–614 ↗

(On Diff #76085)

We only have a single interrupt per sume card, so bus_describe_intr() is not needed here: it should be used only to distinguish multiple irq sources associated with a device.

681 ↗

(On Diff #76085)

Prune empty line.

752–753 ↗

(On Diff #76085)

Initialized at declaration to reclaim three lines of code.

821–823 ↗

(On Diff #76085)

Initialize at declaration to reclaim three lines of code.

883–887 ↗

(On Diff #76085)

Call sume_module_reg_read() only if error == 0 to reclaim three lines of code.

981–985 ↗

(On Diff #76085)

Replace with get_modreg_value() which is equivalent.

[zec]

sys/dev/sume/if_sume.c

280–281 ↗

(On Diff #76085)

I suggest to count rx_packets / rx_bytes before checking whether the interface is actually UP (a few lines above). This would help the administrator to determine the exact source of unwanted traffic, plus would prevent SW and HW RX counters from diverging.

[zec]

sys/dev/sume/if_sume.c

1467–1468 ↗

(On Diff #76085)

sume_get_stats() may sleep on adapter->lock in get_modreg_value(), so it must not be scheduled via taskqueue_create_fast() but rather with taskqueue_create(). My kernel navigation skills are too rusty to recall which debugging option should have captured this, I think this will pass under the radar of both INVARIANTS and WITNESS...

[denissal]

Manpage is checked with both igor and mandoc and gives no formatting errors, so I hope everything is ok now.

[bcr]

OK, thanks for the manpage checks. Good to go from our side of things.

[zec]

I've tested the driver on 11.4-STABLE, and it works reasonably well, given the limitations of the (experimental) FPGA design. Denis made a "make universe" run on CURRENT and I did on 12.1 and the current patch didn't introduce any regressions. Hence, I'm inclined to commit this to HEAD as-is, ideally with a few cosmetic / non-functional changes I suggested in most recent comments. I intend to put on record Reviewed by: zec, bz (source), rgrimes, bcr (manpage)? Any further objections from the reviewers?

share/man/man4/sume.4

55 ↗

(On Diff #76281)

Insert "single" before "DMA transaction", to clarify the inefficiency of the reference NIC DMA design and the huge PIO PCI traffic overhead (to set up the DMA) associated with each packet transfer.

56 ↗

(On Diff #76281)

Drop "in" after "yields", I don't think it is necessary.

62 ↗

(On Diff #76281)

There's also no support for multicast filters...

88–96 ↗

(On Diff #76281)

Having played with the driver over the past few days, it would always successfully recover from rare glitches (presumably in FPGA logic(. Hence, I'm curious whether this comment is still valid and needed, and if not, I'd vote for removing it, or at least to trim it down and simply say that .

I think it would be more important to notify users about the current FPGA design's inability to stop physical ports which have been configured as DOWN from consuming PCI / memory bandwidth, interrupts and CPU cycles, and that there's absolutely noting we can do about it in the driver.

sys/dev/sume/if_sume.c

165 ↗

(On Diff #76281)

I suggest changing the device description to "NetFPGA SUME reference NIC", since the function of the card varies from one FPGA to another - you already have a different driver for the "NICv2" in the works, which I guess binds to another PCI device ID...