Page MenuHomeFreeBSD

Man page for AMD 10GbE driver
ClosedPublic

Authored by rajesh1.kumar_amd.com on Dec 28 2020, 12:23 PM.

Diff Detail

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Thank you for writing this man page. A few style issues I've marked inline. You can run "mandoc -Tlint" and textproc/igor on it to check it further.

share/man/man4/axp.4
32

You can remove this, we don't use it anymore now that we are on git.

119

You need to make a line break here after the sentence stop.

134

Two line breaks needed here at each of the sentence stops in this line.

144

Another line break needed here.

152

Line break needed here after the sentence stop.

173

Line break after "effect."

193

Line break here.

207

You can replace FreeBSD with .Fx like above. Break the lines in this whole paragraph while you're here.

brueffer added inline comments.
share/man/man4/axp.4
58

superfluous "the"

59

Processors -> processors

61

"," -> ":"

71

One space too many before "(TSO)"

91

along with AMD EPYC processors.

95

variables -> variables: (add colon)

97

Since the driver name is axp, are the devices named axp0 or ax0? May be nice to have all names the same, e.g. either axp or ax but not mixed.

99

queues -> queue

102

Terminating "." missing.

141

Maybe a Phab issue, but I see empty lines here (and more further down) instead of ".It" entries.

198

missing space between 4 and ,

205

Missing space between 0 and .

211

Missing terminating dot, e.g. ".Aq Mt andrew@FreeBSD.org ."

216

The punctuation at the end looks a bit strange; can it be just ".An Advanced Micro Devices Inc."?

greg_unrelenting.technology added inline comments.
share/man/man4/axp.4
39

"10Gigabit" is kind of an odd spelling, usually only shortened versions are written without a space ("10G", "10GbE"). Same below

rajesh1.kumar_amd.com marked 22 inline comments as done.

V2 patch with review comments addressed

Thanks for the comments.

Addressed almost all of the previous comments. Mentioned the reason why the sysctls are name "ax" rather than "axp". Please let me know if there are any more comments.

share/man/man4/axp.4
39

Changed to "10G" in all places

97

The Driver name is "axp", but the interfaces come as "axX" (ax0, ax1 etc.,). We name the driver as "axp"(PCI-E based), since there is another old version of the driver which is ACPI based (which is named as axa).

Apart from driver name, rest all notations will be "ax"(interface name, sysctl names). Please let me know if it still needs to be changed to a better way.

99

Corrected another place as well.

141

I have filled with empty lines with .Pp macro. Please correct if wrong.

rpokala added inline comments.
share/man/man4/axp.4
97

In that case, I suggest that the driver should be ax(4), and there should be both a PCIe-based and an ACPI-based attachment mechanism. That would allow both to be used, with only a fairly small amount of attachment-specific code.

For example, see ipmi(4), which has both ipmi_acpi.c and ipmi_pci.c (amongst other attachments). Also ig4(4).

It might make sense to refactor things that way now, even if the initial submit only supports PCIe attachment, so adding ACPI later is easier.

This is just unsolicited advice; feel free to ignore me. :-)

share/man/man4/axp.4
136

The code as described in https://reviews.freebsd.org/D27797 refers to

dev.ax.netmap_test

Not:

dev.ax.X.netmap_test

Meaning this is considered a global device variable and not interface-specific. In my opinion either way works, but making it interface-specific allows for more flexibility regarding the use of netmap.

In any case, a decision should be made.

167

Same as

dev.ax.X.netmap_test
rajesh1.kumar_amd.com added inline comments.
share/man/man4/axp.4
97

@rpokala , Sorry about the delayed response.

Better I will change the driver name to "ax" rather than "axp" and retain the acpi variant as "axa" as now. ACPI variant driver is for an older hardware version. It's unsure whether someone uses it still.

We have another patch D27797 for the driver under review. Also, renaming driver involve changes in source files and config files. So, I will place them as a separate patch later along with the corresponding documentation changes after the above mentioned review is complete.

Please let me know if you have any comments.

136

@stephan.dewt_yahoo.co.uk, sorry about the delayed response

For now, as keeping the sysctl interface specific gives some flexibility, let's retain it same way. Based on need, we shall change it later accordingly.

167

For now, lets keep it interface specific as it gives some flexibility.

rajesh1.kumar_amd.com marked 3 inline comments as done.

V3 patch addressing previous comments.

. Mandoc and Igor issues addressed
. Updated sysctl "netmap_test" to "single_fl" pertaining to driver changes in D27797

share/man/man4/axp.4
136

In that case we need to change the driver code to be interface-specific.

gbe added a subscriber: gbe.

LGTM, since all previous comments are addressed.

share/man/man4/axp.4
97

@rpokala , Sorry about the delayed response.

Better I will change the driver name to "ax" rather than "axp" and retain the acpi variant as "axa" as now. ACPI variant driver is for an older hardware version. It's unsure whether someone uses it still.

I somehow missed that this review is just for the manpage, not the driver as a whole. Of course, the manual must match the driver.

Sorry for the noise.

LGTM, since all previous comments are addressed.

Thanks @gbe for accepting it.

We have some driver changes post this acceptance. I will submit an updated manpage accordingly.

@rpokala, No worries. Thanks for your inputs. I will take care to make changes to have sysctl and driver names in sync.

@gbe, I see this manpage is still not in the main branch for FreeBSD-src. Can this be pushed to main branch and stable/13 branch?

I have some more updates to this manpage. Can I submit a seperate differential for the same? or can we update here itself, since this is still not pushed upstream?

@gbe, I see this manpage is still not in the main branch for FreeBSD-src. Can this be pushed to main branch and stable/13 branch?

I have some more updates to this manpage. Can I submit a seperate differential for the same? or can we update here itself, since this is still not pushed upstream?

I take care of the commit and the MFC later on. Please submit a new differential with updates after the commit. It is so easier to review.

I take care of the commit and the MFC later on. Please submit a new differential with updates after the commit. It is so easier to review

Thanks @gbe. Will wait for the commit and submit the update then.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 24 2021, 2:04 PM
This revision was automatically updated to reflect the committed changes.