From: Jakub Kicinski <kuba@kernel.org> Date: 2021-03-26 02:08:18
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
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-03-26 02:08:18
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(+)
@@ -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ÐTOOL_FEC_NONE)+return-EINVAL;+fecparam.active_fec=0;fecparam.reserved=0;returndev->ethtool_ops->set_fecparam(dev,&fecparam);}/* The main entry point in this file. Called from net/core/dev_ioctl.c */
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-03-26 02:08:18
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(-)
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-03-26 02:08:18
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(-)
@@ -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;+returndev->ethtool_ops->set_fecparam(dev,&fecparam);}/* The main entry point in this file. Called from net/core/dev_ioctl.c */intdev_ethtool(structnet*net,structifreq*ifr){
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-03-26 02:08:18
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(-)
@@ -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;returndev->ethtool_ops->set_fecparam(dev,&fecparam);}/* The main entry point in this file. Called from net/core/dev_ioctl.c */
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-03-26 12:18:10
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>
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-03-26 12:19:19
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>
From: David Miller <davem@davemloft.net> Date: 2021-03-26 21:50:44
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.