diff mbox series

[V3,net-next,06/10] net: hns3: add ethtool priv-flag for DIM

Message ID 1605151998-12633-7-git-send-email-tanhuazhong@huawei.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: hns3: updates for -next | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 151 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Huazhong Tan Nov. 12, 2020, 3:33 a.m. UTC
Add a control private flag in ethtool for enable/disable
DIM feature.

Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
---
 drivers/net/ethernet/hisilicon/hns3/hnae3.h        |  7 +++
 drivers/net/ethernet/hisilicon/hns3/hns3_enet.c    |  1 +
 drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c | 71 ++++++++++++++++++++++
 3 files changed, 79 insertions(+)

Comments

Jakub Kicinski Nov. 14, 2020, 6:54 p.m. UTC | #1
On Thu, 12 Nov 2020 11:33:14 +0800 Huazhong Tan wrote:
> Add a control private flag in ethtool for enable/disable
> DIM feature.
> 
> Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>

Please work on a common ethtool API for the configuration instead of
using private flags.

Private flags were overused because the old IOCTL-based ethtool was
hard to extend, but we have a netlink API now.

For example here you're making a choice between device and DIM
implementation of IRQ coalescing. You can add a new netlink attribute
to the ETHTOOL_MSG_COALESCE_GET/ETHTOOL_MSG_COALESCE_SET commands which
controls the type of adaptive coalescing (if enabled).


One question I don't think we have a strong answer for is how to handle
this extension from ethtool_ops point of view. Should we add a new
"extended" op which drivers may start implementing? Or separate the
structure passed in to the ops from the one used as uAPI?

Thoughts anyone?


Huazhong Tan, since the DIM and EQ/CQ patches may require more
infrastructure work feel free to repost the first 4 patches separately,
I can apply those as is.
Huazhong Tan Nov. 16, 2020, 8:41 a.m. UTC | #2
On 2020/11/15 2:54, Jakub Kicinski wrote:
> On Thu, 12 Nov 2020 11:33:14 +0800 Huazhong Tan wrote:
>> Add a control private flag in ethtool for enable/disable
>> DIM feature.
>>
>> Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>
> 
> Please work on a common ethtool API for the configuration instead of
> using private flags.
> 
> Private flags were overused because the old IOCTL-based ethtool was
> hard to extend, but we have a netlink API now.
> 
> For example here you're making a choice between device and DIM
> implementation of IRQ coalescing. You can add a new netlink attribute
> to the ETHTOOL_MSG_COALESCE_GET/ETHTOOL_MSG_COALESCE_SET commands which
> controls the type of adaptive coalescing (if enabled).
> 

The device's implementation of IRQ coalescing will be removed, if DIM 
works ok for a long time. So could this private flag for DIM be 
uptreamed as a transition scheme? And adding a new netlink attrtibute to 
controls the type of adaptive coalescing seems useless for other drivers.

> 
> One question I don't think we have a strong answer for is how to handle
> this extension from ethtool_ops point of view. Should we add a new
> "extended" op which drivers may start implementing? Or separate the
> structure passed in to the ops from the one used as uAPI?
> 
> Thoughts anyone?
> 
> 
> Huazhong Tan, since the DIM and EQ/CQ patches may require more
> infrastructure work feel free to repost the first 4 patches separately,
> I can apply those as is.
> 

ok, thanks.

> .
>
Jakub Kicinski Nov. 16, 2020, 6:12 p.m. UTC | #3
On Mon, 16 Nov 2020 16:41:45 +0800 tanhuazhong wrote:
> On 2020/11/15 2:54, Jakub Kicinski wrote:
> > On Thu, 12 Nov 2020 11:33:14 +0800 Huazhong Tan wrote:  
> >> Add a control private flag in ethtool for enable/disable
> >> DIM feature.
> >>
> >> Signed-off-by: Huazhong Tan <tanhuazhong@huawei.com>  
> > 
> > Please work on a common ethtool API for the configuration instead of
> > using private flags.
> > 
> > Private flags were overused because the old IOCTL-based ethtool was
> > hard to extend, but we have a netlink API now.
> > 
> > For example here you're making a choice between device and DIM
> > implementation of IRQ coalescing. You can add a new netlink attribute
> > to the ETHTOOL_MSG_COALESCE_GET/ETHTOOL_MSG_COALESCE_SET commands which
> > controls the type of adaptive coalescing (if enabled).
> 
> The device's implementation of IRQ coalescing will be removed, if DIM 
> works ok for a long time. So could this private flag for DIM be 
> uptreamed as a transition scheme? And adding a new netlink attrtibute to 
> controls the type of adaptive coalescing seems useless for other drivers.

