On Wed, 26 May 2021 17:27:39 +0800 Huazhong Tan wrote:
> @@ -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?
