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

[PATCH net-next 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.

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 | 39 +++++++++++++++++++++++++++---------
 net/ethtool/ioctl.c          |  9 +++++++++
 2 files changed, 38 insertions(+), 10 deletions(-)

-- 
2.30.2

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

[Jakub Kicinski]

s/porte/the port/

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 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 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>
---
 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 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.

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

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 36bf435d232c..9e2682a67460 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1376,15 +1376,15 @@ 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.
  */
 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

[PATCH net-next 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>
---
 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 9e2682a67460..517b68c5fcec 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.
  */
 struct ethtool_fecparam {
 	__u32   cmd;
 	/* bitmask of FEC modes */
 	__u32   active_fec;
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 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.

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..8797533ddc4b 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_BIT)
+		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

[PATCH net-next 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/

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

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,
-- 
2.30.2

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

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 2761 bytes --]

Hi Jakub,

url:    https://github.com/0day-ci/linux/commits/Jakub-Kicinski/ethtool-clarify-the-ethtool-FEC-interface/20210325-091411
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 69cdfb530f7b8b094e49555454869afc8140b1bb
config: x86_64-randconfig-m001-20210325 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
net/ethtool/ioctl.c:2589 ethtool_set_fecparam() warn: bitwise AND condition is false here

vim +2589 net/ethtool/ioctl.c

1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2579  static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2580  {
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2581  	struct ethtool_fecparam fecparam;
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2582  
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2583  	if (!dev->ethtool_ops->set_fecparam)
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2584  		return -EOPNOTSUPP;
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2585  
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2586  	if (copy_from_user(&fecparam, useraddr, sizeof(fecparam)))
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2587  		return -EFAULT;
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2588  
15beed7dba77ce net/ethtool/ioctl.c Jakub Kicinski       2021-03-24 @2589  	if (!fecparam.fec || fecparam.fec & ETHTOOL_FEC_NONE_BIT)
                                                                                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This should be if (!fecparam.fec || fecparam.fec & BIT(ETHTOOL_FEC_NONE_BIT))

15beed7dba77ce net/ethtool/ioctl.c Jakub Kicinski       2021-03-24  2590  		return -EINVAL;
15beed7dba77ce net/ethtool/ioctl.c Jakub Kicinski       2021-03-24  2591  
c405852e12f210 net/ethtool/ioctl.c Jakub Kicinski       2021-03-24  2592  	fecparam.active_fec = 0;
76d37e2ba4f23d net/ethtool/ioctl.c Jakub Kicinski       2021-03-24  2593  	fecparam.reserved = 0;
76d37e2ba4f23d net/ethtool/ioctl.c Jakub Kicinski       2021-03-24  2594  
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2595  	return dev->ethtool_ops->set_fecparam(dev, &fecparam);
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2596  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32759 bytes --]

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

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 2804 bytes --]

Hi Jakub,

url:    https://github.com/0day-ci/linux/commits/Jakub-Kicinski/ethtool-clarify-the-ethtool-FEC-interface/20210325-091411
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 69cdfb530f7b8b094e49555454869afc8140b1bb
config: x86_64-randconfig-m001-20210325 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
net/ethtool/ioctl.c:2589 ethtool_set_fecparam() warn: bitwise AND condition is false here

vim +2589 net/ethtool/ioctl.c

1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2579  static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2580  {
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2581  	struct ethtool_fecparam fecparam;
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2582  
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2583  	if (!dev->ethtool_ops->set_fecparam)
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2584  		return -EOPNOTSUPP;
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2585  
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2586  	if (copy_from_user(&fecparam, useraddr, sizeof(fecparam)))
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2587  		return -EFAULT;
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2588  
15beed7dba77ce net/ethtool/ioctl.c Jakub Kicinski       2021-03-24 @2589  	if (!fecparam.fec || fecparam.fec & ETHTOOL_FEC_NONE_BIT)
                                                                                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This should be if (!fecparam.fec || fecparam.fec & BIT(ETHTOOL_FEC_NONE_BIT))

15beed7dba77ce net/ethtool/ioctl.c Jakub Kicinski       2021-03-24  2590  		return -EINVAL;
15beed7dba77ce net/ethtool/ioctl.c Jakub Kicinski       2021-03-24  2591  
c405852e12f210 net/ethtool/ioctl.c Jakub Kicinski       2021-03-24  2592  	fecparam.active_fec = 0;
76d37e2ba4f23d net/ethtool/ioctl.c Jakub Kicinski       2021-03-24  2593  	fecparam.reserved = 0;
76d37e2ba4f23d net/ethtool/ioctl.c Jakub Kicinski       2021-03-24  2594  
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2595  	return dev->ethtool_ops->set_fecparam(dev, &fecparam);
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2596  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 32759 bytes --]

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

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 2804 bytes --]

Hi Jakub,

url:    https://github.com/0day-ci/linux/commits/Jakub-Kicinski/ethtool-clarify-the-ethtool-FEC-interface/20210325-091411
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 69cdfb530f7b8b094e49555454869afc8140b1bb
config: x86_64-randconfig-m001-20210325 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
net/ethtool/ioctl.c:2589 ethtool_set_fecparam() warn: bitwise AND condition is false here

vim +2589 net/ethtool/ioctl.c

