[PATCH net-next v2 0/6] ethtool: clarify the ethtool FEC interface

[PATCH net-next v2 0/6] ethtool: clarify the ethtool FEC interface

[Jakub Kicinski]

Our FEC configuration interface is one of the more confusing.
It also lacks any error checking in the core. This certainly
shows in the varying implementations across the drivers.

Improve the documentation and add most basic checks. Sadly, it's
probably too late now to try to enforce much more uniformity.

Any thoughts & suggestions welcome. Next step is to add netlink
for FEC, then stats.

v2: 
 - fix patch 5
 - adjust kdoc in patches 3 and 6

Jakub Kicinski (6):
  ethtool: fec: fix typo in kdoc
  ethtool: fec: remove long structure description
  ethtool: fec: sanitize ethtool_fecparam->reserved
  ethtool: fec: sanitize ethtool_fecparam->active_fec
  ethtool: fec: sanitize ethtool_fecparam->fec
  ethtool: clarify the ethtool FEC interface

 include/uapi/linux/ethtool.h | 45 +++++++++++++++++++++++++++---------
 net/ethtool/ioctl.c          |  9 ++++++++
 2 files changed, 43 insertions(+), 11 deletions(-)

-- 
2.30.2

[PATCH net-next v2 1/6] ethtool: fec: fix typo in kdoc

[Jakub Kicinski]

s/porte/the port/

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 include/uapi/linux/ethtool.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index cde753bb2093..1433d6278018 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1374,15 +1374,15 @@ struct ethtool_per_queue_op {
 	__u32	queue_mask[__KERNEL_DIV_ROUND_UP(MAX_NUM_QUEUE, 32)];
 	char	data[];
 };
 
 /**
  * struct ethtool_fecparam - Ethernet forward error correction(fec) parameters
  * @cmd: Command number = %ETHTOOL_GFECPARAM or %ETHTOOL_SFECPARAM
- * @active_fec: FEC mode which is active on porte
+ * @active_fec: FEC mode which is active on the port
  * @fec: Bitmask of supported/configured FEC modes
  * @rsvd: Reserved for future extensions. i.e FEC bypass feature.
  *
  * Drivers should reject a non-zero setting of @autoneg when
  * autoneogotiation is disabled (or not supported) for the link.
  *
  */
-- 
2.30.2

[PATCH net-next v2 2/6] ethtool: fec: remove long structure description

[Jakub Kicinski]

Digging through the mailing list archive @autoneg was part
of the first version of the RFC, this left over comment was
pointed out twice in review but wasn't removed.

