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