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

1008

not needed here

[rgrimes]

share/man/man4/sume.4

2

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

32

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

39

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

60

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

8

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.

15

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

6

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?

32

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.

96

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

[zec]

share/man/man4/sume.4

7

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

30–35

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

sys/dev/sume/adapter.h

8

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

15

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

29–35

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

[rgrimes]

sys/dev/sume/adapter.h

15

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

177

ifp is set once and never used: prune it.

180

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

193

num_chnls is unused, prune it.

197

should be *ifp[SUME_NPORTS];

198

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

760–761

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

832–833

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

1043

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

1289

Where does the constant of -3 come from?

1345

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

[zec]

sys/dev/sume/if_sume.c

122

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

[zec]

sys/dev/sume/if_sume.c

244

Remove redundant parentheses.

420

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

509–512

Convert this to a single-line comment.

1210

Add driver or interface name to the message.

1335–1337

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

456–457

Remove redundant parentheses.

648–671

Remove redundant parentheses.

673–686

Remove redundant parentheses.

1052

Remove redundant parentheses.

1406–1407

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

15

I retract my request to move the grant clause.

[denissal]

share/man/man4/sume.4

32

Is this fixed now?

[zec]

sys/dev/sume/if_sume.c

344

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

353–355

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.

410–412

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

532–534

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

553–556

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

584–585

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

32

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

65

s/Cambridge/the NetFPGA project/

sys/dev/sume/adapter.h

36

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

348

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

613–615

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.

682

Prune empty line.

753–754

Initialized at declaration to reclaim three lines of code.

822–824

Initialize at declaration to reclaim three lines of code.

884–888

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

982–986

Replace with get_modreg_value() which is equivalent.

[zec]

sys/dev/sume/if_sume.c

281–282

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

1468–1469

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

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

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

62

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

88–96

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

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