On 2021/5/27 7:56, Jakub Kicinski wrote:
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.
yes, will fix it.
+ 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?
This is a similar approach as struct ethtool_link_ksettings
suggested by Michal in last year's discussion.
https://lore.kernel.org/lkml/20201119220203.fv2uluoeekyoyxrv@xxxxxxxxxxxxxx/
add a new separate structure can make less change. like below
what we have to do is just add a new parameter.
static int wil_ethtoolops_set_coalesce(struct net_device *ndev,
- struct ethtool_coalesce *cp)
+ struct ethtool_coalesce *cp,
+ struct ethtool_ext_coalesce *ext_cp,
+ struct netlink_ext_ack *extack)
{
struct wil6210_priv *wil = ndev_to_wil(ndev);
struct wireless_dev *wdev = ndev->ieee80211_ptr;
If this is ok, i will send a V2 for it.
+ 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?
IOCTL will overwrite the setting with random value,
will a get_coalesce before copy_from_user() to fix it.
Thanks.
Huazhong.
.
