Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ix.4: rename and improve #1494

Closed
wants to merge 4 commits into from
Closed

ix.4: rename and improve #1494

wants to merge 4 commits into from

Conversation

concussious
Copy link
Contributor

Describe better for ongoing projects to improve apropos results and the release hardware notes.

Since Felix Johnson has (seemingly?) not been seen for over 3 years, base these improvements from his (very lightly edited) proposed improvements 1:

  1. name the page accurately
  2. document sysctls

@kev009

Footnotes

  1. https://reviews.freebsd.org/D32531

PR:		213026
MFC after:	3 days
Reported by:	Sergey Akhmatov <[email protected]>
Co-authored-by:	Alexander Ziaee <[email protected]>
@concussious
Copy link
Contributor Author

Instead of LOADER TUNABLES, is it appropriate to use ENVIORNMENT? We have plenty of pages with the former, but the latter is a standardized section as specified in mdoc(7).

share/man/man4/ix.4 Outdated Show resolved Hide resolved
@@ -74,7 +74,8 @@ For information on enabling VLANs, see
.Sh HARDWARE
The
.Nm
driver supports the following cards:
driver supports 10Gb Ethernet PCIe adapters based on the 82598EB controller,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should be undone

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is because the entire hardware section is what appears in the hardware release notes.

https://www.freebsd.org/releases/14.1R/hardware/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry the line comment is not exact enough, the part I would remove is specifically the based on the 82598EB controller. While it is true in some absolute sense, each of these cards is marketed independently and the rest of the family is closer affined to the 82599 (aka X520) while it does not add any particular value to the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mention 82599 in DESCRIPTION? E.g. based on the Intel 82598EB and 82599 Intel(R)...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HARDWARE section would be more appropriate, I would not recommend any generic mention.

The HARDWARE section could be cleaned up/fleshed out a bit based on ixgbe_vendor_info_array in src/sys/dev/ixgbe/if_ix.c.. I would say something like this is accurate:

  • 82598EB
  • 82599 (X520) and X520 Bypass
  • X540 and X540 Bypass
  • X550
  • X552
  • X553

share/man/man4/ix.4 Outdated Show resolved Hide resolved
share/man/man4/ix.4 Show resolved Hide resolved
@kev009
Copy link
Member

kev009 commented Oct 27, 2024

Instead of LOADER TUNABLES, is it appropriate to use ENVIORNMENT? We have plenty of pages with the former, but the latter is a standardized section as specified in mdoc(7).

I will defer to someone that has a doc bit for that.

@bsdimp
Copy link
Member

bsdimp commented Oct 27, 2024

Instead of LOADER TUNABLES, is it appropriate to use ENVIORNMENT? We have plenty of pages with the former, but the latter is a standardized section as specified in mdoc(7).

I will defer to someone that has a doc bit for that.

I've been using LOADER TUNABLES or SYSCTL VARIABLRS. Environment is definitely wrong

Warner

PR:		213026
MFC after:	3 days
Co-authored-by:	Alexander Ziaee <[email protected]>
.It
Intel(R) Ethernet X552 (10Gb/5Gb/2.5Gb/1Gb/100Mb)
.It
Intel(R) Ethernet X553 (10Gb/5Gb/2.5Gb/1Gb/100Mb)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove all the listed speeds because I think all of these adapters can go all the way down to 10mbit with a copper PHY and should be able to do 100mbit SERDES.

It would be helpful to document the 2.5/5Gb capable adapters including the sysctl dev.ix.0.advertise_speed to enable them in auto-negotiate but maybe in a follow up PR to move this along.

@kev009 kev009 self-requested a review October 27, 2024 07:11
+ add controller name to title for search keywords
- remove "for the freebsd operating system" from title/search keywords
- remove `(R)` from the page title for consistency with other drivers
- increase specificity of HARDWARE for inclusion in Release HW Notes
- order HARDWARE in reverse chronological so non-eol appear first
- s/PCI/PCIe/

MFC after:	3 days
Reported by:	kbowling (additional hardware supported)
@concussious
Copy link
Contributor Author

concussious commented Oct 27, 2024

Sorry for pushing again, there was some extra whitespace on 3 of my lines. Thanks so much, this was cool!

@kev009 kev009 self-requested a review October 27, 2024 07:22
freebsd-git pushed a commit that referenced this pull request Oct 27, 2024
+ add controller name to title for search keywords
- remove "for the freebsd operating system" from title/search keywords
- remove `(R)` from the page title for consistency with other drivers
- increase specificity of HARDWARE for inclusion in Release HW Notes
- order HARDWARE in reverse chronological so non-eol appear first
- s/PCI/PCIe/

MFC after:	3 days
Reported by:	kbowling (additional hardware supported)
Pull Request:	#1494
freebsd-git pushed a commit that referenced this pull request Oct 27, 2024
MFC after:	3 days
Pull Request:	#1494
@kev009 kev009 closed this Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants