[NET_NEXT RFC PATCH 0/3] Series short description

[NET_NEXT RFC PATCH 0/3] Series short description

[Catherine Sullivan]

The following patches add debugfs support to the ixgbe driver to give users the
ability to call functions registered in netdev_ops (simulating kernel actions)
as well as the ability to read and write to individual HW registers on the fly.
These options can be useful when debugging and unit testing ixgbe.

The following series implements...

---

Catherine Sullivan (3):
      ixgbe: added reg_ops file to debugfs
      ixgbe: added netdev_ops file to debugfs
      ixgbe: add debugfs support


 drivers/net/ethernet/intel/ixgbe/Makefile        |    2 
 drivers/net/ethernet/intel/ixgbe/ixgbe.h         |   10 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c |  293 ++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    |   17 +
 4 files changed, 320 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c

-- 
Catherine Sullivan

[NET_NEXT RFC PATCH 1/3] ixgbe: add debugfs support

[Catherine Sullivan]

This patch adds debugfs support to the ixgbe driver to give
users the ability to access kernel information and to
simulate kernel events.

The filesystem is set up in the following driver/PCI-instance
hierarchy:
<debugfs>
   |-- ixgbe
	|-- PCI instance
	|	|-- attribute files
	|-- PCI instance
		|-- attribute files

Signed-off-by: Catherine Sullivan <catherine.sullivan@intel.com>
---

 drivers/net/ethernet/intel/ixgbe/Makefile        |    2 -
 drivers/net/ethernet/intel/ixgbe/ixgbe.h         |   10 ++-
 drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c |   82 ++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    |   17 +++++
 4 files changed, 109 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c

diff --git a/drivers/net/ethernet/intel/ixgbe/Makefile b/drivers/net/ethernet/intel/ixgbe/Makefile
index 9cfbf35..a11c8ef 100644
--- a/drivers/net/ethernet/intel/ixgbe/Makefile
+++ b/drivers/net/ethernet/intel/ixgbe/Makefile
@@ -32,7 +32,7 @@
 
 obj-$(CONFIG_IXGBE) += ixgbe.o
 
