Re: [RFC net-next 1/4] ethtool: extend coalesce API
From: Jakub Kicinski <kuba@kernel.org>
Date: 2021-05-26 23:56:37
Also in:
linux-wireless
On Wed, 26 May 2021 17:27:39 +0800 Huazhong Tan wrote:
quoted hunk ↗ jump to hunk
@@ -606,8 +611,12 @@ struct ethtool_ops { struct ethtool_eeprom *, u8 *); int (*set_eeprom)(struct net_device *, struct ethtool_eeprom *, u8 *); - int (*get_coalesce)(struct net_device *, struct ethtool_coalesce *); - int (*set_coalesce)(struct net_device *, struct ethtool_coalesce *); + int (*get_coalesce)(struct net_device *, + struct netlink_ext_ack *,
ext_ack is commonly the last argument AFAIR.
+ struct kernel_ethtool_coalesce *);
Seeing all the driver changes I can't say I'm a huge fan of
the encapsulation. We end up with a local variable for the "base"
structure, e.g.:
static int wil_ethtoolops_set_coalesce(struct net_device *ndev,
- struct ethtool_coalesce *cp)
+ struct netlink_ext_ack *extack,
+ struct kernel_ethtool_coalesce *cp)
{
+ struct ethtool_coalesce *coal_base = &cp->base;
struct wil6210_priv *wil = ndev_to_wil(ndev);
struct wireless_dev *wdev = ndev->ieee80211_ptr;
so why not leave the base alone and pass the new members in a separate
structure?
+ int (*set_coalesce)(struct net_device *, + struct netlink_ext_ack *, + struct kernel_ethtool_coalesce *); void (*get_ringparam)(struct net_device *, struct ethtool_ringparam *); int (*set_ringparam)(struct net_device *,
static noinline_for_stack int ethtool_set_coalesce(struct net_device *dev,
void __user *useraddr)
{
- struct ethtool_coalesce coalesce;
+ struct kernel_ethtool_coalesce coalesce;
int ret;
if (!dev->ethtool_ops->set_coalesce)
return -EOPNOTSUPP;
- if (copy_from_user(&coalesce, useraddr, sizeof(coalesce)))
+ if (copy_from_user(&coalesce.base, useraddr, sizeof(coalesce.base)))
return -EFAULT;
if (!ethtool_set_coalesce_supported(dev, &coalesce))
return -EOPNOTSUPP;
- ret = dev->ethtool_ops->set_coalesce(dev, &coalesce);
+ ret = dev->ethtool_ops->set_coalesce(dev, NULL, &coalesce);
if (!ret)
ethtool_notify(dev, ETHTOOL_MSG_COALESCE_NTF, NULL);
return ret;Should IOCTL overwrite the settings it doesn't know about with 0 or preserve the existing values?