1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2579  static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2580  {
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2581  	struct ethtool_fecparam fecparam;
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2582  
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2583  	if (!dev->ethtool_ops->set_fecparam)
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2584  		return -EOPNOTSUPP;
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2585  
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2586  	if (copy_from_user(&fecparam, useraddr, sizeof(fecparam)))
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2587  		return -EFAULT;
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2588  
15beed7dba77ce net/ethtool/ioctl.c Jakub Kicinski       2021-03-24 @2589  	if (!fecparam.fec || fecparam.fec & ETHTOOL_FEC_NONE_BIT)
                                                                                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This should be if (!fecparam.fec || fecparam.fec & BIT(ETHTOOL_FEC_NONE_BIT))

15beed7dba77ce net/ethtool/ioctl.c Jakub Kicinski       2021-03-24  2590  		return -EINVAL;
15beed7dba77ce net/ethtool/ioctl.c Jakub Kicinski       2021-03-24  2591  
c405852e12f210 net/ethtool/ioctl.c Jakub Kicinski       2021-03-24  2592  	fecparam.active_fec = 0;
76d37e2ba4f23d net/ethtool/ioctl.c Jakub Kicinski       2021-03-24  2593  	fecparam.reserved = 0;
76d37e2ba4f23d net/ethtool/ioctl.c Jakub Kicinski       2021-03-24  2594  
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2595  	return dev->ethtool_ops->set_fecparam(dev, &fecparam);
1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2596  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 32759 bytes --]

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

[Andrew Lunn]

On Wed, Mar 24, 2021 at 06:11:55PM -0700, Jakub Kicinski wrote:
> s/porte/the port/
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

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

    Andrew

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

[Andrew Lunn]

On Wed, Mar 24, 2021 at 06:11:56PM -0700, Jakub Kicinski wrote:
> 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>

    Andrew

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

[Andrew Lunn]

On Wed, Mar 24, 2021 at 06:11:57PM -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.

Hi Jakub

What makes it totally useless for future uses with SET. So the
documentation should probably be something like:

* @reserved: Reserved for future GET extensions.
*
* Older ethtool(1) leave @reserved uninitialised when calling SET or
* GET.  Hence it can only be used to return a value to userspace with
* GET. Currently the value returned is guaranteed to be zero.

The rest looks O.K.

    Andrew

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

[Andrew Lunn]

On Wed, Mar 24, 2021 at 06:11:58PM -0700, Jakub Kicinski wrote:
> 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>

    Andrew

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

[Jakub Kicinski]

On Thu, 25 Mar 2021 13:22:47 +0100 Andrew Lunn wrote:
> On Wed, Mar 24, 2021 at 06:11:57PM -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.  
> 
> Hi Jakub
> 
> What makes it totally useless for future uses with SET. So the
> documentation should probably be something like:
> 
> * @reserved: Reserved for future GET extensions.
> *
> * Older ethtool(1) leave @reserved uninitialised when calling SET or
> * GET.  Hence it can only be used to return a value to userspace with
> * GET. Currently the value returned is guaranteed to be zero.
> 
> The rest looks O.K.

I didn't spell this out because we'll move to netlink as next
step so the ioctl structure is less relevant, but will do!

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

[Jakub Kicinski]

On Thu, 25 Mar 2021 15:00:47 +0300 Dan Carpenter wrote:
> Hi Jakub,
> 
> url:    https://github.com/0day-ci/linux/commits/Jakub-Kicinski/ethtool-clarify-the-ethtool-FEC-interface/20210325-091411
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 69cdfb530f7b8b094e49555454869afc8140b1bb
> config: x86_64-randconfig-m001-20210325 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> smatch warnings:
> net/ethtool/ioctl.c:2589 ethtool_set_fecparam() warn: bitwise AND condition is false here
> 
> vim +2589 net/ethtool/ioctl.c
> 
> 1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2579  static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
> 1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2580  {
> 1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2581  	struct ethtool_fecparam fecparam;
> 1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2582  
> 1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2583  	if (!dev->ethtool_ops->set_fecparam)
> 1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2584  		return -EOPNOTSUPP;
> 1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2585  
> 1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2586  	if (copy_from_user(&fecparam, useraddr, sizeof(fecparam)))
> 1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2587  		return -EFAULT;
> 1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2588  
> 15beed7dba77ce net/ethtool/ioctl.c Jakub Kicinski       2021-03-24 @2589  	if (!fecparam.fec || fecparam.fec & ETHTOOL_FEC_NONE_BIT)
>                                                                                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

:o good catch. s/_BIT//. Thanks!

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

[Jakub Kicinski]

[-- Attachment #1: Type: text/plain, Size: 2097 bytes --]

On Thu, 25 Mar 2021 15:00:47 +0300 Dan Carpenter wrote:
> Hi Jakub,
> 
> url:    https://github.com/0day-ci/linux/commits/Jakub-Kicinski/ethtool-clarify-the-ethtool-FEC-interface/20210325-091411
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 69cdfb530f7b8b094e49555454869afc8140b1bb
> config: x86_64-randconfig-m001-20210325 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> smatch warnings:
> net/ethtool/ioctl.c:2589 ethtool_set_fecparam() warn: bitwise AND condition is false here
> 
> vim +2589 net/ethtool/ioctl.c
> 
> 1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2579  static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
> 1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2580  {
> 1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2581  	struct ethtool_fecparam fecparam;
> 1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2582  
> 1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2583  	if (!dev->ethtool_ops->set_fecparam)
> 1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2584  		return -EOPNOTSUPP;
> 1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2585  
> 1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2586  	if (copy_from_user(&fecparam, useraddr, sizeof(fecparam)))
> 1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2587  		return -EFAULT;
> 1a5f3da20bd966 net/core/ethtool.c  Vidya Sagar Ravipati 2017-07-27  2588  
> 15beed7dba77ce net/ethtool/ioctl.c Jakub Kicinski       2021-03-24 @2589  	if (!fecparam.fec || fecparam.fec & ETHTOOL_FEC_NONE_BIT)
>                                                                                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

:o good catch. s/_BIT//. Thanks!

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

[Edward Cree]

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

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

[Andrew Lunn]

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

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

[Jakub Kicinski]

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 :)