[PATCH v4 net-next 00/10] net/ncsi: Add debugging functionality

[PATCH v4 net-next 00/10] net/ncsi: Add debugging functionality

[Gavin Shan]

This series supports NCSI debugging infrastructure by adding several
ethtool commands and one debugfs file. It was inspired by the reported
issues: No available package and channel are probed successfully.
Obviously, we don't have a debugging infrastructure for NCSI stack yet.

The first 3 patches, fixing some issues, aren't relevant to the
subject. I included them because I expect they can be merged beofre
the code for debugging infrastructure. PATCH[4,5,6,7] adds ethtool
commands to dump NCSI topology, channel information, HW statistics
and SW statistics. PATCH[8] adds debugfs entry /sys/kernel/debug/
ncsi/eth0/pkt, which sends NCSI command packet on write. The NCSI
response packet are dumped on read. PATCH[9,10] fix two issues
found by the debugging functionality.

Changelog
=========
v4:
   * Replace debugfs entries with ethtool commands                   (Dave)
   * Add ethtool commands to dump NCSI topology, channel info and
     HW statistics                                                   (Gavin)
v3:
   * Use pr_debug() instead of pr_warn() upon failure to create
     debugfs directory or file                                       (Joe Perches)
   * Use relative debugfs path/file names in debug messages in
     ncsi-debug.c                                                    (Joe Perches)
   * Use const specifier for @ncsi_pkt_handlers and @ranges          (Joe Perches)
   * Eliminate CONFIG_NET_NCSI_DEBUG ifdef's in *.c                  (Jakub Kicinski)
v2:
   * Use debugfs instead of procfs                                   (Joe Perches)

Gavin Shan (10):
  net/ncsi: Disable HWA mode when no channels are found
  net/ncsi: Properly track channel monitor timer state
  net/ncsi: Enforce failover on link monitor timeout
  net/ncsi: Ethtool operation to get NCSI topology
  net/ncsi: Ethtool operation to get NCSI channel info
  net/ncsi: Ethtool operation to get NCSI hw statistics
  net/ncsi: Ethtool operation to get NCSI sw statistics
  net/ncsi: Support NCSI packet generation
  net/ncsi: No error report on DP response to non-existing package
  net/ncsi: Fix length of GVI response packet

 include/linux/ethtool.h      |   8 +
 include/uapi/linux/ethtool.h | 312 ++++++++++++++++++
 net/core/ethtool.c           | 120 +++++++
 net/ncsi/Kconfig             |   9 +
 net/ncsi/Makefile            |   4 +-
 net/ncsi/internal.h          |  71 ++++
 net/ncsi/ncsi-aen.c          |  14 +-
 net/ncsi/ncsi-cmd.c          |  13 +-
 net/ncsi/ncsi-debug.c        | 759 +++++++++++++++++++++++++++++++++++++++++++
 net/ncsi/ncsi-ethtool.c      | 321 ++++++++++++++++++
 net/ncsi/ncsi-manage.c       |  56 +++-
 net/ncsi/ncsi-rsp.c          |  34 +-
 12 files changed, 1708 insertions(+), 13 deletions(-)
 create mode 100644 net/ncsi/ncsi-debug.c
 create mode 100644 net/ncsi/ncsi-ethtool.c

-- 
2.7.4

[PATCH v4 net-next 01/10] net/ncsi: Disable HWA mode when no channels are found

[Gavin Shan]

When there are no NCSI channels probed, HWA (Hardware Arbitration)
mode is enabled. It's not correct because HWA depends on the fact:
NCSI channels exist and all of them support HWA mode. This disables
HWA when no channels are probed.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 net/ncsi/ncsi-manage.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index a3bd5fa..5073e15 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -839,12 +839,15 @@ static bool ncsi_check_hwa(struct ncsi_dev_priv *ndp)
 	struct ncsi_package *np;
 	struct ncsi_channel *nc;
 	unsigned int cap;
+	bool has_channel = false;
 
 	/* The hardware arbitration is disabled if any one channel
 	 * doesn't support explicitly.
 	 */
 	NCSI_FOR_EACH_PACKAGE(ndp, np) {
 		NCSI_FOR_EACH_CHANNEL(np, nc) {
+			has_channel = true;
+
 			cap = nc->caps[NCSI_CAP_GENERIC].cap;
 			if (!(cap & NCSI_CAP_GENERIC_HWA) ||
 			    (cap & NCSI_CAP_GENERIC_HWA_MASK) !=
@@ -855,8 +858,13 @@ static bool ncsi_check_hwa(struct ncsi_dev_priv *ndp)
 		}
 	}
 
-	ndp->flags |= NCSI_DEV_HWA;
-	return true;
+	if (has_channel) {
+		ndp->flags |= NCSI_DEV_HWA;
+		return true;
+	}
+
+	ndp->flags &= ~NCSI_DEV_HWA;
+	return false;
 }
 
 static int ncsi_enable_hwa(struct ncsi_dev_priv *ndp)
-- 
2.7.4

[PATCH v4 net-next 02/10] net/ncsi: Properly track channel monitor timer state

[Gavin Shan]

The field @monitor.enabled in the NCSI channel descriptor is used
to track the state of channel monitor timer. It indicates the timer's
state (pending or not). We could not start the timer again in its
handler. In that case, We missed to update @monitor.enabled to false.
It leads to below warning printed by WARN_ON_ONCE() when the monitor
is restarted afterwards.

   ------------[ cut here ]------------
   WARNING: CPU: 0 PID: 411 at /var/lib/jenkins/workspace/openbmc-build \
   /distro/ubuntu/target/palmetto/openbmc/build/tmp/work-shared/palmetto \
   net/ncsi/ncsi-manage.c:240 ncsi_start_channel_monitor+0x44/0x7c
   CPU: 0 PID: 411 Comm: kworker/0:3 Not tainted \
   4.7.10-f26558191540830589fe03932d05577957670b8d #1
   Hardware name: ASpeed SoC
   Workqueue: events ncsi_dev_work
   [<c0106cbc>] (unwind_backtrace) from [<c0104f04>] (show_stack+0x10/0x14)
   [<c0104f04>] (show_stack) from [<c010eef8>] (__warn+0xc4/0xf0)
   [<c010eef8>] (__warn) from [<c010f018>] (warn_slowpath_null+0x1c/0x24)
   [<c010f018>] (warn_slowpath_null) from [<c03e1d10>] (ncsi_start_channel_monitor+0x44/0x7c)
   [<c03e1d10>] (ncsi_start_channel_monitor) from [<c03e29c4>] (ncsi_configure_channel+0x27c/0x2dc)
   [<c03e29c4>] (ncsi_configure_channel) from [<c03e31d0>] (ncsi_dev_work+0x39c/0x3e8)
   [<c03e31d0>] (ncsi_dev_work) from [<c0120288>] (process_one_work+0x1b8/0x2fc)
   [<c0120288>] (process_one_work) from [<c01206bc>] (worker_thread+0x2c0/0x3f8)
   [<c01206bc>] (worker_thread) from [<c0124b10>] (kthread+0xd0/0xe8)
   [<c0124b10>] (kthread) from [<c0102250>] (ret_from_fork+0x14/0x24)
   ---[ end trace 110cccf2b038c44d ]---

This fixes the issue by updating @monitor.enabled to false if needed.

Reported-by: Sridevi Ramesh <sridevra@in.ibm.com>
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 net/ncsi/ncsi-manage.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 5073e15..c71a3a5 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -183,11 +183,16 @@ static void ncsi_channel_monitor(unsigned long data)
 	monitor_state = nc->monitor.state;
 	spin_unlock_irqrestore(&nc->lock, flags);
 
-	if (!enabled || chained)
+	if (!enabled || chained) {
+		ncsi_stop_channel_monitor(nc);
 		return;
+	}
+
 	if (state != NCSI_CHANNEL_INACTIVE &&
-	    state != NCSI_CHANNEL_ACTIVE)
+	    state != NCSI_CHANNEL_ACTIVE) {
+		ncsi_stop_channel_monitor(nc);
 		return;
+	}
 
 	switch (monitor_state) {
 	case NCSI_CHANNEL_MONITOR_START:
@@ -199,6 +204,7 @@ static void ncsi_channel_monitor(unsigned long data)
 		nca.req_flags = 0;
 		ret = ncsi_xmit_cmd(&nca);
 		if (ret) {
+			ncsi_stop_channel_monitor(nc);
 			netdev_err(ndp->ndev.dev, "Error %d sending GLS\n",
 				   ret);
 			return;
@@ -218,6 +224,8 @@ static void ncsi_channel_monitor(unsigned long data)
 		nc->state = NCSI_CHANNEL_INVISIBLE;
 		spin_unlock_irqrestore(&nc->lock, flags);
 
+		ncsi_stop_channel_monitor(nc);
+
 		spin_lock_irqsave(&ndp->lock, flags);
 		nc->state = NCSI_CHANNEL_INACTIVE;
 		list_add_tail_rcu(&nc->link, &ndp->channel_queue);
@@ -257,6 +265,10 @@ void ncsi_stop_channel_monitor(struct ncsi_channel *nc)
 	nc->monitor.enabled = false;
 	spin_unlock_irqrestore(&nc->lock, flags);
 
+	/* The timer isn't in pending state if we're deleting the timer
+	 * in its handler. del_timer_sync() can detect it and just does
+	 * nothing.
+	 */
 	del_timer_sync(&nc->monitor.timer);
 }
 
-- 
2.7.4

[PATCH v4 net-next 03/10] net/ncsi: Enforce failover on link monitor timeout

[Gavin Shan]

The NCSI channel has been configured to provide service if its link
monitor timer is enabled, regardless of its state (inactive or active).
So the timeout event on the link monitor indicates the out-of-service
on that channel, for which a failover is needed.

This sets NCSI_DEV_RESHUFFLE flag to enforce failover on link monitor
timeout, regardless the channel's original state (inactive or active).
Also, the link is put into "down" state to give the failing channel
lowest priority when selecting for the active channel. The state of
failing channel should be set to active in order for deinitialization
and failover to be done.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 net/ncsi/ncsi-manage.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index c71a3a5..13ad1f26 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -170,6 +170,7 @@ static void ncsi_channel_monitor(unsigned long data)
 	struct ncsi_channel *nc = (struct ncsi_channel *)data;
 	struct ncsi_package *np = nc->package;
 	struct ncsi_dev_priv *ndp = np->ndp;
+	struct ncsi_channel_mode *ncm;
 	struct ncsi_cmd_arg nca;
 	bool enabled, chained;
 	unsigned int monitor_state;
@@ -214,20 +215,21 @@ static void ncsi_channel_monitor(unsigned long data)
 	case NCSI_CHANNEL_MONITOR_WAIT ... NCSI_CHANNEL_MONITOR_WAIT_MAX:
 		break;
 	default:
-		if (!(ndp->flags & NCSI_DEV_HWA) &&
-		    state == NCSI_CHANNEL_ACTIVE) {
+		if (!(ndp->flags & NCSI_DEV_HWA)) {
 			ncsi_report_link(ndp, true);
 			ndp->flags |= NCSI_DEV_RESHUFFLE;
 		}
 
+		ncm = &nc->modes[NCSI_MODE_LINK];
 		spin_lock_irqsave(&nc->lock, flags);
 		nc->state = NCSI_CHANNEL_INVISIBLE;
+		ncm->data[2] &= ~0x1;
 		spin_unlock_irqrestore(&nc->lock, flags);
 
 		ncsi_stop_channel_monitor(nc);
 
 		spin_lock_irqsave(&ndp->lock, flags);
-		nc->state = NCSI_CHANNEL_INACTIVE;
+		nc->state = NCSI_CHANNEL_ACTIVE;
 		list_add_tail_rcu(&nc->link, &ndp->channel_queue);
 		spin_unlock_irqrestore(&ndp->lock, flags);
 		ncsi_process_next_channel(ndp);
-- 
2.7.4

[PATCH v4 net-next 04/10] net/ncsi: Ethtool operation to get NCSI topology

[Gavin Shan]

This adds ethtool command (ETHTOOL_GNCSICHANNELS) to retrieve the
NCSI channels that are associated with the specified netdev. The
ethtool operation (get_ncsi_channels()) is initialized or destroyed
when the NCSI device is registerred or unregistered. The userspace
and kernel has to negotiate on the total number of NCSI channels
so that userspace can allocate enough memory to convey data. Here
is the example output from modified (private) ethtool:

 # ethtool --ncsi eth0 channels
 2 channels:
 0:0     Active
 0:1

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 include/linux/ethtool.h      |  2 ++
 include/uapi/linux/ethtool.h | 17 ++++++++++
 net/core/ethtool.c           | 40 ++++++++++++++++++++++
 net/ncsi/Makefile            |  3 +-
 net/ncsi/internal.h          |  4 +++
 net/ncsi/ncsi-ethtool.c      | 80 ++++++++++++++++++++++++++++++++++++++++++++
 net/ncsi/ncsi-manage.c       |  6 ++++
 7 files changed, 151 insertions(+), 1 deletion(-)
 create mode 100644 net/ncsi/ncsi-ethtool.c

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 83cc986..720bb4d 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -374,5 +374,7 @@ struct ethtool_ops {
 				      struct ethtool_link_ksettings *);
 	int	(*set_link_ksettings)(struct net_device *,
 				      const struct ethtool_link_ksettings *);
+	int	(*get_ncsi_channels)(struct net_device *,
+				     struct ethtool_ncsi_channels *);
 };
 #endif /* _LINUX_ETHTOOL_H */
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 5f4ea28..e43aacf 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1331,6 +1331,8 @@ struct ethtool_per_queue_op {
 #define ETHTOOL_PHY_GTUNABLE	0x0000004e /* Get PHY tunable configuration */
 #define ETHTOOL_PHY_STUNABLE	0x0000004f /* Set PHY tunable configuration */
 
+#define ETHTOOL_GNCSICHANNELS	0x00000050 /* Get NCSI channels */
+
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
 #define SPARC_ETH_SSET		ETHTOOL_SSET
@@ -1763,4 +1765,19 @@ struct ethtool_link_settings {
 	 * __u32 map_lp_advertising[link_mode_masks_nwords];
 	 */
 };
+
+/**
+ * struct ethtool_ncsi_channels - NCSI channels
+ *
+ * @cmd: Command number = %ETHTOOL_GNCSICHANNELS
+ * @nr_channels: Number of available channels
+ * @id: Array of NCSI channel IDs
+ */
+struct ethtool_ncsi_channels {
+	__u32	cmd;
+	__s16	nr_channels;
+	__u32	id[0];
+#define ETHTOOL_NCSI_CHANNEL_ACTIVE	(1 << 8)
+#define ETHTOOL_NCSI_CHANNEL_FLAGS	0x100
+};
 #endif /* _UAPI_LINUX_ETHTOOL_H */
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 03111a2..7644765 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -756,6 +756,43 @@ static int ethtool_set_link_ksettings(struct net_device *dev,
 	return dev->ethtool_ops->set_link_ksettings(dev, &link_ksettings);
 }
 
+static int ethtool_get_ncsi_channels(struct net_device *dev,
+				     void __user *useraddr)
+{
+	struct ethtool_ncsi_channels *enc;
+	short nr_channels;
+	ssize_t size = 0;
+	int ret;
+
+	if (!dev->ethtool_ops->get_ncsi_channels)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&nr_channels, useraddr + sizeof(enc->cmd),
+			   sizeof(nr_channels)))
+		return -EFAULT;
+
+	size = sizeof(*enc);
+	if (nr_channels > 0)
+		size += nr_channels * sizeof(enc->id[0]);
+
+	enc = kzalloc(size, GFP_KERNEL);
+	if (!enc)
+		return -ENOMEM;
+
+	if (copy_from_user(enc, useraddr, size)) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	ret = dev->ethtool_ops->get_ncsi_channels(dev, enc);
+	if (copy_to_user(useraddr, enc, size))
+		ret = -EFAULT;
+
+out:
+	kfree(enc);
+	return ret;
+}
+
 static void
 warn_incomplete_ethtool_legacy_settings_conversion(const char *details)
 {
@@ -2793,6 +2830,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_PHY_STUNABLE:
 		rc = set_phy_tunable(dev, useraddr);
 		break;
+	case ETHTOOL_GNCSICHANNELS:
+		rc = ethtool_get_ncsi_channels(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}
diff --git a/net/ncsi/Makefile b/net/ncsi/Makefile
index dd12b56..71a258a 100644
--- a/net/ncsi/Makefile
+++ b/net/ncsi/Makefile
@@ -1,4 +1,5 @@
 #
 # Makefile for NCSI API
 #
-obj-$(CONFIG_NET_NCSI) += ncsi-cmd.o ncsi-rsp.o ncsi-aen.o ncsi-manage.o
+obj-$(CONFIG_NET_NCSI) += ncsi-cmd.o ncsi-rsp.o ncsi-aen.o ncsi-manage.o \
+			  ncsi-ethtool.o
diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 1308a56..09a7ba7 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -337,4 +337,8 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
 		 struct packet_type *pt, struct net_device *orig_dev);
 int ncsi_aen_handler(struct ncsi_dev_priv *ndp, struct sk_buff *skb);
 
+/* ethtool */
+void ncsi_ethtool_register_dev(struct net_device *dev);
+void ncsi_ethtool_unregister_dev(struct net_device *dev);
+
 #endif /* __NCSI_INTERNAL_H__ */
diff --git a/net/ncsi/ncsi-ethtool.c b/net/ncsi/ncsi-ethtool.c
new file mode 100644
index 0000000..747aab6
--- /dev/null
+++ b/net/ncsi/ncsi-ethtool.c
@@ -0,0 +1,80 @@
+/*
+ * Copyright Gavin Shan, IBM Corporation 2017.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/netdevice.h>
+#include <linux/ethtool.h>
+
+#include <net/ncsi.h>
+
+#include "internal.h"
+#include "ncsi-pkt.h"
+
+static int ncsi_get_channels(struct net_device *dev,
+			     struct ethtool_ncsi_channels *enc)
+{
+	struct ncsi_dev *nd;
+	struct ncsi_dev_priv *ndp;
+	struct ncsi_package *np;
+	struct ncsi_channel *nc;
+	bool fill_data = !!(enc->nr_channels > 0);
+	short nr_channels = 0;
+	unsigned long flags;
+
+	nd = ncsi_find_dev(dev);
+	if (!nd)
+		return -ENXIO;
+
+	ndp = TO_NCSI_DEV_PRIV(nd);
+	NCSI_FOR_EACH_PACKAGE(ndp, np) {
+		NCSI_FOR_EACH_CHANNEL(np, nc) {
+			if (!fill_data) {
+				nr_channels++;
+				continue;
+			}
+
+			enc->id[nr_channels] = NCSI_TO_CHANNEL(np->id, nc->id);
+			spin_lock_irqsave(&nc->lock, flags);
+			if (nc->state == NCSI_CHANNEL_ACTIVE)
+				enc->id[nr_channels] |=
+					ETHTOOL_NCSI_CHANNEL_ACTIVE;
+			spin_unlock_irqrestore(&nc->lock, flags);
+			nr_channels++;
+		}
+	}
+
+	if (!fill_data)
+		enc->nr_channels = nr_channels;
+
+	return 0;
+}
+
+void ncsi_ethtool_register_dev(struct net_device *dev)
+{
+	struct ethtool_ops *ops;
+
+	ops = (struct ethtool_ops *)(dev->ethtool_ops);
+	if (!ops)
+		return;
+
+	ops->get_ncsi_channels = ncsi_get_channels;
+}
+
+void ncsi_ethtool_unregister_dev(struct net_device *dev)
+{
+	struct ethtool_ops *ops;
+
+	ops = (struct ethtool_ops *)(dev->ethtool_ops);
+	if (!ops)
+		return;
+
+	ops->get_ncsi_channels = NULL;
+}
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 13ad1f26..f1c10f0 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -1260,6 +1260,9 @@ struct ncsi_dev *ncsi_register_dev(struct net_device *dev,
 	list_add_tail_rcu(&ndp->node, &ncsi_dev_list);
 	spin_unlock_irqrestore(&ncsi_dev_lock, flags);
 
+	/* Change ethtool operations */
+	ncsi_ethtool_register_dev(dev);
+
 	/* Register NCSI packet Rx handler */
 	ndp->ptype.type = cpu_to_be16(ETH_P_NCSI);
 	ndp->ptype.func = ncsi_rcv_rsp;
