From: Jakub Kicinski <kuba@kernel.org> Date: 2021-03-25 01:13:01
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
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-03-25 01:13:02
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(-)
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-03-25 01:13:02
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(-)
@@ -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: Jakub Kicinski <kuba@kernel.org> Date: 2021-03-25 01:13:02
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(-)
@@ -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-25 01:13:02
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(+)
@@ -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_BIT)+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: Andrew Lunn <andrew@lunn.ch> Date: 2021-03-25 12:08:09
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>
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-03-25 12:23:41
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
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-03-25 12:26:55
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>
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-03-25 16:03:35
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:
quoted
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!
From: Edward Cree <ecree.xilinx@gmail.com> Date: 2021-03-29 11:57:33
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
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-03-29 12:33:54
On Mon, Mar 29, 2021 at 12:56:30PM +0100, Edward Cree wrote:
On 25/03/2021 01:12, Jakub Kicinski wrote:
quoted
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
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-03-29 17:40:22
On Mon, 29 Mar 2021 12:56:30 +0100 Edward Cree wrote:
On 25/03/2021 01:12, Jakub Kicinski wrote:
quoted
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 :)