On 2021/5/27 7:56, Jakub Kicinski wrote:

On Wed, 26 May 2021 17:27:39 +0800 Huazhong Tan wrote:

quoted

@@ -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.

quoted

+				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@lion.mk-sys.cz/ (local)

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.

quoted

+	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.

.