@@ -1331,6 +1334,9 @@ void ncsi_unregister_dev(struct ncsi_dev *nd)
 
 	dev_remove_pack(&ndp->ptype);
 
+	/* Restore ethtool operations */
+	ncsi_ethtool_unregister_dev(nd->dev);
+
 	list_for_each_entry_safe(np, tmp, &ndp->packages, node)
 		ncsi_remove_package(np);
 
-- 
2.7.4

Re: [PATCH v4 net-next 04/10] net/ncsi: Ethtool operation to get NCSI topology

[Andrew Lunn]

> +void ncsi_ethtool_register_dev(struct net_device *dev)
> +{
> +	struct ethtool_ops *ops;
> +
> +	ops = (struct ethtool_ops *)(dev->ethtool_ops);

Why do you need the cast here?

Ah, is it because net_device has:

          const struct ethtool_ops *ethtool_ops;

i.e. you are casting off the const.

> +	if (!ops)
> +		return;
> +
> +	ops->get_ncsi_channels = ncsi_get_channels;

and here you modify it. Which is going to blow up, because it will be
in a read only segment and should throw an opps when you write to it?

You need to export ncsi_get_channels, and let the underlying driver
add it to its own ethtool_ops.

   Andrew

Re: [PATCH v4 net-next 04/10] net/ncsi: Ethtool operation to get NCSI topology

[Gavin Shan]

On Thu, May 04, 2017 at 02:49:33AM +0200, Andrew Lunn wrote:
>> +void ncsi_ethtool_register_dev(struct net_device *dev)
>> +{
>> +	struct ethtool_ops *ops;
>> +
>> +	ops = (struct ethtool_ops *)(dev->ethtool_ops);
>
>Why do you need the cast here?
>
>Ah, is it because net_device has:
>
>          const struct ethtool_ops *ethtool_ops;
>
>i.e. you are casting off the const.
>
>> +	if (!ops)
>> +		return;
>> +
>> +	ops->get_ncsi_channels = ncsi_get_channels;
>
>and here you modify it. Which is going to blow up, because it will be
>in a read only segment and should throw an opps when you write to it?
>
>You need to export ncsi_get_channels, and let the underlying driver
>add it to its own ethtool_ops.
>

I didn't see oops because of this on the ARM board where I did the
testing. The intention was to make ethtool invisible to drivers, for
simplicity. However, we need to expose ethtool to driver when the HW
and SW statistics are conveyed by ETHTOOL_GSTATS as agree'd in another
thread.

Cheers,
Gavin

Re: [PATCH v4 net-next 04/10] net/ncsi: Ethtool operation to get NCSI topology

[Stephen Hemminger]

On Wed,  3 May 2017 14:44:35 +1000
Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:

> +static int ethtool_get_ncsi_channels(struct net_device *dev,
> +				     void __user *useraddr)

Please don't use an opaque type for this. See how other ethtool
operations take a struct.

> +{
> +	struct ethtool_ncsi_channels *enc;
> +	short nr_channels;
Should be __u16 or unsigned not short.

> +	ssize_t size = 0;
> +	int ret;
> +
> +	if (!dev->ethtool_ops->get_ncsi_channels)
> +		return -EOPNOTSUPP;
> +
> +	if (copy_from_user(&nr_channels, useraddr + sizeof(enc->cmd),
> +			   sizeof(nr_channels)))
> +		return -EFAULT;
> +
> +	size = sizeof(*enc);
> +	if (nr_channels > 0)
> +		size += nr_channels * sizeof(enc->id[0]);

You have no upper bound on number of channels, and therefore an incorrectly
application could grab an excessive amount of kernel memory.

Re: [PATCH v4 net-next 04/10] net/ncsi: Ethtool operation to get NCSI topology

[Gavin Shan]

On Wed, May 03, 2017 at 10:19:44PM -0700, Stephen Hemminger wrote:
>On Wed,  3 May 2017 14:44:35 +1000
>Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
>
>> +static int ethtool_get_ncsi_channels(struct net_device *dev,
>> +				     void __user *useraddr)
>
>Please don't use an opaque type for this. See how other ethtool
>operations take a struct.
>

After checking output from below command, all other ethtool operations
uses "void __user *" or "char __user *".

   git grep static.*useraddr net/core/ethtool.c

>> +{
>> +	struct ethtool_ncsi_channels *enc;
>> +	short nr_channels;
>Should be __u16 or unsigned not short.
>

Nope, It's for signed number. User expects to get number of available
channels when negative number is passed in. When it's positive, it's
going to get the channels' information.

>> +	ssize_t size = 0;
>> +	int ret;
>> +
>> +	if (!dev->ethtool_ops->get_ncsi_channels)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (copy_from_user(&nr_channels, useraddr + sizeof(enc->cmd),
>> +			   sizeof(nr_channels)))
>> +		return -EFAULT;
>> +
>> +	size = sizeof(*enc);
>> +	if (nr_channels > 0)
>> +		size += nr_channels * sizeof(enc->id[0]);
>
>You have no upper bound on number of channels, and therefore an incorrectly
>application could grab an excessive amount of kernel memory.
>

Yeah, I'll limit it to 256 in next respin. 256 is the maximal number
of channels for one particular net device.

Cheers,
Gavin

RE: [PATCH v4 net-next 04/10] net/ncsi: Ethtool operation to get NCSI topology

[David Laight]

From: Gavin Shan
> Sent: 04 May 2017 07:16
> On Wed, May 03, 2017 at 10:19:44PM -0700, Stephen Hemminger wrote:
> >On Wed,  3 May 2017 14:44:35 +1000
> >Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
...
> >> +{
> >> +	struct ethtool_ncsi_channels *enc;
> >> +	short nr_channels;
> >Should be __u16 or unsigned not short.
> >
> 
> Nope, It's for signed number. User expects to get number of available
> channels when negative number is passed in. When it's positive, it's
> going to get the channels' information.

Why 16 bits?
You are just making life hard for the compiler and possibly generating
random padding.

I guess the user is expected to pass -1 first to get the number of
channels, then allocate an appropriate sized array and call again
specifying the number of channels?

What happens if the number of channels changes between the two requests?

I'd also suggest passing the size of each entry (in at least one direction).
That way additional channel information can be added.

	David

Re: [PATCH v4 net-next 04/10] net/ncsi: Ethtool operation to get NCSI topology

[Gavin Shan]

On Thu, May 04, 2017 at 09:31:20AM +0000, David Laight wrote:
>From: Gavin Shan
>> Sent: 04 May 2017 07:16
>> On Wed, May 03, 2017 at 10:19:44PM -0700, Stephen Hemminger wrote:
>> >On Wed,  3 May 2017 14:44:35 +1000
>> >Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
>...
>> >> +{
>> >> +	struct ethtool_ncsi_channels *enc;
>> >> +	short nr_channels;
>> >Should be __u16 or unsigned not short.
>> >
>> 
>> Nope, It's for signed number. User expects to get number of available
>> channels when negative number is passed in. When it's positive, it's
>> going to get the channels' information.
>
>Why 16 bits?
>You are just making life hard for the compiler and possibly generating
>random padding.
>

It's because there are 256 NCSI channels to maximal degree. So 16-bits
is the minial data width to hold it in signed format. Yes, I think
__s32 would be better in this case. However, I would like to discard
the negotiation mechanism in next respin.

>I guess the user is expected to pass -1 first to get the number of
>channels, then allocate an appropriate sized array and call again
>specifying the number of channels?
>

It's correct.

>What happens if the number of channels changes between the two requests?
>

There are only one case the number changes from zero to x. In previous call,
zero is returned and userspace will get nothing. When x channels are probed,
it's stable and won't change. I don't see any problem because of it.

In next respin, I'll pass 256 entries directly. Each entry will have a flag
to indicate it's valid or not. No negotiation will be needed.

>I'd also suggest passing the size of each entry (in at least one direction).
>That way additional channel information can be added.
>

why? we have another command (ETHTOOL_GNCSICINFO) to retrieve information
about the specified channel.

Cheers,
Gavin

RE: [PATCH v4 net-next 04/10] net/ncsi: Ethtool operation to get NCSI topology

[David Laight]

From: Gavin Shan
> Sent: 08 May 2017 01:20
...
> >Why 16 bits?
> >You are just making life hard for the compiler and possibly generating
> >random padding.
> >
> 
> It's because there are 256 NCSI channels to maximal degree. So 16-bits
> is the minial data width to hold it in signed format. Yes, I think
> __s32 would be better in this case. However, I would like to discard
> the negotiation mechanism in next respin.

Just because the domain of a value fits in 16 bits doesn't mean
that a 16bit type is appropriate.

It is generally much better to use 32 (aka machine word) sized
items unless you have an array or are trying to fit a lot of
items into a small memory area.

	David

Re: [PATCH v4 net-next 04/10] net/ncsi: Ethtool operation to get NCSI topology

[Stephen Hemminger]

On Wed,  3 May 2017 14:44:35 +1000
Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:

> +void ncsi_ethtool_register_dev(struct net_device *dev)
> +{
> +	struct ethtool_ops *ops;
> +
> +	ops = (struct ethtool_ops *)(dev->ethtool_ops);
> +	if (!ops)
> +		return;
> +
> +	ops->get_ncsi_channels = ncsi_get_channels;
> +}
> +

Instead of casting away const which opens up potential security
issues. Have two ethtool_ops structures.

Re: [PATCH v4 net-next 04/10] net/ncsi: Ethtool operation to get NCSI topology

[Gavin Shan]

On Wed, May 03, 2017 at 10:21:11PM -0700, Stephen Hemminger wrote:
>On Wed,  3 May 2017 14:44:35 +1000
>Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
>
>> +void ncsi_ethtool_register_dev(struct net_device *dev)
>> +{
>> +	struct ethtool_ops *ops;
>> +
>> +	ops = (struct ethtool_ops *)(dev->ethtool_ops);
>> +	if (!ops)
>> +		return;
>> +
>> +	ops->get_ncsi_channels = ncsi_get_channels;
>> +}
>> +
>
>Instead of casting away const which opens up potential security
>issues. Have two ethtool_ops structures.
>

Thanks for the comments, Stephen. Yeah, It should be corrected
as Andrew already suggested.

Cheers,
Gavin

[PATCH v4 net-next 05/10] net/ncsi: Ethtool operation to get NCSI channel info

[Gavin Shan]

This adds ethtool command (ETHTOOL_GNCSICINFO) to retrieve the
NCSI channel information for the specified one. The simplified
output of this command is shown as follows from the modified
(private) ethtool:

 # ethtool --ncsi eth0 info
 NCSI channel 0:0 version:
   version:        00000000
   alpha2:         00000000
   f/w name:
   f/w version:    00000000
   PCI IDs:        0000 0000 0000 0000
   Manufacture ID: 00000000

   Generic capability:   0000000f (0000000f)
   Multicast capability: 00000007 (00000000)
   Buffer capability:    00004000
   AEN capability:       00000007 (00000000)
   VLAN capability:      00000007 (00000000)
   Filters:              VLAN (2), Mixed (0), MC (2), UC (4)

   Link status:             000ff66b 00000000 00000000
   MAC filter entries:
   00:00:00:00:00:00
   48:c4:b6:f0:a6:d4
   b6:fa:8f:70:00:00
   00:00:00:01:07:34
   VLAN filter entries:
   0004
   bc84

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 include/linux/ethtool.h      |   2 +
 include/uapi/linux/ethtool.h | 151 +++++++++++++++++++++++++++++++++++++++++++
 net/core/ethtool.c           |  22 +++++++
 net/ncsi/ncsi-ethtool.c      | 120 ++++++++++++++++++++++++++++++++++
 4 files changed, 295 insertions(+)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 720bb4d..5704b8b 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -376,5 +376,7 @@ struct ethtool_ops {
 				      const struct ethtool_link_ksettings *);
 	int	(*get_ncsi_channels)(struct net_device *,
 				     struct ethtool_ncsi_channels *);
+	int	(*get_ncsi_channel_info)(struct net_device *,
+					 struct ethtool_ncsi_channel_info *);
 };
 #endif /* _LINUX_ETHTOOL_H */
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index e43aacf..81fbd51 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1332,6 +1332,7 @@ struct ethtool_per_queue_op {
 #define ETHTOOL_PHY_STUNABLE	0x0000004f /* Set PHY tunable configuration */
 
 #define ETHTOOL_GNCSICHANNELS	0x00000050 /* Get NCSI channels */
+#define ETHTOOL_GNCSICINFO	0x00000051 /* Get NCSI channel information */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
@@ -1780,4 +1781,154 @@ struct ethtool_ncsi_channels {
 #define ETHTOOL_NCSI_CHANNEL_ACTIVE	(1 << 8)
 #define ETHTOOL_NCSI_CHANNEL_FLAGS	0x100
 };
+
+/**
+ * struct ethtool_ncsi_channel_info - NCSI channel information
+ *
+ * @cmd: Command number = %ETHTOOL_GNCSICINFO
+ * @id:  NCSI channel identifier
+ * @version: BCD encoded NCSI version
+ * @alpha2: BCD encoded NCSI version
+ * @fw_name: Firmware name string
+ * @fw_version: Firmware version
+ * @pci_ids: PCI identifier
+ * @mf_id: Manufacture identifier
+ * @cap_generic: Generic capability list
+ * @cap_bc: Broadcast capability list
+ * @setting_bc: Broadcast filtering setting
+ * @cap_mc: Multicast capability list
+ * @setting_mc: Multicast filtering setting
+ * @cap_buf: Length of receive buffer
+ * @cap_aen: AEN capability list
+ * @setting_aen: AEN setting
+ * @cap_vlan: VLAN filtering capability list
+ * @setting_vlan: VLAN filltering setting
+ * @cap_vlan_filter: Number of VLAN filtering entries
+ * @cap_mixed_filter: Number of mixed filtering entries
+ * @cap_mc_filter: Number of multicast filtering entries
+ * @cap_uc_filter: Number of unicast filtering entries
+ * @link_status: Link status
+ * @link_other_ind: Link other indication
+ * @link_oem: Link OEM information
+ * @mac_valid_bits: Bitmap for valid MAC filtering entries
+ * @mac: MAC filtering entries
+ * @vlan_valid_bits: Bitmap for valid VLAN filtering entries
+ * @vlan: VLAN filtering entries
+ */
+struct ethtool_ncsi_channel_info {
+	__u32	cmd;
+	__u32	id;
+	__u32	version;
+	__u32	alpha2;
+	__u8	fw_name[12];
+	__u32	fw_version;
+	__u16	pci_ids[4];
+	__u32	mf_id;
+	__u32	cap_generic;
+#define ETHTOOL_NCSI_G_HWA             (1 << 0) /* HW arbitration           */
+#define ETHTOOL_NCSI_G_HDS             (1 << 1) /* HNC driver status change */
+#define ETHTOOL_NCSI_G_FC              (1 << 2) /* HNC to MC flow control   */
+#define ETHTOOL_NCSI_G_FC1             (1 << 3) /* MC to HNC flow control   */
+#define ETHTOOL_NCSI_G_MC              (1 << 4) /* Global MC filtering      */
+#define ETHTOOL_NCSI_G_HWA_MASK        0x60
+#define ETHTOOL_NCSI_G_HWA_UNKNOWN     0x00     /* Unknown HW arbitration   */
+#define ETHTOOL_NCSI_G_HWA_SUPPORT     0x20     /* Supported HW arbitration */
+#define ETHTOOL_NCSI_G_HWA_NOT_SUPPORT 0x40     /* No HW arbitration        */
+#define ETHTOOL_NCSI_G_HWA_RESERVED    0x60     /* Reserved HW arbitration  */
+#define ETHTOOL_NCSI_G_MASK            0x7f
+	__u32	cap_bc;
+	__u32	setting_bc;
+#define ETHTOOL_NCSI_BC_ARP            (1 << 0) /* ARP packet filtering     */
+#define ETHTOOL_NCSI_BC_DHCPC          (1 << 1) /* DHCP client filtering    */
+#define ETHTOOL_NCSI_BC_DHCPS          (1 << 2) /* DHCP server filtering    */
+#define ETHTOOL_NCSI_BC_NETBIOS        (1 << 3) /* NetBIOS packet filtering */
+#define ETHTOOL_NCSI_BC_MASK           0xf
+	__u32	cap_mc;
+	__u32	setting_mc;
+#define ETHTOOL_NCSI_MC_IPV6_NEIGHBOR     (1 << 0) /* IPv6 neighbor filter  */
+#define ETHTOOL_NCSI_MC_IPV6_ROUTER       (1 << 1) /* IPv6 router filter    */
+#define ETHTOOL_NCSI_MC_DHCPV6_RELAY      (1 << 2) /* DHCPv6 relay/server   */
+#define ETHTOOL_NCSI_MC_DHCPV6_WELL_KNOWN (1 << 3) /* DHCPv6 well-known MC  */
+#define ETHTOOL_NCSI_MC_IPV6_MLD          (1 << 4) /* IPv6 MLD filtering    */
+#define ETHTOOL_NCSI_MC_IPV6_NEIGHBOR_S   (1 << 5) /* IPv6 neighbour filter */
+#define ETHTOOL_NCSI_MC_MASK              0x3f
+	__u32	cap_buf;
+	__u32	cap_aen;
+	__u32	setting_aen;
+#define ETHTOOL_NCSI_AEN_LSC           (1 << 0) /* Link status change       */
+#define ETHTOOL_NCSI_AEN_CR            (1 << 1) /* Configuration required   */
+#define ETHTOOL_NCSI_AEN_HDS           (1 << 2) /* HNC driver status        */
+#define ETHTOOL_NCSI_AEN_MASK          0x07
+	__u32	cap_vlan;
+	__u32	setting_vlan;
+#define ETHTOOL_NCSI_VLAN_ONLY         (1 << 0) /* Filter VLAN packet only  */
+#define ETHTOOL_NCSI_VLAN_NO           (1 << 1) /* Filter VLAN and non-VLAN */
+#define ETHTOOL_NCSI_VLAN_ANY          (1 << 2) /* Filter Any-and-non-VLAN  */
+#define ETHTOOL_NCSI_VLAN_MASK         0x07
+	__u8	cap_vlan_filter;
+	__u8	cap_mixed_filter;
+	__u8	cap_mc_filter;
+	__u8	cap_uc_filter;
+	__u32	link_status;
+#define ETHTOOL_NCSI_LINK_UP               (1 << 0)  /* Link up or down       */
+#define ETHTOOL_NCSI_LINK_SPEED_MASK       0x1e      /* Link speed            */
+#define ETHTOOL_NCSI_LINK_SPEED_INVALID         0x0
+#define ETHTOOL_NCSI_LINK_SPEED_10BASE_T_H      0x2
+#define ETHTOOL_NCSI_LINK_SPEED_10BASE_T_F      0x4
+#define ETHTOOL_NCSI_LINK_SPEED_100BASE_TX_H    0x6
+#define ETHTOOL_NCSI_LINK_SPEED_100BASE_T4      0x8
+#define ETHTOOL_NCSI_LINK_SPEED_100BASE_TX_F    0xa
+#define ETHTOOL_NCSI_LINK_SPEED_1000BASE_T_H    0xc
+#define ETHTOOL_NCSI_LINK_SPEED_1000BASE_T_F    0xe
+#define ETHTOOL_NCSI_LINK_SPEED_10GBASE_T       0x10
+#define ETHTOOL_NCSI_LINK_SPEED_20G             0x12
+#define ETHTOOL_NCSI_LINK_SPEED_25G             0x14
+#define ETHTOOL_NCSI_LINK_SPEED_40G             0x16
+#define ETHTOOL_NCSI_LINK_SPEED_50G             0x18
+#define ETHTOOL_NCSI_LINK_SPEED_100G            0x1a
+#define ETHTOOL_NCSI_LINK_SPEED_2_5G            0x1c
+#define ETHTOOL_NCSI_LINK_SPEED_OPTIONAL        0x1e
+#define ETHTOOL_NCSI_LINK_AUTONEG_ENABLED  (1 << 5)  /* Enabled auto-neg      */
+#define ETHTOOL_NCSI_LINK_AUTONEG_DONE     (1 << 6)  /* Auto-neg is done      */
+#define ETHTOOL_NCSI_LINK_PARALLEL         (1 << 7)  /* Parallel detection    */
+#define ETHTOOL_NCSI_LINK_LPA_1000BASE_T_F (1 << 9)  /* LPA: 1000BASE_T_Full  */
+#define ETHTOOL_NCSI_LINK_LPA_1000BASE_T_H (1 << 10) /* LPA: 1000BASE_T_Half  */
+#define ETHTOOL_NCSI_LINK_LPA_100_T4       (1 << 11) /* LPA: 100T4            */
+#define ETHTOOL_NCSI_LINK_LPA_100BASE_TX_F (1 << 12) /* LPA: 100BASE_TX_Full  */
+#define ETHTOOL_NCSI_LINK_LPA_100BASE_TX_H (1 << 13) /* LPA: 100BASE_TX_Half  */
+#define ETHTOOL_NCSI_LINK_LPA_10BASE_T_F   (1 << 14) /* LPA: 10BASE_T_Full    */
+#define ETHTOOL_NCSI_LINK_LPA_10BASE_T_H   (1 << 15) /* LPA: 10BASE_T_Half    */
+#define ETHTOOL_NCSI_LINK_TX_FC            (1 << 16) /* Tx flow control       */
+#define ETHTOOL_NCSI_LINK_RX_FC            (1 << 17) /* Rx flow control       */
+#define ETHTOOL_NCSI_LINK_LPA_FC_MASK      0xc0000
+#define ETHTOOL_NCSI_LINK_LPA_FC_NONE      0x0
+#define ETHTOOL_NCSI_LINK_LPA_FC_SYNC      0x40000
+#define ETHTOOL_NCSI_LINK_LPA_FC_ASYNC     0x80000
+#define ETHTOOL_NCSI_LINK_SERDES           (1 << 20) /* SerDes used or not    */
+#define ETHTOOL_NCSI_LINK_OEM_VALID        (1 << 21)
+#define ETHTOOL_NCSI_LINK_ESPEED_MASK      0xff000000
+#define ETHTOOL_NCSI_LINK_ESPEED_INVALID          0x0
+#define ETHTOOL_NCSI_LINK_ESPEED_10BASE_T_H       0x01000000
+#define ETHTOOL_NCSI_LINK_ESPEED_10BASE_T_F       0x02000000
+#define ETHTOOL_NCSI_LINK_ESPEED_100BASE_TX_H     0x03000000
+#define ETHTOOL_NCSI_LINK_ESPEED_100BASE_T4       0x04000000
+#define ETHTOOL_NCSI_LINK_ESPEED_100BASE_TX_F     0x05000000
+#define ETHTOOL_NCSI_LINK_ESPEED_1000BASE_T_H     0x06000000
+#define ETHTOOL_NCSI_LINK_ESPEED_1000BASE_T_F     0x07000000
+#define ETHTOOL_NCSI_LINK_ESPEED_10GBASE_T        0x08000000
+#define ETHTOOL_NCSI_LINK_ESPEED_20G              0x09000000
+#define ETHTOOL_NCSI_LINK_ESPEED_25G              0x0a000000
+#define ETHTOOL_NCSI_LINK_ESPEED_40G              0x0b000000
+#define ETHTOOL_NCSI_LINK_ESPEED_50G              0x0c000000
+#define ETHTOOL_NCSI_LINK_ESPEED_100G             0x0d000000
+#define ETHTOOL_NCSI_LINK_ESPEED_2_5G             0x0e000000
+#define ETHTOOL_NCSI_LINK_ESPEED_OPTIONAL         0x0f000000
+	__u32	link_other_ind;
+#define ETHTOOL_NCSI_LINK_HNC_DRV_STATUS   (1 << 0)
+	__u32	link_oem;
+	__u32	mac_valid_bits;
+	__u8	mac[8][6];
+	__u32	vlan_valid_bits;
+	__u16	vlan[16];
+};
 #endif /* _UAPI_LINUX_ETHTOOL_H */
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 7644765..116ef10 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -793,6 +793,25 @@ static int ethtool_get_ncsi_channels(struct net_device *dev,
 	return ret;
 }
 
+static int ethtool_get_ncsi_channel_info(struct net_device *dev,
+					 void __user *useraddr)
+{
+	struct ethtool_ncsi_channel_info enci;
+	int ret;
+
+	if (!dev->ethtool_ops->get_ncsi_channel_info)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&enci, useraddr, sizeof(enci)))
+		return -EFAULT;
+
+	ret = dev->ethtool_ops->get_ncsi_channel_info(dev, &enci);
+	if (!ret && copy_to_user(useraddr, &enci, sizeof(enci)))
+		return -EFAULT;
+
+	return ret;
+}
+
 static void
 warn_incomplete_ethtool_legacy_settings_conversion(const char *details)
 {
@@ -2833,6 +2852,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GNCSICHANNELS:
 		rc = ethtool_get_ncsi_channels(dev, useraddr);
 		break;
+	case ETHTOOL_GNCSICINFO:
+		rc = ethtool_get_ncsi_channel_info(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}
diff --git a/net/ncsi/ncsi-ethtool.c b/net/ncsi/ncsi-ethtool.c
index 747aab6..9eee5fb 100644
--- a/net/ncsi/ncsi-ethtool.c
+++ b/net/ncsi/ncsi-ethtool.c
@@ -57,6 +57,124 @@ static int ncsi_get_channels(struct net_device *dev,
 	return 0;
 }
 
+static int ncsi_get_channel_info(struct net_device *dev,
+				 struct ethtool_ncsi_channel_info *enci)
+{
+	struct ncsi_dev *nd;
+	struct ncsi_dev_priv *ndp;
+	struct ncsi_channel *nc;
+	unsigned long flags;
+	int i;
+
+	nd = ncsi_find_dev(dev);
+	if (!nd)
+		return -ENXIO;
+
+	ndp = TO_NCSI_DEV_PRIV(nd);
+	ncsi_find_package_and_channel(ndp, enci->id, NULL, &nc);
+	if (!nc)
+		return -ENXIO;
+
+	spin_lock_irqsave(&nc->lock, flags);
+
+	/* NCSI channel's version */
+	enci->version = nc->version.version;
+	enci->alpha2 = nc->version.alpha2;
+	memcpy(enci->fw_name, nc->version.fw_name, 12);
+	enci->fw_version = nc->version.fw_version;
+	memcpy(enci->pci_ids, nc->version.pci_ids,
+	       4 * sizeof(enci->pci_ids[0]));
+	enci->mf_id = nc->version.mf_id;
+
+	/* NCSI channel's capabilities */
+	enci->cap_generic = (nc->caps[NCSI_CAP_GENERIC].cap &
+			     ETHTOOL_NCSI_G_MASK);
+	enci->cap_bc = (nc->caps[NCSI_CAP_BC].cap &
+			ETHTOOL_NCSI_BC_MASK);
+	enci->cap_mc = (nc->caps[NCSI_CAP_MC].cap &
+			ETHTOOL_NCSI_MC_MASK);
+	enci->cap_buf = nc->caps[NCSI_CAP_BUFFER].cap;
+	enci->cap_aen = (nc->caps[NCSI_CAP_AEN].cap &
+			 ETHTOOL_NCSI_AEN_MASK);
+	enci->cap_vlan = (nc->caps[NCSI_CAP_VLAN].cap &
+			  ETHTOOL_NCSI_VLAN_MASK);
+	for (i = NCSI_FILTER_BASE; i < NCSI_FILTER_MAX; i++) {
+		struct ncsi_channel_filter *ncf;
+		unsigned char *p_cap_filter;
+		unsigned int *p_valid_bits;
+		int entry_size, s_idx, d_idx;
+		void *dest;
+
+		switch (i) {
+		case NCSI_FILTER_VLAN:
+			p_cap_filter = &enci->cap_vlan_filter;
+			entry_size = 2;
+			p_valid_bits = &enci->vlan_valid_bits;
+			dest = enci->vlan;
+			d_idx = 0;
+			break;
+		case NCSI_FILTER_UC:
+			p_cap_filter = &enci->cap_uc_filter;
+			entry_size = 6;
+			p_valid_bits = &enci->mac_valid_bits;
+			dest = enci->mac;
+			d_idx = 0;
+			break;
+		case NCSI_FILTER_MC:
+			p_cap_filter = &enci->cap_mc_filter;
+			entry_size = 6;
+			break;
+		case NCSI_FILTER_MIXED:
+			p_cap_filter = &enci->cap_mixed_filter;
+			entry_size = 6;
+			break;
+		default:
+			continue;
+		}
+
+		*p_cap_filter = 0;
+		ncf = nc->filters[i];
+		if (!ncf)
+			continue;
+
+		*p_cap_filter = ncf->total;
+		s_idx = -1;
+		while ((s_idx = find_next_bit((void *)&ncf->bitmap,
+					      ncf->total, s_idx + 1))
+			< ncf->total) {
+			memcpy(dest + (d_idx * entry_size),
+			       ((void *)(ncf->data)) + (s_idx * entry_size),
+			       entry_size);
+			*p_valid_bits |= (1 << d_idx);
+
+			d_idx++;
+		}
+	}
+
+	/* NCSI channel's settings */
+	enci->setting_bc = nc->modes[NCSI_MODE_BC].enable ?
+			   nc->modes[NCSI_MODE_BC].data[0] : 0;
+	enci->setting_bc &= ETHTOOL_NCSI_BC_MASK;
+	enci->setting_mc = nc->modes[NCSI_MODE_MC].enable ?
+			   nc->modes[NCSI_MODE_MC].data[0] : 0;
+	enci->setting_mc &= ETHTOOL_NCSI_MC_MASK;
+	enci->setting_aen = nc->modes[NCSI_MODE_AEN].enable ?
+			    nc->modes[NCSI_MODE_AEN].data[0] : 0;
+	enci->setting_aen &= ETHTOOL_NCSI_AEN_MASK;
+	enci->setting_vlan = nc->modes[NCSI_MODE_VLAN].enable ?
+			     nc->modes[NCSI_MODE_VLAN].data[0] : 0;
+	enci->setting_vlan &= ETHTOOL_NCSI_VLAN_MASK;
+
+	/* NCSI channel's link status */
+	enci->link_status = nc->modes[NCSI_MODE_LINK].data[2];
+	enci->link_other_ind = nc->modes[NCSI_MODE_LINK].data[3];
+	enci->link_oem = nc->modes[NCSI_MODE_LINK].data[4];
+
+	spin_unlock_irqrestore(&nc->lock, flags);
+
+	return 0;
+}
+
 void ncsi_ethtool_register_dev(struct net_device *dev)
 {
 	struct ethtool_ops *ops;
@@ -66,6 +184,7 @@ void ncsi_ethtool_register_dev(struct net_device *dev)
 		return;
 
 	ops->get_ncsi_channels = ncsi_get_channels;
+	ops->get_ncsi_channel_info = ncsi_get_channel_info;
 }
 
 void ncsi_ethtool_unregister_dev(struct net_device *dev)
@@ -77,4 +196,5 @@ void ncsi_ethtool_unregister_dev(struct net_device *dev)
 		return;
 
 	ops->get_ncsi_channels = NULL;
+	ops->get_ncsi_channel_info = NULL;
 }
-- 
2.7.4

Re: [PATCH v4 net-next 05/10] net/ncsi: Ethtool operation to get NCSI channel info

[kbuild test robot]

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

Hi Gavin,

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Gavin-Shan/net-ncsi-Add-debugging-functionality/20170503-185932
config: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=alpha 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   In file included from include/linux/string.h:18:0,
                    from include/linux/bitmap.h:8,
                    from include/linux/nodemask.h:94,
                    from include/linux/mmzone.h:16,
                    from include/linux/gfp.h:5,
                    from include/linux/kmod.h:22,
                    from include/linux/module.h:13,
                    from net/ncsi/ncsi-ethtool.c:10:
   net/ncsi/ncsi-ethtool.c: In function 'ncsi_get_channel_info':
>> arch/alpha/include/asm/string.h:21:16: warning: 'dest' may be used uninitialized in this function [-Wmaybe-uninitialized]
    #define memcpy __builtin_memcpy
                   ^~~~~~~~~~~~~~~~
   net/ncsi/ncsi-ethtool.c:106:9: note: 'dest' was declared here
      void *dest;
            ^~~~
   net/ncsi/ncsi-ethtool.c:105:26: warning: 'd_idx' may be used uninitialized in this function [-Wmaybe-uninitialized]
      int entry_size, s_idx, d_idx;
                             ^~~~~
   net/ncsi/ncsi-ethtool.c:148:18: warning: 'p_valid_bits' may be used uninitialized in this function [-Wmaybe-uninitialized]
       *p_valid_bits |= (1 << d_idx);
       ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
--
   In file included from include/linux/string.h:18:0,
                    from include/linux/bitmap.h:8,
                    from include/linux/nodemask.h:94,
                    from include/linux/mmzone.h:16,
                    from include/linux/gfp.h:5,
                    from include/linux/kmod.h:22,
                    from include/linux/module.h:13,
                    from net//ncsi/ncsi-ethtool.c:10:
   net//ncsi/ncsi-ethtool.c: In function 'ncsi_get_channel_info':
>> arch/alpha/include/asm/string.h:21:16: warning: 'dest' may be used uninitialized in this function [-Wmaybe-uninitialized]
    #define memcpy __builtin_memcpy
                   ^~~~~~~~~~~~~~~~
   net//ncsi/ncsi-ethtool.c:106:9: note: 'dest' was declared here
      void *dest;
            ^~~~
   net//ncsi/ncsi-ethtool.c:105:26: warning: 'd_idx' may be used uninitialized in this function [-Wmaybe-uninitialized]
      int entry_size, s_idx, d_idx;
                             ^~~~~
   net//ncsi/ncsi-ethtool.c:148:18: warning: 'p_valid_bits' may be used uninitialized in this function [-Wmaybe-uninitialized]
       *p_valid_bits |= (1 << d_idx);
       ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~

vim +/dest +21 arch/alpha/include/asm/string.h

^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16   5  
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16   6  /*
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16   7   * GCC of any recent vintage doesn't do stupid things with bcopy.
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16   8   * EGCS 1.1 knows all about expanding memcpy inline, others don't.
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16   9   *
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16  10   * Similarly for a memset with data = 0.
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16  11   */
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16  12  
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16  13  #define __HAVE_ARCH_MEMCPY
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16  14  extern void * memcpy(void *, const void *, size_t);
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16  15  #define __HAVE_ARCH_MEMMOVE
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16  16  extern void * memmove(void *, const void *, size_t);
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16  17  
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16  18  /* For backward compatibility with modules.  Unused otherwise.  */
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16  19  extern void * __memcpy(void *, const void *, size_t);
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16  20  
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16 @21  #define memcpy __builtin_memcpy
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16  22  
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16  23  #define __HAVE_ARCH_MEMSET
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16  24  extern void * __constant_c_memset(void *, unsigned long, size_t);
a47e5bb5 arch/alpha/include/asm/string.h Richard Henderson 2013-07-11  25  extern void * ___memset(void *, int, size_t);
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16  26  extern void * __memset(void *, int, size_t);
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16  27  extern void * memset(void *, int, size_t);
^1da177e include/asm-alpha/string.h      Linus Torvalds    2005-04-16  28  
a47e5bb5 arch/alpha/include/asm/string.h Richard Henderson 2013-07-11  29  /* For gcc 3.x, we cannot have the inline function named "memset" because

:::::: The code at line 21 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 49744 bytes --]

[PATCH v4 net-next 06/10] net/ncsi: Ethtool operation to get NCSI hw statistics

[Gavin Shan]

This adds ethtool command (ETHTOOL_GNCSISTATS) to retrieve the
NCSI hardware statistics. The simplified output of this command is
shown as follows from the modified (private) ethtool. It's obvious
the HW statistics isn't fetched from hardware yet, which is to be
sorted out later.

 # ethtool --ncsi eth0 stats
 NCSI statistics as below

   hnc_cnt_hi:           0
   hnc_cnt_lo:           0
   hnc_rx_bytes:         0
   hnc_tx_bytes:         0
   hnc_rx_uc_pkts:       0
   hnc_rx_mc_pkts:       0
   hnc_rx_bc_pkts:       0
   hnc_tx_uc_pkts:       0
   hnc_tx_mc_pkts:       0
   hnc_tx_bc_pkts:       0
   hnc_fcs_err:          0
   hnc_align_err:        0
   hnc_false_carrier:    0
   hnc_runt_pkts:        0
   hnc_jabber_pkts:      0
   hnc_rx_pause_xon:     0
   hnc_rx_pause_xoff:    0
   hnc_tx_pause_xon:     0
   hnc_tx_pause_xoff:    0
   hnc_tx_s_collision:   0
   hnc_tx_m_collision:   0
   hnc_l_collision:      0
   hnc_e_collision:      0
   hnc_rx_ctl_frames:    0
   hnc_rx_64_frames:     0
   hnc_rx_127_frames:    0
   hnc_rx_255_frames:    0
   hnc_rx_511_frames:    0
   hnc_rx_1023_frames:   0
   hnc_rx_1522_frames:   0
   hnc_rx_9022_frames:   0
   hnc_tx_64_frames:     0
   hnc_tx_127_frames:    0
   hnc_tx_255_frames:    0
   hnc_tx_511_frames:    0
   hnc_tx_1023_frames:   0
   hnc_tx_1522_frames:   0
   hnc_tx_9022_frames:   0
   hnc_rx_valid_bytes:   0
   hnc_rx_runt_pkts:     0
   hnc_rx_jabber_pkts:   0
   ncsi_rx_cmds:         0
   ncsi_dropped_cmds:    0
   ncsi_cmd_type_errs:   0
   ncsi_cmd_csum_errs:   0
   ncsi_rx_pkts:         0
   ncsi_tx_pkts:         0
   ncsi_tx_aen_pkts:     0
   pt_tx_pkts:           0
   pt_tx_dropped:        0
   pt_tx_channel_err:    0
   pt_tx_us_err:         0
   pt_rx_pkts:           0
   pt_rx_dropped:        0
   pt_rx_channel_err:    0
   pt_rx_us_err:         0
   pt_rx_os_err:         0

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 include/linux/ethtool.h      |   2 +
 include/uapi/linux/ethtool.h | 124 +++++++++++++++++++++++++++++++++++++++++++
 net/core/ethtool.c           |  29 ++++++++++
 net/ncsi/ncsi-ethtool.c      |  87 ++++++++++++++++++++++++++++++
 4 files changed, 242 insertions(+)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 5704b8b..6d712ca 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -378,5 +378,7 @@ struct ethtool_ops {
 				     struct ethtool_ncsi_channels *);
 	int	(*get_ncsi_channel_info)(struct net_device *,
 					 struct ethtool_ncsi_channel_info *);
+	int	(*get_ncsi_stats)(struct net_device *,
+				  struct ethtool_ncsi_stats *);
 };
 #endif /* _LINUX_ETHTOOL_H */
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 81fbd51..472773c 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1333,6 +1333,7 @@ struct ethtool_per_queue_op {
 
 #define ETHTOOL_GNCSICHANNELS	0x00000050 /* Get NCSI channels */
 #define ETHTOOL_GNCSICINFO	0x00000051 /* Get NCSI channel information */
+#define ETHTOOL_GNCSISTATS	0x00000052 /* Get NCSI HW statistics */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
@@ -1931,4 +1932,127 @@ struct ethtool_ncsi_channel_info {
 	__u32	vlan_valid_bits;
 	__u16	vlan[16];
 };
+
+/**
+ * struct ethtool_ncsi_stats - NCSI hardware statistics
+ *
+ * @cmd: Command number = %ETHTOOL_GNCSISTATS
+ * @hnc_cnt_hi: Counter cleared
+ * @hnc_cnt_lo: Counter cleared
+ * @hnc_rx_bytes: Rx bytes
+ * @hnc_tx_bytes: Tx bytes
+ * @hnc_rx_uc_pkts: Rx UC packets
+ * @hnc_rx_mc_pkts: Rx MC packets
+ * @hnc_rx_bc_pkts: Rx BC packets
+ * @hnc_tx_uc_pkts: Tx UC packets
+ * @hnc_tx_mc_pkts: Tx MC packets
+ * @hnc_tx_bc_pkts: Tx BC packets
+ * @hnc_fcs_err: FCS errors
+ * @hnc_align_err: Alignment errors
+ * @hnc_false_carrier: False carrier detection
+ * @hnc_runt_pkts: Rx runt packets
+ * @hnc_jabber_pkts: Rx jabber packets
+ * @hnc_rx_pause_xon: Rx pause XON frames
+ * @hnc_rx_pause_xoff: Rx XOFF frames
+ * @hnc_tx_pause_xon: Tx XON frames
+ * @hnc_tx_pause_xoff: Tx XOFF frames
+ * @hnc_tx_s_collision: Single collision frames
+ * @hnc_tx_m_collision: Multiple collision frames
+ * @hnc_l_collision: Late collision frames
+ * @hnc_e_collision: Excessive collision frames
+ * @hnc_rx_ctl_frames: Rx control frames
+ * @hnc_rx_64_frames: Rx 64-bytes frames
+ * @hnc_rx_127_frames: Rx 65-127 bytes frames
+ * @hnc_rx_255_frames: Rx 128-255 bytes frames
+ * @hnc_rx_511_frames: Rx 256-511 bytes frames
+ * @hnc_rx_1023_frames: Rx 512-1023 bytes frames
+ * @hnc_rx_1522_frames: Rx 1024-1522 bytes frames
+ * @hnc_rx_9022_frames: Rx 1523-9022 bytes frames
+ * @hnc_tx_64_frames: Tx 64-bytes frames
+ * @hnc_tx_127_frames: Tx 65-127 bytes frames
+ * @hnc_tx_255_frames: Tx 128-255 bytes frames
+ * @hnc_tx_511_frames: Tx 256-511 bytes frames
+ * @hnc_tx_1023_frames: Tx 512-1023 bytes frames
+ * @hnc_tx_1522_frames: Tx 1024-1522 bytes frames
+ * @hnc_tx_9022_frames: Tx 1523-9022 bytes frames
+ * @hnc_rx_valid_bytes: Rx valid bytes
+ * @hnc_rx_runt_pkts: Rx error runt packets
+ * @hnc_rx_jabber_pkts: Rx error jabber packets
+ * @ncsi_rx_cmds: Rx NCSI commands
+ * @ncsi_dropped_cmds: Dropped commands
+ * @ncsi_cmd_type_errs: Command type errors
+ * @ncsi_cmd_csum_errs: Command checksum errors
+ * @ncsi_rx_pkts: Rx NCSI packets
+ * @ncsi_tx_pkts: Tx NCSI packets
+ * @ncsi_tx_aen_pkts: Tx AEN packets
+ * @pt_tx_pkts: Tx packets
+ * @pt_tx_dropped: Tx dropped packets
+ * @pt_tx_channel_err: Tx channel errors
+ * @pt_tx_us_err: Tx undersize errors
+ * @pt_rx_pkts: Rx packets
+ * @pt_rx_dropped: Rx dropped packets
+ * @pt_rx_channel_err: Rx channel errors
+ * @pt_rx_us_err: Rx undersize errors
+ * @pt_rx_os_err: Rx oversize errors
+ */
+struct ethtool_ncsi_stats {
+	__u32	cmd;
+	__u64	hnc_cnt_hi;
+	__u64	hnc_cnt_lo;
+	__u64	hnc_rx_bytes;
+	__u64	hnc_tx_bytes;
+	__u64	hnc_rx_uc_pkts;
+	__u64	hnc_rx_mc_pkts;
+	__u64	hnc_rx_bc_pkts;
+	__u64	hnc_tx_uc_pkts;
+	__u64	hnc_tx_mc_pkts;
+	__u64	hnc_tx_bc_pkts;
+	__u64	hnc_fcs_err;
+	__u64	hnc_align_err;
+	__u64	hnc_false_carrier;
+	__u64	hnc_runt_pkts;
+	__u64	hnc_jabber_pkts;
+	__u64	hnc_rx_pause_xon;
+	__u64	hnc_rx_pause_xoff;
+	__u64	hnc_tx_pause_xon;
+	__u64	hnc_tx_pause_xoff;
+	__u64	hnc_tx_s_collision;
+	__u64	hnc_tx_m_collision;
+	__u64	hnc_l_collision;
+	__u64	hnc_e_collision;
+	__u64	hnc_rx_ctl_frames;
+	__u64	hnc_rx_64_frames;
+	__u64	hnc_rx_127_frames;
+	__u64	hnc_rx_255_frames;
+	__u64	hnc_rx_511_frames;
+	__u64	hnc_rx_1023_frames;
+	__u64	hnc_rx_1522_frames;
+	__u64	hnc_rx_9022_frames;
+	__u64	hnc_tx_64_frames;
+	__u64	hnc_tx_127_frames;
+	__u64	hnc_tx_255_frames;
+	__u64	hnc_tx_511_frames;
+	__u64	hnc_tx_1023_frames;
+	__u64	hnc_tx_1522_frames;
+	__u64	hnc_tx_9022_frames;
+	__u64	hnc_rx_valid_bytes;
+	__u64	hnc_rx_runt_pkts;
+	__u64	hnc_rx_jabber_pkts;
+	__u64	ncsi_rx_cmds;
+	__u64	ncsi_dropped_cmds;
+	__u64	ncsi_cmd_type_errs;
+	__u64	ncsi_cmd_csum_errs;
+	__u64	ncsi_rx_pkts;
+	__u64	ncsi_tx_pkts;
+	__u64	ncsi_tx_aen_pkts;
+	__u64	pt_tx_pkts;
+	__u64	pt_tx_dropped;
+	__u64	pt_tx_channel_err;
+	__u64	pt_tx_us_err;
+	__u64	pt_rx_pkts;
+	__u64	pt_rx_dropped;
+	__u64	pt_rx_channel_err;
+	__u64	pt_rx_us_err;
+	__u64	pt_rx_os_err;
+};
 #endif /* _UAPI_LINUX_ETHTOOL_H */
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 116ef10..f26aa36 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -812,6 +812,32 @@ static int ethtool_get_ncsi_channel_info(struct net_device *dev,
 	return ret;
 }
 
+static int ethtool_get_ncsi_stats(struct net_device *dev,
+				  void __user *useraddr)
+{
+	struct ethtool_ncsi_stats *ens;
+	int ret;
+
+	if (!dev->ethtool_ops->get_ncsi_stats)
+		return -EOPNOTSUPP;
+
+	ens = kzalloc(sizeof(*ens), GFP_KERNEL);
+	if (!ens)
+		return -ENOMEM;
+
+	if (copy_from_user(ens, useraddr, sizeof(*ens))) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	ret = dev->ethtool_ops->get_ncsi_stats(dev, ens);
+	if (!ret && copy_to_user(useraddr, ens, sizeof(*ens)))
+		ret = -EFAULT;
+out:
+	kfree(ens);
+	return ret;
+}
+
 static void
 warn_incomplete_ethtool_legacy_settings_conversion(const char *details)
 {
@@ -2855,6 +2881,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GNCSICINFO:
 		rc = ethtool_get_ncsi_channel_info(dev, useraddr);
 		break;
+	case ETHTOOL_GNCSISTATS:
+		rc = ethtool_get_ncsi_stats(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}
diff --git a/net/ncsi/ncsi-ethtool.c b/net/ncsi/ncsi-ethtool.c
index 9eee5fb..1ccdb50 100644
--- a/net/ncsi/ncsi-ethtool.c
+++ b/net/ncsi/ncsi-ethtool.c
@@ -175,6 +175,91 @@ static int ncsi_get_channel_info(struct net_device *dev,
 	return 0;
 }
 
+static int ncsi_get_stats(struct net_device *dev,
+			  struct ethtool_ncsi_stats *ens)
+{
+	struct ncsi_dev *nd;
+	struct ncsi_dev_priv *ndp;
+	struct ncsi_package *np;
+	struct ncsi_channel *nc;
+	struct ncsi_channel_stats *ncs;
+	unsigned long flags;
+
+	nd = ncsi_find_dev(dev);
+	if (!nd)
+		return -ENXIO;
+
+	ndp = TO_NCSI_DEV_PRIV(nd);
+	NCSI_FOR_EACH_PACKAGE(ndp, np) {
+		NCSI_FOR_EACH_CHANNEL(np, nc) {
+			spin_lock_irqsave(&nc->lock, flags);
+
+			ncs = &nc->stats;
+			ens->hnc_cnt_hi += ncs->hnc_cnt_hi;
+			ens->hnc_cnt_lo += ncs->hnc_cnt_lo;
+			ens->hnc_rx_bytes += ncs->hnc_rx_bytes;
+			ens->hnc_tx_bytes += ncs->hnc_tx_bytes;
+			ens->hnc_rx_uc_pkts += ncs->hnc_rx_uc_pkts;
+			ens->hnc_rx_mc_pkts += ncs->hnc_rx_mc_pkts;
+			ens->hnc_rx_bc_pkts += ncs->hnc_rx_bc_pkts;
+			ens->hnc_tx_uc_pkts += ncs->hnc_tx_uc_pkts;
+			ens->hnc_tx_mc_pkts += ncs->hnc_tx_mc_pkts;
+			ens->hnc_tx_bc_pkts += ncs->hnc_tx_bc_pkts;
+			ens->hnc_fcs_err += ncs->hnc_fcs_err;
+			ens->hnc_align_err += ncs->hnc_align_err;
+			ens->hnc_false_carrier += ncs->hnc_false_carrier;
+			ens->hnc_runt_pkts += ncs->hnc_runt_pkts;
+			ens->hnc_jabber_pkts += ncs->hnc_jabber_pkts;
+			ens->hnc_rx_pause_xon += ncs->hnc_rx_pause_xon;
+			ens->hnc_rx_pause_xoff += ncs->hnc_rx_pause_xoff;
+			ens->hnc_tx_pause_xon += ncs->hnc_tx_pause_xon;
+			ens->hnc_tx_pause_xoff += ncs->hnc_tx_pause_xoff;
+			ens->hnc_tx_s_collision += ncs->hnc_tx_s_collision;
+			ens->hnc_tx_m_collision += ncs->hnc_tx_m_collision;
+			ens->hnc_l_collision += ncs->hnc_l_collision;
+			ens->hnc_e_collision += ncs->hnc_e_collision;
+			ens->hnc_rx_ctl_frames += ncs->hnc_rx_ctl_frames;
+			ens->hnc_rx_64_frames += ncs->hnc_rx_64_frames;
+			ens->hnc_rx_127_frames += ncs->hnc_rx_127_frames;
+			ens->hnc_rx_255_frames += ncs->hnc_rx_255_frames;
+			ens->hnc_rx_511_frames += ncs->hnc_rx_511_frames;
+			ens->hnc_rx_1023_frames += ncs->hnc_rx_1023_frames;
+			ens->hnc_rx_1522_frames += ncs->hnc_rx_1522_frames;
+			ens->hnc_rx_9022_frames += ncs->hnc_rx_9022_frames;
+			ens->hnc_tx_64_frames += ncs->hnc_tx_64_frames;
+			ens->hnc_tx_127_frames += ncs->hnc_tx_127_frames;
+			ens->hnc_tx_255_frames += ncs->hnc_tx_255_frames;
+			ens->hnc_tx_511_frames += ncs->hnc_tx_511_frames;
+			ens->hnc_tx_1023_frames += ncs->hnc_tx_1023_frames;
+			ens->hnc_tx_1522_frames += ncs->hnc_tx_1522_frames;
+			ens->hnc_tx_9022_frames += ncs->hnc_tx_9022_frames;
+			ens->hnc_rx_valid_bytes += ncs->hnc_rx_valid_bytes;
+			ens->hnc_rx_runt_pkts += ncs->hnc_rx_runt_pkts;
+			ens->hnc_rx_jabber_pkts += ncs->hnc_rx_jabber_pkts;
+			ens->ncsi_rx_cmds += ncs->ncsi_rx_cmds;
+			ens->ncsi_dropped_cmds += ncs->ncsi_dropped_cmds;
+			ens->ncsi_cmd_type_errs += ncs->ncsi_cmd_type_errs;
+			ens->ncsi_cmd_csum_errs += ncs->ncsi_cmd_csum_errs;
+			ens->ncsi_rx_pkts += ncs->ncsi_rx_pkts;
+			ens->ncsi_tx_pkts += ncs->ncsi_tx_pkts;
+			ens->ncsi_tx_aen_pkts += ncs->ncsi_tx_aen_pkts;
+			ens->pt_tx_pkts += ncs->pt_tx_pkts;
+			ens->pt_tx_dropped += ncs->pt_tx_dropped;
+			ens->pt_tx_channel_err += ncs->pt_tx_channel_err;
+			ens->pt_tx_us_err += ncs->pt_tx_us_err;
+			ens->pt_rx_pkts += ncs->pt_rx_pkts;
+			ens->pt_rx_dropped += ncs->pt_rx_dropped;
+			ens->pt_rx_channel_err += ncs->pt_rx_channel_err;
+			ens->pt_rx_us_err += ncs->pt_rx_us_err;
+			ens->pt_rx_os_err += ncs->pt_rx_os_err;
+
+			spin_unlock_irqrestore(&nc->lock, flags);
+		}
+	}
+
+	return 0;
+}
+
 void ncsi_ethtool_register_dev(struct net_device *dev)
 {
 	struct ethtool_ops *ops;
@@ -185,6 +270,7 @@ void ncsi_ethtool_register_dev(struct net_device *dev)
 
 	ops->get_ncsi_channels = ncsi_get_channels;
 	ops->get_ncsi_channel_info = ncsi_get_channel_info;
+	ops->get_ncsi_stats = ncsi_get_stats;
 }
 
 void ncsi_ethtool_unregister_dev(struct net_device *dev)
@@ -197,4 +283,5 @@ void ncsi_ethtool_unregister_dev(struct net_device *dev)
 
 	ops->get_ncsi_channels = NULL;
 	ops->get_ncsi_channel_info = NULL;
+	ops->get_ncsi_stats = NULL;
 }
-- 
2.7.4

Re: [PATCH v4 net-next 06/10] net/ncsi: Ethtool operation to get NCSI hw statistics

[Andrew Lunn]

On Wed, May 03, 2017 at 02:44:37PM +1000, Gavin Shan wrote:
> This adds ethtool command (ETHTOOL_GNCSISTATS) to retrieve the
> NCSI hardware statistics.

Hi Gavin

I've not been following this patchset, so maybe i'm about to ask a
question which has already been asked and answered.

Why cannot use just use ethtool -S for this?

    Andrew

Re: [PATCH v4 net-next 06/10] net/ncsi: Ethtool operation to get NCSI hw statistics

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Wed, 3 May 2017 14:47:22 +0200

> On Wed, May 03, 2017 at 02:44:37PM +1000, Gavin Shan wrote:
>> This adds ethtool command (ETHTOOL_GNCSISTATS) to retrieve the
>> NCSI hardware statistics.
> 
> Hi Gavin
> 
> I've not been following this patchset, so maybe i'm about to ask a
> question which has already been asked and answered.
> 
> Why cannot use just use ethtool -S for this?

Indeed, when I said to use ethtool for these NCSI hw stats I meant
that the "ethtool -S" be used, not some new ethtool command.

Re: [PATCH v4 net-next 06/10] net/ncsi: Ethtool operation to get NCSI hw statistics

[Gavin Shan]

On Wed, May 03, 2017 at 09:18:23AM -0400, David Miller wrote:
>From: Andrew Lunn <andrew@lunn.ch>
>Date: Wed, 3 May 2017 14:47:22 +0200
>
>> On Wed, May 03, 2017 at 02:44:37PM +1000, Gavin Shan wrote:
>>> This adds ethtool command (ETHTOOL_GNCSISTATS) to retrieve the
>>> NCSI hardware statistics.
>> 
>> Hi Gavin
>> 
>> I've not been following this patchset, so maybe i'm about to ask a
>> question which has already been asked and answered.
>> 
>> Why cannot use just use ethtool -S for this?
>
>Indeed, when I said to use ethtool for these NCSI hw stats I meant
>that the "ethtool -S" be used, not some new ethtool command.
>

Thanks for the comments. Feel free to ask any questions which would
make the code clear and better. There are couple of factors I thought
separate command is better than ETHTOOL_GSTATS: The statistic items from
ETHTOOL_GSTATS are variable, meaning the kernel needs provide the layout
of the statistic items via ETHTOOL_GSSET_INFO and ETHTOOL_GSTRINGS.
NCSI HW statistics aren't following this and their layout is fixed.
Besides, statistics for ETHTOOL_GSTATS are maintained in local MAC,
but NCSI HW statistics are collected from NIC on far-end. So they're
different from this point. Lastly, the NCSI software statistics needs
separate command. It'd better to have separate command for HW statistics
as well, to make things consistent.

Cheers,
Gavin

Re: [PATCH v4 net-next 06/10] net/ncsi: Ethtool operation to get NCSI hw statistics

[David Miller]

From: Gavin Shan <gwshan@linux.vnet.ibm.com>
Date: Thu, 4 May 2017 10:05:34 +1000

> On Wed, May 03, 2017 at 09:18:23AM -0400, David Miller wrote:
>>From: Andrew Lunn <andrew@lunn.ch>
>>Date: Wed, 3 May 2017 14:47:22 +0200
>>
>>> On Wed, May 03, 2017 at 02:44:37PM +1000, Gavin Shan wrote:
>>>> This adds ethtool command (ETHTOOL_GNCSISTATS) to retrieve the
>>>> NCSI hardware statistics.
>>> 
>>> Hi Gavin
>>> 
>>> I've not been following this patchset, so maybe i'm about to ask a
>>> question which has already been asked and answered.
>>> 
>>> Why cannot use just use ethtool -S for this?
>>
>>Indeed, when I said to use ethtool for these NCSI hw stats I meant
>>that the "ethtool -S" be used, not some new ethtool command.
>>
> 
> Thanks for the comments. Feel free to ask any questions which would
> make the code clear and better. There are couple of factors I thought
> separate command is better than ETHTOOL_GSTATS: The statistic items from
> ETHTOOL_GSTATS are variable, meaning the kernel needs provide the layout
> of the statistic items via ETHTOOL_GSSET_INFO and ETHTOOL_GSTRINGS.
> NCSI HW statistics aren't following this and their layout is fixed.
> Besides, statistics for ETHTOOL_GSTATS are maintained in local MAC,
> but NCSI HW statistics are collected from NIC on far-end. So they're
> different from this point. Lastly, the NCSI software statistics needs
> separate command. It'd better to have separate command for HW statistics
> as well, to make things consistent.

ETHTOOL_GSTATS are for device specific statistics.  Whether they are
fixed in your implementation or not.

Re: [PATCH v4 net-next 06/10] net/ncsi: Ethtool operation to get NCSI hw statistics

[Gavin Shan]

On Wed, May 03, 2017 at 08:16:43PM -0400, David Miller wrote:
>From: Gavin Shan <gwshan@linux.vnet.ibm.com>
>Date: Thu, 4 May 2017 10:05:34 +1000
>> On Wed, May 03, 2017 at 09:18:23AM -0400, David Miller wrote:
>>>From: Andrew Lunn <andrew@lunn.ch>
>>>Date: Wed, 3 May 2017 14:47:22 +0200
>>>
>>>> On Wed, May 03, 2017 at 02:44:37PM +1000, Gavin Shan wrote:
>>>>> This adds ethtool command (ETHTOOL_GNCSISTATS) to retrieve the
>>>>> NCSI hardware statistics.
>>>> 
>>>> Hi Gavin
>>>> 
>>>> I've not been following this patchset, so maybe i'm about to ask a
>>>> question which has already been asked and answered.
>>>> 
>>>> Why cannot use just use ethtool -S for this?
>>>
>>>Indeed, when I said to use ethtool for these NCSI hw stats I meant
>>>that the "ethtool -S" be used, not some new ethtool command.
>>>
>> 
>> Thanks for the comments. Feel free to ask any questions which would
>> make the code clear and better. There are couple of factors I thought
>> separate command is better than ETHTOOL_GSTATS: The statistic items from
>> ETHTOOL_GSTATS are variable, meaning the kernel needs provide the layout
>> of the statistic items via ETHTOOL_GSSET_INFO and ETHTOOL_GSTRINGS.
>> NCSI HW statistics aren't following this and their layout is fixed.
>> Besides, statistics for ETHTOOL_GSTATS are maintained in local MAC,
>> but NCSI HW statistics are collected from NIC on far-end. So they're
>> different from this point. Lastly, the NCSI software statistics needs
>> separate command. It'd better to have separate command for HW statistics
>> as well, to make things consistent.
>
>ETHTOOL_GSTATS are for device specific statistics.  Whether they are
>fixed in your implementation or not.
>

For NCSI HW statistics, they're always fixed. It's assured by NCSI
specification. It seems my explanation isn't convincing enough. I'm
fine to convey NCSI HW statistics through ETHTOOL_GSTATS (ETHTOOL_GSSET_INFO
and ETHTOOL_GSTRINGS). Dave, please confirm it's what you're happy to
see?

Cheers,
Gavin

Re: [PATCH v4 net-next 06/10] net/ncsi: Ethtool operation to get NCSI hw statistics

[Andrew Lunn]

On Thu, May 04, 2017 at 10:05:34AM +1000, Gavin Shan wrote:
> On Wed, May 03, 2017 at 09:18:23AM -0400, David Miller wrote:
> >From: Andrew Lunn <andrew@lunn.ch>
> >Date: Wed, 3 May 2017 14:47:22 +0200
> >
> >> On Wed, May 03, 2017 at 02:44:37PM +1000, Gavin Shan wrote:
> >>> This adds ethtool command (ETHTOOL_GNCSISTATS) to retrieve the
> >>> NCSI hardware statistics.
> >> 
> >> Hi Gavin
> >> 
> >> I've not been following this patchset, so maybe i'm about to ask a
> >> question which has already been asked and answered.
> >> 
> >> Why cannot use just use ethtool -S for this?
> >
> >Indeed, when I said to use ethtool for these NCSI hw stats I meant
> >that the "ethtool -S" be used, not some new ethtool command.
> >
> 
> Thanks for the comments. Feel free to ask any questions which would
> make the code clear and better. There are couple of factors I thought
> separate command is better than ETHTOOL_GSTATS: The statistic items from
> ETHTOOL_GSTATS are variable, meaning the kernel needs provide the layout
> of the statistic items via ETHTOOL_GSSET_INFO and ETHTOOL_GSTRINGS.
> NCSI HW statistics aren't following this and their layout is fixed.

That does not stop you from using it for a fixed layout. And what
happens when a new version of the standard is released with more
statistics?

> Besides, statistics for ETHTOOL_GSTATS are maintained in local MAC,
> but NCSI HW statistics are collected from NIC on far-end. So they're
> different from this point. 

You might want to take a look at what is happening with Ethernet
switches. Again, we have two sets of statistics for each port on the
switch. We have the statistics from the hardware, and statistics from
the software. Frames that come in one interface and the hardware
forwards out another interface are not seen by the software. But
frames which originate/terminate in the host, or the switch does not
know what to do with and so are passed to the host, are seen by the
software. Mellanox can tell you more. Your local and remote are not
that different.

> Lastly, the NCSI software statistics needs separate command. It'd
> better to have separate command for HW statistics as well, to make
> things consistent.

Are software statistics defined in DSP0222? Since they are software, i
kind of expect you want the flexibility to add more later. The
ETHTOOL_GSSET_INFO and ETHTOOL_GSTRINGS gives you that flexibility,
without having to change ethtool.

	Andrew

Re: [PATCH v4 net-next 06/10] net/ncsi: Ethtool operation to get NCSI hw statistics

[Gavin Shan]

On Thu, May 04, 2017 at 02:34:52AM +0200, Andrew Lunn wrote:
>On Thu, May 04, 2017 at 10:05:34AM +1000, Gavin Shan wrote:
>> On Wed, May 03, 2017 at 09:18:23AM -0400, David Miller wrote:
>> >From: Andrew Lunn <andrew@lunn.ch>
>> >Date: Wed, 3 May 2017 14:47:22 +0200
>> >
>> >> On Wed, May 03, 2017 at 02:44:37PM +1000, Gavin Shan wrote:
>> >>> This adds ethtool command (ETHTOOL_GNCSISTATS) to retrieve the
>> >>> NCSI hardware statistics.
>> >> 
>> >> Hi Gavin
>> >> 
>> >> I've not been following this patchset, so maybe i'm about to ask a
>> >> question which has already been asked and answered.
>> >> 
>> >> Why cannot use just use ethtool -S for this?
>> >
>> >Indeed, when I said to use ethtool for these NCSI hw stats I meant
>> >that the "ethtool -S" be used, not some new ethtool command.
>> >
>> 
>> Thanks for the comments. Feel free to ask any questions which would
>> make the code clear and better. There are couple of factors I thought
>> separate command is better than ETHTOOL_GSTATS: The statistic items from
>> ETHTOOL_GSTATS are variable, meaning the kernel needs provide the layout
>> of the statistic items via ETHTOOL_GSSET_INFO and ETHTOOL_GSTRINGS.
>> NCSI HW statistics aren't following this and their layout is fixed.
>
>That does not stop you from using it for a fixed layout. And what
>happens when a new version of the standard is released with more
>statistics?
>

Correct, I don't see the difference between NCSI spec 1.01 and 1.1.0.
So I assume it's pretty stable, but still have potential to change.

I agree to collect NCSI HW statistic with ETHTOOL_GSTATS as I said
in another thread.

>> Besides, statistics for ETHTOOL_GSTATS are maintained in local MAC,
>> but NCSI HW statistics are collected from NIC on far-end. So they're
>> different from this point. 
>
>You might want to take a look at what is happening with Ethernet
>switches. Again, we have two sets of statistics for each port on the
>switch. We have the statistics from the hardware, and statistics from
>the software. Frames that come in one interface and the hardware
>forwards out another interface are not seen by the software. But
>frames which originate/terminate in the host, or the switch does not
>know what to do with and so are passed to the host, are seen by the
>software. Mellanox can tell you more. Your local and remote are not
>that different.
>

Yeah, it makes sense. Thanks for making an example here.

>> Lastly, the NCSI software statistics needs separate command. It'd
>> better to have separate command for HW statistics as well, to make
>> things consistent.
>
>Are software statistics defined in DSP0222? Since they are software, i
>kind of expect you want the flexibility to add more later. The
>ETHTOOL_GSSET_INFO and ETHTOOL_GSTRINGS gives you that flexibility,
>without having to change ethtool.
>

No, they're purely software implementation, not defined in DSP0222.
These statistics count the NCSI packets seen by software, not hardware.
It's used for diagnosis.

It's a nice suggestion to convey SW statistic with ETHTOOL_GSTATS,
ETHTOOL_GSSET_INFO and ETHTOOL_GSTRINGS, for better flexibility. I'll
follow this in next respin.

Cheers,
Gavin

[PATCH v4 net-next 07/10] net/ncsi: Ethtool operation to get NCSI sw statistics

[Gavin Shan]

This adds ethtool command (ETHTOOL_GNCSISWSTATS) to retrieve the
NCSI software statistics. The simplified output of this command is
shown as follows from the modified (private) ethtool.

 COMMAND      OK       TIMEOUT  ERROR
 ====================================
 CIS          32       29       0
 SP           10       7        0
 DP           17       14       0
 EC           1        0        0
 ECNT         1        0        0
 AE           1        0        0
 GLS          10       0        0
 SMA          1        0        0
 DBF          1        0        0
 GC           2        0        0
 GP           2        0        0

 RESPONSE     OK       TIMEOUT  ERROR
 ====================================
 CIS          3        0        0
 SP           3        0        0
 DP           2        0        1
 EC           1        0        0
 ECNT         1        0        0
 AE           1        0        0
 GLS          10       0        0
 SMA          1        0        0
 DBF          1        0        0
 GC           0        0        2
 GP           2        0        0

 AEN          OK       TIMEOUT  ERROR
 ====================================

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 include/linux/ethtool.h      |  2 ++
 include/uapi/linux/ethtool.h | 20 ++++++++++++++++++++
 net/core/ethtool.c           | 29 ++++++++++++++++++++++++++++
 net/ncsi/Kconfig             |  9 +++++++++
 net/ncsi/Makefile            |  1 +
 net/ncsi/internal.h          | 13 +++++++++++++
 net/ncsi/ncsi-aen.c          | 14 +++++++++++++-
 net/ncsi/ncsi-cmd.c          | 12 +++++++++++-
 net/ncsi/ncsi-debug.c        | 45 ++++++++++++++++++++++++++++++++++++++++++++
 net/ncsi/ncsi-ethtool.c      | 34 +++++++++++++++++++++++++++++++++
 net/ncsi/ncsi-manage.c       |  4 ++++
 net/ncsi/ncsi-rsp.c          | 19 ++++++++++++++++++-
 12 files changed, 199 insertions(+), 3 deletions(-)
 create mode 100644 net/ncsi/ncsi-debug.c

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 6d712ca..eb57142 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -380,5 +380,7 @@ struct ethtool_ops {
 					 struct ethtool_ncsi_channel_info *);
 	int	(*get_ncsi_stats)(struct net_device *,
 				  struct ethtool_ncsi_stats *);
+	int	(*get_ncsi_sw_stats)(struct net_device *,
+				     struct ethtool_ncsi_sw_stats *);
 };
 #endif /* _LINUX_ETHTOOL_H */
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 472773c..bf6fa2b 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1334,6 +1334,7 @@ struct ethtool_per_queue_op {
 #define ETHTOOL_GNCSICHANNELS	0x00000050 /* Get NCSI channels */
 #define ETHTOOL_GNCSICINFO	0x00000051 /* Get NCSI channel information */
 #define ETHTOOL_GNCSISTATS	0x00000052 /* Get NCSI HW statistics */
+#define ETHTOOL_GNCSISWSTATS	0x00000053 /* Get NCSI software statistics */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
@@ -2055,4 +2056,23 @@ struct ethtool_ncsi_stats {
 	__u64	pt_rx_us_err;
 	__u64	pt_rx_os_err;
 };
+
+/**
+ * struct ethtool_ncsi_sw_stats - NCSI software statistics
+ *
+ * @cmd: Command number = %ETHTOOL_GNCSISWSTATS
+ * @command: Statistics for sent command packets
+ * @response: Statistics for received response packets
+ * @aen: Statistics for received AEN packets
+ */
+struct ethtool_ncsi_sw_stats {
+	__u32	cmd;
+#define ETHTOOL_NCSI_SW_STAT_OK		0
+#define ETHTOOL_NCSI_SW_STAT_TIMEOUT	1
+#define ETHTOOL_NCSI_SW_STAT_ERROR	2
+#define ETHTOOL_NCSI_SW_STAT_MAX	3
+	__u64	command[128][ETHTOOL_NCSI_SW_STAT_MAX];
+	__u64	response[128][ETHTOOL_NCSI_SW_STAT_MAX];
+	__u64	aen[256][ETHTOOL_NCSI_SW_STAT_MAX];
+};
 #endif /* _UAPI_LINUX_ETHTOOL_H */
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index f26aa36..998d29b 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -838,6 +838,32 @@ static int ethtool_get_ncsi_stats(struct net_device *dev,
 	return ret;
 }
 
+static int ethtool_get_ncsi_sw_stats(struct net_device *dev,
+				     void __user *useraddr)
+{
+	struct ethtool_ncsi_sw_stats *enss;
+	int ret;
+
+	if (!dev->ethtool_ops->get_ncsi_sw_stats)
+		return -EOPNOTSUPP;
+
+	enss = kzalloc(sizeof(*enss), GFP_KERNEL);
+	if (!enss)
+		return -ENOMEM;
+
+	if (copy_from_user(&enss->cmd, useraddr, sizeof(enss->cmd))) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	ret = dev->ethtool_ops->get_ncsi_sw_stats(dev, enss);
+	if (!ret && copy_to_user(useraddr, enss, sizeof(*enss)))
+		ret = -EFAULT;
+out:
+	kfree(enss);
+	return ret;
+}
+
 static void
 warn_incomplete_ethtool_legacy_settings_conversion(const char *details)
 {
@@ -2884,6 +2910,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GNCSISTATS:
 		rc = ethtool_get_ncsi_stats(dev, useraddr);
 		break;
+	case ETHTOOL_GNCSISWSTATS:
+		rc = ethtool_get_ncsi_sw_stats(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}
diff --git a/net/ncsi/Kconfig b/net/ncsi/Kconfig
index 08a8a60..9e59145 100644
--- a/net/ncsi/Kconfig
+++ b/net/ncsi/Kconfig
@@ -10,3 +10,12 @@ config NET_NCSI
 	  support. Enable this only if your system connects to a network
 	  device via NCSI and the ethernet driver you're using supports
 	  the protocol explicitly.
+
+config NET_NCSI_DEBUG
+	bool "Enable NCSI debugging"
+	depends on NET_NCSI && DEBUG_FS
+	default n
+	---help---
+	  This enables the interfaces (e.g. debugfs) for NCSI debugging purpose.
+
+	  If unsure, say N.
diff --git a/net/ncsi/Makefile b/net/ncsi/Makefile
index 71a258a..4e0c5d2 100644
--- a/net/ncsi/Makefile
+++ b/net/ncsi/Makefile
@@ -3,3 +3,4 @@
 #
 obj-$(CONFIG_NET_NCSI) += ncsi-cmd.o ncsi-rsp.o ncsi-aen.o ncsi-manage.o \
 			  ncsi-ethtool.o
+obj-$(CONFIG_NET_NCSI_DEBUG) += ncsi-debug.o
diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 09a7ba7..5a6cd74 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -275,6 +275,9 @@ struct ncsi_dev_priv {
 	struct list_head    channel_queue;   /* Config queue of channels   */
 	struct work_struct  work;            /* For channel management     */
 	struct packet_type  ptype;           /* NCSI packet Rx handler     */
+#ifdef CONFIG_NET_NCSI_DEBUG
+	struct ethtool_ncsi_sw_stats stats;  /* NCSI software statistics   */
+#endif /* CONFIG_NET_NCSI_DEBUG */
 	struct list_head    node;            /* Form NCSI device list      */
 };
 
@@ -341,4 +344,14 @@ int ncsi_aen_handler(struct ncsi_dev_priv *ndp, struct sk_buff *skb);
 void ncsi_ethtool_register_dev(struct net_device *dev);
 void ncsi_ethtool_unregister_dev(struct net_device *dev);
 
+/* Debugging functionality */
+#ifdef CONFIG_NET_NCSI_DEBUG
+void ncsi_dev_update_stats(struct ncsi_dev_priv *ndp,
+			   int type, int subtype, int errno);
+#else
+static inline void ncsi_dev_update_stats(struct ncsi_dev_priv *ndp,
+					 int type, int subtype, int errno)
+{
+}
+#endif /* CONFIG_NET_NCSI_DEBUG */
 #endif /* __NCSI_INTERNAL_H__ */
diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
index 6898e72..7a3d181 100644
--- a/net/ncsi/ncsi-aen.c
+++ b/net/ncsi/ncsi-aen.c
@@ -206,16 +206,28 @@ int ncsi_aen_handler(struct ncsi_dev_priv *ndp, struct sk_buff *skb)
 	}
 
 	if (!nah) {
+		ncsi_dev_update_stats(ndp, NCSI_PKT_AEN, h->type,
+				      ETHTOOL_NCSI_SW_STAT_ERROR);
 		netdev_warn(ndp->ndev.dev, "Invalid AEN (0x%x) received\n",
 			    h->type);
 		return -ENOENT;
 	}
 
 	ret = ncsi_validate_aen_pkt(h, nah->payload);
-	if (ret)
+	if (ret) {
+		ncsi_dev_update_stats(ndp, NCSI_PKT_AEN, h->type,
+				      ETHTOOL_NCSI_SW_STAT_ERROR);
 		goto out;
+	}
 
 	ret = nah->handler(ndp, h);
+	if (!ret) {
+		ncsi_dev_update_stats(ndp, NCSI_PKT_AEN, h->type,
+				      ETHTOOL_NCSI_SW_STAT_OK);
+	} else {
+		ncsi_dev_update_stats(ndp, NCSI_PKT_AEN, h->type,
+				      ETHTOOL_NCSI_SW_STAT_ERROR);
+	}
 out:
 	consume_skb(skb);
 	return ret;
diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
index db7083b..875ff07 100644
--- a/net/ncsi/ncsi-cmd.c
+++ b/net/ncsi/ncsi-cmd.c
@@ -323,6 +323,8 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
 	}
 
 	if (!nch) {
+		ncsi_dev_update_stats(nca->ndp, nca->type, 0,
+				      ETHTOOL_NCSI_SW_STAT_ERROR);
 		netdev_err(nca->ndp->ndev.dev,
 			   "Cannot send packet with type 0x%02x\n", nca->type);
 		return -ENOENT;
@@ -331,13 +333,18 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
 	/* Get packet payload length and allocate the request */
 	nca->payload = nch->payload;
 	nr = ncsi_alloc_command(nca);
-	if (!nr)
+	if (!nr) {
+		ncsi_dev_update_stats(nca->ndp, nca->type, 0,
+				      ETHTOOL_NCSI_SW_STAT_ERROR);
 		return -ENOMEM;
+	}
 
 	/* Prepare the packet */
 	nca->id = nr->id;
 	ret = nch->handler(nr->cmd, nca);
 	if (ret) {
+		ncsi_dev_update_stats(nca->ndp, nca->type, 0,
+				      ETHTOOL_NCSI_SW_STAT_ERROR);
 		ncsi_free_request(nr);
 		return ret;
 	}
@@ -359,9 +366,12 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
 	skb_get(nr->cmd);
 	ret = dev_queue_xmit(nr->cmd);
 	if (ret < 0) {
+		ncsi_dev_update_stats(nca->ndp, nca->type, 0,
+				      ETHTOOL_NCSI_SW_STAT_ERROR);
 		ncsi_free_request(nr);
 		return ret;
 	}
 
+	ncsi_dev_update_stats(nca->ndp, nca->type, 0, ETHTOOL_NCSI_SW_STAT_OK);
 	return 0;
 }
diff --git a/net/ncsi/ncsi-debug.c b/net/ncsi/ncsi-debug.c
new file mode 100644
index 0000000..0e6c038
--- /dev/null
+++ b/net/ncsi/ncsi-debug.c
@@ -0,0 +1,45 @@
+/*
+ * Copyright Gavin Shan, IBM Corporation 2017.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/atomic.h>
+#include <linux/netdevice.h>
+#include <linux/debugfs.h>
+#include <linux/skbuff.h>
+
+#include <net/ncsi.h>
+#include <net/net_namespace.h>
+#include <net/sock.h>
+
+#include "internal.h"
+#include "ncsi-pkt.h"
+
+void ncsi_dev_update_stats(struct ncsi_dev_priv *ndp,
+			   int type, int subtype, int errno)
+{
+	unsigned long flags;
+
+	if (errno >= ETHTOOL_NCSI_SW_STAT_MAX)
+		return;
+
+	spin_lock_irqsave(&ndp->lock, flags);
+
+	if (type == NCSI_PKT_AEN) {
+		if (subtype < 256)
+			ndp->stats.aen[subtype][errno]++;
+	} else if (type < 128) {
+		ndp->stats.command[type][errno]++;
+	} else if (type < 256) {
+		ndp->stats.response[type - 128][errno]++;
+	}
+
+	spin_unlock_irqrestore(&ndp->lock, flags);
+}
diff --git a/net/ncsi/ncsi-ethtool.c b/net/ncsi/ncsi-ethtool.c
index 1ccdb50..82642ae 100644
--- a/net/ncsi/ncsi-ethtool.c
+++ b/net/ncsi/ncsi-ethtool.c
@@ -260,6 +260,38 @@ static int ncsi_get_stats(struct net_device *dev,
 	return 0;
 }
 
+#ifdef CONFIG_NET_NCSI_DEBUG
+static int ncsi_get_sw_stats(struct net_device *dev,
+			     struct ethtool_ncsi_sw_stats *enss)
+{
+	struct ncsi_dev *nd;
+	struct ncsi_dev_priv *ndp;
+	unsigned long flags;
+
+	nd = ncsi_find_dev(dev);
+	if (!nd)
+		return -ENXIO;
+
+	ndp = TO_NCSI_DEV_PRIV(nd);
+	spin_lock_irqsave(&ndp->lock, flags);
+	memcpy(enss->command, ndp->stats.command,
+	       128 * ETHTOOL_NCSI_SW_STAT_MAX * sizeof(enss->command[0][0]));
+	memcpy(enss->response, ndp->stats.response,
+	       128 * ETHTOOL_NCSI_SW_STAT_MAX * sizeof(enss->response[0][0]));
+	memcpy(enss->aen, ndp->stats.aen,
+	       256 * ETHTOOL_NCSI_SW_STAT_MAX * sizeof(enss->aen[0][0]));
+	spin_unlock_irqrestore(&ndp->lock, flags);
+
+	return 0;
+}
+#else
+static int ncsi_get_sw_stats(struct net_device *dev,
+			     struct ethtool_ncsi_sw_stats *enss)
+{
+	return -EOPNOTSUPP;
+}
+#endif /* CONFIG_NET_NCSI_DEBUG */
+
 void ncsi_ethtool_register_dev(struct net_device *dev)
 {
 	struct ethtool_ops *ops;
@@ -271,6 +303,7 @@ void ncsi_ethtool_register_dev(struct net_device *dev)
 	ops->get_ncsi_channels = ncsi_get_channels;
 	ops->get_ncsi_channel_info = ncsi_get_channel_info;
 	ops->get_ncsi_stats = ncsi_get_stats;
+	ops->get_ncsi_sw_stats = ncsi_get_sw_stats;
 }
 
 void ncsi_ethtool_unregister_dev(struct net_device *dev)
@@ -284,4 +317,5 @@ void ncsi_ethtool_unregister_dev(struct net_device *dev)
 	ops->get_ncsi_channels = NULL;
 	ops->get_ncsi_channel_info = NULL;
 	ops->get_ncsi_stats = NULL;
+	ops->get_ncsi_sw_stats = NULL;
 }
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index f1c10f0..8365a5b 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -521,6 +521,7 @@ static void ncsi_request_timeout(unsigned long data)
 {
 	struct ncsi_request *nr = (struct ncsi_request *)data;
 	struct ncsi_dev_priv *ndp = nr->ndp;
+	struct ncsi_pkt_hdr *hdr;
 	unsigned long flags;
 
 	/* If the request already had associated response,
@@ -534,6 +535,9 @@ static void ncsi_request_timeout(unsigned long data)
 	}
 	spin_unlock_irqrestore(&ndp->lock, flags);
 
+	hdr = (struct ncsi_pkt_hdr *)skb_network_header(nr->cmd);
+	ncsi_dev_update_stats(ndp, hdr->type, 0, ETHTOOL_NCSI_SW_STAT_TIMEOUT);
+
 	/* Release the request */
 	ncsi_free_request(nr);
 }
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 087db77..d362d2c 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -998,6 +998,8 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
 	}
 
 	if (!nrh) {
+		ncsi_dev_update_stats(ndp, hdr->type, 0,
+				      ETHTOOL_NCSI_SW_STAT_ERROR);
 		netdev_err(nd->dev, "Received unrecognized packet (0x%x)\n",
 			   hdr->type);
 		return -ENOENT;
@@ -1008,12 +1010,16 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
 	nr = &ndp->requests[hdr->id];
 	if (!nr->used) {
 		spin_unlock_irqrestore(&ndp->lock, flags);
+		ncsi_dev_update_stats(ndp, hdr->type, 0,
+				      ETHTOOL_NCSI_SW_STAT_TIMEOUT);
 		return -ENODEV;
 	}
 
 	nr->rsp = skb;
 	if (!nr->enabled) {
 		spin_unlock_irqrestore(&ndp->lock, flags);
+		ncsi_dev_update_stats(ndp, hdr->type, 0,
+				      ETHTOOL_NCSI_SW_STAT_TIMEOUT);
 		ret = -ENOENT;
 		goto out;
 	}
@@ -1024,11 +1030,22 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
 	if (payload < 0)
 		payload = ntohs(hdr->length);
 	ret = ncsi_validate_rsp_pkt(nr, payload);
-	if (ret)
+	if (ret) {
+		ncsi_dev_update_stats(ndp, hdr->type, 0,
+				      ETHTOOL_NCSI_SW_STAT_ERROR);
 		goto out;
+	}
 
 	/* Process the packet */
 	ret = nrh->handler(nr);
+	if (!ret) {
+		ncsi_dev_update_stats(ndp, hdr->type, 0,
+				      ETHTOOL_NCSI_SW_STAT_OK);
+	} else {
+		ncsi_dev_update_stats(ndp, hdr->type, 0,
+				      ETHTOOL_NCSI_SW_STAT_ERROR);
+	}
+
 out:
 	ncsi_free_request(nr);
 	return ret;
-- 
2.7.4

[PATCH v4 net-next 08/10] net/ncsi: Support NCSI packet generation

[Gavin Shan]

This introduces /sys/kernel/debug/ncsi/eth0/pkt. The debugfs entry
can accept parameters to produce NCSI command packet. The received
NCSI response packet is dumped on read. Below is an example to send
CIS command and dump its response.

   # echo CIS,0,0 > /sys/kernel/debug/ncsi/eth0/pkt
   # cat /sys/kernel/debug/ncsi/eth0/pkt
   NCSI response [CIS] packet received

   00 01 dd 80 00 0004 0000 0000

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 net/ncsi/internal.h    |  54 ++++
 net/ncsi/ncsi-cmd.c    |   1 +
 net/ncsi/ncsi-debug.c  | 714 +++++++++++++++++++++++++++++++++++++++++++++++++
 net/ncsi/ncsi-manage.c |  10 +
 net/ncsi/ncsi-rsp.c    |  11 +
 5 files changed, 790 insertions(+)

diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 5a6cd74..0a9210d 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -215,6 +215,9 @@ struct ncsi_request {
 	bool                 used;    /* Request that has been assigned  */
 	unsigned int         flags;   /* NCSI request property           */
 #define NCSI_REQ_FLAG_EVENT_DRIVEN	1
+#ifdef CONFIG_NET_NCSI_DEBUG
+#define NCSI_REQ_FLAG_DEBUG		2
+#endif
 	struct ncsi_dev_priv *ndp;    /* Associated NCSI device          */
 	struct sk_buff       *cmd;    /* Associated NCSI command packet  */
 	struct sk_buff       *rsp;    /* Associated NCSI response packet */
@@ -277,6 +280,15 @@ struct ncsi_dev_priv {
 	struct packet_type  ptype;           /* NCSI packet Rx handler     */
 #ifdef CONFIG_NET_NCSI_DEBUG
 	struct ethtool_ncsi_sw_stats stats;  /* NCSI software statistics   */
+	struct dentry       *dentry;         /* Debugfs directory           */
+	struct {
+		struct dentry  *dentry;
+		unsigned int   req;
+#define NCSI_PKT_REQ_FREE	0
+#define NCSI_PKT_REQ_BUSY	0xFFFFFFFF
+		int            errno;
+		struct sk_buff *rsp;
+	} pkt;
 #endif /* CONFIG_NET_NCSI_DEBUG */
 	struct list_head    node;            /* Form NCSI device list      */
 };
@@ -348,10 +360,52 @@ void ncsi_ethtool_unregister_dev(struct net_device *dev);
 #ifdef CONFIG_NET_NCSI_DEBUG
 void ncsi_dev_update_stats(struct ncsi_dev_priv *ndp,
 			   int type, int subtype, int errno);
+int ncsi_dev_init_debug(struct ncsi_dev_priv *ndp);
+void ncsi_dev_release_debug(struct ncsi_dev_priv *ndp);
+void ncsi_dev_reset_debug_pkt(struct ncsi_dev_priv *ndp,
+			      struct sk_buff *skb, int errno);
+
+static inline bool ncsi_dev_is_debug_pkt(struct ncsi_dev_priv *ndp,
+					 struct ncsi_request *nr)
+{
+	return ((nr->flags & NCSI_REQ_FLAG_DEBUG) && ndp->pkt.req == nr->id);
+}
+
+static inline void ncsi_dev_set_debug_pkt(struct ncsi_dev_priv *ndp,
+					  struct ncsi_request *nr)
+{
+	if (nr->flags & NCSI_REQ_FLAG_DEBUG)
+		ndp->pkt.req = nr->id;
+}
 #else
 static inline void ncsi_dev_update_stats(struct ncsi_dev_priv *ndp,
 					 int type, int subtype, int errno)
 {
 }
+
+static inline int ncsi_dev_init_debug(struct ncsi_dev_priv *ndp)
+{
+	return -ENOTTY;
+}
+
+static inline void ncsi_dev_release_debug(struct ncsi_dev_priv *ndp)
+{
+}
+
+static inline bool ncsi_dev_is_debug_pkt(struct ncsi_dev_priv *ndp,
+					 struct ncsi_request *nr)
+{
+	return false;
+}
+
+static inline void ncsi_dev_set_debug_pkt(struct ncsi_dev_priv *ndp,
+					  struct ncsi_request *nr)
+{
+}
+
+static inline void ncsi_dev_reset_debug_pkt(struct ncsi_dev_priv *ndp,
+					    struct sk_buff *skb, int errno)
+{
+}
 #endif /* CONFIG_NET_NCSI_DEBUG */
 #endif /* __NCSI_INTERNAL_H__ */
diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
index 875ff07..cfcda87 100644
--- a/net/ncsi/ncsi-cmd.c
+++ b/net/ncsi/ncsi-cmd.c
@@ -361,6 +361,7 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
 	 */
 	nr->enabled = true;
 	mod_timer(&nr->timer, jiffies + 1 * HZ);
+	ncsi_dev_set_debug_pkt(nca->ndp, nr);
 
 	/* Send NCSI packet */
 	skb_get(nr->cmd);
diff --git a/net/ncsi/ncsi-debug.c b/net/ncsi/ncsi-debug.c
index 0e6c038..5a5f058 100644
--- a/net/ncsi/ncsi-debug.c
+++ b/net/ncsi/ncsi-debug.c
@@ -22,6 +22,9 @@
 #include "internal.h"
 #include "ncsi-pkt.h"
 
+static struct dentry *ncsi_dentry;
+static const char *ncsi_pkt_type_name(unsigned int type);
+
 void ncsi_dev_update_stats(struct ncsi_dev_priv *ndp,
 			   int type, int subtype, int errno)
 {
@@ -43,3 +46,714 @@ void ncsi_dev_update_stats(struct ncsi_dev_priv *ndp,
 
 	spin_unlock_irqrestore(&ndp->lock, flags);
 }
+
+void ncsi_dev_reset_debug_pkt(struct ncsi_dev_priv *ndp,
+			      struct sk_buff *skb, int errno)
+{
+	unsigned long flags;
+	struct sk_buff *old;
+
+	spin_lock_irqsave(&ndp->lock, flags);
+	ndp->pkt.req = NCSI_PKT_REQ_FREE;
+	ndp->pkt.errno = errno;
+
+	old = ndp->pkt.rsp;
+	ndp->pkt.rsp = skb;
+	spin_unlock_irqrestore(&ndp->lock, flags);
+
+	consume_skb(old);
+}
+
+static int ncsi_pkt_input_default(struct ncsi_dev_priv *ndp,
+				  struct ncsi_cmd_arg *nca, char *buf)
+{
+	return 0;
+}
+
+static int ncsi_pkt_input_params(char *buf, int *outval, int count)
+{
+	int num, i;
+
+	for (i = 0; i < count; i++, outval++) {
+		if (sscanf(buf, "%x%n", outval, &num) != 1)
+			return -EINVAL;
+
+		if (buf[num] == ',')
+			buf += (count + 1);
+		else
+			buf += count;
+	}
+
+	return 0;
+}
+
+static int ncsi_pkt_input_sp(struct ncsi_dev_priv *ndp,
+			     struct ncsi_cmd_arg *nca, char *buf)
+{
+	int param, ret;
+
+	/* The hardware arbitration will be configured according
+	 * to the NCSI's capability if it's not specified.
+	 */
+	ret = ncsi_pkt_input_params(buf, &param, 1);
+	if (!ret && param != 0 && param != 1)
+		return -EINVAL;
+	else if (ret)
+		param = (ndp->flags & NCSI_DEV_HWA) ? 1 : 0;
+
+	nca->bytes[0] = param;
+
+	return 0;
+}
+
+static int ncsi_pkt_input_dc(struct ncsi_dev_priv *ndp,
+			     struct ncsi_cmd_arg *nca, char *buf)
+{
+	int param, ret;
+
+	/* Allow link down will be disallowed if it's not specified */
+	ret = ncsi_pkt_input_params(buf, &param, 1);
+	if (!ret && param != 0 && param != 1)
+		return -EINVAL;
+	else if (ret)
+		param = 0;
+
+	nca->bytes[0] = param;
+
+	return 0;
+}
+
+static int ncsi_pkt_input_ae(struct ncsi_dev_priv *ndp,
+			     struct ncsi_cmd_arg *nca, char *buf)
+{
+	int param[2], ret;
+
+	/* MC ID and AE mode are mandatory */
+	ret = ncsi_pkt_input_params(buf, param, 2);
+	if (ret)
+		return -EINVAL;
+
+	nca->bytes[0] = param[0];
+	nca->dwords[1] = param[1];
+
+	return 0;
+}
+
+static int ncsi_pkt_input_sl(struct ncsi_dev_priv *ndp,
+			     struct ncsi_cmd_arg *nca, char *buf)
+{
+	int param[2], ret;
+
+	/* Link mode and OEM mode are mandatory */
+	ret = ncsi_pkt_input_params(buf, param, 2);
+	if (ret)
+		return -EINVAL;
+
+	nca->dwords[0] = param[0];
+	nca->dwords[1] = param[1];
+
+	return 0;
+}
+
+static int ncsi_pkt_input_svf(struct ncsi_dev_priv *ndp,
+			      struct ncsi_cmd_arg *nca, char *buf)
+{
+	int param[3], ret;
+
+	/* VLAN ID, table index and enable */
+	ret = ncsi_pkt_input_params(buf, param, 3);
+	if (ret)
+		return -EINVAL;
+
+	if (param[2] != 0 && param[2] != 1)
+		return -EINVAL;
+
+	nca->words[0] = param[0];
+	nca->bytes[2] = param[1];
+	nca->bytes[3] = param[2];
+
+	return 0;
+}
+
+static int ncsi_pkt_input_ev(struct ncsi_dev_priv *ndp,
+			     struct ncsi_cmd_arg *nca, char *buf)
+{
+	int param, ret;
+
+	/* VLAN filter mode */
+	ret = ncsi_pkt_input_params(buf, &param, 1);
+	if (ret)
+		return -EINVAL;
+
+	nca->bytes[0] = param;
+
+	return 0;
+}
+
+static int ncsi_pkt_input_sma(struct ncsi_dev_priv *ndp,
+			      struct ncsi_cmd_arg *nca, char *buf)
+{
+	int param[8], ret;
+
+	/* MAC address, MAC table index, Address type and operation */
+	ret = ncsi_pkt_input_params(buf, param, 8);
+	if (ret)
+		return -EINVAL;
+
+	if (param[7] & ~0x9)
+		return -EINVAL;
+
+	nca->bytes[0] = param[0];
+	nca->bytes[1] = param[1];
+	nca->bytes[2] = param[2];
+	nca->bytes[3] = param[3];
+	nca->bytes[4] = param[4];
+	nca->bytes[5] = param[5];
+
+	nca->bytes[6] = param[6];
+	nca->bytes[7] = param[7];
+
+	return 0;
+}
+
+static int ncsi_pkt_input_ebf(struct ncsi_dev_priv *ndp,
+			      struct ncsi_cmd_arg *nca, char *buf)
+{
+	int param, ret;
+
+	/* Broadcast filter mode */
+	ret = ncsi_pkt_input_params(buf, &param, 1);
+	if (ret)
+		return -EINVAL;
+
+	nca->dwords[0] = param;
+
+	return 0;
+}
+
+static int ncsi_pkt_input_egmf(struct ncsi_dev_priv *ndp,
+			       struct ncsi_cmd_arg *nca, char *buf)
+{
+	int param, ret;
+
+	/* Global multicast filter mode */
+	ret = ncsi_pkt_input_params(buf, &param, 1);
+	if (ret)
+		return -EINVAL;
+
+	nca->dwords[0] = param;
+
+	return 0;
+}
+
+static int ncsi_pkt_input_snfc(struct ncsi_dev_priv *ndp,
+			       struct ncsi_cmd_arg *nca, char *buf)
+{
+	int param, ret;
+
+	/* NCSI flow control mode */
+	ret = ncsi_pkt_input_params(buf, &param, 1);
+	if (ret)
+		return -EINVAL;
+
+	nca->bytes[0] = param;
+
+	return 0;
+}
+
+static void ncsi_pkt_output_header(struct ncsi_dev_priv *ndp,
+				   struct seq_file *seq,
+				   struct ncsi_rsp_pkt_hdr *h)
+{
+	seq_printf(seq, "NCSI response [%s] packet received\n\n",
+		   ncsi_pkt_type_name(h->common.type - 0x80));
+	seq_printf(seq, "%02x %02x %02x %02x %02x %04x %04x %04x\n",
+		   h->common.mc_id, h->common.revision, h->common.id,
+		   h->common.type, h->common.channel, ntohs(h->common.length),
+		   ntohs(h->code), ntohs(h->reason));
+}
+
+static int ncsi_pkt_output_default(struct ncsi_dev_priv *ndp,
+				   struct seq_file *seq,
+				   struct sk_buff *skb)
+{
+	struct ncsi_rsp_pkt_hdr *hdr;
+
+	hdr = (struct ncsi_rsp_pkt_hdr *)skb_network_header(skb);
+	ncsi_pkt_output_header(ndp, seq, hdr);
+
+	return 0;
+}
+
+static int ncsi_pkt_output_gls(struct ncsi_dev_priv *ndp,
+			       struct seq_file *seq, struct sk_buff *skb)
+{
+	struct ncsi_rsp_gls_pkt *gls;
+
+	ncsi_pkt_output_default(ndp, seq, skb);
+
+	gls = (struct ncsi_rsp_gls_pkt *)skb_network_header(skb);
+	seq_printf(seq, "Status: %08x Other: %08x OEM: %08x\n",
+		   ntohl(gls->status), ntohl(gls->other),
+		   ntohl(gls->oem_status));
+
+	return 0;
+}
+
+static int ncsi_pkt_output_gvi(struct ncsi_dev_priv *ndp,
+			       struct seq_file *seq, struct sk_buff *skb)
+{
+	struct ncsi_rsp_gvi_pkt *gvi;
+
+	ncsi_pkt_output_default(ndp, seq, skb);
+
+	gvi = (struct ncsi_rsp_gvi_pkt *)skb_network_header(skb);
+	seq_printf(seq, "NCSI Version: %08x Alpha2: %02x\n",
+		   ntohl(gvi->ncsi_version), gvi->alpha2);
+	seq_printf(seq, "Firmware: %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x Version: %08x\n",
+		   gvi->fw_name[0], gvi->fw_name[1], gvi->fw_name[2],
+		   gvi->fw_name[3], gvi->fw_name[4], gvi->fw_name[5],
+		   gvi->fw_name[6], gvi->fw_name[7], gvi->fw_name[8],
+		   gvi->fw_name[9], gvi->fw_name[10], gvi->fw_name[11],
+		   ntohl(gvi->fw_version));
+	seq_printf(seq, "PCI: %04x %04x %04x %04x Manufacture ID: %08x\n",
+		   ntohs(gvi->pci_ids[0]), ntohs(gvi->pci_ids[1]),
+		   ntohs(gvi->pci_ids[2]), ntohs(gvi->pci_ids[3]),
+		   ntohl(gvi->mf_id));
+
+	return 0;
+}
+
+static int ncsi_pkt_output_gc(struct ncsi_dev_priv *ndp,
+			      struct seq_file *seq, struct sk_buff *skb)
+{
+	struct ncsi_rsp_gc_pkt *gc;
+
+	ncsi_pkt_output_default(ndp, seq, skb);
+
+	gc = (struct ncsi_rsp_gc_pkt *)skb_network_header(skb);
+	seq_printf(seq, "Cap: %08x BC: %08x MC: %08x Buf: %08x AEN: %08x\n",
+		   ntohl(gc->cap), ntohl(gc->bc_cap), ntohl(gc->mc_cap),
+		   ntohl(gc->buf_cap), ntohl(gc->aen_cap));
+	seq_printf(seq, "VLAN: %02x Mixed: %02x MC: %02x UC: %02x\n",
+		   gc->vlan_cnt, gc->mixed_cnt, gc->mc_cnt, gc->uc_cnt);
+	seq_printf(seq, "VLAN Mode: %02x Channels: %02x\n",
+		   gc->vlan_mode, gc->channel_cnt);
+
+	return 0;
+}
+
+static int ncsi_pkt_output_gp(struct ncsi_dev_priv *ndp,
+			      struct seq_file *seq, struct sk_buff *skb)
+{
+	struct ncsi_rsp_gp_pkt *gp;
+
+	ncsi_pkt_output_default(ndp, seq, skb);
+
+	gp = (struct ncsi_rsp_gp_pkt *)skb_network_header(skb);
+	seq_printf(seq, "MAC: %02x %02x VLAN: %02x %04x\n",
+		   gp->mac_cnt, gp->mac_enable, gp->vlan_cnt,
+		   ntohs(gp->vlan_enable));
+	seq_printf(seq, "Link: %08x BC: %08x Valid: %08x\n",
+		   ntohl(gp->link_mode), ntohl(gp->bc_mode),
+		   ntohl(gp->valid_modes));
+	seq_printf(seq, "VLAN: %02x FC: %02x AEN: %08x\n",
+		   gp->vlan_mode, gp->fc_mode, ntohl(gp->aen_mode));
+	seq_printf(seq, "MAC: %02x %02x %02x %02x %02x %02x VLAN: %04x\n",
+		   gp->mac[5], gp->mac[4], gp->mac[3], gp->mac[2],
+		   gp->mac[1], gp->mac[0], ntohs(gp->vlan));
+
+	return 0;
+}
+
+static int ncsi_pkt_output_gcps(struct ncsi_dev_priv *ndp,
+				struct seq_file *seq, struct sk_buff *skb)
+{
+	struct ncsi_rsp_gcps_pkt *gcps;
+
+	ncsi_pkt_output_default(ndp, seq, skb);
+
+	gcps = (struct ncsi_rsp_gcps_pkt *)skb_network_header(skb);
+	seq_printf(seq, "cnt_hi: %08x cnt_lo: %08x rx_bytes: %08x\n",
+		   ntohl(gcps->cnt_hi), ntohl(gcps->cnt_lo),
+		   ntohl(gcps->rx_bytes));
+	seq_printf(seq, "tx_bytes: %08x rx_uc_pkts: %08x rx_mc_pkts: %08x\n",
+		   ntohl(gcps->tx_bytes), ntohl(gcps->rx_uc_pkts),
+		   ntohl(gcps->rx_mc_pkts));
+	seq_printf(seq, "rx_bc_pkts: %08x tx_uc_pkts: %08x tx_mc_pkts: %08x\n",
+		   ntohl(gcps->rx_bc_pkts), ntohl(gcps->tx_uc_pkts),
+		   ntohl(gcps->tx_mc_pkts));
+	seq_printf(seq, "tx_bc_pkts: %08x fcs_err: %08x align_err: %08x\n",
+		   ntohl(gcps->tx_bc_pkts), ntohl(gcps->fcs_err),
+		   ntohl(gcps->align_err));
+	seq_printf(seq, "false_carrier: %08x runt_pkts: %08x jabber_pkts: %08x\n",
+		   ntohl(gcps->false_carrier), ntohl(gcps->runt_pkts),
+		   ntohl(gcps->jabber_pkts));
+	seq_printf(seq, "rx_pause_xon: %08x rx_pause_xoff: %08x tx_pause_xon: %08x",
+		   ntohl(gcps->rx_pause_xon), ntohl(gcps->rx_pause_xoff),
+		   ntohl(gcps->tx_pause_xon));
+	seq_printf(seq, "tx_pause_xoff: %08x tx_s_collision: %08x tx_m_collision: %08x\n",
+		   ntohl(gcps->tx_pause_xoff), ntohl(gcps->tx_s_collision),
+		   ntohl(gcps->tx_m_collision));
+	seq_printf(seq, "l_collision: %08x e_collision: %08x rx_ctl_frames: %08x\n",
+		   ntohl(gcps->l_collision), ntohl(gcps->e_collision),
+		   ntohl(gcps->rx_ctl_frames));
+	seq_printf(seq, "rx_64_frames: %08x rx_127_frames: %08x rx_255_frames: %08x\n",
+		   ntohl(gcps->rx_64_frames), ntohl(gcps->rx_127_frames),
+		   ntohl(gcps->rx_255_frames));
+	seq_printf(seq, "rx_511_frames: %08x rx_1023_frames: %08x rx_1522_frames: %08x\n",
+		   ntohl(gcps->rx_511_frames), ntohl(gcps->rx_1023_frames),
+		   ntohl(gcps->rx_1522_frames));
+	seq_printf(seq, "rx_9022_frames: %08x tx_64_frames: %08x tx_127_frames: %08x\n",
+		   ntohl(gcps->rx_9022_frames), ntohl(gcps->tx_64_frames),
+		   ntohl(gcps->tx_127_frames));
+	seq_printf(seq, "tx_255_frames: %08x tx_511_frames: %08x tx_1023_frames: %08x\n",
+		   ntohl(gcps->tx_255_frames), ntohl(gcps->tx_511_frames),
+		   ntohl(gcps->tx_1023_frames));
+	seq_printf(seq, "tx_1522_frames: %08x tx_9022_frames: %08x rx_valid_bytes: %08x\n",
+		   ntohl(gcps->tx_1522_frames), ntohl(gcps->tx_9022_frames),
+		   ntohl(gcps->rx_valid_bytes));
+	seq_printf(seq, "rx_runt_pkts: %08x rx_jabber_pkts: %08x\n",
+		   ntohl(gcps->rx_runt_pkts), ntohl(gcps->rx_jabber_pkts));
+
+	return 0;
+}
+
+static int ncsi_pkt_output_gns(struct ncsi_dev_priv *ndp,
+			       struct seq_file *seq, struct sk_buff *skb)
+{
+	struct ncsi_rsp_gns_pkt *gns;
+
+	ncsi_pkt_output_default(ndp, seq, skb);
+
+	gns = (struct ncsi_rsp_gns_pkt *)skb_network_header(skb);
+	seq_printf(seq, "rx_cmds: %08x dropped_cmds: %08x cmd_type_errs: %08x\n",
+		   ntohl(gns->rx_cmds), ntohl(gns->dropped_cmds),
+		   ntohl(gns->cmd_type_errs));
+	seq_printf(seq, "cmd_csum_errs: %08x rx_pkts: %08x tx_pkts: %08x\n",
+		   ntohl(gns->cmd_csum_errs), ntohl(gns->rx_pkts),
+		   ntohl(gns->tx_pkts));
+	seq_printf(seq, "tx_aen_pkts: %08x\n",
+		   ntohl(gns->tx_aen_pkts));
+
+	return 0;
+}
+
+static int ncsi_pkt_output_gnpts(struct ncsi_dev_priv *ndp,
+				 struct seq_file *seq, struct sk_buff *skb)
+{
+	struct ncsi_rsp_gnpts_pkt *gnpts;
+
+	ncsi_pkt_output_default(ndp, seq, skb);
+
+	gnpts = (struct ncsi_rsp_gnpts_pkt *)skb_network_header(skb);
+	seq_printf(seq, "tx_pkts: %08x tx_dropped: %08x tx_channel_err: %08x\n",
+		   ntohl(gnpts->tx_pkts), ntohl(gnpts->tx_dropped),
+		   ntohl(gnpts->tx_channel_err));
+	seq_printf(seq, "tx_us_err: %08x rx_pkts: %08x rx_dropped: %08x\n",
+		   ntohl(gnpts->tx_us_err), ntohl(gnpts->rx_pkts),
+		   ntohl(gnpts->rx_dropped));
+	seq_printf(seq, "rx_channel_err: %08x rx_us_err: %08x rx_os_err: %08x\n",
+		   ntohl(gnpts->rx_channel_err), ntohl(gnpts->rx_us_err),
+		   ntohl(gnpts->rx_os_err));
+
+	return 0;
+}
+
+static int ncsi_pkt_output_gps(struct ncsi_dev_priv *ndp,
+			       struct seq_file *seq, struct sk_buff *skb)
+{
+	struct ncsi_rsp_gps_pkt *gps;
+
+	ncsi_pkt_output_default(ndp, seq, skb);
+
+	gps = (struct ncsi_rsp_gps_pkt *)skb_network_header(skb);
+	seq_printf(seq, "Status: %08x\n", ntohl(gps->status));
+
+	return 0;
+}
+
+static int ncsi_pkt_output_gpuuid(struct ncsi_dev_priv *ndp,
+				  struct seq_file *seq, struct sk_buff *skb)
+{
+	struct ncsi_rsp_gpuuid_pkt *gpuuid;
+
+	ncsi_pkt_output_default(ndp, seq, skb);
+
+	gpuuid = (struct ncsi_rsp_gpuuid_pkt *)skb_network_header(skb);
+	seq_printf(seq, "UUID: %02x %02x %02x %02x %02x %02x %02x %02x\n",
+		   gpuuid->uuid[15], gpuuid->uuid[14], gpuuid->uuid[13],
+		   gpuuid->uuid[12], gpuuid->uuid[11], gpuuid->uuid[10],
+		   gpuuid->uuid[9],  gpuuid->uuid[8]);
+	seq_printf(seq, "      %02x %02x %02x %02x %02x %02x %02x %02x\n",
+		   gpuuid->uuid[7], gpuuid->uuid[6], gpuuid->uuid[5],
+		   gpuuid->uuid[4], gpuuid->uuid[3], gpuuid->uuid[2],
+		   gpuuid->uuid[1], gpuuid->uuid[0]);
+
+	return 0;
+}
+
+static const struct ncsi_pkt_handler {
+	unsigned char	type;
+	const char	*name;
+	int		(*input)(struct ncsi_dev_priv *ndp,
+				 struct ncsi_cmd_arg *nca, char *buf);
+	int		(*output)(struct ncsi_dev_priv *ndp,
+				  struct seq_file *seq, struct sk_buff *skb);
+} ncsi_pkt_handlers[] = {
+	{ NCSI_PKT_CMD_CIS,    "CIS",
+	  ncsi_pkt_input_default, ncsi_pkt_output_default },
+	{ NCSI_PKT_CMD_SP,     "SP",
+	  ncsi_pkt_input_sp,      ncsi_pkt_output_default },
+	{ NCSI_PKT_CMD_DP,     "DP",
+	  ncsi_pkt_input_default, ncsi_pkt_output_default },
+	{ NCSI_PKT_CMD_EC,     "EC",
+	  ncsi_pkt_input_default, ncsi_pkt_output_default },
+	{ NCSI_PKT_CMD_DC,     "DC",
+	  ncsi_pkt_input_dc,      ncsi_pkt_output_default },
+	{ NCSI_PKT_CMD_RC,     "RC",
+	  ncsi_pkt_input_default, ncsi_pkt_output_default },
+	{ NCSI_PKT_CMD_ECNT,   "ECNT",
+	  ncsi_pkt_input_default, ncsi_pkt_output_default },
+	{ NCSI_PKT_CMD_DCNT,   "DCNT",
+	  ncsi_pkt_input_default, ncsi_pkt_output_default },
+	{ NCSI_PKT_CMD_AE,     "AE",
+	  ncsi_pkt_input_ae,      ncsi_pkt_output_default },
+	{ NCSI_PKT_CMD_SL,     "SL",
+	  ncsi_pkt_input_sl,      ncsi_pkt_output_default },
+	{ NCSI_PKT_CMD_GLS,    "GLS",
+	  ncsi_pkt_input_default, ncsi_pkt_output_gls     },
+	{ NCSI_PKT_CMD_SVF,    "SVF",
+	  ncsi_pkt_input_svf,     ncsi_pkt_output_default },
+	{ NCSI_PKT_CMD_EV,     "EV",
+	  ncsi_pkt_input_ev,      ncsi_pkt_output_default },
+	{ NCSI_PKT_CMD_DV,     "DV",
+	  ncsi_pkt_input_default, ncsi_pkt_output_default },
+	{ NCSI_PKT_CMD_SMA,    "SMA",
+	  ncsi_pkt_input_sma,     ncsi_pkt_output_default },
+	{ NCSI_PKT_CMD_EBF,    "EBF",
+	  ncsi_pkt_input_ebf,     ncsi_pkt_output_default },
+	{ NCSI_PKT_CMD_DBF,    "DBF",
+	  ncsi_pkt_input_default, ncsi_pkt_output_default },
+	{ NCSI_PKT_CMD_EGMF,   "EGMF",
+	  ncsi_pkt_input_egmf,    ncsi_pkt_output_default },
+	{ NCSI_PKT_CMD_DGMF,   "DGMF",
+	  ncsi_pkt_input_default, ncsi_pkt_output_default },
+	{ NCSI_PKT_CMD_SNFC,   "SNFC",
+	  ncsi_pkt_input_snfc,    ncsi_pkt_output_default },
+	{ NCSI_PKT_CMD_GVI,    "GVI",
+	  ncsi_pkt_input_default, ncsi_pkt_output_gvi     },
+	{ NCSI_PKT_CMD_GC,     "GC",
+	  ncsi_pkt_input_default, ncsi_pkt_output_gc      },
+	{ NCSI_PKT_CMD_GP,     "GP",
+	  ncsi_pkt_input_default, ncsi_pkt_output_gp      },
+	{ NCSI_PKT_CMD_GCPS,   "GCPS",
+	  ncsi_pkt_input_default, ncsi_pkt_output_gcps    },
+	{ NCSI_PKT_CMD_GNS,    "GNS",
+	  ncsi_pkt_input_default, ncsi_pkt_output_gns     },
+	{ NCSI_PKT_CMD_GNPTS,  "GNPTS",
+	  ncsi_pkt_input_default, ncsi_pkt_output_gnpts   },
+	{ NCSI_PKT_CMD_GPS,    "GPS",
+	  ncsi_pkt_input_default, ncsi_pkt_output_gps     },
+	{ NCSI_PKT_CMD_OEM,    "OEM",
+	  NULL,                   NULL                    },
+	{ NCSI_PKT_CMD_PLDM,   "PLDM",
+	  NULL,                   NULL                    },
+	{ NCSI_PKT_CMD_GPUUID, "GPUUID",
+	  ncsi_pkt_input_default, ncsi_pkt_output_gpuuid  },
+};
+
+static const char *ncsi_pkt_type_name(unsigned int type)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ncsi_pkt_handlers); i++) {
+		if (ncsi_pkt_handlers[i].type == type)
+			return ncsi_pkt_handlers[i].name;
+	}
+
+	return "N/A";
+}
+
+static int ncsi_dev_pkt_seq_show(struct seq_file *seq, void *v)
+{
+	struct ncsi_dev_priv *ndp = seq->private;
+	const struct ncsi_pkt_handler *h = NULL;
+	struct ncsi_pkt_hdr *hdr;
+	struct sk_buff *skb;
+	int i, errno;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ndp->lock, flags);
+	errno = ndp->pkt.errno;
+	skb = ndp->pkt.rsp;
+	ndp->pkt.rsp = NULL;
+	spin_unlock_irqrestore(&ndp->lock, flags);
+	ncsi_dev_reset_debug_pkt(ndp, NULL, 0);
+
+	if (errno) {
+		WARN_ON(skb);
+		seq_printf(seq, "Error %d receiving response packet\n", errno);
+		return 0;
+	} else if (!skb) {
+		seq_puts(seq, "No available response packet\n");
+		return 0;
+	}
+
+	hdr = (struct ncsi_pkt_hdr *)skb_network_header(skb);
+	for (i = 0; i < ARRAY_SIZE(ncsi_pkt_handlers); i++) {
+		if (ncsi_pkt_handlers[i].type == (hdr->type - 0x80)) {
+			h = &ncsi_pkt_handlers[i];
+			break;
+		}
+	}
+
+	if (!h || !h->output) {
+		consume_skb(skb);
+		return 0;
+	}
+
+	h->output(ndp, seq, skb);
+	return 0;
+}
+
+static int ncsi_dev_pkt_seq_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, ncsi_dev_pkt_seq_show, inode->i_private);
+}
+
+static ssize_t ncsi_dev_pkt_seq_write(struct file *file,
+				      const char __user *buffer,
+				      size_t count, loff_t *pos)
+{
+	struct seq_file *seq = file->private_data;
+	struct ncsi_dev_priv *ndp = seq->private;
+	struct ncsi_cmd_arg nca;
+	char buf[64], name[64], *pbuf;
+	const struct ncsi_pkt_handler *h;
+	int num, package, channel, i, ret;
+	unsigned long flags;
+
+	if (count >= sizeof(buf))
+		return -EINVAL;
+
+	/* Copy the buffer from user space. Currently we have 64 bytes as
+	 * the length limitation. It should be enough as there are no bunch
+	 * of parameters to be specified when sending NCSI command packet.
+	 */
+	memset(buf, 0, sizeof(buf));
+	if (copy_from_user(buf, buffer, count))
+		return -EFAULT;
+
+	/* Extract the specified command */
+	memset(name, 0, sizeof(name));
+	pbuf = strchr(buf, ',');
+	if (!pbuf)
+		return -EINVAL;
+	memcpy(name, buf, pbuf - buf);
+	pbuf++;
+
+	/* Extract mandatory parameters: package and channel ID */
+	memset(&nca, 0, sizeof(struct ncsi_cmd_arg));
+	if (sscanf(pbuf, "%x,%x%n", &package, &channel, &num) != 2)
+		return -EINVAL;
+	if (package < 0 || package >= 8 ||
+	    channel < 0 || channel > NCSI_RESERVED_CHANNEL)
+		return -EINVAL;
+
+	nca.package = package;
+	nca.channel = channel;
+	if (pbuf[num] == ',')
+		pbuf += (count + 1);
+	else
+		pbuf += count;
+
+	/* Search for handler */
+	h = NULL;
+	for (i = 0; i < ARRAY_SIZE(ncsi_pkt_handlers); i++) {
+		if (!strcmp(ncsi_pkt_handlers[i].name, name)) {
+			h = &ncsi_pkt_handlers[i];
+			nca.type = h->type;
+			break;
+		}
+	}
+
+	if (!h || !h->input)
+		return -ERANGE;
+
+	/* Sort out additional parameters */
+	nca.ndp = ndp;
+	nca.req_flags = NCSI_REQ_FLAG_DEBUG;
+	ret = h->input(ndp, &nca, pbuf);
+	if (ret)
+		return ret;
+
+	/* This interface works in serialized fashion, meaning new command
+	 * cannot be sent until previous one has been finalized.
+	 */
+	spin_lock_irqsave(&ndp->lock, flags);
+	if (ndp->pkt.req != NCSI_PKT_REQ_FREE) {
+		spin_unlock_irqrestore(&ndp->lock, flags);
+		return -EBUSY;
+	}
+
+	ndp->pkt.req = NCSI_PKT_REQ_BUSY;
+	spin_unlock_irqrestore(&ndp->lock, flags);
+
+	ret = ncsi_xmit_cmd(&nca);
+	if (ret) {
+		spin_lock_irqsave(&ndp->lock, flags);
+		ndp->pkt.req = NCSI_PKT_REQ_FREE;
+		spin_unlock_irqrestore(&ndp->lock, flags);
+		return ret;
+	}
+
+	return count;
+}
+
+static const struct file_operations ncsi_dev_pkt_fops = {
+	.owner   = THIS_MODULE,
+	.open    = ncsi_dev_pkt_seq_open,
+	.read    = seq_read,
+	.write   = ncsi_dev_pkt_seq_write,
+	.llseek  = seq_lseek,
+	.release = seq_release,
+};
+
+int ncsi_dev_init_debug(struct ncsi_dev_priv *ndp)
+{
+	if (WARN_ON_ONCE(ndp->dentry))
+		return 0;
+
+	if (!ncsi_dentry) {
+		ncsi_dentry = debugfs_create_dir("ncsi", NULL);
+		if (!ncsi_dentry) {
+			pr_debug("Failed to create debugfs directory 'ncsi'\n");
+			return -ENOMEM;
+		}
+	}
+
+	ndp->dentry = debugfs_create_dir(netdev_name(ndp->ndev.dev),
+					 ncsi_dentry);
+	if (!ndp->dentry) {
+		pr_debug("Failed to create debugfs directory 'ncsi/%s'\n",
+			 netdev_name(ndp->ndev.dev));
+		return -ENOMEM;
+	}
+
+	ndp->pkt.dentry = debugfs_create_file("pkt", 0600, ndp->dentry,
+					      ndp, &ncsi_dev_pkt_fops);
+	if (!ndp->pkt.dentry) {
+		pr_debug("Failed to create debugfs file 'ncsi/%s/pkt'\n",
+			 netdev_name(ndp->ndev.dev));
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+void ncsi_dev_release_debug(struct ncsi_dev_priv *ndp)
+{
+	debugfs_remove(ndp->pkt.dentry);
+	debugfs_remove(ndp->dentry);
+}
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 8365a5b..ea8ea0e 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -537,6 +537,8 @@ static void ncsi_request_timeout(unsigned long data)
 
 	hdr = (struct ncsi_pkt_hdr *)skb_network_header(nr->cmd);
 	ncsi_dev_update_stats(ndp, hdr->type, 0, ETHTOOL_NCSI_SW_STAT_TIMEOUT);
+	if (ncsi_dev_is_debug_pkt(ndp, nr))
+		ncsi_dev_reset_debug_pkt(ndp, NULL, -ETIMEDOUT);
 
 	/* Release the request */
 	ncsi_free_request(nr);
@@ -1287,6 +1289,13 @@ int ncsi_start_dev(struct ncsi_dev *nd)
 		return -ENOTTY;
 
 	if (!(ndp->flags & NCSI_DEV_PROBED)) {
+		/* The debugging functionality should have been initialized
+		 * when registerring the NCSI device. As the network device
+		 * name isn't available that time, we have to delay the work
+		 * to here.
+		 */
+		ncsi_dev_init_debug(ndp);
+
 		nd->state = ncsi_dev_state_probe;
 		schedule_work(&ndp->work);
 		return 0;
@@ -1336,6 +1345,7 @@ void ncsi_unregister_dev(struct ncsi_dev *nd)
 	struct ncsi_package *np, *tmp;
 	unsigned long flags;
 
+	ncsi_dev_release_debug(ndp);
 	dev_remove_pack(&ndp->ptype);
 
 	/* Restore ethtool operations */
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index d362d2c..41479a4 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -1033,10 +1033,21 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
 	if (ret) {
 		ncsi_dev_update_stats(ndp, hdr->type, 0,
 				      ETHTOOL_NCSI_SW_STAT_ERROR);
+		if (ncsi_dev_is_debug_pkt(ndp, nr))
+			ncsi_dev_reset_debug_pkt(ndp, NULL, -EINVAL);
+
 		goto out;
 	}
 
 	/* Process the packet */
+	if (ncsi_dev_is_debug_pkt(ndp, nr)) {
+		ncsi_dev_update_stats(ndp, hdr->type, 0,
+				      ETHTOOL_NCSI_SW_STAT_OK);
+		ncsi_dev_reset_debug_pkt(ndp, nr->rsp, 0);
+		nr->rsp = NULL;
+		goto out;
+	}
+
 	ret = nrh->handler(nr);
 	if (!ret) {
 		ncsi_dev_update_stats(ndp, hdr->type, 0,
-- 
2.7.4

Re: [PATCH v4 net-next 08/10] net/ncsi: Support NCSI packet generation

[Andrew Lunn]

On Wed, May 03, 2017 at 02:44:39PM +1000, Gavin Shan wrote:
> This introduces /sys/kernel/debug/ncsi/eth0/pkt. The debugfs entry
> can accept parameters to produce NCSI command packet. The received
> NCSI response packet is dumped on read. Below is an example to send
> CIS command and dump its response.
> 
>    # echo CIS,0,0 > /sys/kernel/debug/ncsi/eth0/pkt
>    # cat /sys/kernel/debug/ncsi/eth0/pkt
>    NCSI response [CIS] packet received
> 
>    00 01 dd 80 00 0004 0000 0000

Hi Gavin

Could this be done with a raw socket for Tx and
libpcap/tcpdump/wireshart for Rx?

      Andrew

Re: [PATCH v4 net-next 08/10] net/ncsi: Support NCSI packet generation

[Gavin Shan]

On Wed, May 03, 2017 at 02:52:54PM +0200, Andrew Lunn wrote:
>On Wed, May 03, 2017 at 02:44:39PM +1000, Gavin Shan wrote:
>> This introduces /sys/kernel/debug/ncsi/eth0/pkt. The debugfs entry
>> can accept parameters to produce NCSI command packet. The received
>> NCSI response packet is dumped on read. Below is an example to send
>> CIS command and dump its response.
>> 
>>    # echo CIS,0,0 > /sys/kernel/debug/ncsi/eth0/pkt
>>    # cat /sys/kernel/debug/ncsi/eth0/pkt
>>    NCSI response [CIS] packet received
>> 
>>    00 01 dd 80 00 0004 0000 0000
>
>Could this be done with a raw socket for Tx and
>libpcap/tcpdump/wireshart for Rx?
>

Andrew, it's really good question. Unfortunately, I don't think it can
be done solely by raw socket to transmit NCSI command packet because the
[packet sequence number] field in the packet can't be same to any used ones.
Otherwise the remote NIC will be confused and start to reponse abnormally.

We could reserve some sequence number to be used by raw socket. However, to
avoid  duplicated packet sequence number from raw socket should be done. I
think it's overall more complexed than current implementation (debugfs). Also,
it's going to introduce protocol specific rules to raw socket implementation.
I'm not sure it's worthy.

Cheers,
Gavin

Re: [PATCH v4 net-next 08/10] net/ncsi: Support NCSI packet generation

[Andrew Lunn]

On Thu, May 04, 2017 at 04:31:57PM +1000, Gavin Shan wrote:
> On Wed, May 03, 2017 at 02:52:54PM +0200, Andrew Lunn wrote:
> >On Wed, May 03, 2017 at 02:44:39PM +1000, Gavin Shan wrote:
> >> This introduces /sys/kernel/debug/ncsi/eth0/pkt. The debugfs entry
> >> can accept parameters to produce NCSI command packet. The received
> >> NCSI response packet is dumped on read. Below is an example to send
> >> CIS command and dump its response.
> >> 
> >>    # echo CIS,0,0 > /sys/kernel/debug/ncsi/eth0/pkt
> >>    # cat /sys/kernel/debug/ncsi/eth0/pkt
> >>    NCSI response [CIS] packet received
> >> 
> >>    00 01 dd 80 00 0004 0000 0000
> >
> >Could this be done with a raw socket for Tx and
> >libpcap/tcpdump/wireshart for Rx?
> >
> 
> Andrew, it's really good question. Unfortunately, I don't think it can
> be done solely by raw socket to transmit NCSI command packet because the
> [packet sequence number] field in the packet can't be same to any used ones.
> Otherwise the remote NIC will be confused and start to reponse abnormally.

Just to make sure i'm on the same page. We are talking about:

https://www.dmtf.org/sites/default/files/standards/documents/DSP0222_1.0.1.pdf

and the sequence number is the Instance ID. This is an 8-bit number,
and if it receives a message with the previously used IID, it should
assume it is a re-transmit of the request and send back the old
response. It is a very simple scheme, no windowing, only one
outstanding command/response, just one response buffered in case of a
re-transmit.

> We could reserve some sequence number to be used by raw socket.

I don't think that works. You can only have one command
'inflight'. Packets from a raw socket would have to go through your
state machine. Which makes it complex.

libpcap should however still work. So you should probably do all the
receive side in user space.

      Andrew

Re: [PATCH v4 net-next 08/10] net/ncsi: Support NCSI packet generation

[Gavin Shan]

On Thu, May 04, 2017 at 02:00:35PM +0200, Andrew Lunn wrote:
>On Thu, May 04, 2017 at 04:31:57PM +1000, Gavin Shan wrote:
>> On Wed, May 03, 2017 at 02:52:54PM +0200, Andrew Lunn wrote:
>> >On Wed, May 03, 2017 at 02:44:39PM +1000, Gavin Shan wrote:
>> >> This introduces /sys/kernel/debug/ncsi/eth0/pkt. The debugfs entry
>> >> can accept parameters to produce NCSI command packet. The received
>> >> NCSI response packet is dumped on read. Below is an example to send
>> >> CIS command and dump its response.
>> >> 
>> >>    # echo CIS,0,0 > /sys/kernel/debug/ncsi/eth0/pkt
>> >>    # cat /sys/kernel/debug/ncsi/eth0/pkt
>> >>    NCSI response [CIS] packet received
>> >> 
>> >>    00 01 dd 80 00 0004 0000 0000
>> >
>> >Could this be done with a raw socket for Tx and
>> >libpcap/tcpdump/wireshart for Rx?
>> >
>> 
>> Andrew, it's really good question. Unfortunately, I don't think it can
>> be done solely by raw socket to transmit NCSI command packet because the
>> [packet sequence number] field in the packet can't be same to any used ones.
>> Otherwise the remote NIC will be confused and start to reponse abnormally.
>
>Just to make sure i'm on the same page. We are talking about:
>
>https://www.dmtf.org/sites/default/files/standards/documents/DSP0222_1.0.1.pdf
>
>and the sequence number is the Instance ID. This is an 8-bit number,
>and if it receives a message with the previously used IID, it should
>assume it is a re-transmit of the request and send back the old
>response. It is a very simple scheme, no windowing, only one
>outstanding command/response, just one response buffered in case of a
>re-transmit.
>
>> We could reserve some sequence number to be used by raw socket.
>
>I don't think that works. You can only have one command
>'inflight'. Packets from a raw socket would have to go through your
>state machine. Which makes it complex.
>
>libpcap should however still work. So you should probably do all the
>receive side in user space.
>

Andrew, yeah, we're on same page about the sequence number. Yes, I
agree it's going to make thing complex to transmit packet through
raw socket. So lets keep using debugfs as we had.

I need to dig how libpcap receives packets. It's appreciated if you
can give some hints about that. However, I don't see the benefit to
receive packets by libpcap, could you claim?

Cheers,
Gavin

Re: [PATCH v4 net-next 08/10] net/ncsi: Support NCSI packet generation

[Andrew Lunn]

> I need to dig how libpcap receives packets. It's appreciated if you
> can give some hints about that. However, I don't see the benefit to
> receive packets by libpcap, could you claim?

The base interface should already be doing it for you. Try it! Run
tcpdump or wireshark and you should see the NCSI packets going
out/coming in. Look for the ethertype. Neither tcpdump or wireshark
appear to have dissectors for NCSI, but it should be simple to
write. I've written them before, and it is easy.

Doing it in userspace will give you a much nice environment to work
in. Wireshark can link the response to the request, do a much more
detailed decode than what you want to do in kernel space, and in
general it is safer. Wrongly decoding and printing protocols in kernel
space can lead to a remote kernel vulnerability. Getting it wrong in
user space 'just' allows a remote hack of a user account.

     Andrew

Re: [PATCH v4 net-next 08/10] net/ncsi: Support NCSI packet generation

[Gavin Shan]

On Mon, May 08, 2017 at 02:56:32AM +0200, Andrew Lunn wrote:
>> I need to dig how libpcap receives packets. It's appreciated if you
>> can give some hints about that. However, I don't see the benefit to
>> receive packets by libpcap, could you claim?
>
>The base interface should already be doing it for you. Try it! Run
>tcpdump or wireshark and you should see the NCSI packets going
>out/coming in. Look for the ethertype. Neither tcpdump or wireshark
>appear to have dissectors for NCSI, but it should be simple to
>write. I've written them before, and it is easy.
>
>Doing it in userspace will give you a much nice environment to work
>in. Wireshark can link the response to the request, do a much more
>detailed decode than what you want to do in kernel space, and in
>general it is safer. Wrongly decoding and printing protocols in kernel
>space can lead to a remote kernel vulnerability. Getting it wrong in
>user space 'just' allows a remote hack of a user account.
>

Andrew, thanks a lot for the hints. Yeah, I think it's implemented based
on AF_PACKET + ETH_P_ALL. NCSI packets has been included.

The output from (libpcap and debugfs) will be different, but I assume it's
not a problem. libpcap usually outputs all Tx/Rx NCSI packets while debugfs
file outputs received NCSI response packets that correspond to the NCSI command
packets sent via the debugfs interface.

In next respin, I'll move the Rx path to libpcap while the Tx is left in
the debugfs interace.

Cheers,
Gavin

[PATCH v4 net-next 09/10] net/ncsi: No error report on DP response to non-existing package

[Gavin Shan]

The issue was found from /sys/kernel/debug/ncsi/eth0/stats. The
first step in NCSI package/channel enumeration is deselect all
packages by sending DP (Deselect Package) commands. The remote
NIC replies with response while the corresponding package isn't
populated yet and it is treated as an error wrongly.

 # ethtool --ncsi eth0 swstats
     :
 RESPONSE     OK       TIMEOUT  ERROR
 =======================================
 DP           2        0        1

This fixes the issue by ignoring the error in DP response handler,
when the corresponding package isn't existing. With this applied,
no error reported from DP response packets.

 # ethtool --ncsi eth0 swstats
     :
 RESPONSE     OK       TIMEOUT  ERROR
 =======================================
 DP           3        0        0

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 net/ncsi/ncsi-rsp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 41479a4..5097d86 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -118,7 +118,7 @@ static int ncsi_rsp_handler_dp(struct ncsi_request *nr)
 	ncsi_find_package_and_channel(ndp, rsp->rsp.common.channel,
 				      &np, NULL);
 	if (!np)
-		return -ENODEV;
+		return 0;
 
 	/* Change state of all channels attached to the package */
 	NCSI_FOR_EACH_CHANNEL(np, nc) {
-- 
2.7.4

[PATCH v4 net-next 10/10] net/ncsi: Fix length of GVI response packet

[Gavin Shan]

The length of GVI (GetVersionInfo) response packet should be 40 instead
of 36. This issue was found from /sys/kernel/debug/ncsi/eth0/stats.

 # ethtool --ncsi eth0 swstats
     :
 RESPONSE     OK       TIMEOUT  ERROR
 =======================================
 GVI          0        0        2

With this applied, no error reported on GVI response packets:

 # ethtool --ncsi eth0 swstats
     :
 RESPONSE     OK       TIMEOUT  ERROR
 =======================================
 GVI          2        0        0

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 net/ncsi/ncsi-rsp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 5097d86..a3e0f4f 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -951,7 +951,7 @@ static struct ncsi_rsp_handler {
 	{ NCSI_PKT_RSP_EGMF,    4, ncsi_rsp_handler_egmf    },
 	{ NCSI_PKT_RSP_DGMF,    4, ncsi_rsp_handler_dgmf    },
 	{ NCSI_PKT_RSP_SNFC,    4, ncsi_rsp_handler_snfc    },
-	{ NCSI_PKT_RSP_GVI,    36, ncsi_rsp_handler_gvi     },
+	{ NCSI_PKT_RSP_GVI,    40, ncsi_rsp_handler_gvi     },
 	{ NCSI_PKT_RSP_GC,     32, ncsi_rsp_handler_gc      },
 	{ NCSI_PKT_RSP_GP,     -1, ncsi_rsp_handler_gp      },
 	{ NCSI_PKT_RSP_GCPS,  172, ncsi_rsp_handler_gcps    },
-- 
2.7.4

Re: [PATCH v4 net-next 00/10] net/ncsi: Add debugging functionality

[David Miller]

Sorry, the net-next tree is closed right now as we are in the merge
window.

Please resubmit this when the net-next tree opens back up.

Thank you.

Re: [PATCH v4 net-next 00/10] net/ncsi: Add debugging functionality

[Gavin Shan]

On Wed, May 03, 2017 at 01:25:18AM -0400, David Miller wrote:
>
>Sorry, the net-next tree is closed right now as we are in the merge
>window.
>
>Please resubmit this when the net-next tree opens back up.
>
>Thank you.
>

Sorry that I didn't catch the merge window. Thanks for the tip. I'll
resubmit after net-next is open again.

Cheers,
Gavin

Re: [PATCH v4 net-next 00/10] net/ncsi: Add debugging functionality

[Andrew Lunn]

On Wed, May 03, 2017 at 02:44:31PM +1000, Gavin Shan wrote:
> This series supports NCSI debugging infrastructure by adding several
> ethtool commands and one debugfs file. It was inspired by the reported
> issues: No available package and channel are probed successfully.
> Obviously, we don't have a debugging infrastructure for NCSI stack yet.
> 
> The first 3 patches, fixing some issues, aren't relevant to the
> subject. 

...

> PATCH[9,10] fix two issues found by the debugging functionality.

Hi Gavin

If they are real fixes, you should submit them separately. Please
include Fixes: tags, and indicate if they need to be included in
-stable.

David will probably accept fixes while the merge window is closed.

	Andrew

Re: [PATCH v4 net-next 00/10] net/ncsi: Add debugging functionality

[Gavin Shan]

On Wed, May 03, 2017 at 02:58:02PM +0200, Andrew Lunn wrote:
>On Wed, May 03, 2017 at 02:44:31PM +1000, Gavin Shan wrote:
>> This series supports NCSI debugging infrastructure by adding several
>> ethtool commands and one debugfs file. It was inspired by the reported
>> issues: No available package and channel are probed successfully.
>> Obviously, we don't have a debugging infrastructure for NCSI stack yet.
>> 
>> The first 3 patches, fixing some issues, aren't relevant to the
>> subject. 
>
>...
>
>> PATCH[9,10] fix two issues found by the debugging functionality.
>
>Hi Gavin
>
>If they are real fixes, you should submit them separately. Please
>include Fixes: tags, and indicate if they need to be included in
>-stable.
>
>David will probably accept fixes while the merge window is closed.
>
>	Andrew
>

Andrew, thanks a lot for the good tip. Yeah, I agree and will follow
in next respin. I think the patches fixing issues should be rebased
to "net" and reposted?

git://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git

Cheers,
Gavin