[PATCH net-next v2 00/12] ethtool: consolidate parameter checking for irq coalescing

[PATCH net-next v2 00/12] ethtool: consolidate parameter checking for irq coalescing

[Jakub Kicinski]

Hi!

This set aims to simplify and unify the unsupported irq
coalescing parameter handling.

First patch adds a bitmask which drivers should fill in
in their ethtool_ops structs to declare which parameters
they support. Core will then ensure that driver callback
won't see any parameter outside of that set.

This allows us to save some LoC and make sure all drivers
respond the same to unsupported parameters.

If any parameter driver does not support is set to a value
other than 0 core will return -EINVAL. In the future we can
reject any present but unsupported netlink attribute, without
assuming 0 means unset. We can also add some prints or extack,
perhaps a'la Intel's current code.

I started converting the drivers alphabetically but then
realized that for the first set it's probably best to
address a representative mix of actively developed drivers.

According to my unreliable math there are roughly 69 drivers
in the tree which support some form of interrupt coalescing
settings via ethtool. Of these roughly 17 reject parameters
they don't support.

I hope drivers which ignore the parameters don't care, and
won't care about the slight change in behavior. Once all
drivers are converted we can make the checking mandatory.

I've only tested the e1000e and virtio patches, the rest builds.

v2: fix up ice and virtio conversions

Jakub Kicinski (12):
  ethtool: add infrastructure for centralized checking of coalescing
    parameters
  xgbe: let core reject the unsupported coalescing parameters
  enic: let core reject the unsupported coalescing parameters
  stmmac: let core reject the unsupported coalescing parameters
  nfp: let core reject the unsupported coalescing parameters
  ionic: let core reject the unsupported coalescing parameters
  hisilicon: let core reject the unsupported coalescing parameters
  ice: let core reject the unsupported coalescing parameters
  bnxt: reject unsupported coalescing params
  mlx5: reject unsupported coalescing params
  e1000e: reject unsupported coalescing params
  virtio_net: reject unsupported coalescing params

 drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c  | 26 +-------
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  6 ++
 .../net/ethernet/cisco/enic/enic_ethtool.c    | 23 ++-----
 drivers/net/ethernet/hisilicon/hip04_eth.c    | 16 +----
 drivers/net/ethernet/intel/e1000e/ethtool.c   |  1 +
 drivers/net/ethernet/intel/ice/ice_ethtool.c  | 59 +----------------
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  |  3 +
 .../net/ethernet/mellanox/mlx5/core/en_rep.c  |  6 ++
 .../mellanox/mlx5/core/ipoib/ethtool.c        |  3 +
 .../ethernet/netronome/nfp/nfp_net_ethtool.c  | 22 +------
 .../ethernet/pensando/ionic/ionic_ethtool.c   | 23 +------
 .../ethernet/stmicro/stmmac/stmmac_ethtool.c  | 16 +----
 drivers/net/virtio_net.c                      | 14 +----
 include/linux/ethtool.h                       | 45 ++++++++++++-
 net/ethtool/ioctl.c                           | 63 +++++++++++++++++++
 15 files changed, 143 insertions(+), 183 deletions(-)

-- 
2.24.1

[PATCH net-next v2 01/12] ethtool: add infrastructure for centralized checking of coalescing parameters

[Jakub Kicinski]

Linux supports 22 different interrupt coalescing parameters.
No driver implements them all. Some drivers just ignore the
ones they don't support, while others have to carry a long
list of checks to reject unsupported settings.

To simplify the drivers add the ability to specify inside
ethtool_ops which parameters are supported and let the core
reject attempts to set any other one.

This commit makes the mechanism an opt-in, only drivers which
set ethtool_opts->coalesce_types to a non-zero value will have
the checks enforced.

The same mask is used for global and per queue settings.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/linux/ethtool.h | 45 +++++++++++++++++++++++++++--
 net/ethtool/ioctl.c     | 63 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+), 3 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 23373978cb3c..3c7328f02ba3 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -177,8 +177,44 @@ void ethtool_convert_legacy_u32_to_link_mode(unsigned long *dst,
 bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
 				     const unsigned long *src);
 