The information whether the adaptive behavior is implemented by DIM,
device or custom driver implementation is useful regardless. Right now
users only see "adaptive" and don't know what implements it - device,
DIM or is it a custom implementation in the driver. So regardless if
you remove the priv flag, the "read"/"get" side of the information will
still be useful.

Besides you have another priv flag in this set that needs to be
converted to a generic attribute - the one for the timer reset
behavior.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/hisilicon/hns3/hnae3.h b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
index f9d4d23..18b3e43 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hnae3.h
+++ b/drivers/net/ethernet/hisilicon/hns3/hnae3.h
@@ -716,6 +716,11 @@  struct hnae3_roce_private_info {
 #define HNAE3_UPE		(HNAE3_USER_UPE | HNAE3_OVERFLOW_UPE)
 #define HNAE3_MPE		(HNAE3_USER_MPE | HNAE3_OVERFLOW_MPE)
 
+enum hnae3_pflag {
+	HNAE3_PFLAG_DIM_ENABLE,
+	HNAE3_PFLAG_MAX
+};
+
 struct hnae3_handle {
 	struct hnae3_client *client;
 	struct pci_dev *pdev;
@@ -738,6 +743,8 @@  struct hnae3_handle {
 
 	/* Network interface message level enabled bits */
 	u32 msg_enable;
+
+	unsigned long priv_flags;
 };
 
 #define hnae3_set_field(origin, mask, shift, val) \
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 9e895b9..a567557 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
@@ -4246,6 +4246,7 @@  static int hns3_client_init(struct hnae3_handle *handle)
 
 	set_bit(HNS3_NIC_STATE_INITED, &priv->state);
 	set_bit(HNS3_NIC_STATE_DIM_ENABLE, &priv->state);
+	handle->priv_flags |= BIT(HNAE3_PFLAG_DIM_ENABLE);
 
 	if (netif_msg_drv(handle))
 		hns3_info_show(priv);
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
index c30d5d3..e8adc70 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_ethtool.c
@@ -18,6 +18,11 @@  struct hns3_sfp_type {
 	u8 ext_type;
 };
 
+struct hns3_pflag_desc {
+	char name[ETH_GSTRING_LEN];
+	void (*handler)(struct net_device *netdev, bool enable);
+};
+
 /* tqp related stats */
 #define HNS3_TQP_STAT(_string, _member)	{			\
 	.stats_string = _string,				\
@@ -59,6 +64,8 @@  static const struct hns3_stats hns3_rxq_stats[] = {
 	HNS3_TQP_STAT("non_reuse_pg", non_reuse_pg),
 };
 
+#define HNS3_PRIV_FLAGS_LEN ARRAY_SIZE(hns3_priv_flags)
+
 #define HNS3_RXQ_STATS_COUNT ARRAY_SIZE(hns3_rxq_stats)
 
 #define HNS3_TQP_STATS_COUNT (HNS3_TXQ_STATS_COUNT + HNS3_RXQ_STATS_COUNT)
@@ -394,6 +401,26 @@  static void hns3_self_test(struct net_device *ndev,
 	netif_dbg(h, drv, ndev, "self test end\n");
 }
 
+static void hns3_update_state(struct net_device *netdev,
+			      enum hns3_nic_state state, bool enable)
+{
+	struct hns3_nic_priv *priv = netdev_priv(netdev);
+
+	if (enable)
+		set_bit(state, &priv->state);
+	else
+		clear_bit(state, &priv->state);
+}
+
+static void hns3_update_dim_state(struct net_device *netdev, bool enable)
+{
+	hns3_update_state(netdev, HNS3_NIC_STATE_DIM_ENABLE, enable);
+}
+
+static const struct hns3_pflag_desc hns3_priv_flags[HNAE3_PFLAG_MAX] = {
+	{ "dim_enable",		hns3_update_dim_state },
+};
+
 static int hns3_get_sset_count(struct net_device *netdev, int stringset)
 {
 	struct hnae3_handle *h = hns3_get_handle(netdev);
@@ -410,6 +437,9 @@  static int hns3_get_sset_count(struct net_device *netdev, int stringset)
 	case ETH_SS_TEST:
 		return ops->get_sset_count(h, stringset);
 
+	case ETH_SS_PRIV_FLAGS:
+		return HNAE3_PFLAG_MAX;
+
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -463,6 +493,7 @@  static void hns3_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
 	struct hnae3_handle *h = hns3_get_handle(netdev);
 	const struct hnae3_ae_ops *ops = h->ae_algo->ops;
 	char *buff = (char *)data;
+	int i;
 
 	if (!ops->get_strings)
 		return;
@@ -475,6 +506,13 @@  static void hns3_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
 	case ETH_SS_TEST:
 		ops->get_strings(h, stringset, data);
 		break;
+	case ETH_SS_PRIV_FLAGS:
+		for (i = 0; i < HNS3_PRIV_FLAGS_LEN; i++) {
+			snprintf(buff, ETH_GSTRING_LEN, "%s",
+				 hns3_priv_flags[i].name);
+			buff += ETH_GSTRING_LEN;
+		}
+		break;
 	default:
 		break;
 	}
@@ -1516,6 +1554,35 @@  static int hns3_get_module_eeprom(struct net_device *netdev,
 	return ops->get_module_eeprom(handle, ee->offset, ee->len, data);
 }
 
+static u32 hns3_get_priv_flags(struct net_device *netdev)
+{
+	struct hnae3_handle *handle = hns3_get_handle(netdev);
+
+	return handle->priv_flags;
+}
+
+static int hns3_set_priv_flags(struct net_device *netdev, u32 pflags)
+{
+	struct hnae3_handle *handle = hns3_get_handle(netdev);
+	u32 changed = pflags ^ handle->priv_flags;
+	u32 i;
+
+	for (i = 0; i < HNAE3_PFLAG_MAX; i++) {
+		if (changed & BIT(i)) {
+			bool enable = !(handle->priv_flags & BIT(i));
+
+			if (enable)
+				handle->priv_flags |= BIT(i);
+			else
+				handle->priv_flags &= ~BIT(i);
+
+			hns3_priv_flags[i].handler(netdev, enable);
+		}
+	}
+
+	return 0;
+}
+
 #define HNS3_ETHTOOL_COALESCE	(ETHTOOL_COALESCE_USECS |		\
 				 ETHTOOL_COALESCE_USE_ADAPTIVE |	\
 				 ETHTOOL_COALESCE_RX_USECS_HIGH |	\
@@ -1546,6 +1613,8 @@  static const struct ethtool_ops hns3vf_ethtool_ops = {
 	.get_link = hns3_get_link,
 	.get_msglevel = hns3_get_msglevel,
 	.set_msglevel = hns3_set_msglevel,
+	.get_priv_flags = hns3_get_priv_flags,
+	.set_priv_flags = hns3_set_priv_flags,
 };
 
 static const struct ethtool_ops hns3_ethtool_ops = {
@@ -1582,6 +1651,8 @@  static const struct ethtool_ops hns3_ethtool_ops = {
 	.set_fecparam = hns3_set_fecparam,
 	.get_module_info = hns3_get_module_info,
 	.get_module_eeprom = hns3_get_module_eeprom,
+	.get_priv_flags = hns3_get_priv_flags,
+	.set_priv_flags = hns3_set_priv_flags,
 };
 
 void hns3_ethtool_set_ops(struct net_device *netdev)