On 2020/11/19 12:15, Andrew Lunn wrote:

quoted

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 9ca87bc..afd8de2 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h

@@ -433,6 +433,7 @@ struct ethtool_modinfo {
   *	a TX interrupt, when the packet rate is above @pkt_rate_high.
   * @rate_sample_interval: How often to do adaptive coalescing packet rate
   *	sampling, measured in seconds.  Must not be zero.
+ * @use_dim: Use DIM for IRQ coalescing, if adaptive coalescing is enabled.
   *
   * Each pair of (usecs, max_frames) fields specifies that interrupts
   * should be coalesced until

@@ -483,6 +484,7 @@ struct ethtool_coalesce {
  	__u32	tx_coalesce_usecs_high;
  	__u32	tx_max_coalesced_frames_high;
  	__u32	rate_sample_interval;
+	__u32	use_dim;
  };

You cannot do this.

static noinline_for_stack int ethtool_set_coalesce(struct net_device *dev,
                                                    void __user *useraddr)
{
         struct ethtool_coalesce coalesce;
         int ret;

         if (!dev->ethtool_ops->set_coalesce)
                 return -EOPNOTSUPP;

         if (copy_from_user(&coalesce, useraddr, sizeof(coalesce)))
                 return -EFAULT;

An old ethtool binary is not going to set this extra last byte to
anything meaningful. You cannot tell if you have an old or new user
space, so you have no idea if it put anything into use_dim, or if it
is random junk.

You have to leave the IOCTL interface unchanged, and limit this new
feature to the netlink API.

Hi, Andrew.
thanks for pointing out this problem, i will fix it.
without callling set_coalesce/set_coalesce of ethtool_ops, do you have 
any suggestion for writing/reading this new attribute to/from the 
driver? add a new field in net_device or a new callback function in 
ethtool_ops seems not good.

quoted

diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index e2bf36e..e3458d9 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h

@@ -366,6 +366,7 @@ enum {
  	ETHTOOL_A_COALESCE_TX_USECS_HIGH,		/* u32 */
  	ETHTOOL_A_COALESCE_TX_MAX_FRAMES_HIGH,		/* u32 */
  	ETHTOOL_A_COALESCE_RATE_SAMPLE_INTERVAL,	/* u32 */
+	ETHTOOL_A_COALESCE_USE_DIM,			/* u8 */

This appears to be a boolean? So /* flag */ would be better. Or do you
think there is scope for a few different algorithms, and an enum would
be better. If so, you should add the enum with the two current
options.

	Andrew

ok, boolean seems enough.

Thanks.
Huazhong.

.