+#define ETHTOOL_COALESCE_RX_USECS		BIT(0)
+#define ETHTOOL_COALESCE_RX_MAX_FRAMES		BIT(1)
+#define ETHTOOL_COALESCE_RX_USECS_IRQ		BIT(2)
+#define ETHTOOL_COALESCE_RX_MAX_FRAMES_IRQ	BIT(3)
+#define ETHTOOL_COALESCE_TX_USECS		BIT(4)
+#define ETHTOOL_COALESCE_TX_MAX_FRAMES		BIT(5)
+#define ETHTOOL_COALESCE_TX_USECS_IRQ		BIT(6)
+#define ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ	BIT(7)
+#define ETHTOOL_COALESCE_STATS_BLOCK_USECS	BIT(8)
+#define ETHTOOL_COALESCE_USE_ADAPTIVE_RX	BIT(9)
+#define ETHTOOL_COALESCE_USE_ADAPTIVE_TX	BIT(10)
+#define ETHTOOL_COALESCE_PKT_RATE_LOW		BIT(11)
+#define ETHTOOL_COALESCE_RX_USECS_LOW		BIT(12)
+#define ETHTOOL_COALESCE_RX_MAX_FRAMES_LOW	BIT(13)
+#define ETHTOOL_COALESCE_TX_USECS_LOW		BIT(14)
+#define ETHTOOL_COALESCE_TX_MAX_FRAMES_LOW	BIT(15)
+#define ETHTOOL_COALESCE_PKT_RATE_HIGH		BIT(16)
+#define ETHTOOL_COALESCE_RX_USECS_HIGH		BIT(17)
+#define ETHTOOL_COALESCE_RX_MAX_FRAMES_HIGH	BIT(18)
+#define ETHTOOL_COALESCE_TX_USECS_HIGH		BIT(19)
+#define ETHTOOL_COALESCE_TX_MAX_FRAMES_HIGH	BIT(20)
+#define ETHTOOL_COALESCE_RATE_SAMPLE_INTERVAL	BIT(21)
+
+#define ETHTOOL_COALESCE_USECS						\
+	(ETHTOOL_COALESCE_RX_USECS | ETHTOOL_COALESCE_TX_USECS)
+#define ETHTOOL_COALESCE_MAX_FRAMES					\
+	(ETHTOOL_COALESCE_RX_MAX_FRAMES | ETHTOOL_COALESCE_TX_MAX_FRAMES)
+#define ETHTOOL_COALESCE_USECS_IRQ					\
+	(ETHTOOL_COALESCE_RX_USECS_IRQ | ETHTOOL_COALESCE_TX_USECS_IRQ)
+#define ETHTOOL_COALESCE_MAX_FRAMES_IRQ		\
+	(ETHTOOL_COALESCE_RX_MAX_FRAMES_IRQ |	\
+	 ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ)
+#define ETHTOOL_COALESCE_USE_ADAPTIVE					\
+	(ETHTOOL_COALESCE_USE_ADAPTIVE_RX | ETHTOOL_COALESCE_USE_ADAPTIVE_TX)
+
 /**
  * struct ethtool_ops - optional netdev operations
+ * @coalesce_types: supported types of interrupt coalescing.
  * @get_drvinfo: Report driver/device information.  Should only set the
  *	@driver, @version, @fw_version and @bus_info fields.  If not
  *	implemented, the @driver and @bus_info fields will be filled in
@@ -207,8 +243,9 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
  *	or zero.
  * @get_coalesce: Get interrupt coalescing parameters.  Returns a negative
  *	error code or zero.
- * @set_coalesce: Set interrupt coalescing parameters.  Returns a negative
- *	error code or zero.
+ * @set_coalesce: Set interrupt coalescing parameters.  Supported coalescing
+ *	types should be set in @coalesce_types.
+ *	Returns a negative error code or zero.
  * @get_ringparam: Report ring sizes
  * @set_ringparam: Set ring sizes.  Returns a negative error code or zero.
  * @get_pauseparam: Report pause parameters
@@ -292,7 +329,8 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
  * @set_per_queue_coalesce: Set interrupt coalescing parameters per queue.
  *	It must check that the given queue number is valid. If neither a RX nor
  *	a TX queue has this number, return -EINVAL. If only a RX queue or a TX
- *	queue has this number, ignore the inapplicable fields.
+ *	queue has this number, ignore the inapplicable fields. Supported
+ *	coalescing types should be set in @coalesce_types.
  *	Returns a negative error code or zero.
  * @get_link_ksettings: Get various device settings including Ethernet link
  *	settings. The %cmd and %link_mode_masks_nwords fields should be
@@ -323,6 +361,7 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
  * of the generic netdev features interface.
  */
 struct ethtool_ops {
+	u32	coalesce_types;
 	void	(*get_drvinfo)(struct net_device *, struct ethtool_drvinfo *);
 	int	(*get_regs_len)(struct net_device *);
 	void	(*get_regs)(struct net_device *, struct ethtool_regs *, void *);
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index f2fe8e5896dc..ae1a6eea36f1 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -1544,6 +1544,61 @@ static noinline_for_stack int ethtool_get_coalesce(struct net_device *dev,
 	return 0;
 }
 
+static bool
+ethtool_set_coalesce_supported(struct net_device *dev,
+			       struct ethtool_coalesce *coalesce)
+{
+	u32 used_types = 0;
+
+	if (coalesce->rx_coalesce_usecs)
+		used_types |= ETHTOOL_COALESCE_RX_USECS;
+	if (coalesce->rx_max_coalesced_frames)
+		used_types |= ETHTOOL_COALESCE_RX_MAX_FRAMES;
+	if (coalesce->rx_coalesce_usecs_irq)
+		used_types |= ETHTOOL_COALESCE_RX_USECS_IRQ;
+	if (coalesce->rx_max_coalesced_frames_irq)
+		used_types |= ETHTOOL_COALESCE_RX_MAX_FRAMES_IRQ;
+	if (coalesce->tx_coalesce_usecs)
+		used_types |= ETHTOOL_COALESCE_TX_USECS;
+	if (coalesce->tx_max_coalesced_frames)
+		used_types |= ETHTOOL_COALESCE_TX_MAX_FRAMES;
+	if (coalesce->tx_coalesce_usecs_irq)
+		used_types |= ETHTOOL_COALESCE_TX_USECS_IRQ;
+	if (coalesce->tx_max_coalesced_frames_irq)
+		used_types |= ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ;
+	if (coalesce->stats_block_coalesce_usecs)
+		used_types |= ETHTOOL_COALESCE_STATS_BLOCK_USECS;
+	if (coalesce->use_adaptive_rx_coalesce)
+		used_types |= ETHTOOL_COALESCE_USE_ADAPTIVE_RX;
+	if (coalesce->use_adaptive_tx_coalesce)
+		used_types |= ETHTOOL_COALESCE_USE_ADAPTIVE_TX;
+	if (coalesce->pkt_rate_low)
+		used_types |= ETHTOOL_COALESCE_PKT_RATE_LOW;
+	if (coalesce->rx_coalesce_usecs_low)
+		used_types |= ETHTOOL_COALESCE_RX_USECS_LOW;
+	if (coalesce->rx_max_coalesced_frames_low)
+		used_types |= ETHTOOL_COALESCE_RX_MAX_FRAMES_LOW;
+	if (coalesce->tx_coalesce_usecs_low)
+		used_types |= ETHTOOL_COALESCE_TX_USECS_LOW;
+	if (coalesce->tx_max_coalesced_frames_low)
+		used_types |= ETHTOOL_COALESCE_TX_MAX_FRAMES_LOW;
+	if (coalesce->pkt_rate_high)
+		used_types |= ETHTOOL_COALESCE_PKT_RATE_HIGH;
+	if (coalesce->rx_coalesce_usecs_high)
+		used_types |= ETHTOOL_COALESCE_RX_USECS_HIGH;
+	if (coalesce->rx_max_coalesced_frames_high)
+		used_types |= ETHTOOL_COALESCE_RX_MAX_FRAMES_HIGH;
+	if (coalesce->tx_coalesce_usecs_high)
+		used_types |= ETHTOOL_COALESCE_TX_USECS_HIGH;
+	if (coalesce->tx_max_coalesced_frames_high)
+		used_types |= ETHTOOL_COALESCE_TX_MAX_FRAMES_HIGH;
+	if (coalesce->rate_sample_interval)
+		used_types |= ETHTOOL_COALESCE_RATE_SAMPLE_INTERVAL;
+
+	return !dev->ethtool_ops->coalesce_types ||
+		(dev->ethtool_ops->coalesce_types & used_types) == used_types;
+}
+
 static noinline_for_stack int ethtool_set_coalesce(struct net_device *dev,
 						   void __user *useraddr)
 {
@@ -1555,6 +1610,9 @@ static noinline_for_stack int ethtool_set_coalesce(struct net_device *dev,
 	if (copy_from_user(&coalesce, useraddr, sizeof(coalesce)))
 		return -EFAULT;
 
+	if (!ethtool_set_coalesce_supported(dev, &coalesce))
+		return -EINVAL;
+
 	return dev->ethtool_ops->set_coalesce(dev, &coalesce);
 }
 
@@ -2336,6 +2394,11 @@ ethtool_set_per_queue_coalesce(struct net_device *dev,
 			goto roll_back;
 		}
 
+		if (!ethtool_set_coalesce_supported(dev, &coalesce)) {
+			ret = -EINVAL;
+			goto roll_back;
+		}
+
 		ret = dev->ethtool_ops->set_per_queue_coalesce(dev, bit, &coalesce);
 		if (ret != 0)
 			goto roll_back;
-- 
2.24.1

[PATCH net-next v2 02/12] xgbe: let core reject the unsupported coalescing parameters

[Jakub Kicinski]

Set ethtool_ops->coalesce_types to let the core reject
unsupported coalescing parameters.

This driver correctly rejects all unsupported parameters.
We are losing the print, and changing the return value
from EOPNOTSUPP to EINVAL.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c | 26 ++------------------
 1 file changed, 2 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c b/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
index b23c8ee24ee3..e373991c9905 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-ethtool.c
@@ -450,30 +450,6 @@ static int xgbe_set_coalesce(struct net_device *netdev,
 	unsigned int rx_frames, rx_riwt, rx_usecs;
 	unsigned int tx_frames;
 
-	/* Check for not supported parameters  */
-	if ((ec->rx_coalesce_usecs_irq) ||
-	    (ec->rx_max_coalesced_frames_irq) ||
-	    (ec->tx_coalesce_usecs) ||
-	    (ec->tx_coalesce_usecs_irq) ||
-	    (ec->tx_max_coalesced_frames_irq) ||
-	    (ec->stats_block_coalesce_usecs) ||
-	    (ec->use_adaptive_rx_coalesce) ||
-	    (ec->use_adaptive_tx_coalesce) ||
-	    (ec->pkt_rate_low) ||
-	    (ec->rx_coalesce_usecs_low) ||
-	    (ec->rx_max_coalesced_frames_low) ||
-	    (ec->tx_coalesce_usecs_low) ||
-	    (ec->tx_max_coalesced_frames_low) ||
-	    (ec->pkt_rate_high) ||
-	    (ec->rx_coalesce_usecs_high) ||
-	    (ec->rx_max_coalesced_frames_high) ||
-	    (ec->tx_coalesce_usecs_high) ||
-	    (ec->tx_max_coalesced_frames_high) ||
-	    (ec->rate_sample_interval)) {
-		netdev_err(netdev, "unsupported coalescing parameter\n");
-		return -EOPNOTSUPP;
-	}
-
 	rx_riwt = hw_if->usec_to_riwt(pdata, ec->rx_coalesce_usecs);
 	rx_usecs = ec->rx_coalesce_usecs;
 	rx_frames = ec->rx_max_coalesced_frames;
@@ -837,6 +813,8 @@ static int xgbe_set_channels(struct net_device *netdev,
 }
 
 static const struct ethtool_ops xgbe_ethtool_ops = {
+	.coalesce_types = ETHTOOL_COALESCE_RX_USECS |
+			  ETHTOOL_COALESCE_MAX_FRAMES,
 	.get_drvinfo = xgbe_get_drvinfo,
 	.get_msglevel = xgbe_get_msglevel,
 	.set_msglevel = xgbe_set_msglevel,
-- 
2.24.1

[PATCH net-next v2 03/12] enic: let core reject the unsupported coalescing parameters

[Jakub Kicinski]

Set ethtool_ops->coalesce_types to let the core reject
unsupported coalescing parameters.

This driver correctly rejects all unsupported parameters.
No functional changes.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 .../net/ethernet/cisco/enic/enic_ethtool.c    | 23 ++++---------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
index ebd5c2cf1efe..bff487a2c5be 100644
--- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
+++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
@@ -324,25 +324,6 @@ static int enic_coalesce_valid(struct enic *enic,
 	u32 rx_coalesce_usecs_low = min_t(u32, coalesce_usecs_max,
 					  ec->rx_coalesce_usecs_low);
 
-	if (ec->rx_max_coalesced_frames		||
-	    ec->rx_coalesce_usecs_irq		||
-	    ec->rx_max_coalesced_frames_irq	||
-	    ec->tx_max_coalesced_frames		||
-	    ec->tx_coalesce_usecs_irq		||
-	    ec->tx_max_coalesced_frames_irq	||
-	    ec->stats_block_coalesce_usecs	||
-	    ec->use_adaptive_tx_coalesce	||
-	    ec->pkt_rate_low			||
-	    ec->rx_max_coalesced_frames_low	||
-	    ec->tx_coalesce_usecs_low		||
-	    ec->tx_max_coalesced_frames_low	||
-	    ec->pkt_rate_high			||
-	    ec->rx_max_coalesced_frames_high	||
-	    ec->tx_coalesce_usecs_high		||
-	    ec->tx_max_coalesced_frames_high	||
-	    ec->rate_sample_interval)
-		return -EINVAL;
-
 	if ((vnic_dev_get_intr_mode(enic->vdev) != VNIC_DEV_INTR_MODE_MSIX) &&
 	    ec->tx_coalesce_usecs)
 		return -EINVAL;
@@ -636,6 +617,10 @@ static int enic_get_ts_info(struct net_device *netdev,
 }
 
 static const struct ethtool_ops enic_ethtool_ops = {
+	.coalesce_types = ETHTOOL_COALESCE_USECS |
+			  ETHTOOL_COALESCE_USE_ADAPTIVE_RX |
+			  ETHTOOL_COALESCE_RX_USECS_LOW |
+			  ETHTOOL_COALESCE_RX_USECS_HIGH,
 	.get_drvinfo = enic_get_drvinfo,
 	.get_msglevel = enic_get_msglevel,
 	.set_msglevel = enic_set_msglevel,
-- 
2.24.1

[PATCH net-next v2 04/12] stmmac: let core reject the unsupported coalescing parameters

[Jakub Kicinski]

Set ethtool_ops->coalesce_types to let the core reject
unsupported coalescing parameters.

This driver correctly rejects all unsupported parameters.
As a side effect of these changes the error code for
unsupported params changes from EOPNOTSUPP to EINVAL.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index b29603ec744c..6f551e75ad72 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -732,20 +732,6 @@ static int stmmac_set_coalesce(struct net_device *dev,
 	u32 rx_cnt = priv->plat->rx_queues_to_use;
 	unsigned int rx_riwt;
 
-	/* Check not supported parameters  */
-	if ((ec->rx_coalesce_usecs_irq) ||
-	    (ec->rx_max_coalesced_frames_irq) || (ec->tx_coalesce_usecs_irq) ||
-	    (ec->use_adaptive_rx_coalesce) || (ec->use_adaptive_tx_coalesce) ||
-	    (ec->pkt_rate_low) || (ec->rx_coalesce_usecs_low) ||
-	    (ec->rx_max_coalesced_frames_low) || (ec->tx_coalesce_usecs_high) ||
-	    (ec->tx_max_coalesced_frames_low) || (ec->pkt_rate_high) ||
-	    (ec->tx_coalesce_usecs_low) || (ec->rx_coalesce_usecs_high) ||
-	    (ec->rx_max_coalesced_frames_high) ||
-	    (ec->tx_max_coalesced_frames_irq) ||
-	    (ec->stats_block_coalesce_usecs) ||
-	    (ec->tx_max_coalesced_frames_high) || (ec->rate_sample_interval))
-		return -EOPNOTSUPP;
-
 	if (priv->use_riwt && (ec->rx_coalesce_usecs > 0)) {
 		rx_riwt = stmmac_usec2riwt(ec->rx_coalesce_usecs, priv);
 
@@ -914,6 +900,8 @@ static int stmmac_set_tunable(struct net_device *dev,
 }
 
 static const struct ethtool_ops stmmac_ethtool_ops = {
+	.coalesce_types = ETHTOOL_COALESCE_USECS |
+			  ETHTOOL_COALESCE_MAX_FRAMES,
 	.begin = stmmac_check_if_running,
 	.get_drvinfo = stmmac_ethtool_getdrvinfo,
 	.get_msglevel = stmmac_ethtool_getmsglevel,
-- 
2.24.1

[PATCH net-next v2 05/12] nfp: let core reject the unsupported coalescing parameters

[Jakub Kicinski]

Set ethtool_ops->coalesce_types to let the core reject
unsupported coalescing parameters.

This driver correctly rejects all unsupported parameters.
As a side effect of these changes the error code for
unsupported params changes from EOPNOTSUPP to EINVAL.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 .../ethernet/netronome/nfp/nfp_net_ethtool.c  | 22 ++-----------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
index d648e32c0520..71354ff2fe7d 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
@@ -1343,26 +1343,6 @@ static int nfp_net_set_coalesce(struct net_device *netdev,
 	struct nfp_net *nn = netdev_priv(netdev);
 	unsigned int factor;
 
-	if (ec->rx_coalesce_usecs_irq ||
-	    ec->rx_max_coalesced_frames_irq ||
-	    ec->tx_coalesce_usecs_irq ||
-	    ec->tx_max_coalesced_frames_irq ||
-	    ec->stats_block_coalesce_usecs ||
-	    ec->use_adaptive_rx_coalesce ||
-	    ec->use_adaptive_tx_coalesce ||
-	    ec->pkt_rate_low ||
-	    ec->rx_coalesce_usecs_low ||
-	    ec->rx_max_coalesced_frames_low ||
-	    ec->tx_coalesce_usecs_low ||
-	    ec->tx_max_coalesced_frames_low ||
-	    ec->pkt_rate_high ||
-	    ec->rx_coalesce_usecs_high ||
-	    ec->rx_max_coalesced_frames_high ||
-	    ec->tx_coalesce_usecs_high ||
-	    ec->tx_max_coalesced_frames_high ||
-	    ec->rate_sample_interval)
-		return -EOPNOTSUPP;
-
 	/* Compute factor used to convert coalesce '_usecs' parameters to
 	 * ME timestamp ticks.  There are 16 ME clock cycles for each timestamp
 	 * count.
@@ -1476,6 +1456,8 @@ static int nfp_net_set_channels(struct net_device *netdev,
 }
 
 static const struct ethtool_ops nfp_net_ethtool_ops = {
+	.coalesce_types = ETHTOOL_COALESCE_USECS |
+			  ETHTOOL_COALESCE_MAX_FRAMES,
 	.get_drvinfo		= nfp_net_get_drvinfo,
 	.get_link		= ethtool_op_get_link,
 	.get_ringparam		= nfp_net_get_ringparam,
-- 
2.24.1

[PATCH net-next v2 06/12] ionic: let core reject the unsupported coalescing parameters

[Jakub Kicinski]

Set ethtool_ops->coalesce_types to let the core reject
unsupported coalescing parameters.

This driver correctly rejects all unsupported parameters.
No functional changes.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Acked-by: Shannon Nelson <snelson@pensando.io>
---
 .../ethernet/pensando/ionic/ionic_ethtool.c   | 23 +------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
index f778fff034f5..83ea35715533 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
@@ -412,28 +412,6 @@ static int ionic_set_coalesce(struct net_device *netdev,
 	unsigned int i;
 	u32 coal;
 
-	if (coalesce->rx_max_coalesced_frames ||
-	    coalesce->rx_coalesce_usecs_irq ||
-	    coalesce->rx_max_coalesced_frames_irq ||
-	    coalesce->tx_max_coalesced_frames ||
-	    coalesce->tx_coalesce_usecs_irq ||
-	    coalesce->tx_max_coalesced_frames_irq ||
-	    coalesce->stats_block_coalesce_usecs ||
-	    coalesce->use_adaptive_rx_coalesce ||
-	    coalesce->use_adaptive_tx_coalesce ||
-	    coalesce->pkt_rate_low ||
-	    coalesce->rx_coalesce_usecs_low ||
-	    coalesce->rx_max_coalesced_frames_low ||
-	    coalesce->tx_coalesce_usecs_low ||
-	    coalesce->tx_max_coalesced_frames_low ||
-	    coalesce->pkt_rate_high ||
-	    coalesce->rx_coalesce_usecs_high ||
-	    coalesce->rx_max_coalesced_frames_high ||
-	    coalesce->tx_coalesce_usecs_high ||
-	    coalesce->tx_max_coalesced_frames_high ||
-	    coalesce->rate_sample_interval)
-		return -EINVAL;
-
 	ident = &lif->ionic->ident;
 	if (ident->dev.intr_coal_div == 0) {
 		netdev_warn(netdev, "bad HW value in dev.intr_coal_div = %d\n",
@@ -784,6 +762,7 @@ static int ionic_nway_reset(struct net_device *netdev)
 }
 
 static const struct ethtool_ops ionic_ethtool_ops = {
+	.coalesce_types = ETHTOOL_COALESCE_USECS,
 	.get_drvinfo		= ionic_get_drvinfo,
 	.get_regs_len		= ionic_get_regs_len,
 	.get_regs		= ionic_get_regs,
-- 
2.24.1

[PATCH net-next v2 07/12] hisilicon: let core reject the unsupported coalescing parameters

[Jakub Kicinski]

Set ethtool_ops->coalesce_types to let the core reject
unsupported coalescing parameters.

This driver correctly rejects all unsupported parameters.
As a side effect of these changes the error code for
unsupported params changes from EOPNOTSUPP to EINVAL.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/hisilicon/hip04_eth.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index d9718b87279d..74d1a5778c3e 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -811,20 +811,6 @@ static int hip04_set_coalesce(struct net_device *netdev,
 {
 	struct hip04_priv *priv = netdev_priv(netdev);
 
-	/* Check not supported parameters  */
-	if ((ec->rx_max_coalesced_frames) || (ec->rx_coalesce_usecs_irq) ||
-	    (ec->rx_max_coalesced_frames_irq) || (ec->tx_coalesce_usecs_irq) ||
-	    (ec->use_adaptive_rx_coalesce) || (ec->use_adaptive_tx_coalesce) ||
-	    (ec->pkt_rate_low) || (ec->rx_coalesce_usecs_low) ||
-	    (ec->rx_max_coalesced_frames_low) || (ec->tx_coalesce_usecs_high) ||
-	    (ec->tx_max_coalesced_frames_low) || (ec->pkt_rate_high) ||
-	    (ec->tx_coalesce_usecs_low) || (ec->rx_coalesce_usecs_high) ||
-	    (ec->rx_max_coalesced_frames_high) || (ec->rx_coalesce_usecs) ||
-	    (ec->tx_max_coalesced_frames_irq) ||
-	    (ec->stats_block_coalesce_usecs) ||
-	    (ec->tx_max_coalesced_frames_high) || (ec->rate_sample_interval))
-		return -EOPNOTSUPP;
-
 	if ((ec->tx_coalesce_usecs > HIP04_MAX_TX_COALESCE_USECS ||
 	     ec->tx_coalesce_usecs < HIP04_MIN_TX_COALESCE_USECS) ||
 	    (ec->tx_max_coalesced_frames > HIP04_MAX_TX_COALESCE_FRAMES ||
@@ -845,6 +831,8 @@ static void hip04_get_drvinfo(struct net_device *netdev,
 }
 
 static const struct ethtool_ops hip04_ethtool_ops = {
+	.coalesce_types		= ETHTOOL_COALESCE_TX_USECS |
+				  ETHTOOL_COALESCE_TX_MAX_FRAMES,
 	.get_coalesce		= hip04_get_coalesce,
 	.set_coalesce		= hip04_set_coalesce,
 	.get_drvinfo		= hip04_get_drvinfo,
-- 
2.24.1

[PATCH net-next v2 08/12] ice: let core reject the unsupported coalescing parameters

[Jakub Kicinski]

Set ethtool_ops->coalesce_types to let the core reject
unsupported coalescing parameters.

This driver correctly rejects all unsupported parameters.
As a side effect of these changes the info message about
the bad parameter will no longer be printed. We also
always reject the tx_coalesce_usecs_high param, even
if the target queue pair does not have a TX queue.

v2: allow adaptive TX

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/intel/ice/ice_ethtool.c | 59 +-------------------
 1 file changed, 3 insertions(+), 56 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index ab37dddb225b..67e162b653e7 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -3452,12 +3452,6 @@ ice_set_rc_coalesce(enum ice_container_type c_type, struct ethtool_coalesce *ec,
 
 		break;
 	case ICE_TX_CONTAINER:
-		if (ec->tx_coalesce_usecs_high) {
-			netdev_info(vsi->netdev, "setting %s-usecs-high is not supported\n",
-				    c_type_str);
-			return -EINVAL;
-		}
-
 		use_adaptive_coalesce = ec->use_adaptive_tx_coalesce;
 		coalesce_usecs = ec->tx_coalesce_usecs;
 
@@ -3533,53 +3527,6 @@ ice_set_q_coalesce(struct ice_vsi *vsi, struct ethtool_coalesce *ec, int q_num)
 	return 0;
 }
 
-/**
- * ice_is_coalesce_param_invalid - check for unsupported coalesce parameters
- * @netdev: pointer to the netdev associated with this query
- * @ec: ethtool structure to fill with driver's coalesce settings
- *
- * Print netdev info if driver doesn't support one of the parameters
- * and return error. When any parameters will be implemented, remove only
- * this parameter from param array.
- */
-static int
-ice_is_coalesce_param_invalid(struct net_device *netdev,
-			      struct ethtool_coalesce *ec)
-{
-	struct ice_ethtool_not_used {
-		u32 value;
-		const char *name;
-	} param[] = {
-		{ec->stats_block_coalesce_usecs, "stats-block-usecs"},
-		{ec->rate_sample_interval, "sample-interval"},
-		{ec->pkt_rate_low, "pkt-rate-low"},
-		{ec->pkt_rate_high, "pkt-rate-high"},
-		{ec->rx_max_coalesced_frames, "rx-frames"},
-		{ec->rx_coalesce_usecs_irq, "rx-usecs-irq"},
-		{ec->rx_max_coalesced_frames_irq, "rx-frames-irq"},
-		{ec->tx_max_coalesced_frames, "tx-frames"},
-		{ec->tx_coalesce_usecs_irq, "tx-usecs-irq"},
-		{ec->tx_max_coalesced_frames_irq, "tx-frames-irq"},
-		{ec->rx_coalesce_usecs_low, "rx-usecs-low"},
-		{ec->rx_max_coalesced_frames_low, "rx-frames-low"},
-		{ec->tx_coalesce_usecs_low, "tx-usecs-low"},
-		{ec->tx_max_coalesced_frames_low, "tx-frames-low"},
-		{ec->rx_max_coalesced_frames_high, "rx-frames-high"},
-		{ec->tx_max_coalesced_frames_high, "tx-frames-high"}
-	};
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(param); i++) {
-		if (param[i].value) {
-			netdev_info(netdev, "Setting %s not supported\n",
-				    param[i].name);
-			return -EINVAL;
-		}
-	}
-
-	return 0;
-}
-
 /**
  * ice_print_if_odd_usecs - print message if user tries to set odd [tx|rx]-usecs
  * @netdev: netdev used for print
@@ -3620,9 +3567,6 @@ __ice_set_coalesce(struct net_device *netdev, struct ethtool_coalesce *ec,
 	struct ice_netdev_priv *np = netdev_priv(netdev);
 	struct ice_vsi *vsi = np->vsi;
 
-	if (ice_is_coalesce_param_invalid(netdev, ec))
-		return -EINVAL;
-
 	if (q_num < 0) {
 		struct ice_q_vector *q_vector = vsi->q_vectors[0];
 		int v_idx;
@@ -3817,6 +3761,9 @@ ice_get_module_eeprom(struct net_device *netdev,
 }
 
 static const struct ethtool_ops ice_ethtool_ops = {
+	.coalesce_types = ETHTOOL_COALESCE_USECS |
+			  ETHTOOL_COALESCE_USE_ADAPTIVE |
+			  ETHTOOL_COALESCE_RX_USECS_HIGH,
 	.get_link_ksettings	= ice_get_link_ksettings,
 	.set_link_ksettings	= ice_set_link_ksettings,
 	.get_drvinfo		= ice_get_drvinfo,
-- 
2.24.1

[PATCH net-next v2 09/12] bnxt: reject unsupported coalescing params

[Jakub Kicinski]

Set ethtool_ops->coalesce_types to let the core reject
unsupported coalescing parameters.

This driver did not previously reject unsupported parameters.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index e8fc1671c581..1316432f70e1 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -3473,6 +3473,12 @@ void bnxt_ethtool_free(struct bnxt *bp)
 }
 
 const struct ethtool_ops bnxt_ethtool_ops = {
+	.coalesce_types = ETHTOOL_COALESCE_USECS |
+			  ETHTOOL_COALESCE_MAX_FRAMES |
+			  ETHTOOL_COALESCE_USECS_IRQ |
+			  ETHTOOL_COALESCE_MAX_FRAMES_IRQ |
+			  ETHTOOL_COALESCE_STATS_BLOCK_USECS |
+			  ETHTOOL_COALESCE_USE_ADAPTIVE_RX,
 	.get_link_ksettings	= bnxt_get_link_ksettings,
 	.set_link_ksettings	= bnxt_set_link_ksettings,
 	.get_pauseparam		= bnxt_get_pauseparam,
-- 
2.24.1

[PATCH net-next v2 10/12] mlx5: reject unsupported coalescing params

[Jakub Kicinski]

Set ethtool_ops->coalesce_types to let the core reject
unsupported coalescing parameters.

This driver did not previously reject unsupported parameters.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c    | 3 +++
 drivers/net/ethernet/mellanox/mlx5/core/en_rep.c        | 6 ++++++
 drivers/net/ethernet/mellanox/mlx5/core/ipoib/ethtool.c | 3 +++
 3 files changed, 12 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index 06f6f08ff5eb..739a0771944a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -1965,6 +1965,9 @@ static int mlx5e_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd)
 }
 
 const struct ethtool_ops mlx5e_ethtool_ops = {
+	.coalesce_types = ETHTOOL_COALESCE_USECS |
+			  ETHTOOL_COALESCE_MAX_FRAMES |
+			  ETHTOOL_COALESCE_USE_ADAPTIVE,
 	.get_drvinfo       = mlx5e_get_drvinfo,
 	.get_link          = ethtool_op_get_link,
 	.get_strings       = mlx5e_get_strings,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
index 1a8897f80547..abb8071e58c5 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
@@ -376,6 +376,9 @@ static int mlx5e_uplink_rep_set_link_ksettings(struct net_device *netdev,
 }
 
 static const struct ethtool_ops mlx5e_rep_ethtool_ops = {
+	.coalesce_types = ETHTOOL_COALESCE_USECS |
+			  ETHTOOL_COALESCE_MAX_FRAMES |
+			  ETHTOOL_COALESCE_USE_ADAPTIVE,
 	.get_drvinfo	   = mlx5e_rep_get_drvinfo,
 	.get_link	   = ethtool_op_get_link,
 	.get_strings       = mlx5e_rep_get_strings,
@@ -392,6 +395,9 @@ static const struct ethtool_ops mlx5e_rep_ethtool_ops = {
 };
 
 static const struct ethtool_ops mlx5e_uplink_rep_ethtool_ops = {
+	.coalesce_types = ETHTOOL_COALESCE_USECS |
+			  ETHTOOL_COALESCE_MAX_FRAMES |
+			  ETHTOOL_COALESCE_USE_ADAPTIVE,
 	.get_drvinfo	   = mlx5e_uplink_rep_get_drvinfo,
 	.get_link	   = ethtool_op_get_link,
 	.get_strings       = mlx5e_rep_get_strings,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ethtool.c
index 90cb50fe17fd..0deeed70f128 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ethtool.c
@@ -235,6 +235,9 @@ static int mlx5i_get_link_ksettings(struct net_device *netdev,
 }
 
 const struct ethtool_ops mlx5i_ethtool_ops = {
+	.coalesce_types = ETHTOOL_COALESCE_USECS |
+			  ETHTOOL_COALESCE_MAX_FRAMES |
+			  ETHTOOL_COALESCE_USE_ADAPTIVE,
 	.get_drvinfo        = mlx5i_get_drvinfo,
 	.get_strings        = mlx5i_get_strings,
 	.get_sset_count     = mlx5i_get_sset_count,
-- 
2.24.1

[PATCH net-next v2 11/12] e1000e: reject unsupported coalescing params

[Jakub Kicinski]

Set ethtool_ops->coalesce_types to let the core reject
unsupported coalescing parameters.

This driver did not previously reject unsupported parameters.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/ethernet/intel/e1000e/ethtool.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
index 9e7881db7859..9aef691a0e3a 100644
--- a/drivers/net/ethernet/intel/e1000e/ethtool.c
+++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
@@ -2307,6 +2307,7 @@ static int e1000e_get_ts_info(struct net_device *netdev,
 }
 
 static const struct ethtool_ops e1000_ethtool_ops = {
+	.coalesce_types = ETHTOOL_COALESCE_RX_USECS,
 	.get_drvinfo		= e1000_get_drvinfo,
 	.get_regs_len		= e1000_get_regs_len,
 	.get_regs		= e1000_get_regs,
-- 
2.24.1

[PATCH net-next v2 12/12] virtio_net: reject unsupported coalescing params

[Jakub Kicinski]

Set ethtool_ops->coalesce_types to let the core reject
unsupported coalescing parameters.

This driver correctly rejects all unsupported parameters.
No functional changes.

v2: correctly handle rx-frames (and adjust the commit msg)

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/virtio_net.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 84c0d9581f93..c2f2cb933ff7 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2202,23 +2202,14 @@ static int virtnet_get_link_ksettings(struct net_device *dev,
 static int virtnet_set_coalesce(struct net_device *dev,
 				struct ethtool_coalesce *ec)
 {
-	struct ethtool_coalesce ec_default = {
-		.cmd = ETHTOOL_SCOALESCE,
-		.rx_max_coalesced_frames = 1,
-	};
 	struct virtnet_info *vi = netdev_priv(dev);
 	int i, napi_weight;
 
-	if (ec->tx_max_coalesced_frames > 1)
+	if (ec->tx_max_coalesced_frames > 1 ||
+	    ec->rx_max_coalesced_frames != 1)
 		return -EINVAL;
 
-	ec_default.tx_max_coalesced_frames = ec->tx_max_coalesced_frames;
 	napi_weight = ec->tx_max_coalesced_frames ? NAPI_POLL_WEIGHT : 0;
-
-	/* disallow changes to fields not explicitly tested above */
-	if (memcmp(ec, &ec_default, sizeof(ec_default)))
-		return -EINVAL;
-
 	if (napi_weight ^ vi->sq[0].napi.weight) {
 		if (dev->flags & IFF_UP)
 			return -EBUSY;
@@ -2273,6 +2264,7 @@ static void virtnet_update_settings(struct virtnet_info *vi)
 }
 
 static const struct ethtool_ops virtnet_ethtool_ops = {
+	.coalesce_types = ETHTOOL_COALESCE_MAX_FRAMES,
 	.get_drvinfo = virtnet_get_drvinfo,
 	.get_link = ethtool_op_get_link,
 	.get_ringparam = virtnet_get_ringparam,
-- 
2.24.1

Re: [PATCH net-next v2 09/12] bnxt: reject unsupported coalescing params

[Michael Chan]

On Tue, Mar 3, 2020 at 8:34 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Set ethtool_ops->coalesce_types to let the core reject
> unsupported coalescing parameters.
>
> This driver did not previously reject unsupported parameters.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Michael Chan <michael.chan@broadcom.com>

Thanks.

Re: [PATCH net-next v2 01/12] ethtool: add infrastructure for centralized checking of coalescing parameters

[Michal Kubecek]

On Tue, Mar 03, 2020 at 08:33:43PM -0800, Jakub Kicinski wrote:
> Linux supports 22 different interrupt coalescing parameters.
> No driver implements them all. Some drivers just ignore the
> ones they don't support, while others have to carry a long
> list of checks to reject unsupported settings.
> 
> To simplify the drivers add the ability to specify inside
> ethtool_ops which parameters are supported and let the core
> reject attempts to set any other one.
> 
> This commit makes the mechanism an opt-in, only drivers which
> set ethtool_opts->coalesce_types to a non-zero value will have
> the checks enforced.
> 
> The same mask is used for global and per queue settings.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
[...]
> +static bool
> +ethtool_set_coalesce_supported(struct net_device *dev,
> +			       struct ethtool_coalesce *coalesce)
> +{
> +	u32 used_types = 0;
> +
> +	if (coalesce->rx_coalesce_usecs)
> +		used_types |= ETHTOOL_COALESCE_RX_USECS;
> +	if (coalesce->rx_max_coalesced_frames)
> +		used_types |= ETHTOOL_COALESCE_RX_MAX_FRAMES;
> +	if (coalesce->rx_coalesce_usecs_irq)
> +		used_types |= ETHTOOL_COALESCE_RX_USECS_IRQ;
> +	if (coalesce->rx_max_coalesced_frames_irq)
> +		used_types |= ETHTOOL_COALESCE_RX_MAX_FRAMES_IRQ;
> +	if (coalesce->tx_coalesce_usecs)
> +		used_types |= ETHTOOL_COALESCE_TX_USECS;
> +	if (coalesce->tx_max_coalesced_frames)
> +		used_types |= ETHTOOL_COALESCE_TX_MAX_FRAMES;
> +	if (coalesce->tx_coalesce_usecs_irq)
> +		used_types |= ETHTOOL_COALESCE_TX_USECS_IRQ;
> +	if (coalesce->tx_max_coalesced_frames_irq)
> +		used_types |= ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ;
> +	if (coalesce->stats_block_coalesce_usecs)
> +		used_types |= ETHTOOL_COALESCE_STATS_BLOCK_USECS;
> +	if (coalesce->use_adaptive_rx_coalesce)
> +		used_types |= ETHTOOL_COALESCE_USE_ADAPTIVE_RX;
> +	if (coalesce->use_adaptive_tx_coalesce)
> +		used_types |= ETHTOOL_COALESCE_USE_ADAPTIVE_TX;
> +	if (coalesce->pkt_rate_low)
> +		used_types |= ETHTOOL_COALESCE_PKT_RATE_LOW;
> +	if (coalesce->rx_coalesce_usecs_low)
> +		used_types |= ETHTOOL_COALESCE_RX_USECS_LOW;
> +	if (coalesce->rx_max_coalesced_frames_low)
> +		used_types |= ETHTOOL_COALESCE_RX_MAX_FRAMES_LOW;
> +	if (coalesce->tx_coalesce_usecs_low)
> +		used_types |= ETHTOOL_COALESCE_TX_USECS_LOW;
> +	if (coalesce->tx_max_coalesced_frames_low)
> +		used_types |= ETHTOOL_COALESCE_TX_MAX_FRAMES_LOW;
> +	if (coalesce->pkt_rate_high)
> +		used_types |= ETHTOOL_COALESCE_PKT_RATE_HIGH;
> +	if (coalesce->rx_coalesce_usecs_high)
> +		used_types |= ETHTOOL_COALESCE_RX_USECS_HIGH;
> +	if (coalesce->rx_max_coalesced_frames_high)
> +		used_types |= ETHTOOL_COALESCE_RX_MAX_FRAMES_HIGH;
> +	if (coalesce->tx_coalesce_usecs_high)
> +		used_types |= ETHTOOL_COALESCE_TX_USECS_HIGH;
> +	if (coalesce->tx_max_coalesced_frames_high)
> +		used_types |= ETHTOOL_COALESCE_TX_MAX_FRAMES_HIGH;
> +	if (coalesce->rate_sample_interval)
> +		used_types |= ETHTOOL_COALESCE_RATE_SAMPLE_INTERVAL;

Just an idea: perhaps we could use the fact that struct ethtool_coalesce
is de facto an array so that this block could be replaced by a loop like

	u32 supported_types = dev->ethtool_ops->coalesce_types;
	const u32 *values = &coalesce->rx_coalesce_usecs;

	for (i = 0; i < __ETHTOOL_COALESCE_COUNT; i++)
		if (values[i] && !(supported_types & BIT(i)))
			return false;

and to be sure, BUILD_BUG_ON() or static_assert() check that the offset
of ->rate_sample_interval matches ETHTOOL_COALESCE_RATE_SAMPLE_INTERVAL.

> +	return !dev->ethtool_ops->coalesce_types ||
> +		(dev->ethtool_ops->coalesce_types & used_types) == used_types;
> +}

I suggest to move the check for !dev->ethtool_ops->coalesce_types to the
beginning of the function so that we avoid calculating the bitmap if we
are not going to check it anyway.

Michal

Re: [PATCH net-next v2 01/12] ethtool: add infrastructure for centralized checking of coalescing parameters

[Jakub Kicinski]

On Wed, 4 Mar 2020 08:59:26 +0100 Michal Kubecek wrote:
> Just an idea: perhaps we could use the fact that struct ethtool_coalesce
> is de facto an array so that this block could be replaced by a loop like
> 
> 	u32 supported_types = dev->ethtool_ops->coalesce_types;
> 	const u32 *values = &coalesce->rx_coalesce_usecs;
> 
> 	for (i = 0; i < __ETHTOOL_COALESCE_COUNT; i++)
> 		if (values[i] && !(supported_types & BIT(i)))
> 			return false;
> 
> and to be sure, BUILD_BUG_ON() or static_assert() check that the offset
> of ->rate_sample_interval matches ETHTOOL_COALESCE_RATE_SAMPLE_INTERVAL.

I kind of prefer the greppability over the saved 40 lines :(
But I'm happy to change if we get more votes for the more concise
version. Or perhaps the Intel version with the warnings printed.

> > +	return !dev->ethtool_ops->coalesce_types ||
> > +		(dev->ethtool_ops->coalesce_types & used_types) == used_types;
> > +}  
> 
> I suggest to move the check for !dev->ethtool_ops->coalesce_types to the
> beginning of the function so that we avoid calculating the bitmap if we
> are not going to check it anyway.

Good point!

Re: [PATCH net-next v2 08/12] ice: let core reject the unsupported coalescing parameters

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

On Tue, 2020-03-03 at 20:33 -0800, Jakub Kicinski wrote:
> Set ethtool_ops->coalesce_types to let the core reject
> unsupported coalescing parameters.
> 
> This driver correctly rejects all unsupported parameters.
> As a side effect of these changes the info message about
> the bad parameter will no longer be printed. We also
> always reject the tx_coalesce_usecs_high param, even
> if the target queue pair does not have a TX queue.
> 
> v2: allow adaptive TX
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

> ---
>  drivers/net/ethernet/intel/ice/ice_ethtool.c | 59 +-----------------
> --
>  1 file changed, 3 insertions(+), 56 deletions(-)


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next v2 11/12] e1000e: reject unsupported coalescing params

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 434 bytes --]

On Tue, 2020-03-03 at 20:33 -0800, Jakub Kicinski wrote:
> Set ethtool_ops->coalesce_types to let the core reject
> unsupported coalescing parameters.
> 
> This driver did not previously reject unsupported parameters.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

> ---
>  drivers/net/ethernet/intel/e1000e/ethtool.c | 1 +
>  1 file changed, 1 insertion(+)


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net-next v2 01/12] ethtool: add infrastructure for centralized checking of coalescing parameters

[Alexander Duyck]

On Wed, 2020-03-04 at 10:00 -0800, Jakub Kicinski wrote:
> On Wed, 4 Mar 2020 08:59:26 +0100 Michal Kubecek wrote:
> > Just an idea: perhaps we could use the fact that struct ethtool_coalesce
> > is de facto an array so that this block could be replaced by a loop like
> > 
> > 	u32 supported_types = dev->ethtool_ops->coalesce_types;
> > 	const u32 *values = &coalesce->rx_coalesce_usecs;
> > 
> > 	for (i = 0; i < __ETHTOOL_COALESCE_COUNT; i++)
> > 		if (values[i] && !(supported_types & BIT(i)))
> > 			return false;
> > 
> > and to be sure, BUILD_BUG_ON() or static_assert() check that the offset
> > of ->rate_sample_interval matches ETHTOOL_COALESCE_RATE_SAMPLE_INTERVAL.
> 
> I kind of prefer the greppability over the saved 40 lines :(
> But I'm happy to change if we get more votes for the more concise
> version. Or perhaps the Intel version with the warnings printed.

I agree that it would make more sense to replace the types with an enum
definition, and then use the enum to define bits to be used by the
drivers.

> > > +	return !dev->ethtool_ops->coalesce_types ||
> > > +		(dev->ethtool_ops->coalesce_types & used_types) == used_types;
> > > +}  
> > 
> > I suggest to move the check for !dev->ethtool_ops->coalesce_types to the
> > beginning of the function so that we avoid calculating the bitmap if we
> > are not going to check it anyway.
> 
> Good point!

So one thing I just wanted to point out. The used_types won't necessarily
be correct because it is only actually checking for non-zero types. There
are some of these values where a zero is a valid input and the driver will
accept it, such as rx_coalesce_usecs for ixgbe. As such we might want to
rename the value to nonzero_types instead of used_types.

Re: [PATCH net-next v2 01/12] ethtool: add infrastructure for centralized checking of coalescing parameters

[Edward Cree]

On 04/03/2020 18:12, Alexander Duyck wrote:
> So one thing I just wanted to point out. The used_types won't necessarily
> be correct because it is only actually checking for non-zero types. There
> are some of these values where a zero is a valid input
Presumably in the netlink ethtool world we'll want to instead check if
 the attribute was passed?  Not sure but that might affect what semantics
 we want to imply now.

Re: [PATCH net-next v2 01/12] ethtool: add infrastructure for centralized checking of coalescing parameters

[Jakub Kicinski]

On Wed, 04 Mar 2020 10:12:30 -0800 Alexander Duyck wrote:
> On Wed, 2020-03-04 at 10:00 -0800, Jakub Kicinski wrote:
> > On Wed, 4 Mar 2020 08:59:26 +0100 Michal Kubecek wrote:  
> > > Just an idea: perhaps we could use the fact that struct ethtool_coalesce
> > > is de facto an array so that this block could be replaced by a loop like
> > > 
> > > 	u32 supported_types = dev->ethtool_ops->coalesce_types;
> > > 	const u32 *values = &coalesce->rx_coalesce_usecs;
> > > 
> > > 	for (i = 0; i < __ETHTOOL_COALESCE_COUNT; i++)
> > > 		if (values[i] && !(supported_types & BIT(i)))
> > > 			return false;
> > > 
> > > and to be sure, BUILD_BUG_ON() or static_assert() check that the offset
> > > of ->rate_sample_interval matches ETHTOOL_COALESCE_RATE_SAMPLE_INTERVAL.  
> > 
> > I kind of prefer the greppability over the saved 40 lines :(
> > But I'm happy to change if we get more votes for the more concise
> > version. Or perhaps the Intel version with the warnings printed.  
> 
> I agree that it would make more sense to replace the types with an enum
> definition, and then use the enum to define bits to be used by the
> drivers.

The only use for the enum would then be to automate the bit assignment?
Sounds like we would save some lines for the code and added some for
the definition. Maybe I'm missing the advantage the enum brings 🤔

> > > > +	return !dev->ethtool_ops->coalesce_types ||
> > > > +		(dev->ethtool_ops->coalesce_types & used_types) == used_types;
> > > > +}    
> > > 
> > > I suggest to move the check for !dev->ethtool_ops->coalesce_types to the
> > > beginning of the function so that we avoid calculating the bitmap if we
> > > are not going to check it anyway.  
> > 
> > Good point!  
> 
> So one thing I just wanted to point out. The used_types won't necessarily
> be correct because it is only actually checking for non-zero types. There
> are some of these values where a zero is a valid input and the driver will
> accept it, such as rx_coalesce_usecs for ixgbe. As such we might want to
> rename the value to nonzero_types instead of used_types.

Okay, I'll rename. I was also wondering if it should be "params" not
"types". Initially I was hoping there are categories of coalescing that
drivers implement, each with set of params. But it seems each vendor is
just picking fields they like. I think I'll do s/types/params/ as well.

Re: [PATCH net-next v2 01/12] ethtool: add infrastructure for centralized checking of coalescing parameters

[Jakub Kicinski]

On Wed, 4 Mar 2020 18:16:13 +0000 Edward Cree wrote:
> On 04/03/2020 18:12, Alexander Duyck wrote:
> > So one thing I just wanted to point out. The used_types won't necessarily
> > be correct because it is only actually checking for non-zero types. There
> > are some of these values where a zero is a valid input  
> Presumably in the netlink ethtool world we'll want to instead check if
>  the attribute was passed?  Not sure but that might affect what semantics
>  we want to imply now.

I mean... it's just a local variable in a helper... :)

I think netlink will need its own helper based on attribute presence,
as you said.

Re: [PATCH net-next v2 01/12] ethtool: add infrastructure for centralized checking of coalescing parameters

[Michal Kubecek]

On Wed, Mar 04, 2020 at 06:16:13PM +0000, Edward Cree wrote:
> On 04/03/2020 18:12, Alexander Duyck wrote:
> > So one thing I just wanted to point out. The used_types won't necessarily
> > be correct because it is only actually checking for non-zero types. There
> > are some of these values where a zero is a valid input
> Presumably in the netlink ethtool world we'll want to instead check if
> the attribute was passed?

Yes, that's one of the advantages: you can see which attributes
user(space) wants to set instead of guessing by comparing to zero or
current value.

> Not sure but that might affect what semantics we want to imply now.

My understanding is that Alexander's comment was only about the name of
the variable, not the semantics. The test will look different in netlink
implementation as I will also want to pass the information about (first)
offending attribute in extack.

Michal

Re: [PATCH net-next v2 01/12] ethtool: add infrastructure for centralized checking of coalescing parameters

[Michal Kubecek]

On Wed, Mar 04, 2020 at 10:00:50AM -0800, Jakub Kicinski wrote:
> On Wed, 4 Mar 2020 08:59:26 +0100 Michal Kubecek wrote:
> > Just an idea: perhaps we could use the fact that struct ethtool_coalesce
> > is de facto an array so that this block could be replaced by a loop like
> > 
> > 	u32 supported_types = dev->ethtool_ops->coalesce_types;
> > 	const u32 *values = &coalesce->rx_coalesce_usecs;
> > 
> > 	for (i = 0; i < __ETHTOOL_COALESCE_COUNT; i++)
> > 		if (values[i] && !(supported_types & BIT(i)))
> > 			return false;
> > 
> > and to be sure, BUILD_BUG_ON() or static_assert() check that the offset
> > of ->rate_sample_interval matches ETHTOOL_COALESCE_RATE_SAMPLE_INTERVAL.
> 
> I kind of prefer the greppability over the saved 40 lines :(
> But I'm happy to change if we get more votes for the more concise
> version. Or perhaps the Intel version with the warnings printed.

No problem, it was just an idea, I can see that each approach has its
advantages.

Michal

Re: [PATCH net-next v2 01/12] ethtool: add infrastructure for centralized checking of coalescing parameters

[Alexander Duyck]

On Wed, 2020-03-04 at 10:27 -0800, Jakub Kicinski wrote:
> On Wed, 04 Mar 2020 10:12:30 -0800 Alexander Duyck wrote:
> > On Wed, 2020-03-04 at 10:00 -0800, Jakub Kicinski wrote:
> > > On Wed, 4 Mar 2020 08:59:26 +0100 Michal Kubecek wrote:  
> > > > Just an idea: perhaps we could use the fact that struct ethtool_coalesce
> > > > is de facto an array so that this block could be replaced by a loop like
> > > > 
> > > > 	u32 supported_types = dev->ethtool_ops->coalesce_types;
> > > > 	const u32 *values = &coalesce->rx_coalesce_usecs;
> > > > 
> > > > 	for (i = 0; i < __ETHTOOL_COALESCE_COUNT; i++)
> > > > 		if (values[i] && !(supported_types & BIT(i)))
> > > > 			return false;
> > > > 
> > > > and to be sure, BUILD_BUG_ON() or static_assert() check that the offset
> > > > of ->rate_sample_interval matches ETHTOOL_COALESCE_RATE_SAMPLE_INTERVAL.  
> > > 
> > > I kind of prefer the greppability over the saved 40 lines :(
> > > But I'm happy to change if we get more votes for the more concise
> > > version. Or perhaps the Intel version with the warnings printed.  
> > 
> > I agree that it would make more sense to replace the types with an enum
> > definition, and then use the enum to define bits to be used by the
> > drivers.
> 
> The only use for the enum would then be to automate the bit assignment?
> Sounds like we would save some lines for the code and added some for
> the definition. Maybe I'm missing the advantage the enum brings 🤔

Well if you wanted to you could probably also update ethtool_coalesce to
support a unioned __u32 array if you wanted to be more explicit about it. 

I just figured that by making in an enum it becomes less error prone since
you can't accidentally leave a gap or end up renumbering things
unintentionally. Combine that with some logic to take care of the bit
shifting and it wouldn't differ much from how we handle the netdev feature
flags and the like.

> > > > > +	return !dev->ethtool_ops->coalesce_types ||
> > > > > +		(dev->ethtool_ops->coalesce_types & used_types) == used_types;
> > > > > +}    
> > > > 
> > > > I suggest to move the check for !dev->ethtool_ops->coalesce_types to the
> > > > beginning of the function so that we avoid calculating the bitmap if we
> > > > are not going to check it anyway.  
> > > 
> > > Good point!  
> > 
> > So one thing I just wanted to point out. The used_types won't necessarily
> > be correct because it is only actually checking for non-zero types. There
> > are some of these values where a zero is a valid input and the driver will
> > accept it, such as rx_coalesce_usecs for ixgbe. As such we might want to
> > rename the value to nonzero_types instead of used_types.
> 
> Okay, I'll rename. I was also wondering if it should be "params" not
> "types". Initially I was hoping there are categories of coalescing that
> drivers implement, each with set of params. But it seems each vendor is
> just picking fields they like. I think I'll do s/types/params/ as well.

Makes sense to me.

Re: [PATCH net-next v2 01/12] ethtool: add infrastructure for centralized checking of coalescing parameters

[Jacob Keller]

On 3/3/2020 8:33 PM, Jakub Kicinski wrote:
> Linux supports 22 different interrupt coalescing parameters.
> No driver implements them all. Some drivers just ignore the
> ones they don't support, while others have to carry a long
> list of checks to reject unsupported settings.
> > To simplify the drivers add the ability to specify inside
> ethtool_ops which parameters are supported and let the core
> reject attempts to set any other one.
> 

Nice!

Seems straight forward to me.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> This commit makes the mechanism an opt-in, only drivers which
> set ethtool_opts->coalesce_types to a non-zero value will have
> the checks enforced.
> 

Makes sense. We can enforce it in the future.

> The same mask is used for global and per queue settings.
> 

Seems reasonable to me. It is unlikely that per-queue and global
settings would ever be different.

> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  include/linux/ethtool.h | 45 +++++++++++++++++++++++++++--
>  net/ethtool/ioctl.c     | 63 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 105 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 23373978cb3c..3c7328f02ba3 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -177,8 +177,44 @@ void ethtool_convert_legacy_u32_to_link_mode(unsigned long *dst,
>  bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
>  				     const unsigned long *src);
>  
> +#define ETHTOOL_COALESCE_RX_USECS		BIT(0)
> +#define ETHTOOL_COALESCE_RX_MAX_FRAMES		BIT(1)
> +#define ETHTOOL_COALESCE_RX_USECS_IRQ		BIT(2)
> +#define ETHTOOL_COALESCE_RX_MAX_FRAMES_IRQ	BIT(3)
> +#define ETHTOOL_COALESCE_TX_USECS		BIT(4)
> +#define ETHTOOL_COALESCE_TX_MAX_FRAMES		BIT(5)
> +#define ETHTOOL_COALESCE_TX_USECS_IRQ		BIT(6)
> +#define ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ	BIT(7)
> +#define ETHTOOL_COALESCE_STATS_BLOCK_USECS	BIT(8)
> +#define ETHTOOL_COALESCE_USE_ADAPTIVE_RX	BIT(9)
> +#define ETHTOOL_COALESCE_USE_ADAPTIVE_TX	BIT(10)
> +#define ETHTOOL_COALESCE_PKT_RATE_LOW		BIT(11)
> +#define ETHTOOL_COALESCE_RX_USECS_LOW		BIT(12)
> +#define ETHTOOL_COALESCE_RX_MAX_FRAMES_LOW	BIT(13)
> +#define ETHTOOL_COALESCE_TX_USECS_LOW		BIT(14)
> +#define ETHTOOL_COALESCE_TX_MAX_FRAMES_LOW	BIT(15)
> +#define ETHTOOL_COALESCE_PKT_RATE_HIGH		BIT(16)
> +#define ETHTOOL_COALESCE_RX_USECS_HIGH		BIT(17)
> +#define ETHTOOL_COALESCE_RX_MAX_FRAMES_HIGH	BIT(18)
> +#define ETHTOOL_COALESCE_TX_USECS_HIGH		BIT(19)
> +#define ETHTOOL_COALESCE_TX_MAX_FRAMES_HIGH	BIT(20)
> +#define ETHTOOL_COALESCE_RATE_SAMPLE_INTERVAL	BIT(21)
> +
> +#define ETHTOOL_COALESCE_USECS						\
> +	(ETHTOOL_COALESCE_RX_USECS | ETHTOOL_COALESCE_TX_USECS)
> +#define ETHTOOL_COALESCE_MAX_FRAMES					\
> +	(ETHTOOL_COALESCE_RX_MAX_FRAMES | ETHTOOL_COALESCE_TX_MAX_FRAMES)
> +#define ETHTOOL_COALESCE_USECS_IRQ					\
> +	(ETHTOOL_COALESCE_RX_USECS_IRQ | ETHTOOL_COALESCE_TX_USECS_IRQ)
> +#define ETHTOOL_COALESCE_MAX_FRAMES_IRQ		\
> +	(ETHTOOL_COALESCE_RX_MAX_FRAMES_IRQ |	\
> +	 ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ)
> +#define ETHTOOL_COALESCE_USE_ADAPTIVE					\
> +	(ETHTOOL_COALESCE_USE_ADAPTIVE_RX | ETHTOOL_COALESCE_USE_ADAPTIVE_TX)
> +
>  /**
>   * struct ethtool_ops - optional netdev operations
> + * @coalesce_types: supported types of interrupt coalescing.
>   * @get_drvinfo: Report driver/device information.  Should only set the
>   *	@driver, @version, @fw_version and @bus_info fields.  If not
>   *	implemented, the @driver and @bus_info fields will be filled in
> @@ -207,8 +243,9 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
>   *	or zero.
>   * @get_coalesce: Get interrupt coalescing parameters.  Returns a negative
>   *	error code or zero.
> - * @set_coalesce: Set interrupt coalescing parameters.  Returns a negative
> - *	error code or zero.
> + * @set_coalesce: Set interrupt coalescing parameters.  Supported coalescing
> + *	types should be set in @coalesce_types.
> + *	Returns a negative error code or zero.
>   * @get_ringparam: Report ring sizes
>   * @set_ringparam: Set ring sizes.  Returns a negative error code or zero.
>   * @get_pauseparam: Report pause parameters
> @@ -292,7 +329,8 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
>   * @set_per_queue_coalesce: Set interrupt coalescing parameters per queue.
>   *	It must check that the given queue number is valid. If neither a RX nor
>   *	a TX queue has this number, return -EINVAL. If only a RX queue or a TX
> - *	queue has this number, ignore the inapplicable fields.
> + *	queue has this number, ignore the inapplicable fields. Supported
> + *	coalescing types should be set in @coalesce_types.
>   *	Returns a negative error code or zero.
>   * @get_link_ksettings: Get various device settings including Ethernet link
>   *	settings. The %cmd and %link_mode_masks_nwords fields should be
> @@ -323,6 +361,7 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
>   * of the generic netdev features interface.
>   */
>  struct ethtool_ops {
> +	u32	coalesce_types;

It feels weird to me to put this data in ops, but I can't think of a
better place.

Thanks,
Jake

Re: [PATCH net-next v2 08/12] ice: let core reject the unsupported coalescing parameters

[Jacob Keller]

On 3/3/2020 8:33 PM, Jakub Kicinski wrote:
> Set ethtool_ops->coalesce_types to let the core reject
> unsupported coalescing parameters.
> 
> This driver correctly rejects all unsupported parameters.
> As a side effect of these changes the info message about
> the bad parameter will no longer be printed. We also
> always reject the tx_coalesce_usecs_high param, even
> if the target queue pair does not have a TX queue.
> 

Sure. The extra messaging isn't really helpful, and we could make it
part of the extended ack message in ethtool core later for callers who
use the ethlink interface.

Always rejecting tx_coalesce_usecs_high makes sense to me, we don't
support it at all.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Re: [PATCH net-next v2 01/12] ethtool: add infrastructure for centralized checking of coalescing parameters

[Jacob Keller]

On 3/4/2020 10:00 AM, Jakub Kicinski wrote:
> On Wed, 4 Mar 2020 08:59:26 +0100 Michal Kubecek wrote:
>> Just an idea: perhaps we could use the fact that struct ethtool_coalesce
>> is de facto an array so that this block could be replaced by a loop like
>>
>> 	u32 supported_types = dev->ethtool_ops->coalesce_types;
>> 	const u32 *values = &coalesce->rx_coalesce_usecs;
>>
>> 	for (i = 0; i < __ETHTOOL_COALESCE_COUNT; i++)
>> 		if (values[i] && !(supported_types & BIT(i)))
>> 			return false;
>>
>> and to be sure, BUILD_BUG_ON() or static_assert() check that the offset
>> of ->rate_sample_interval matches ETHTOOL_COALESCE_RATE_SAMPLE_INTERVAL.
> 
> I kind of prefer the greppability over the saved 40 lines :(
> But I'm happy to change if we get more votes for the more concise
> version. Or perhaps the Intel version with the warnings printed.
> 

We could go the looped route, but I like being able to search the code
for references. Seems like the main point of the loop would be to
simplify catching new added parameters in the future.

I don't really have a strong preference.

Thanks,
Jake

Re: [PATCH net-next v2 01/12] ethtool: add infrastructure for centralized checking of coalescing parameters

[Jakub Kicinski]

On Wed, 04 Mar 2020 10:50:57 -0800 Alexander Duyck wrote:
> I just figured that by making in an enum it becomes less error prone since
> you can't accidentally leave a gap or end up renumbering things
> unintentionally.

True. That said the API will remain frozen for a little bit
longer - until the netlink conversion of this op.

> Combine that with some logic to take care of the bit
> shifting and it wouldn't differ much from how we handle the netdev feature
> flags and the like.

To be honest it was netdev features that made me dislike the model
slightly :)

Drivers sometimes print features as a hex u64 and trying to decode
that with the automatically assigned bits takes extra effort :S

With straight up defines I can do:

for i in `seq 0 32`; do 
	[ $((x & (1<<i))) -ne 0 ] && git grep "ETHTOOL_COALESCE_.*BIT($i)"
done

where x is set to whatever the driver printed. E.g.:

$ x=0xc37c0; for i in `seq 0 22`; do [ $((x & (1<<i))) -ne 0 ] && git grep  "ETHTOOL_COALESCE_.*BIT($i)"; done
include/linux/ethtool.h:#define ETHTOOL_COALESCE_TX_USECS_IRQ           BIT(6)
include/linux/ethtool.h:#define ETHTOOL_COALESCE_TX_MAX_FRAMES_IRQ      BIT(7)
include/linux/ethtool.h:#define ETHTOOL_COALESCE_STATS_BLOCK_USECS      BIT(8)
include/linux/ethtool.h:#define ETHTOOL_COALESCE_USE_ADAPTIVE_RX        BIT(9)
include/linux/ethtool.h:#define ETHTOOL_COALESCE_USE_ADAPTIVE_TX        BIT(10)
include/linux/ethtool.h:#define ETHTOOL_COALESCE_RX_USECS_LOW           BIT(12)
include/linux/ethtool.h:#define ETHTOOL_COALESCE_RX_MAX_FRAMES_LOW      BIT(13)
include/linux/ethtool.h:#define ETHTOOL_COALESCE_RX_MAX_FRAMES_HIGH     BIT(18)
include/linux/ethtool.h:#define ETHTOOL_COALESCE_TX_USECS_HIGH          BIT(19)

With enum there are no explicit numbers so nothing to grep for.
I could probably squeeze this information out of debug info, but 
debug info incantations don't stick in my memory.

IDK if that's a sane or valid reason, LMK if you feel strongly and
I'll convert. We'll probably revisit this anyway when netlink comes
with the attribute presence checking.