-ixgbe-objs := ixgbe_main.o ixgbe_common.o ixgbe_ethtool.o \
+ixgbe-objs := ixgbe_main.o ixgbe_common.o ixgbe_ethtool.o ixgbe_debugfs.o\
               ixgbe_82599.o ixgbe_82598.o ixgbe_phy.o ixgbe_sriov.o \
               ixgbe_mbx.o ixgbe_x540.o ixgbe_sysfs.o ixgbe_lib.o
 
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index bd5885a..f3f4dd4 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -583,6 +583,9 @@ struct ixgbe_adapter {
 #ifdef CONFIG_IXGBE_HWMON
 	struct hwmon_buff ixgbe_hwmon_buff;
 #endif /* CONFIG_IXGBE_HWMON */
+#ifdef CONFIG_DEBUG_FS
+	struct dentry *ixgbe_dbg_adapter;
+#endif /*CONFIG_DEBUG_FS*/
 };
 
 struct ixgbe_fdir_filter {
@@ -711,7 +714,12 @@ extern int ixgbe_fcoe_get_hbainfo(struct net_device *netdev,
 				  struct netdev_fcoe_hbainfo *info);
 extern u8 ixgbe_fcoe_get_tc(struct ixgbe_adapter *adapter);
 #endif /* IXGBE_FCOE */
-
+#ifdef CONFIG_DEBUG_FS
+extern void ixgbe_dbg_adapter_init(struct ixgbe_adapter *adapter);
+extern void ixgbe_dbg_adapter_exit(struct ixgbe_adapter *adapter);
+extern void ixgbe_dbg_init(void);
+extern void ixgbe_dbg_exit(void);
+#endif /* CONFIG_DEBUG_FS */
 static inline struct netdev_queue *txring_txq(const struct ixgbe_ring *ring)
 {
 	return netdev_get_tx_queue(ring->netdev, ring->queue_index);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c
new file mode 100644
index 0000000..423700f
--- /dev/null
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c
@@ -0,0 +1,82 @@
+/*******************************************************************************
+
+  Intel 10 Gigabit PCI Express Linux driver
+  Copyright(c) 1999 - 2012 Intel Corporation.
+
+  This program is free software; you can redistribute it and/or modify it
+  under the terms and conditions of the GNU General Public License,
+  version 2, as published by the Free Software Foundation.
+
+  This program is distributed in the hope it will be useful, but WITHOUT
+  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+  FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+  more details.
+
+  You should have received a copy of the GNU General Public License along with
+  this program; if not, write to the Free Software Foundation, Inc.,
+  51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+
+  The full GNU General Public License is included in this distribution in
+  the file called "COPYING".
+
+  Contact Information:
+  e1000-devel Mailing List <e1000-devel@lists.sourceforge.net>
+  Intel Corporation, 5200 N.E. Elam Young Parkway, Hillsboro, OR 97124-6497
+
+*******************************************************************************/
+
+#ifdef CONFIG_DEBUG_FS
+
+#include <linux/debugfs.h>
+#include <linux/module.h>
+
+#include "ixgbe.h"
+
+static struct dentry *ixgbe_dbg_root;
+
+/**
+ * ixgbe_dbg_adapter_init - setup the debugfs directory for the adapter
+ * @adapter: the adapter that is starting up
+ **/
+void ixgbe_dbg_adapter_init(struct ixgbe_adapter *adapter)
+{
+	const char *name = pci_name(adapter->pdev);
+
+	adapter->ixgbe_dbg_adapter = debugfs_create_dir(name, ixgbe_dbg_root);
+	if (adapter->ixgbe_dbg_adapter) {
+		/* create files here */
+	} else {
+		e_dev_err("debugfs entry for %s failed\n", name);
+	}
+}
+
+/**
+ * ixgbe_dbg_adapter_exit - clear out the adapter's debugfs entries
+ * @pf: the pf that is stopping
+ **/
+void ixgbe_dbg_adapter_exit(struct ixgbe_adapter *adapter)
+{
+	if (adapter->ixgbe_dbg_adapter)
+		debugfs_remove_recursive(adapter->ixgbe_dbg_adapter);
+	adapter->ixgbe_dbg_adapter = NULL;
+}
+
+/**
+ * ixgbe_dbg_init - start up debugfs for the driver
+ **/
+void ixgbe_dbg_init(void)
+{
+	ixgbe_dbg_root = debugfs_create_dir(ixgbe_driver_name, NULL);
+	if (ixgbe_dbg_root == NULL)
+		pr_err("init of debugfs failed\n");
+}
+
+/**
+ * ixgbe_dbg_exit - clean out the driver's debugfs entries
+ **/
+void ixgbe_dbg_exit(void)
+{
+	debugfs_remove_recursive(ixgbe_dbg_root);
+}
+
+#endif /* CONFIG_DEBUG_FS */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 7910000..503dcb6 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7383,6 +7383,10 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
 		e_err(probe, "failed to allocate sysfs resources\n");
 #endif /* CONFIG_IXGBE_HWMON */
 
+#ifdef CONFIG_DEBUG_FS
+	ixgbe_dbg_adapter_init(adapter);
+#endif /* CONFIG_DEBUG_FS */
+
 	return 0;
 
 err_register:
@@ -7417,6 +7421,10 @@ static void __devexit ixgbe_remove(struct pci_dev *pdev)
 	struct ixgbe_adapter *adapter = pci_get_drvdata(pdev);
 	struct net_device *netdev = adapter->netdev;
 
+#ifdef CONFIG_DEBUG_FS
+	ixgbe_dbg_adapter_exit(adapter);
+#endif /*CONFIG_DEBUG_FS */
+
 	set_bit(__IXGBE_DOWN, &adapter->state);
 	cancel_work_sync(&adapter->service_task);
 
@@ -7672,6 +7680,10 @@ static int __init ixgbe_init_module(void)
 	pr_info("%s - version %s\n", ixgbe_driver_string, ixgbe_driver_version);
 	pr_info("%s\n", ixgbe_copyright);
 
+#ifdef CONFIG_DEBUG_FS
+	ixgbe_dbg_init();
+#endif /* CONFIG_DEBUG_FS */
+
 #ifdef CONFIG_IXGBE_DCA
 	dca_register_notify(&dca_notifier);
 #endif
@@ -7694,6 +7706,11 @@ static void __exit ixgbe_exit_module(void)
 	dca_unregister_notify(&dca_notifier);
 #endif
 	pci_unregister_driver(&ixgbe_driver);
+
+#ifdef CONFIG_DEBUG_FS
+	ixgbe_dbg_exit();
+#endif /* CONFIG_DEBUG_FS */
+
 	rcu_barrier(); /* Wait for completion of call_rcu()'s */
 }
 

[NET_NEXT RFC PATCH 2/3] ixgbe: added netdev_ops file to debugfs

[Catherine Sullivan]

Added the netdev_ops file to debugfs with a command to call the
ndo_tx_timeout function to give users the ability to simulate a
tx_timeout call made by the kernel.

Signed-off-by: Catherine Sullivan <catherine.sullivan@intel.com>
---

 drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c |  103 ++++++++++++++++++++++
 1 files changed, 101 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c
index 423700f..dfe68a9 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c
@@ -34,6 +34,103 @@
 
 static struct dentry *ixgbe_dbg_root;
 
+static char ixgbe_dbg_netdev_ops_buf[256] = "";
+
+/**
+ * ixgbe_dbg_netdev_ops_open - prep the debugfs netdev_ops data item
+ *			       when opened
+ * @inode: inode that was opened
+ * @filp: file info
+ *
+ * Stash the adapter pointer hiding in the inode into the file pointer
+ * where we can find it later in the read and write calls
+ **/
+static int ixgbe_dbg_netdev_ops_open(struct inode *inode, struct file *filp)
+{
+	filp->private_data = inode->i_private;
+	return 0;
+}
+
+/**
+ * ixgbe_dbg_netdev_ops_read - read for netdev_ops datum
+ * @filp: the opened file
+ * @buffer: where to write the data for the user to read
+ * @count: the size of the user's buffer
+ * @ppos: file position offset
+ **/
+static ssize_t ixgbe_dbg_netdev_ops_read(struct file *filp,
+					 char __user *buffer,
+					 size_t count, loff_t *ppos)
+{
+	struct ixgbe_adapter *adapter = filp->private_data;
+	char buf[256];
+	int bytes_not_copied;
+	int len;
+
+	/* don't allow partial reads */
+	if (*ppos != 0)
+		return 0;
+
+	len = snprintf(buf, sizeof(buf), "%s: %s\n",
+		       adapter->netdev->name, ixgbe_dbg_netdev_ops_buf);
+	if (count < len)
+		return -ENOSPC;
+	bytes_not_copied = copy_to_user(buffer, buf, len);
+	if (bytes_not_copied < 0)
+		return bytes_not_copied;
+
+	*ppos = len;
+	return len;
+}
+
+/**
+ * ixgbe_dbg_netdev_ops_write - write into netdev_ops datum
+ * @filp: the opened file
+ * @buffer: where to find the user's data
+ * @count: the length of the user's data
+ * @ppos: file position offset
+ **/
+static ssize_t ixgbe_dbg_netdev_ops_write(struct file *filp,
+					  const char __user *buffer,
+					  size_t count, loff_t *ppos)
+{
+	struct ixgbe_adapter *adapter = filp->private_data;
+	int bytes_not_copied;
+
+	/* don't allow partial writes */
+	if (*ppos != 0)
+		return 0;
+	if (count >= sizeof(ixgbe_dbg_netdev_ops_buf))
+		return -ENOSPC;
+
+	bytes_not_copied = copy_from_user(ixgbe_dbg_netdev_ops_buf,
+					  buffer, count);
+	if (bytes_not_copied < 0)
+		return bytes_not_copied;
+	else if (bytes_not_copied > 0)
+		count -= bytes_not_copied;
+	if (count < 0)
+		return -ENOSPC;
+	ixgbe_dbg_netdev_ops_buf[count] = '\0';
+
+	if (strncmp(ixgbe_dbg_netdev_ops_buf, "tx_timeout", 10) == 0) {
+		adapter->netdev->netdev_ops->ndo_tx_timeout(adapter->netdev);
+		e_dev_info("tx_timeout called\n");
+	} else {
+		e_dev_info("Unkown command: %s\n", ixgbe_dbg_netdev_ops_buf);
+		e_dev_info("Available commands:\n");
+		e_dev_info("    tx_timeout\n");
+	}
+	return count;
+}
+
+static const struct file_operations ixgbe_dbg_netdev_ops_fops = {
+	.owner = THIS_MODULE,
+	.open = ixgbe_dbg_netdev_ops_open,
+	.read = ixgbe_dbg_netdev_ops_read,
+	.write = ixgbe_dbg_netdev_ops_write,
+};
+
 /**
  * ixgbe_dbg_adapter_init - setup the debugfs directory for the adapter
  * @adapter: the adapter that is starting up
@@ -41,10 +138,12 @@ static struct dentry *ixgbe_dbg_root;
 void ixgbe_dbg_adapter_init(struct ixgbe_adapter *adapter)
 {
 	const char *name = pci_name(adapter->pdev);
-
+	struct dentry *pfile;
 	adapter->ixgbe_dbg_adapter = debugfs_create_dir(name, ixgbe_dbg_root);
 	if (adapter->ixgbe_dbg_adapter) {
-		/* create files here */
+		pfile = debugfs_create_file("netdev_ops", 0600,
+					    adapter->ixgbe_dbg_adapter, adapter,
+					    &ixgbe_dbg_netdev_ops_fops);
 	} else {
 		e_dev_err("debugfs entry for %s failed\n", name);
 	}

