Details
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 36027 Build 32916: arc lint + arc unit
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 | ||
|---|---|---|
| 33 | You can remove this, we don't use it anymore now that we are on git. | |
| 120 | You need to make a line break here after the sentence stop. | |
| 135 | Two line breaks needed here at each of the sentence stops in this line. | |
| 145 | Another line break needed here. | |
| 153 | Line break needed here after the sentence stop. | |
| 174 | Line break after "effect." | |
| 194 | Line break here. | |
| 208 | You can replace FreeBSD with .Fx like above. Break the lines in this whole paragraph while you're here. | |
| share/man/man4/axp.4 | ||
|---|---|---|
| 59 | superfluous "the" | |
| 60 | Processors -> processors | |
| 62 | "," -> ":" | |
| 72 | One space too many before "(TSO)" | |
| 92 | along with AMD EPYC processors. | |
| 96 | variables -> variables: (add colon) | |
| 98 | 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. | |
| 100 | queues -> queue | |
| 103 | Terminating "." missing. | |
| 142 | Maybe a Phab issue, but I see empty lines here (and more further down) instead of ".It" entries. | |
| 199 | missing space between 4 and , | |
| 206 | Missing space between 0 and . | |
| 212 | Missing terminating dot, e.g. ".Aq Mt andrew@FreeBSD.org ." | |
| 217 | The punctuation at the end looks a bit strange; can it be just ".An Advanced Micro Devices Inc."? | |
| share/man/man4/axp.4 | ||
|---|---|---|
| 40 | "10Gigabit" is kind of an odd spelling, usually only shortened versions are written without a space ("10G", "10GbE"). Same below | |
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 | ||
|---|---|---|
| 40 | Changed to "10G" in all places | |
| 98 | 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. | |
| 100 | Corrected another place as well. | |
| 142 | I have filled with empty lines with .Pp macro. Please correct if wrong. | |
| share/man/man4/axp.4 | ||
|---|---|---|
| 98 | 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 | ||
|---|---|---|
| 137 | 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. | |
| 168 | Same as dev.ax.X.netmap_test | |
| share/man/man4/axp.4 | ||
|---|---|---|
| 98 | @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. | |
| 137 | @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. | |
| 168 | For now, lets keep it interface specific as it gives some flexibility. | |
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 | ||
|---|---|---|
| 137 | In that case we need to change the driver code to be interface-specific. | |
| share/man/man4/axp.4 | ||
|---|---|---|
| 98 |
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. | |
@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.