diff mbox series

[net-next,6/6] ethtool: clarify the ethtool FEC interface

Message ID 20210325011200.145818-7-kuba@kernel.org (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series ethtool: clarify the ethtool FEC interface | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 6 maintainers not CCed: linux-api@vger.kernel.org petr.vorel@gmail.com amitc@mellanox.com linux@rempel-privat.de dmurphy@ti.com meirl@mellanox.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 3024 this patch: 3024
netdev/kdoc fail Errors and warnings before: 54 this patch: 55
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 52 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 3142 this patch: 3142
netdev/header_inline success Link

Commit Message

Jakub Kicinski March 25, 2021, 1:12 a.m. UTC
The definition of the FEC driver interface is quite unclear.
Improve the documentation.

This is based on current driver and user space code, as well
as the discussions about the interface:

RFC v1 (24 Oct 2016): https://lore.kernel.org/netdev/1477363849-36517-1-git-send-email-vidya@cumulusnetworks.com/
 - this version has the autoneg field
 - no active_fec field
 - none vs off confusion is already present

RFC v2 (10 Feb 2017): https://lore.kernel.org/netdev/1486727004-11316-1-git-send-email-vidya@cumulusnetworks.com/
 - autoneg removed
 - active_fec added

v1 (10 Feb 2017): https://lore.kernel.org/netdev/1486751311-42019-1-git-send-email-vidya@cumulusnetworks.com/
 - no changes in the code

v1 (24 Jun 2017):  https://lore.kernel.org/netdev/1498331985-8525-1-git-send-email-roopa@cumulusnetworks.com/
 - include in tree user

v2 (27 Jul 2017): https://lore.kernel.org/netdev/1501199248-24695-1-git-send-email-roopa@cumulusnetworks.com/

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/uapi/linux/ethtool.h | 37 +++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

Comments

Edward Cree March 29, 2021, 11:56 a.m. UTC | #1
On 25/03/2021 01:12, Jakub Kicinski wrote:
> Drivers should reject mixing %ETHTOOL_FEC_AUTO_BIT with other
> + * FEC modes, because it's unclear whether in this case other modes constrain
> + * AUTO or are independent choices.

Does this mean you want me to spin a patch to sfc to reject this?
Currently for us e.g. AUTO|RS means use RS if the cable and link partner
 both support it, otherwise let firmware choose (presumably between BASER
 and OFF) based on cable/module & link partner caps and/or parallel detect.
We took this approach because our requirements writers believed that
 customers would have a need for this setting; they called it "prefer FEC",
 and I think the idea was to use FEC if possible (even on cables where the
 IEEE-recommended default is no FEC, such as CA-25G-N 3m DAC) but allow
 fallback to no FEC if e.g. link partner doesn't advertise FEC in AN.
Similarly, AUTO|BASER ("prefer BASE-R FEC") might be desired by a user who
 wants to use BASE-R if possible to minimise latency, but fall back to RS
 FEC if the cable or link partner insists on it (eg CA-25G-L 5m DAC).
Whether we were right and all this is actually useful, I couldn't say.

-ed
Andrew Lunn March 29, 2021, 12:32 p.m. UTC | #2
On Mon, Mar 29, 2021 at 12:56:30PM +0100, Edward Cree wrote:
> On 25/03/2021 01:12, Jakub Kicinski wrote:
> > Drivers should reject mixing %ETHTOOL_FEC_AUTO_BIT with other
> > + * FEC modes, because it's unclear whether in this case other modes constrain
> > + * AUTO or are independent choices.
> 
> Does this mean you want me to spin a patch to sfc to reject this?
> Currently for us e.g. AUTO|RS means use RS if the cable and link partner
>  both support it, otherwise let firmware choose (presumably between BASER
>  and OFF) based on cable/module & link partner caps and/or parallel detect.
> We took this approach because our requirements writers believed that
>  customers would have a need for this setting; they called it "prefer FEC",
>  and I think the idea was to use FEC if possible (even on cables where the
>  IEEE-recommended default is no FEC, such as CA-25G-N 3m DAC) but allow
>  fallback to no FEC if e.g. link partner doesn't advertise FEC in AN.
> Similarly, AUTO|BASER ("prefer BASE-R FEC") might be desired by a user who
>  wants to use BASE-R if possible to minimise latency, but fall back to RS
>  FEC if the cable or link partner insists on it (eg CA-25G-L 5m DAC).
> Whether we were right and all this is actually useful, I couldn't say.

Jacub was talking about adding a netlink API as the next step. You
should feed this in as a requirement for that. Being able to express
preferences in the API in an explicitly documented way.

It there any other existing ethtool setting which could be used as a
model? EEE, master/slave? I would class pause as an anti model, that
is frequently done wrong :-(

       Andrew
Jakub Kicinski March 29, 2021, 5:39 p.m. UTC | #3
On Mon, 29 Mar 2021 12:56:30 +0100 Edward Cree wrote:
> On 25/03/2021 01:12, Jakub Kicinski wrote:
> > Drivers should reject mixing %ETHTOOL_FEC_AUTO_BIT with other
> > + * FEC modes, because it's unclear whether in this case other modes constrain
> > + * AUTO or are independent choices.  
> 
> Does this mean you want me to spin a patch to sfc to reject this?
> Currently for us e.g. AUTO|RS means use RS if the cable and link partner
>  both support it, otherwise let firmware choose (presumably between BASER
>  and OFF) based on cable/module & link partner caps and/or parallel detect.
> We took this approach because our requirements writers believed that
>  customers would have a need for this setting; they called it "prefer FEC",
>  and I think the idea was to use FEC if possible (even on cables where the
>  IEEE-recommended default is no FEC, such as CA-25G-N 3m DAC) but allow
>  fallback to no FEC if e.g. link partner doesn't advertise FEC in AN.
> Similarly, AUTO|BASER ("prefer BASE-R FEC") might be desired by a user who
>  wants to use BASE-R if possible to minimise latency, but fall back to RS
>  FEC if the cable or link partner insists on it (eg CA-25G-L 5m DAC).
> Whether we were right and all this is actually useful, I couldn't say.

Interesting combo. Up to you, the API is quite unclear, I think users
shouldn't expect anything beyond single bit set to work across
implementations. IMHO supporting anything beyond that is just code
complexity for little to no gain. But then again, as long as you don't
confuse AUTO with autoneg there's no burning need to change :)
diff mbox series

Patch

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 517b68c5fcec..f6ef7d42c7a1 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1372,35 +1372,58 @@  struct ethtool_per_queue_op {
 	__u32	cmd;
 	__u32	sub_command;
 	__u32	queue_mask[__KERNEL_DIV_ROUND_UP(MAX_NUM_QUEUE, 32)];
 	char	data[];
 };
 
 /**
- * struct ethtool_fecparam - Ethernet forward error correction(fec) parameters
+ * struct ethtool_fecparam - Ethernet Forward Error Correction parameters
  * @cmd: Command number = %ETHTOOL_GFECPARAM or %ETHTOOL_SFECPARAM
- * @active_fec: FEC mode which is active on the port, GET only.
- * @fec: Bitmask of supported/configured FEC modes
+ * @active_fec: FEC mode which is active on the port, single bit set, GET only.
+ * @fec: Bitmask of configured FEC modes.
  * @reserved: Reserved for future extensions, ignore on GET, write 0 for SET.
+ *
+ * FEC modes supported by the device can be read via %ETHTOOL_GLINKSETTINGS.
+ * FEC settings are configured by link autonegotiation whenever it's enabled.
+ * With autoneg on %ETHTOOL_GFECPARAM can be used to read the current mode.
+ *
+ * When autoneg is disabled %ETHTOOL_SFECPARAM controls the FEC settings.
+ * It is recommended that drivers only accept a single bit set in @fec.
+ * When multiple bits are set in @fec drivers may pick mode in an implementation
+ * dependent way. Drivers should reject mixing %ETHTOOL_FEC_AUTO_BIT with other
+ * FEC modes, because it's unclear whether in this case other modes constrain
+ * AUTO or are independent choices.
+ * Drivers must reject SET requests if they support none of the requested modes.
+ *
+ * If device does not support FEC drivers may use %ETHTOOL_FEC_NONE instead
+ * of returning %EOPNOTSUPP from %ETHTOOL_GFECPARAM.
+ *
+ * See enum ethtool_fec_config_bits for definition of valid bits for both
+ * @fec and @active_fec.
  */
 struct ethtool_fecparam {
 	__u32   cmd;
 	/* bitmask of FEC modes */
 	__u32   active_fec;
 	__u32   fec;
 	__u32   reserved;
 };
 
 /**
  * enum ethtool_fec_config_bits - flags definition of ethtool_fec_configuration
- * @ETHTOOL_FEC_NONE: FEC mode configuration is not supported
- * @ETHTOOL_FEC_AUTO: Default/Best FEC mode provided by driver
+ * @ETHTOOL_FEC_NONE: FEC mode configuration is not supported. Should not
+ *		      be used together with other bits. GET only.
+ * @ETHTOOL_FEC_AUTO: Select default/best FEC mode automatically, usually based
+ *		      link mode and SFP parameters read from module's EEPROM.
+ *		      This bit does _not_ mean autonegotiation.
  * @ETHTOOL_FEC_OFF: No FEC Mode
- * @ETHTOOL_FEC_RS: Reed-Solomon Forward Error Detection mode
- * @ETHTOOL_FEC_BASER: Base-R/Reed-Solomon Forward Error Detection mode
+ * @ETHTOOL_FEC_RS: Reed-Solomon FEC Mode
+ * @ETHTOOL_FEC_BASER: Base-R/Reed-Solomon FEC Mode
+ * @ETHTOOL_FEC_LLRS: Low Latency Reed Solomon FEC Mode (25G/50G Ethernet
+ *		      Consortium)
  */
 enum ethtool_fec_config_bits {
 	ETHTOOL_FEC_NONE_BIT,
 	ETHTOOL_FEC_AUTO_BIT,
 	ETHTOOL_FEC_OFF_BIT,
 	ETHTOOL_FEC_RS_BIT,
 	ETHTOOL_FEC_BASER_BIT,