[NET_NEXT RFC PATCH 3/3] ixgbe: added reg_ops file to debugfs

[Catherine Sullivan]

Added the reg_ops file to debugfs with commands to read and write
a register to give users the ability to read and write individual
registers on the fly.

Signed-off-by: Catherine Sullivan <catherine.sullivan@intel.com>
---

 drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c |  112 ++++++++++++++++++++++
 1 files changed, 112 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c
index dfe68a9..2558014 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c
@@ -34,6 +34,115 @@
 
 static struct dentry *ixgbe_dbg_root;
 
+static char ixgbe_dbg_reg_ops_buf[256] = "";
+
+/**
+ * ixgbe_dbg_reg_ops_open - prep the debugfs pokee data item when opened
+ * @inode: inode that was opened
+ * @filp:  file info
+ *
+ * Stash the adapter pointer hiding in the inode into the file pointer where
+ * we can find it later in the read and write calls
+ **/
+static int ixgbe_dbg_reg_ops_open(struct inode *inode, struct file *filp)
+{
+	filp->private_data = inode->i_private;
+	return 0;
+}
+
+/**
+ * ixgbe_dbg_reg_ops_read - read for reg_ops datum
+ * @filp: the opened file
+ * @buffer: where to write the data for the user to read
+ * @count: the size of the user's buffer
+ * @ppos: file position offset
+ **/
+static ssize_t ixgbe_dbg_reg_ops_read(struct file *filp, char __user *buffer,
+				    size_t count, loff_t *ppos)
+{
+	struct ixgbe_adapter *adapter = filp->private_data;
+	char buf[256];
+	int bytes_not_copied;
+	int len;
+
+	/* don't allow partial reads */
+	if (*ppos != 0)
+		return 0;
+
+	len = snprintf(buf, sizeof(buf), "%s: %s\n",
+		       adapter->netdev->name, ixgbe_dbg_reg_ops_buf);
+	if (count < len)
+		return -ENOSPC;
+	bytes_not_copied = copy_to_user(buffer, buf, len);
+	if (bytes_not_copied < 0)
+		return bytes_not_copied;
+
+	*ppos = len;
+	return len;
+}
+
+/**
+ * ixgbe_dbg_reg_ops_write - write into reg_ops datum
+ * @filp: the opened file
+ * @buffer: where to find the user's data
+ * @count: the length of the user's data
+ * @ppos: file position offset
+ **/
+static ssize_t ixgbe_dbg_reg_ops_write(struct file *filp,
+				     const char __user *buffer,
+				     size_t count, loff_t *ppos)
+{
+	struct ixgbe_adapter *adapter = filp->private_data;
+	int bytes_not_copied;
+
+	/* don't allow partial writes */
+	if (*ppos != 0)
+		return 0;
+	if (count >= sizeof(ixgbe_dbg_reg_ops_buf))
+		return -ENOSPC;
+
+	bytes_not_copied = copy_from_user(ixgbe_dbg_reg_ops_buf, buffer, count);
+	if (bytes_not_copied < 0)
+		return bytes_not_copied;
+	ixgbe_dbg_reg_ops_buf[count] = '\0';
+
+	if (strncmp(ixgbe_dbg_reg_ops_buf, "write", 5) == 0) {
+		u32 reg, value;
+		int cnt;
+		cnt = sscanf(&ixgbe_dbg_reg_ops_buf[5], "%x %x", &reg, &value);
+		if (cnt == 2) {
+			IXGBE_WRITE_REG(&adapter->hw, reg, value);
+			value = IXGBE_READ_REG(&adapter->hw, reg);
+			e_dev_info("write: 0x%08x = 0x%08x\n", reg, value);
+		} else {
+			e_dev_info("write reg value\n");
+		}
+	} else if (strncmp(ixgbe_dbg_reg_ops_buf, "read", 4) == 0) {
+		u32 reg, value;
+		int cnt;
+		cnt = sscanf(&ixgbe_dbg_reg_ops_buf[4], "%x", &reg);
+		if (cnt == 1) {
+			value = IXGBE_READ_REG(&adapter->hw, reg);
+			e_dev_info("read 0x%08x = 0x%08x\n", reg, value);
+		} else {
+			e_dev_info("read reg\n");
+		}
+	} else {
+		e_dev_info("Unknown command %s\n", ixgbe_dbg_reg_ops_buf);
+		e_dev_info("Available commands:\n");
+		e_dev_info("   read reg\n");
+		e_dev_info("   write reg value\n");
+	}
+	return count;
+}
+
+static const struct file_operations ixgbe_dbg_reg_ops_fops = {
+	.owner = THIS_MODULE,
+	.open =  ixgbe_dbg_reg_ops_open,
+	.read =  ixgbe_dbg_reg_ops_read,
+	.write = ixgbe_dbg_reg_ops_write,
+};
+
 static char ixgbe_dbg_netdev_ops_buf[256] = "";
 
 /**
@@ -141,6 +250,9 @@ void ixgbe_dbg_adapter_init(struct ixgbe_adapter *adapter)
 	struct dentry *pfile;
 	adapter->ixgbe_dbg_adapter = debugfs_create_dir(name, ixgbe_dbg_root);
 	if (adapter->ixgbe_dbg_adapter) {
+		pfile = debugfs_create_file("reg_ops", 0600,
+					    adapter->ixgbe_dbg_adapter, adapter,
+					    &ixgbe_dbg_reg_ops_fops);
 		pfile = debugfs_create_file("netdev_ops", 0600,
 					    adapter->ixgbe_dbg_adapter, adapter,
 					    &ixgbe_dbg_netdev_ops_fops);

Re: [NET_NEXT RFC PATCH 1/3] ixgbe: add debugfs support

[Stephen Hemminger]

On Wed, 09 May 2012 16:09:40 -0700
Catherine Sullivan <catherine.sullivan@intel.com> wrote:

> This patch adds debugfs support to the ixgbe driver to give
> users the ability to access kernel information and to
> simulate kernel events.
> 
> The filesystem is set up in the following driver/PCI-instance
> hierarchy:
> <debugfs>
>    |-- ixgbe
> 	|-- PCI instance
> 	|	|-- attribute files
> 	|-- PCI instance
> 		|-- attribute files
> 
> Signed-off-by: Catherine Sullivan <catherine.sullivan@intel.com>

This should be an optional configuration since it is meant for special
case usage. See SKY2_DEBUG

Re: [NET_NEXT RFC PATCH 2/3] ixgbe: added netdev_ops file to debugfs

[Stephen Hemminger]

On Wed, 09 May 2012 16:09:45 -0700
Catherine Sullivan <catherine.sullivan@intel.com> wrote:

> +		e_dev_info("Unkown command: %s\n", ixgbe_dbg_netdev_ops_buf);

Spelling?

Re: [NET_NEXT RFC PATCH 2/3] ixgbe: added netdev_ops file to debugfs

[Stephen Hemminger]

On Wed, 09 May 2012 16:09:45 -0700
Catherine Sullivan <catherine.sullivan@intel.com> wrote:

> Added the netdev_ops file to debugfs with a command to call the
> ndo_tx_timeout function to give users the ability to simulate a
> tx_timeout call made by the kernel.
> 
> Signed-off-by: Catherine Sullivan <catherine.sullivan@intel.com>

What is the justification for creating a new API here?

Your exposing only one thing 'tx_timeout' and that is a generic property
of the device (not ixgbe specific). That value is already available via
sysfs.

Re: [NET_NEXT RFC PATCH 3/3] ixgbe: added reg_ops file to debugfs

[Stephen Hemminger]

On Wed, 09 May 2012 16:09:50 -0700
Catherine Sullivan <catherine.sullivan@intel.com> wrote:

> Added the reg_ops file to debugfs with commands to read and write
> a register to give users the ability to read and write individual
> registers on the fly.
> 
> Signed-off-by: Catherine Sullivan <catherine.sullivan@intel.com>
> ---
> 
>  drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c |  112 ++++++++++++++++++++++
>  1 files changed, 112 insertions(+), 0 deletions(-)
> 

Aren't these registers already in ethtool? You are also
allowing write without any security checking.

Re: [NET_NEXT RFC PATCH 3/3] ixgbe: added reg_ops file to debugfs

[Ben Hutchings]

On Wed, 2012-05-09 at 16:14 -0700, Stephen Hemminger wrote:
> On Wed, 09 May 2012 16:09:50 -0700
> Catherine Sullivan <catherine.sullivan@intel.com> wrote:
> 
> > Added the reg_ops file to debugfs with commands to read and write
> > a register to give users the ability to read and write individual
> > registers on the fly.
> > 
> > Signed-off-by: Catherine Sullivan <catherine.sullivan@intel.com>
> > ---
> > 
> >  drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c |  112 ++++++++++++++++++++++
> >  1 files changed, 112 insertions(+), 0 deletions(-)
> > 
> 
> Aren't these registers already in ethtool?

ethtool register access is read-only and would be very heavy-weight for
interactive debugging.  Not sure this is the best interface to
read/write registers, but it's certainly not redundant.

> You are also allowing write without any security checking.

The file permissions are set to 0600 so it's root-only.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [NET_NEXT RFC PATCH 3/3] ixgbe: added reg_ops file to debugfs

[Stephen Hemminger]

On Thu, 10 May 2012 01:18:10 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> On Wed, 2012-05-09 at 16:14 -0700, Stephen Hemminger wrote:
> > On Wed, 09 May 2012 16:09:50 -0700
> > Catherine Sullivan <catherine.sullivan@intel.com> wrote:
> > 
> > > Added the reg_ops file to debugfs with commands to read and write
> > > a register to give users the ability to read and write individual
> > > registers on the fly.
> > > 
> > > Signed-off-by: Catherine Sullivan <catherine.sullivan@intel.com>
> > > ---
> > > 
> > >  drivers/net/ethernet/intel/ixgbe/ixgbe_debugfs.c |  112 ++++++++++++++++++++++
> > >  1 files changed, 112 insertions(+), 0 deletions(-)
> > > 
> > 
> > Aren't these registers already in ethtool?
> 
> ethtool register access is read-only and would be very heavy-weight for
> interactive debugging.  Not sure this is the best interface to
> read/write registers, but it's certainly not redundant.

For debugging couldn't you could mmap the PCI  space? That would allow writing
a user level debugger for the hardware.

RE: [NET_NEXT RFC PATCH 2/3] ixgbe: added netdev_ops file to debugfs

[Sullivan, Catherine]

Could you point me to where this is available in sysfs please? I'm not familiar with it. 

Thanks, 
Catherine

-----Original Message-----
From: Stephen Hemminger [mailto:shemminger@vyatta.com] 
Sent: Wednesday, May 09, 2012 4:13 PM
To: Sullivan, Catherine
Cc: netdev@vger.kernel.org
Subject: Re: [NET_NEXT RFC PATCH 2/3] ixgbe: added netdev_ops file to debugfs

On Wed, 09 May 2012 16:09:45 -0700
Catherine Sullivan <catherine.sullivan@intel.com> wrote:

> Added the netdev_ops file to debugfs with a command to call the 
> ndo_tx_timeout function to give users the ability to simulate a 
> tx_timeout call made by the kernel.
> 
> Signed-off-by: Catherine Sullivan <catherine.sullivan@intel.com>

What is the justification for creating a new API here?

Your exposing only one thing 'tx_timeout' and that is a generic property of the device (not ixgbe specific). That value is already available via sysfs.

Re: [NET_NEXT RFC PATCH 2/3] ixgbe: added netdev_ops file to debugfs

[David Miller]

From: "Sullivan, Catherine" <catherine.sullivan@intel.com>
Date: Thu, 10 May 2012 20:28:28 +0000

> Could you point me to where this is available in sysfs please? I'm
> not familiar with it.

Is the 'find' program not properly functioning on your computer?

[davem@drr net-next]$ find /sys -type f -name tx_timeout
/sys/devices/virtual/net/lo/queues/tx-0/tx_timeout
/sys/devices/virtual/net/redhat0/queues/tx-0/tx_timeout
/sys/devices/pci0000:00/0000:00:1c.2/0000:06:00.0/net/p34p1/queues/tx-0/tx_timeout
[davem@drr net-next]$ 

RE: [NET_NEXT RFC PATCH 2/3] ixgbe: added netdev_ops file to debugfs

[Sullivan, Catherine]

I was aware of this, however this does not allow a call to the ndo_tx_timeout function which is what my patch does. Is there a way to do this with sysfs? I could not find this functionality. 

-----Original Message-----
From: David Miller [mailto:davem@davemloft.net] 
Sent: Thursday, May 10, 2012 1:32 PM

From: "Sullivan, Catherine" <catherine.sullivan@intel.com>
Date: Thu, 10 May 2012 20:28:28 +0000

> Could you point me to where this is available in sysfs please? I'm not 
> familiar with it.

Is the 'find' program not properly functioning on your computer?

[davem@drr net-next]$ find /sys -type f -name tx_timeout /sys/devices/virtual/net/lo/queues/tx-0/tx_timeout
/sys/devices/virtual/net/redhat0/queues/tx-0/tx_timeout
/sys/devices/pci0000:00/0000:00:1c.2/0000:06:00.0/net/p34p1/queues/tx-0/tx_timeout
[davem@drr net-next]$ 

Re: [NET_NEXT RFC PATCH 2/3] ixgbe: added netdev_ops file to debugfs

[David Miller]

From: "Sullivan, Catherine" <catherine.sullivan@intel.com>
Date: Thu, 10 May 2012 21:14:49 +0000

> I was aware of this, however this does not allow a call to the
> ndo_tx_timeout function which is what my patch does. Is there a way
> to do this with sysfs? I could not find this functionality.

Please stop top-posting.

Re: [NET_NEXT RFC PATCH 2/3] ixgbe: added netdev_ops file to debugfs

[Stephen Hemminger]

On Thu, 10 May 2012 21:14:49 +0000
"Sullivan, Catherine" <catherine.sullivan@intel.com> wrote:

> I was aware of this, however this does not allow a call to the ndo_tx_timeout function which is what my patch does. Is there a way to do this with sysfs? I could not find this functionality. 
> 
> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net] 
> Sent: Thursday, May 10, 2012 1:32 PM
> 
> From: "Sullivan, Catherine" <catherine.sullivan@intel.com>
> Date: Thu, 10 May 2012 20:28:28 +0000
> 
> > Could you point me to where this is available in sysfs please? I'm not 
> > familiar with it.
> 
> Is the 'find' program not properly functioning on your computer?
> 
> [davem@drr net-next]$ find /sys -type f -name tx_timeout /sys/devices/virtual/net/lo/queues/tx-0/tx_timeout
> /sys/devices/virtual/net/redhat0/queues/tx-0/tx_timeout
> /sys/devices/pci0000:00/0000:00:1c.2/0000:06:00.0/net/p34p1/queues/tx-0/tx_timeout
> [davem@drr net-next]$ 

Are you trying to induce timeout for some tests or code coverage?

RE: [NET_NEXT RFC PATCH 2/3] ixgbe: added netdev_ops file to debugfs

[Sullivan, Catherine]

> From: Stephen Hemminger [mailto:shemminger@vyatta.com]
> Sent: Thursday, May 10, 2012 2:36 PM
> 
> On Thu, 10 May 2012 21:14:49 +0000
> "Sullivan, Catherine" <catherine.sullivan@intel.com> wrote:
> 
> > I was aware of this, however this does not allow a call to the
> ndo_tx_timeout function which is what my patch does. Is there a way to
> do this with sysfs? I could not find this functionality.
> >
> > -----Original Message-----
> > From: David Miller [mailto:davem@davemloft.net]
> > Sent: Thursday, May 10, 2012 1:32 PM
> >
> > From: "Sullivan, Catherine" <catherine.sullivan@intel.com>
> > Date: Thu, 10 May 2012 20:28:28 +0000
> >
> > > Could you point me to where this is available in sysfs please? I'm
> > > not familiar with it.
> >
> > Is the 'find' program not properly functioning on your computer?
> >
> > [davem@drr net-next]$ find /sys -type f -name tx_timeout
> > /sys/devices/virtual/net/lo/queues/tx-0/tx_timeout
> > /sys/devices/virtual/net/redhat0/queues/tx-0/tx_timeout
> >
> /sys/devices/pci0000:00/0000:00:1c.2/0000:06:00.0/net/p34p1/queues/tx-
> > 0/tx_timeout
> > [davem@drr net-next]$
> 
> Are you trying to induce timeout for some tests or code coverage?

Yes, we are using this for testing and debugging purposes.

RE: [NET_NEXT RFC PATCH 1/3] ixgbe: add debugfs support

[Sullivan, Catherine]

> From: Stephen Hemminger [mailto:shemminger@vyatta.com]
> Sent: Wednesday, May 09, 2012 4:10 PM
> 
> On Wed, 09 May 2012 16:09:40 -0700
> Catherine Sullivan <catherine.sullivan@intel.com> wrote:
> 
> > This patch adds debugfs support to the ixgbe driver to give users the
> > ability to access kernel information and to simulate kernel events.
> >
> > The filesystem is set up in the following driver/PCI-instance
> > hierarchy:
> > <debugfs>
> >    |-- ixgbe
> > 	|-- PCI instance
> > 	|	|-- attribute files
> > 	|-- PCI instance
> > 		|-- attribute files
> >
> > Signed-off-by: Catherine Sullivan <catherine.sullivan@intel.com>
> 
> This should be an optional configuration since it is meant for special
> case usage. See SKY2_DEBUG

After looking through the kernel, there doesn't seem to be a clear precedence for this. As was pointed out, SKY2_DEBUG is an optional configuration on its own. However regmap uses debugfs, and it is only optional based on CONFIG_DEBUG_FS, which is how this patch is currently set up. This patch does not have much overhead and we would prefer that it be enabled with CONFIG_DEBUG_FS.