The sentence is an exact copy-paste from pauseparam.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 include/uapi/linux/ethtool.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 1433d6278018..36bf435d232c 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1377,18 +1377,14 @@ struct ethtool_per_queue_op {
 
 /**
  * struct ethtool_fecparam - Ethernet forward error correction(fec) parameters
  * @cmd: Command number = %ETHTOOL_GFECPARAM or %ETHTOOL_SFECPARAM
  * @active_fec: FEC mode which is active on the port
  * @fec: Bitmask of supported/configured FEC modes
  * @rsvd: Reserved for future extensions. i.e FEC bypass feature.
- *
- * Drivers should reject a non-zero setting of @autoneg when
- * autoneogotiation is disabled (or not supported) for the link.
- *
  */
 struct ethtool_fecparam {
 	__u32   cmd;
 	/* bitmask of FEC modes */
 	__u32   active_fec;
 	__u32   fec;
 	__u32   reserved;
-- 
2.30.2

[PATCH net-next v2 3/6] ethtool: fec: sanitize ethtool_fecparam->reserved

[Jakub Kicinski]

struct ethtool_fecparam::reserved is never looked at by the core.
Make sure it's actually 0. Unfortunately we can't return an error
because old ethtool doesn't zero-initialize the structure for SET.
On GET we can be more verbose, there are no in tree (ab)users.

Fix up the kdoc on the structure. Remove the mention of FEC
bypass. Seems like a niche thing to configure in the first
place.

v2: - also mention the zero-init-on-SET kerfuffle in kdoc

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/uapi/linux/ethtool.h | 6 +++++-
 net/ethtool/ioctl.c          | 5 +++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 36bf435d232c..39a7d285b32b 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1376,15 +1376,19 @@ struct ethtool_per_queue_op {
 };
 
 /**
  * struct ethtool_fecparam - Ethernet forward error correction(fec) parameters
  * @cmd: Command number = %ETHTOOL_GFECPARAM or %ETHTOOL_SFECPARAM
  * @active_fec: FEC mode which is active on the port
  * @fec: Bitmask of supported/configured FEC modes
- * @rsvd: Reserved for future extensions. i.e FEC bypass feature.
+ * @reserved: Reserved for future extensions, ignore on GET, write 0 for SET.
+ *
+ * Note that @reserved was never validated on input and ethtool user space
+ * left it uninitialized when calling SET. Hence going forward it can only be
+ * used to return a value to userspace with GET.
  */
 struct ethtool_fecparam {
 	__u32   cmd;
 	/* bitmask of FEC modes */
 	__u32   active_fec;
 	__u32   fec;
 	__u32   reserved;
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 0788cc3b3114..be3549023d89 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2564,14 +2564,17 @@ static int ethtool_get_fecparam(struct net_device *dev, void __user *useraddr)
 	if (!dev->ethtool_ops->get_fecparam)
 		return -EOPNOTSUPP;
 
 	rc = dev->ethtool_ops->get_fecparam(dev, &fecparam);
 	if (rc)
 		return rc;
 
+	if (WARN_ON_ONCE(fecparam.reserved))
+		fecparam.reserved = 0;
+
 	if (copy_to_user(useraddr, &fecparam, sizeof(fecparam)))
 		return -EFAULT;
 	return 0;
 }
 
 static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
 {
@@ -2579,14 +2582,16 @@ static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
 
 	if (!dev->ethtool_ops->set_fecparam)
 		return -EOPNOTSUPP;
 
 	if (copy_from_user(&fecparam, useraddr, sizeof(fecparam)))
 		return -EFAULT;
 
+	fecparam.reserved = 0;
+
 	return dev->ethtool_ops->set_fecparam(dev, &fecparam);
 }
 
 /* The main entry point in this file.  Called from net/core/dev_ioctl.c */
 
 int dev_ethtool(struct net *net, struct ifreq *ifr)
 {
-- 
2.30.2

Re: [PATCH net-next v2 3/6] ethtool: fec: sanitize ethtool_fecparam->reserved

[Andrew Lunn]

On Thu, Mar 25, 2021 at 07:07:24PM -0700, Jakub Kicinski wrote:
> struct ethtool_fecparam::reserved is never looked at by the core.
> Make sure it's actually 0. Unfortunately we can't return an error
> because old ethtool doesn't zero-initialize the structure for SET.
> On GET we can be more verbose, there are no in tree (ab)users.
> 
> Fix up the kdoc on the structure. Remove the mention of FEC
> bypass. Seems like a niche thing to configure in the first
> place.
> 
> v2: - also mention the zero-init-on-SET kerfuffle in kdoc
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next v2 4/6] ethtool: fec: sanitize ethtool_fecparam->active_fec

[Jakub Kicinski]

struct ethtool_fecparam::active_fec is a GET-only field,
all in-tree drivers correctly ignore it on SET. Clear
the field on SET to avoid any confusion. Again, we can't
reject non-zero now since ethtool user space does not
zero-init the param correctly.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 include/uapi/linux/ethtool.h | 2 +-
 net/ethtool/ioctl.c          | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 39a7d285b32b..78027aa0161a 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1374,15 +1374,15 @@ struct ethtool_per_queue_op {
 	__u32	queue_mask[__KERNEL_DIV_ROUND_UP(MAX_NUM_QUEUE, 32)];
 	char	data[];
 };
 
 /**
  * struct ethtool_fecparam - Ethernet forward error correction(fec) parameters
  * @cmd: Command number = %ETHTOOL_GFECPARAM or %ETHTOOL_SFECPARAM
- * @active_fec: FEC mode which is active on the port
+ * @active_fec: FEC mode which is active on the port, GET only.
  * @fec: Bitmask of supported/configured FEC modes
  * @reserved: Reserved for future extensions, ignore on GET, write 0 for SET.
  *
  * Note that @reserved was never validated on input and ethtool user space
  * left it uninitialized when calling SET. Hence going forward it can only be
  * used to return a value to userspace with GET.
  */
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index be3549023d89..237ffe5440ef 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2582,14 +2582,15 @@ static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
 
 	if (!dev->ethtool_ops->set_fecparam)
 		return -EOPNOTSUPP;
 
 	if (copy_from_user(&fecparam, useraddr, sizeof(fecparam)))
 		return -EFAULT;
 
+	fecparam.active_fec = 0;
 	fecparam.reserved = 0;
 
 	return dev->ethtool_ops->set_fecparam(dev, &fecparam);
 }
 
 /* The main entry point in this file.  Called from net/core/dev_ioctl.c */
 
-- 
2.30.2

[PATCH net-next v2 5/6] ethtool: fec: sanitize ethtool_fecparam->fec

[Jakub Kicinski]

Reject NONE on set, this mode means device does not support
FEC so it's a little out of place in the set interface.

This should be safe to do - user space ethtool does not allow
the use of NONE on set. A few drivers treat it the same as OFF,
but none use it instead of OFF.

Similarly reject an empty FEC mask. The common user space tool
will not send such requests and most drivers correctly reject
it already.

v2: - use mask not bit pos

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/ethtool/ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 237ffe5440ef..26b3e7086075 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2582,14 +2582,17 @@ static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
 
 	if (!dev->ethtool_ops->set_fecparam)
 		return -EOPNOTSUPP;
 
 	if (copy_from_user(&fecparam, useraddr, sizeof(fecparam)))
 		return -EFAULT;
 
+	if (!fecparam.fec || fecparam.fec & ETHTOOL_FEC_NONE)
+		return -EINVAL;
+
 	fecparam.active_fec = 0;
 	fecparam.reserved = 0;
 
 	return dev->ethtool_ops->set_fecparam(dev, &fecparam);
 }
 
 /* The main entry point in this file.  Called from net/core/dev_ioctl.c */
