new driver: Tehuti networks tn40xx device driver.
Needs ReviewPublic
Actions

Authored by ami_tehutinetworks.net on Jan 16 2019, 1:31 PM.

Details

Reviewers

kevans
shurd
erj

Group Reviewers

network
iflib

Summary

~kevans/tn40xx-build.diff

Added tn40xx

Changes after first round of tests.

Diff Detail

Lint

Lint Warnings

Severity	Location	Code	Message
Warning	sys/dev/tn40xx/TLK10232_phy.h:	CHMOD1	Invalid Executable
Warning	sys/dev/tn40xx/tn40_mbuf.c:	CHMOD1	Invalid Executable

Unit

No Test Coverage

Build Status

Buildable 21982
Build 21216: arc lint + arc unit

Event Timeline

ami_tehutinetworks.net created this revision.Jan 16 2019, 1:31 PM

Harbormaster completed remote builds in B21982: Diff 52891.Jan 16 2019, 1:31 PM

linimon retitled this revision from Tehuti networks tn40xx device driver. to new driver: Tehuti networks tn40xx device driver..Jan 19 2019, 2:45 PM

lwhsu added a reviewer: kevans.Feb 9 2019, 11:01 AM

Hi,

I'll drop in to provide some initial review within the next couple of days, now that I've done a bit of dabbling in the area. Apologies for the delay.

Also, there's some kind of "update" link on the right hand side of this revision that you can use to update the diff when the time comes. I'll commandeer/abandon the other copy of this review to reduce confusion. =)

kevans mentioned this in D17607: [new driver] Tehuti networks tn40xx device driver..Mar 15 2019, 2:36 AM

Adding a couple more reviewers, because I'm afraid I can't provide actionable feedback. I suspect this should be rebased on top of iflib, so tagging the iflib folk to see if they can provide some guidance.

I was wondering if we can have a man page along with this as my first question was "what kind of driver is this?" Sadly the description of this review doesn't say much.

It would also be good to know which parts of the driver are "common core" (possibly shared with other OSes) and which are FreeBSD specific. There is not much point in reviewing style etc on shared code.

Most files seem to be missing a license and one files has a GPL license hint, which means it'll need core approval [ https://www.freebsd.org/internal/software-license.html ]. If this is "vendor" code it would be nice if it could be re-licensed.

There is code which is clearly FreeBSD specific that needs to be converted to comply with style(9).

Why did you feel the need to write your own mbuf copy and defrag routines?

Did you consider using iflib? That way you would not have to deal with parsing packets, or mbufs or even busdma. There is a fairly clean interface in iflib, such that iflib parses mbufs and then passes DMA s/g lists to a drivers transmit routine. This is used by the Intel and Broadcom drivers (among others).

sys/dev/tn40xx/tn40_hw.c
2888	Didn't you just increase nr_frags when we you added the new segment around line 3004? FWIW, if you care at all about packet rate, it would likely to faster to confirm you have trailing space on the mbuf up to 60b, and zero-pad it out to 60b. That way you can just have 1 DMA segment across PCIe rather than 2.