-- 
2.30.2

Re: [PATCH net-next v2 5/6] ethtool: fec: sanitize ethtool_fecparam->fec

[Andrew Lunn]

On Thu, Mar 25, 2021 at 07:07:26PM -0700, Jakub Kicinski wrote:
> Reject NONE on set, this mode means device does not support
> FEC so it's a little out of place in the set interface.
> 
> This should be safe to do - user space ethtool does not allow
> the use of NONE on set. A few drivers treat it the same as OFF,
> but none use it instead of OFF.
> 
> Similarly reject an empty FEC mask. The common user space tool
> will not send such requests and most drivers correctly reject
> it already.
> 
> v2: - use mask not bit pos
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

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

[Jakub Kicinski]

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/

v2: - make enum kdoc reference bits

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

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 78027aa0161a..868b513d4f54 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1372,39 +1372,62 @@ 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.
  *
  * Note that @reserved was never validated on input and ethtool user space
  * left it uninitialized when calling SET. Hence going forward it can only be
  * used to return a value to userspace with GET.
+ *
+ * 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_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_NONE_BIT: FEC mode configuration is not supported. Should not
+ *			be used together with other bits. GET only.
+ * @ETHTOOL_FEC_AUTO_BIT: 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_BIT: No FEC Mode
+ * @ETHTOOL_FEC_RS_BIT: Reed-Solomon FEC Mode
+ * @ETHTOOL_FEC_BASER_BIT: Base-R/Reed-Solomon FEC Mode
+ * @ETHTOOL_FEC_LLRS_BIT: 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,
-- 
2.30.2

Re: [PATCH net-next v2 6/6] ethtool: clarify the ethtool FEC interface

[Andrew Lunn]

On Thu, Mar 25, 2021 at 07:07:27PM -0700, Jakub Kicinski wrote:
> 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/
> 
> v2: - make enum kdoc reference bits
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 0/6] ethtool: clarify the ethtool FEC interface

[David Miller]

From: Jakub Kicinski <kuba@kernel.org>
Date: Thu, 25 Mar 2021 19:07:21 -0700

> Our FEC configuration interface is one of the more confusing.
> It also lacks any error checking in the core. This certainly
> shows in the varying implementations across the drivers.
> 
> Improve the documentation and add most basic checks. Sadly, it's
> probably too late now to try to enforce much more uniformity.
> 
> Any thoughts & suggestions welcome. Next step is to add netlink
> for FEC, then stats.
> 
> v2: 
>  - fix patch 5
>  - adjust kdoc in patches 3 and 6

Jakub, I applied v1 so please send fiixups relative to that, thank you.