[PATCH v1 1/6] staging: qlge: Initialize devlink health dump framework for the dlge driver

[PATCH v1 1/6] staging: qlge: Initialize devlink health dump framework for the dlge driver

[Coiby Xu]

Initialize devlink health dump framework for the dlge driver so the
coredump could be done via devlink.

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/Kconfig        |  1 +
 drivers/staging/qlge/Makefile       |  2 +-
 drivers/staging/qlge/qlge.h         |  9 +++++++
 drivers/staging/qlge/qlge_devlink.c | 38 +++++++++++++++++++++++++++++
 drivers/staging/qlge/qlge_devlink.h |  8 ++++++
 drivers/staging/qlge/qlge_main.c    | 28 +++++++++++++++++++++
 6 files changed, 85 insertions(+), 1 deletion(-)
 create mode 100644 drivers/staging/qlge/qlge_devlink.c
 create mode 100644 drivers/staging/qlge/qlge_devlink.h

diff --git a/drivers/staging/qlge/Kconfig b/drivers/staging/qlge/Kconfig
index a3cb25a3ab80..6d831ed67965 100644
--- a/drivers/staging/qlge/Kconfig
+++ b/drivers/staging/qlge/Kconfig
@@ -3,6 +3,7 @@
 config QLGE
 	tristate "QLogic QLGE 10Gb Ethernet Driver Support"
 	depends on ETHERNET && PCI
+	select NET_DEVLINK
 	help
 	This driver supports QLogic ISP8XXX 10Gb Ethernet cards.
 
diff --git a/drivers/staging/qlge/Makefile b/drivers/staging/qlge/Makefile
index 1dc2568e820c..07c1898a512e 100644
--- a/drivers/staging/qlge/Makefile
+++ b/drivers/staging/qlge/Makefile
@@ -5,4 +5,4 @@
 
 obj-$(CONFIG_QLGE) += qlge.o
 
-qlge-objs := qlge_main.o qlge_dbg.o qlge_mpi.o qlge_ethtool.o
+qlge-objs := qlge_main.o qlge_dbg.o qlge_mpi.o qlge_ethtool.o qlge_devlink.o
diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
index b295990e361b..290e754450c5 100644
--- a/drivers/staging/qlge/qlge.h
+++ b/drivers/staging/qlge/qlge.h
@@ -2060,6 +2060,14 @@ struct nic_operations {
 	int (*port_initialize)(struct ql_adapter *qdev);
 };
 
+
+
+struct qlge_devlink {
+        struct ql_adapter *qdev;
+        struct net_device *ndev;
+        struct devlink_health_reporter *reporter;
+};
+
 /*
  * The main Adapter structure definition.
  * This structure has all fields relevant to the hardware.
@@ -2077,6 +2085,7 @@ struct ql_adapter {
 	struct pci_dev *pdev;
 	struct net_device *ndev;	/* Parent NET device */
 
+	struct qlge_devlink *ql_devlink;
 	/* Hardware information */
 	u32 chip_rev_id;
 	u32 fw_rev_id;
diff --git a/drivers/staging/qlge/qlge_devlink.c b/drivers/staging/qlge/qlge_devlink.c
new file mode 100644
index 000000000000..aa45e7e368c0
--- /dev/null
+++ b/drivers/staging/qlge/qlge_devlink.c
@@ -0,0 +1,38 @@
+#include "qlge.h"
+#include "qlge_devlink.h"
+
+static int
+qlge_reporter_coredump(struct devlink_health_reporter *reporter,
+			struct devlink_fmsg *fmsg, void *priv_ctx,
+			struct netlink_ext_ack *extack)
+{
+	return 0;
+}
+
+static const struct devlink_health_reporter_ops qlge_reporter_ops = {
+		.name = "dummy",
+		.dump = qlge_reporter_coredump,
+};
+
+int qlge_health_create_reporters(struct qlge_devlink *priv)
+{
+	int err;
+
+	struct devlink_health_reporter *reporter;
+	struct devlink *devlink;
+
+	devlink = priv_to_devlink(priv);
+	reporter =
+		devlink_health_reporter_create(devlink, &qlge_reporter_ops,
+					       0,
+					       priv);
+	if (IS_ERR(reporter)) {
+		netdev_warn(priv->ndev,
+			    "Failed to create reporter, err = %ld\n",
+			    PTR_ERR(reporter));
+		return PTR_ERR(reporter);
+	}
+	priv->reporter = reporter;
+
+	return err;
+}
diff --git a/drivers/staging/qlge/qlge_devlink.h b/drivers/staging/qlge/qlge_devlink.h
new file mode 100644
index 000000000000..c91f7a29e805
--- /dev/null
+++ b/drivers/staging/qlge/qlge_devlink.h
@@ -0,0 +1,8 @@
+#ifndef QLGE_DEVLINK_H
+#define QLGE_DEVLINK_H
+
+#include <net/devlink.h>
+
+int qlge_health_create_reporters(struct qlge_devlink *priv);
+
+#endif /* QLGE_DEVLINK_H */
diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index 27da386f9d87..135225530e51 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -42,6 +42,7 @@
 #include <net/ip6_checksum.h>
 
 #include "qlge.h"
+#include "qlge_devlink.h"
 
 char qlge_driver_name[] = DRV_NAME;
 const char qlge_driver_version[] = DRV_VERSION;
@@ -4549,6 +4550,8 @@ static void ql_timer(struct timer_list *t)
 	mod_timer(&qdev->timer, jiffies + (5 * HZ));
 }
 
+static const struct devlink_ops qlge_devlink_ops;
+
 static int qlge_probe(struct pci_dev *pdev,
 		      const struct pci_device_id *pci_entry)
 {
@@ -4556,6 +4559,13 @@ static int qlge_probe(struct pci_dev *pdev,
 	struct ql_adapter *qdev = NULL;
 	static int cards_found;
 	int err = 0;
+	struct devlink *devlink;
+	struct qlge_devlink *ql_devlink;
+
+	devlink = devlink_alloc(&qlge_devlink_ops, sizeof(struct qlge_devlink));
+	if (!devlink)
+		return -ENOMEM;
+	ql_devlink = devlink_priv(devlink);
 
 	ndev = alloc_etherdev_mq(sizeof(struct ql_adapter),
 				 min(MAX_CPUS,
@@ -4614,6 +4624,16 @@ static int qlge_probe(struct pci_dev *pdev,
 		free_netdev(ndev);
 		return err;
 	}
+
+	err = devlink_register(devlink, &pdev->dev);
+	if (err) {
+		goto devlink_free;
+	}
+
+	qlge_health_create_reporters(ql_devlink);
+	ql_devlink->qdev = qdev;
+	ql_devlink->ndev = ndev;
+	qdev->ql_devlink = ql_devlink;
 	/* Start up the timer to trigger EEH if
 	 * the bus goes dead
 	 */
@@ -4624,6 +4644,10 @@ static int qlge_probe(struct pci_dev *pdev,
 	atomic_set(&qdev->lb_count, 0);
 	cards_found++;
 	return 0;
+
+devlink_free:
+	devlink_free(devlink);
+	return err;
 }
 
 netdev_tx_t ql_lb_send(struct sk_buff *skb, struct net_device *ndev)
@@ -4640,12 +4664,16 @@ static void qlge_remove(struct pci_dev *pdev)
 {
 	struct net_device *ndev = pci_get_drvdata(pdev);
 	struct ql_adapter *qdev = netdev_priv(ndev);
+	struct devlink *devlink = priv_to_devlink(qdev->ql_devlink);
 
 	del_timer_sync(&qdev->timer);
 	ql_cancel_all_work_sync(qdev);
 	unregister_netdev(ndev);
 	ql_release_all(pdev);
 	pci_disable_device(pdev);
+	devlink_health_reporter_destroy(qdev->ql_devlink->reporter);
+	devlink_unregister(devlink);
+	devlink_free(devlink);
 	free_netdev(ndev);
 }
 
-- 
2.28.0

Re: [PATCH v1 1/6] staging: qlge: Initialize devlink health dump framework for the dlge driver

[Willem de Bruijn]

On Thu, Oct 8, 2020 at 7:58 AM Coiby Xu <coiby.xu@gmail.com> wrote:
>
> Initialize devlink health dump framework for the dlge driver so the
> coredump could be done via devlink.
>
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>

> @@ -4556,6 +4559,13 @@ static int qlge_probe(struct pci_dev *pdev,
>         struct ql_adapter *qdev = NULL;
>         static int cards_found;
>         int err = 0;
> +       struct devlink *devlink;
> +       struct qlge_devlink *ql_devlink;
> +
> +       devlink = devlink_alloc(&qlge_devlink_ops, sizeof(struct qlge_devlink));
> +       if (!devlink)
> +               return -ENOMEM;
> +       ql_devlink = devlink_priv(devlink);
>
>         ndev = alloc_etherdev_mq(sizeof(struct ql_adapter),
>                                  min(MAX_CPUS,

need to goto devlink_free instead of return -ENOMEM here, too.

> @@ -4614,6 +4624,16 @@ static int qlge_probe(struct pci_dev *pdev,
>                 free_netdev(ndev);
>                 return err;

and here

>         }
> +
> +       err = devlink_register(devlink, &pdev->dev);
> +       if (err) {
> +               goto devlink_free;
> +       }
> +
> +       qlge_health_create_reporters(ql_devlink);
> +       ql_devlink->qdev = qdev;
> +       ql_devlink->ndev = ndev;
> +       qdev->ql_devlink = ql_devlink;
>         /* Start up the timer to trigger EEH if
>          * the bus goes dead
>          */
> @@ -4624,6 +4644,10 @@ static int qlge_probe(struct pci_dev *pdev,
>         atomic_set(&qdev->lb_count, 0);
>         cards_found++;
>         return 0;
> +
> +devlink_free:
> +       devlink_free(devlink);
> +       return err;
>  }

Re: [PATCH v1 1/6] staging: qlge: Initialize devlink health dump framework for the dlge driver

[Coiby Xu]

On Thu, Oct 08, 2020 at 08:22:44AM -0400, Willem de Bruijn wrote:
>On Thu, Oct 8, 2020 at 7:58 AM Coiby Xu <coiby.xu@gmail.com> wrote:
>>
>> Initialize devlink health dump framework for the dlge driver so the
>> coredump could be done via devlink.
>>
>> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
>
>> @@ -4556,6 +4559,13 @@ static int qlge_probe(struct pci_dev *pdev,
>>         struct ql_adapter *qdev = NULL;
>>         static int cards_found;
>>         int err = 0;
>> +       struct devlink *devlink;
>> +       struct qlge_devlink *ql_devlink;
>> +
>> +       devlink = devlink_alloc(&qlge_devlink_ops, sizeof(struct qlge_devlink));
>> +       if (!devlink)
>> +               return -ENOMEM;
>> +       ql_devlink = devlink_priv(devlink);
>>
>>         ndev = alloc_etherdev_mq(sizeof(struct ql_adapter),
>>                                  min(MAX_CPUS,
>
>need to goto devlink_free instead of return -ENOMEM here, too.
>
>> @@ -4614,6 +4624,16 @@ static int qlge_probe(struct pci_dev *pdev,
>>                 free_netdev(ndev);
>>                 return err;
>
>and here
>
Thank you for reviewing this work and the speedy feedback! I'll fix it
in v2.
>>         }
>> +
>> +       err = devlink_register(devlink, &pdev->dev);
>> +       if (err) {
>> +               goto devlink_free;
>> +       }
>> +
>> +       qlge_health_create_reporters(ql_devlink);
>> +       ql_devlink->qdev = qdev;
>> +       ql_devlink->ndev = ndev;
>> +       qdev->ql_devlink = ql_devlink;
>>         /* Start up the timer to trigger EEH if
>>          * the bus goes dead
>>          */
>> @@ -4624,6 +4644,10 @@ static int qlge_probe(struct pci_dev *pdev,
>>         atomic_set(&qdev->lb_count, 0);
>>         cards_found++;
>>         return 0;
>> +
>> +devlink_free:
>> +       devlink_free(devlink);
>> +       return err;
>>  }

--
Best regards,
Coiby

Re: [PATCH v1 1/6] staging: qlge: Initialize devlink health dump framework for the dlge driver

[Coiby Xu]

On Thu, Oct 08, 2020 at 08:22:44AM -0400, Willem de Bruijn wrote:
>On Thu, Oct 8, 2020 at 7:58 AM Coiby Xu <coiby.xu@gmail.com> wrote:
>>
>> Initialize devlink health dump framework for the dlge driver so the
>> coredump could be done via devlink.
>>
>> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
>
>> @@ -4556,6 +4559,13 @@ static int qlge_probe(struct pci_dev *pdev,
>>         struct ql_adapter *qdev = NULL;
>>         static int cards_found;
>>         int err = 0;
>> +       struct devlink *devlink;
>> +       struct qlge_devlink *ql_devlink;
>> +
>> +       devlink = devlink_alloc(&qlge_devlink_ops, sizeof(struct qlge_devlink));
>> +       if (!devlink)
>> +               return -ENOMEM;
>> +       ql_devlink = devlink_priv(devlink);
>>
>>         ndev = alloc_etherdev_mq(sizeof(struct ql_adapter),
>>                                  min(MAX_CPUS,
>
>need to goto devlink_free instead of return -ENOMEM here, too.

devlink_alloc return NULL only if kzalloc return NULL. So we
shouldn't go to devlink_free which will call kfree.
>
>> @@ -4614,6 +4624,16 @@ static int qlge_probe(struct pci_dev *pdev,
>>                 free_netdev(ndev);
>>                 return err;
>
>and here

But I should goto devlink_free here. Thank you for pointing out my
neglect.
>
>>         }
>> +
>> +       err = devlink_register(devlink, &pdev->dev);
>> +       if (err) {
>> +               goto devlink_free;
>> +       }
>> +
>> +       qlge_health_create_reporters(ql_devlink);
>> +       ql_devlink->qdev = qdev;
>> +       ql_devlink->ndev = ndev;
>> +       qdev->ql_devlink = ql_devlink;
>>         /* Start up the timer to trigger EEH if
>>          * the bus goes dead
>>          */
>> @@ -4624,6 +4644,10 @@ static int qlge_probe(struct pci_dev *pdev,
>>         atomic_set(&qdev->lb_count, 0);
>>         cards_found++;
>>         return 0;
>> +
>> +devlink_free:
>> +       devlink_free(devlink);
>> +       return err;
>>  }

--
Best regards,
Coiby

Re: [PATCH v1 1/6] staging: qlge: Initialize devlink health dump framework for the dlge driver

[Dan Carpenter]

On Thu, Oct 08, 2020 at 07:58:03PM +0800, Coiby Xu wrote:
> Initialize devlink health dump framework for the dlge driver so the
> coredump could be done via devlink.
> 
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
>  drivers/staging/qlge/Kconfig        |  1 +
>  drivers/staging/qlge/Makefile       |  2 +-
>  drivers/staging/qlge/qlge.h         |  9 +++++++
>  drivers/staging/qlge/qlge_devlink.c | 38 +++++++++++++++++++++++++++++
>  drivers/staging/qlge/qlge_devlink.h |  8 ++++++
>  drivers/staging/qlge/qlge_main.c    | 28 +++++++++++++++++++++
>  6 files changed, 85 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/staging/qlge/qlge_devlink.c
>  create mode 100644 drivers/staging/qlge/qlge_devlink.h
> 
> diff --git a/drivers/staging/qlge/Kconfig b/drivers/staging/qlge/Kconfig
> index a3cb25a3ab80..6d831ed67965 100644
> --- a/drivers/staging/qlge/Kconfig
> +++ b/drivers/staging/qlge/Kconfig
> @@ -3,6 +3,7 @@
>  config QLGE
>  	tristate "QLogic QLGE 10Gb Ethernet Driver Support"
>  	depends on ETHERNET && PCI
> +	select NET_DEVLINK
>  	help
>  	This driver supports QLogic ISP8XXX 10Gb Ethernet cards.
>  
> diff --git a/drivers/staging/qlge/Makefile b/drivers/staging/qlge/Makefile
> index 1dc2568e820c..07c1898a512e 100644
> --- a/drivers/staging/qlge/Makefile
> +++ b/drivers/staging/qlge/Makefile
> @@ -5,4 +5,4 @@
>  
>  obj-$(CONFIG_QLGE) += qlge.o
>  
> -qlge-objs := qlge_main.o qlge_dbg.o qlge_mpi.o qlge_ethtool.o
> +qlge-objs := qlge_main.o qlge_dbg.o qlge_mpi.o qlge_ethtool.o qlge_devlink.o
> diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
> index b295990e361b..290e754450c5 100644
> --- a/drivers/staging/qlge/qlge.h
> +++ b/drivers/staging/qlge/qlge.h
> @@ -2060,6 +2060,14 @@ struct nic_operations {
>  	int (*port_initialize)(struct ql_adapter *qdev);
>  };
>  
> +
> +

Having multiple blank lines in a row leads to a static checker warning.
Please run `checkpatch.pl --strict` over your patches.

> +struct qlge_devlink {
> +        struct ql_adapter *qdev;
> +        struct net_device *ndev;
> +        struct devlink_health_reporter *reporter;
> +};
> +
>  /*
>   * The main Adapter structure definition.
>   * This structure has all fields relevant to the hardware.
> @@ -2077,6 +2085,7 @@ struct ql_adapter {
>  	struct pci_dev *pdev;
>  	struct net_device *ndev;	/* Parent NET device */
>  
> +	struct qlge_devlink *ql_devlink;
>  	/* Hardware information */
>  	u32 chip_rev_id;
>  	u32 fw_rev_id;
> diff --git a/drivers/staging/qlge/qlge_devlink.c b/drivers/staging/qlge/qlge_devlink.c
> new file mode 100644
> index 000000000000..aa45e7e368c0
> --- /dev/null
> +++ b/drivers/staging/qlge/qlge_devlink.c
> @@ -0,0 +1,38 @@
> +#include "qlge.h"
> +#include "qlge_devlink.h"
> +
> +static int
> +qlge_reporter_coredump(struct devlink_health_reporter *reporter,
> +			struct devlink_fmsg *fmsg, void *priv_ctx,
> +			struct netlink_ext_ack *extack)
> +{
> +	return 0;
> +}
> +
> +static const struct devlink_health_reporter_ops qlge_reporter_ops = {
> +		.name = "dummy",
> +		.dump = qlge_reporter_coredump,

Indented too far.

> +};
> +
> +int qlge_health_create_reporters(struct qlge_devlink *priv)
> +{
> +	int err;
> +

No blanks in the middle of declarations.

> +	struct devlink_health_reporter *reporter;
> +	struct devlink *devlink;

Generally this driver declares variables in "Reverse Christmas Tree"
order.  The names are orderred by the length of the line from longest
to shortest.

	type long_name;
	type medium;
	type short;

> +
> +	devlink = priv_to_devlink(priv);
> +	reporter =
> +		devlink_health_reporter_create(devlink, &qlge_reporter_ops,
> +					       0,
> +					       priv);

Break this up like so:

	reporter = devlink_health_reporter_create(devlink, &qlge_reporter_ops,
						  0, priv);


> +	if (IS_ERR(reporter)) {
> +		netdev_warn(priv->ndev,
> +			    "Failed to create reporter, err = %ld\n",
> +			    PTR_ERR(reporter));
> +		return PTR_ERR(reporter);

No point in returning an error if the caller doesn't check?

> +	}
> +	priv->reporter = reporter;
> +
> +	return err;

err is uninitialized.  "return 0;"

> +}
> diff --git a/drivers/staging/qlge/qlge_devlink.h b/drivers/staging/qlge/qlge_devlink.h
> new file mode 100644
> index 000000000000..c91f7a29e805
> --- /dev/null
> +++ b/drivers/staging/qlge/qlge_devlink.h
> @@ -0,0 +1,8 @@
> +#ifndef QLGE_DEVLINK_H
> +#define QLGE_DEVLINK_H
> +
> +#include <net/devlink.h>
> +
> +int qlge_health_create_reporters(struct qlge_devlink *priv);
> +
> +#endif /* QLGE_DEVLINK_H */
> diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
> index 27da386f9d87..135225530e51 100644
> --- a/drivers/staging/qlge/qlge_main.c
> +++ b/drivers/staging/qlge/qlge_main.c
> @@ -42,6 +42,7 @@
>  #include <net/ip6_checksum.h>
>  
>  #include "qlge.h"
> +#include "qlge_devlink.h"
>  
>  char qlge_driver_name[] = DRV_NAME;
>  const char qlge_driver_version[] = DRV_VERSION;
> @@ -4549,6 +4550,8 @@ static void ql_timer(struct timer_list *t)
>  	mod_timer(&qdev->timer, jiffies + (5 * HZ));
>  }
>  
> +static const struct devlink_ops qlge_devlink_ops;
> +
>  static int qlge_probe(struct pci_dev *pdev,
>  		      const struct pci_device_id *pci_entry)
>  {
> @@ -4556,6 +4559,13 @@ static int qlge_probe(struct pci_dev *pdev,
>  	struct ql_adapter *qdev = NULL;
>  	static int cards_found;
>  	int err = 0;
> +	struct devlink *devlink;
> +	struct qlge_devlink *ql_devlink;
> +
> +	devlink = devlink_alloc(&qlge_devlink_ops, sizeof(struct qlge_devlink));
> +	if (!devlink)
> +		return -ENOMEM;
> +	ql_devlink = devlink_priv(devlink);
>  
>  	ndev = alloc_etherdev_mq(sizeof(struct ql_adapter),
>  				 min(MAX_CPUS,
> @@ -4614,6 +4624,16 @@ static int qlge_probe(struct pci_dev *pdev,
>  		free_netdev(ndev);
>  		return err;
>  	}
> +
> +	err = devlink_register(devlink, &pdev->dev);
> +	if (err) {
> +		goto devlink_free;
> +	}

Checkpatch warning.

regards,
dan carpenter

Re: [PATCH v1 1/6] staging: qlge: Initialize devlink health dump framework for the dlge driver

[Coiby Xu]

On Thu, Oct 08, 2020 at 04:31:42PM +0300, Dan Carpenter wrote:
>On Thu, Oct 08, 2020 at 07:58:03PM +0800, Coiby Xu wrote:
>> Initialize devlink health dump framework for the dlge driver so the
>> coredump could be done via devlink.
>>
>> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
>> ---
>>  drivers/staging/qlge/Kconfig        |  1 +
>>  drivers/staging/qlge/Makefile       |  2 +-
>>  drivers/staging/qlge/qlge.h         |  9 +++++++
>>  drivers/staging/qlge/qlge_devlink.c | 38 +++++++++++++++++++++++++++++
>>  drivers/staging/qlge/qlge_devlink.h |  8 ++++++
>>  drivers/staging/qlge/qlge_main.c    | 28 +++++++++++++++++++++
>>  6 files changed, 85 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/staging/qlge/qlge_devlink.c
>>  create mode 100644 drivers/staging/qlge/qlge_devlink.h
>>
>> diff --git a/drivers/staging/qlge/Kconfig b/drivers/staging/qlge/Kconfig
>> index a3cb25a3ab80..6d831ed67965 100644
>> --- a/drivers/staging/qlge/Kconfig
>> +++ b/drivers/staging/qlge/Kconfig
>> @@ -3,6 +3,7 @@
>>  config QLGE
>>  	tristate "QLogic QLGE 10Gb Ethernet Driver Support"
>>  	depends on ETHERNET && PCI
>> +	select NET_DEVLINK
>>  	help
>>  	This driver supports QLogic ISP8XXX 10Gb Ethernet cards.
>>
>> diff --git a/drivers/staging/qlge/Makefile b/drivers/staging/qlge/Makefile
>> index 1dc2568e820c..07c1898a512e 100644
>> --- a/drivers/staging/qlge/Makefile
>> +++ b/drivers/staging/qlge/Makefile
>> @@ -5,4 +5,4 @@
>>
>>  obj-$(CONFIG_QLGE) += qlge.o
>>
>> -qlge-objs := qlge_main.o qlge_dbg.o qlge_mpi.o qlge_ethtool.o
>> +qlge-objs := qlge_main.o qlge_dbg.o qlge_mpi.o qlge_ethtool.o qlge_devlink.o
>> diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
>> index b295990e361b..290e754450c5 100644
>> --- a/drivers/staging/qlge/qlge.h
>> +++ b/drivers/staging/qlge/qlge.h
>> @@ -2060,6 +2060,14 @@ struct nic_operations {
>>  	int (*port_initialize)(struct ql_adapter *qdev);
>>  };
>>
>> +
>> +
>
>Having multiple blank lines in a row leads to a static checker warning.
>Please run `checkpatch.pl --strict` over your patches.
>
>> +struct qlge_devlink {
>> +        struct ql_adapter *qdev;
>> +        struct net_device *ndev;
>> +        struct devlink_health_reporter *reporter;
>> +};
>> +
>>  /*
>>   * The main Adapter structure definition.
>>   * This structure has all fields relevant to the hardware.
>> @@ -2077,6 +2085,7 @@ struct ql_adapter {
>>  	struct pci_dev *pdev;
>>  	struct net_device *ndev;	/* Parent NET device */
>>
>> +	struct qlge_devlink *ql_devlink;
>>  	/* Hardware information */
>>  	u32 chip_rev_id;
>>  	u32 fw_rev_id;
>> diff --git a/drivers/staging/qlge/qlge_devlink.c b/drivers/staging/qlge/qlge_devlink.c
>> new file mode 100644
>> index 000000000000..aa45e7e368c0
>> --- /dev/null
>> +++ b/drivers/staging/qlge/qlge_devlink.c
>> @@ -0,0 +1,38 @@
>> +#include "qlge.h"
>> +#include "qlge_devlink.h"
>> +
>> +static int
>> +qlge_reporter_coredump(struct devlink_health_reporter *reporter,
>> +			struct devlink_fmsg *fmsg, void *priv_ctx,
>> +			struct netlink_ext_ack *extack)
>> +{
>> +	return 0;
>> +}
>> +
>> +static const struct devlink_health_reporter_ops qlge_reporter_ops = {
>> +		.name = "dummy",
>> +		.dump = qlge_reporter_coredump,
>
>Indented too far.
>
>> +};
>> +
>> +int qlge_health_create_reporters(struct qlge_devlink *priv)
>> +{
>> +	int err;
>> +
>
>No blanks in the middle of declarations.
>
>> +	struct devlink_health_reporter *reporter;
>> +	struct devlink *devlink;
>
>Generally this driver declares variables in "Reverse Christmas Tree"
>order.  The names are orderred by the length of the line from longest
>to shortest.
>
>	type long_name;
>	type medium;
>	type short;
>
>> +
>> +	devlink = priv_to_devlink(priv);
>> +	reporter =
>> +		devlink_health_reporter_create(devlink, &qlge_reporter_ops,
>> +					       0,
>> +					       priv);
>
>Break this up like so:
>
>	reporter = devlink_health_reporter_create(devlink, &qlge_reporter_ops,
>						  0, priv);
>
>
>> +	if (IS_ERR(reporter)) {
>> +		netdev_warn(priv->ndev,
>> +			    "Failed to create reporter, err = %ld\n",
>> +			    PTR_ERR(reporter));
>> +		return PTR_ERR(reporter);
>
>No point in returning an error if the caller doesn't check?
>
>> +	}
>> +	priv->reporter = reporter;
>> +
>> +	return err;
>
>err is uninitialized.  "return 0;"
>
>> +}
>> diff --git a/drivers/staging/qlge/qlge_devlink.h b/drivers/staging/qlge/qlge_devlink.h
>> new file mode 100644
>> index 000000000000..c91f7a29e805
>> --- /dev/null
>> +++ b/drivers/staging/qlge/qlge_devlink.h
>> @@ -0,0 +1,8 @@
>> +#ifndef QLGE_DEVLINK_H
>> +#define QLGE_DEVLINK_H
>> +
>> +#include <net/devlink.h>
>> +
>> +int qlge_health_create_reporters(struct qlge_devlink *priv);
>> +
>> +#endif /* QLGE_DEVLINK_H */
>> diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
>> index 27da386f9d87..135225530e51 100644
>> --- a/drivers/staging/qlge/qlge_main.c
>> +++ b/drivers/staging/qlge/qlge_main.c
>> @@ -42,6 +42,7 @@
>>  #include <net/ip6_checksum.h>
>>
>>  #include "qlge.h"
>> +#include "qlge_devlink.h"
>>
>>  char qlge_driver_name[] = DRV_NAME;
>>  const char qlge_driver_version[] = DRV_VERSION;
>> @@ -4549,6 +4550,8 @@ static void ql_timer(struct timer_list *t)
>>  	mod_timer(&qdev->timer, jiffies + (5 * HZ));
>>  }
>>
>> +static const struct devlink_ops qlge_devlink_ops;
>> +
>>  static int qlge_probe(struct pci_dev *pdev,
>>  		      const struct pci_device_id *pci_entry)
>>  {
>> @@ -4556,6 +4559,13 @@ static int qlge_probe(struct pci_dev *pdev,
>>  	struct ql_adapter *qdev = NULL;
>>  	static int cards_found;
>>  	int err = 0;
>> +	struct devlink *devlink;
>> +	struct qlge_devlink *ql_devlink;
>> +
>> +	devlink = devlink_alloc(&qlge_devlink_ops, sizeof(struct qlge_devlink));
>> +	if (!devlink)
>> +		return -ENOMEM;
>> +	ql_devlink = devlink_priv(devlink);
>>
>>  	ndev = alloc_etherdev_mq(sizeof(struct ql_adapter),
>>  				 min(MAX_CPUS,
>> @@ -4614,6 +4624,16 @@ static int qlge_probe(struct pci_dev *pdev,
>>  		free_netdev(ndev);
>>  		return err;
>>  	}
>> +
>> +	err = devlink_register(devlink, &pdev->dev);
>> +	if (err) {
>> +		goto devlink_free;
>> +	}
>
>Checkpatch warning.
>

Thank you for the reminding!

>regards,
>dan carpenter
>

--
Best regards,
Coiby

Re: [PATCH v1 1/6] staging: qlge: Initialize devlink health dump framework for the dlge driver

[kernel test robot]

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

Hi Coiby,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]

url:    https://github.com/0day-ci/linux/commits/Coiby-Xu/staging-qlge-Re-writing-the-debugging-features/20201008-200002
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 76c3bdd67d27289b9e407113821eab2a70bbcca6
config: arm64-randconfig-r025-20201008 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 8da0df3d6dcc0dd42740be60b0da4ec201190904)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/0day-ci/linux/commit/dda18d66af0554f1f2b69f9a61335f3de9ec5124
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Coiby-Xu/staging-qlge-Re-writing-the-debugging-features/20201008-200002
        git checkout dda18d66af0554f1f2b69f9a61335f3de9ec5124
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/staging/qlge/qlge_devlink.c:37:9: warning: variable 'err' is uninitialized when used here [-Wuninitialized]
           return err;
                  ^~~
   drivers/staging/qlge/qlge_devlink.c:19:9: note: initialize the variable 'err' to silence this warning
           int err;
                  ^
                   = 0
   1 warning generated.

vim +/err +37 drivers/staging/qlge/qlge_devlink.c

    16	
    17	int qlge_health_create_reporters(struct qlge_devlink *priv)
    18	{
    19		int err;
    20	
    21		struct devlink_health_reporter *reporter;
    22		struct devlink *devlink;
    23	
    24		devlink = priv_to_devlink(priv);
    25		reporter =
    26			devlink_health_reporter_create(devlink, &qlge_reporter_ops,
    27						       0,
    28						       priv);
    29		if (IS_ERR(reporter)) {
    30			netdev_warn(priv->ndev,
    31				    "Failed to create reporter, err = %ld\n",
    32				    PTR_ERR(reporter));
    33			return PTR_ERR(reporter);
    34		}
    35		priv->reporter = reporter;
    36	
  > 37		return err;

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

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

Re: [PATCH v1 1/6] staging: qlge: Initialize devlink health dump framework for the dlge driver

[Benjamin Poirier]

On 2020-10-08 19:58 +0800, Coiby Xu wrote:
> Initialize devlink health dump framework for the dlge driver so the
> coredump could be done via devlink.
> 
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
>  drivers/staging/qlge/Kconfig        |  1 +
>  drivers/staging/qlge/Makefile       |  2 +-
>  drivers/staging/qlge/qlge.h         |  9 +++++++
>  drivers/staging/qlge/qlge_devlink.c | 38 +++++++++++++++++++++++++++++
>  drivers/staging/qlge/qlge_devlink.h |  8 ++++++
>  drivers/staging/qlge/qlge_main.c    | 28 +++++++++++++++++++++
>  6 files changed, 85 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/staging/qlge/qlge_devlink.c
>  create mode 100644 drivers/staging/qlge/qlge_devlink.h
> 
> diff --git a/drivers/staging/qlge/Kconfig b/drivers/staging/qlge/Kconfig
> index a3cb25a3ab80..6d831ed67965 100644
> --- a/drivers/staging/qlge/Kconfig
> +++ b/drivers/staging/qlge/Kconfig
> @@ -3,6 +3,7 @@
>  config QLGE
>  	tristate "QLogic QLGE 10Gb Ethernet Driver Support"
>  	depends on ETHERNET && PCI
> +	select NET_DEVLINK
>  	help
>  	This driver supports QLogic ISP8XXX 10Gb Ethernet cards.
>  
> diff --git a/drivers/staging/qlge/Makefile b/drivers/staging/qlge/Makefile
> index 1dc2568e820c..07c1898a512e 100644
> --- a/drivers/staging/qlge/Makefile
> +++ b/drivers/staging/qlge/Makefile
> @@ -5,4 +5,4 @@
>  
>  obj-$(CONFIG_QLGE) += qlge.o
>  
> -qlge-objs := qlge_main.o qlge_dbg.o qlge_mpi.o qlge_ethtool.o
> +qlge-objs := qlge_main.o qlge_dbg.o qlge_mpi.o qlge_ethtool.o qlge_devlink.o
> diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
> index b295990e361b..290e754450c5 100644
> --- a/drivers/staging/qlge/qlge.h
> +++ b/drivers/staging/qlge/qlge.h
> @@ -2060,6 +2060,14 @@ struct nic_operations {
>  	int (*port_initialize)(struct ql_adapter *qdev);
>  };
>  
> +
> +
> +struct qlge_devlink {
> +        struct ql_adapter *qdev;
> +        struct net_device *ndev;

This member should be removed, it is unused throughout the rest of the
series. Indeed, it's simple to use qdev->ndev and that's what
qlge_reporter_coredump() does.

Re: [PATCH v1 1/6] staging: qlge: Initialize devlink health dump framework for the dlge driver

[Coiby Xu]

On Sat, Oct 10, 2020 at 04:35:14PM +0900, Benjamin Poirier wrote:
>On 2020-10-08 19:58 +0800, Coiby Xu wrote:
>> Initialize devlink health dump framework for the dlge driver so the
>> coredump could be done via devlink.
>>
>> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
>> ---
>>  drivers/staging/qlge/Kconfig        |  1 +
>>  drivers/staging/qlge/Makefile       |  2 +-
>>  drivers/staging/qlge/qlge.h         |  9 +++++++
>>  drivers/staging/qlge/qlge_devlink.c | 38 +++++++++++++++++++++++++++++
>>  drivers/staging/qlge/qlge_devlink.h |  8 ++++++
>>  drivers/staging/qlge/qlge_main.c    | 28 +++++++++++++++++++++
>>  6 files changed, 85 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/staging/qlge/qlge_devlink.c
>>  create mode 100644 drivers/staging/qlge/qlge_devlink.h
>>
>> diff --git a/drivers/staging/qlge/Kconfig b/drivers/staging/qlge/Kconfig
>> index a3cb25a3ab80..6d831ed67965 100644
>> --- a/drivers/staging/qlge/Kconfig
>> +++ b/drivers/staging/qlge/Kconfig
>> @@ -3,6 +3,7 @@
>>  config QLGE
>>  	tristate "QLogic QLGE 10Gb Ethernet Driver Support"
>>  	depends on ETHERNET && PCI
>> +	select NET_DEVLINK
>>  	help
>>  	This driver supports QLogic ISP8XXX 10Gb Ethernet cards.
>>
>> diff --git a/drivers/staging/qlge/Makefile b/drivers/staging/qlge/Makefile
>> index 1dc2568e820c..07c1898a512e 100644
>> --- a/drivers/staging/qlge/Makefile
>> +++ b/drivers/staging/qlge/Makefile
>> @@ -5,4 +5,4 @@
>>
>>  obj-$(CONFIG_QLGE) += qlge.o
>>
>> -qlge-objs := qlge_main.o qlge_dbg.o qlge_mpi.o qlge_ethtool.o
>> +qlge-objs := qlge_main.o qlge_dbg.o qlge_mpi.o qlge_ethtool.o qlge_devlink.o
>> diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
>> index b295990e361b..290e754450c5 100644
>> --- a/drivers/staging/qlge/qlge.h
>> +++ b/drivers/staging/qlge/qlge.h
>> @@ -2060,6 +2060,14 @@ struct nic_operations {
>>  	int (*port_initialize)(struct ql_adapter *qdev);
>>  };
>>
>> +
>> +
>> +struct qlge_devlink {
>> +        struct ql_adapter *qdev;
>> +        struct net_device *ndev;
>
>This member should be removed, it is unused throughout the rest of the
>series. Indeed, it's simple to use qdev->ndev and that's what
>qlge_reporter_coredump() does.

It reminds me that I forgot to reply to one of your comments in RFC and
sorry for that,
>> +
>> +
>> +struct qlge_devlink {
>> +        struct ql_adapter *qdev;
>> +        struct net_device *ndev;
>
>I don't have experience implementing devlink callbacks but looking at
>some other devlink users (mlx4, ionic, ice), all of them use devlink
>priv space for their main private structure. That would be struct
>ql_adapter in this case. Is there a good reason to stray from that
>pattern?

struct ql_adapter which is created via alloc_etherdev_mq is the
private space of struct net_device so we can't use ql_adapter as the
the devlink private space simultaneously. Thus struct qlge_devlink is
required.

--
Best regards,
Coiby

Re: [PATCH v1 1/6] staging: qlge: Initialize devlink health dump framework for the dlge driver

[Benjamin Poirier]

On 2020-10-10 18:24 +0800, Coiby Xu wrote:
> On Sat, Oct 10, 2020 at 04:35:14PM +0900, Benjamin Poirier wrote:
> > On 2020-10-08 19:58 +0800, Coiby Xu wrote:
> > > Initialize devlink health dump framework for the dlge driver so the
> > > coredump could be done via devlink.
> > > 
> > > Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> > > ---
> > >  drivers/staging/qlge/Kconfig        |  1 +
> > >  drivers/staging/qlge/Makefile       |  2 +-
> > >  drivers/staging/qlge/qlge.h         |  9 +++++++
> > >  drivers/staging/qlge/qlge_devlink.c | 38 +++++++++++++++++++++++++++++
> > >  drivers/staging/qlge/qlge_devlink.h |  8 ++++++
> > >  drivers/staging/qlge/qlge_main.c    | 28 +++++++++++++++++++++
> > >  6 files changed, 85 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/staging/qlge/qlge_devlink.c
> > >  create mode 100644 drivers/staging/qlge/qlge_devlink.h
> > > 
> > > diff --git a/drivers/staging/qlge/Kconfig b/drivers/staging/qlge/Kconfig
> > > index a3cb25a3ab80..6d831ed67965 100644
> > > --- a/drivers/staging/qlge/Kconfig
> > > +++ b/drivers/staging/qlge/Kconfig
> > > @@ -3,6 +3,7 @@
> > >  config QLGE
> > >  	tristate "QLogic QLGE 10Gb Ethernet Driver Support"
> > >  	depends on ETHERNET && PCI
> > > +	select NET_DEVLINK
> > >  	help
> > >  	This driver supports QLogic ISP8XXX 10Gb Ethernet cards.
> > > 
> > > diff --git a/drivers/staging/qlge/Makefile b/drivers/staging/qlge/Makefile
> > > index 1dc2568e820c..07c1898a512e 100644
> > > --- a/drivers/staging/qlge/Makefile
> > > +++ b/drivers/staging/qlge/Makefile
> > > @@ -5,4 +5,4 @@
> > > 
> > >  obj-$(CONFIG_QLGE) += qlge.o
> > > 
> > > -qlge-objs := qlge_main.o qlge_dbg.o qlge_mpi.o qlge_ethtool.o
> > > +qlge-objs := qlge_main.o qlge_dbg.o qlge_mpi.o qlge_ethtool.o qlge_devlink.o
> > > diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
> > > index b295990e361b..290e754450c5 100644
> > > --- a/drivers/staging/qlge/qlge.h
> > > +++ b/drivers/staging/qlge/qlge.h
> > > @@ -2060,6 +2060,14 @@ struct nic_operations {
> > >  	int (*port_initialize)(struct ql_adapter *qdev);
> > >  };
> > > 
> > > +
> > > +
> > > +struct qlge_devlink {
> > > +        struct ql_adapter *qdev;
> > > +        struct net_device *ndev;
> > 
> > This member should be removed, it is unused throughout the rest of the
> > series. Indeed, it's simple to use qdev->ndev and that's what
> > qlge_reporter_coredump() does.
> 
> It reminds me that I forgot to reply to one of your comments in RFC and
> sorry for that,
> > > +
> > > +
> > > +struct qlge_devlink {
> > > +        struct ql_adapter *qdev;
> > > +        struct net_device *ndev;
> > 
> > I don't have experience implementing devlink callbacks but looking at
> > some other devlink users (mlx4, ionic, ice), all of them use devlink
> > priv space for their main private structure. That would be struct
> > ql_adapter in this case. Is there a good reason to stray from that
> > pattern?

Thanks for getting back to that comment.

> 
> struct ql_adapter which is created via alloc_etherdev_mq is the
> private space of struct net_device so we can't use ql_adapter as the
> the devlink private space simultaneously. Thus struct qlge_devlink is
> required.

Looking at those drivers (mlx4, ionic, ice), the usage of
alloc_etherdev_mq() is not really an obstacle. Definitely, some members
would need to be moved from ql_adapter to qlge_devlink to use that
pattern.

I think, but didn't check in depth, that in those drivers, the devlink
device is tied to the pci device and can exist independently of the
netdev, at least in principle.

In any case, I see now that some other drivers (bnxt, liquidio) have a
pattern similar to what you use so I guess that's acceptable too.

Re: [PATCH v1 1/6] staging: qlge: Initialize devlink health dump framework for the dlge driver

[Coiby Xu]

On Sat, Oct 10, 2020 at 10:48:55PM +0900, Benjamin Poirier wrote:
>On 2020-10-10 18:24 +0800, Coiby Xu wrote:
>> On Sat, Oct 10, 2020 at 04:35:14PM +0900, Benjamin Poirier wrote:
>> > On 2020-10-08 19:58 +0800, Coiby Xu wrote:
>> > > Initialize devlink health dump framework for the dlge driver so the
>> > > coredump could be done via devlink.
>> > >
>> > > Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
>> > > ---
>> > >  drivers/staging/qlge/Kconfig        |  1 +
>> > >  drivers/staging/qlge/Makefile       |  2 +-
>> > >  drivers/staging/qlge/qlge.h         |  9 +++++++
>> > >  drivers/staging/qlge/qlge_devlink.c | 38 +++++++++++++++++++++++++++++
>> > >  drivers/staging/qlge/qlge_devlink.h |  8 ++++++
>> > >  drivers/staging/qlge/qlge_main.c    | 28 +++++++++++++++++++++
>> > >  6 files changed, 85 insertions(+), 1 deletion(-)
>> > >  create mode 100644 drivers/staging/qlge/qlge_devlink.c
>> > >  create mode 100644 drivers/staging/qlge/qlge_devlink.h
>> > >
>> > > diff --git a/drivers/staging/qlge/Kconfig b/drivers/staging/qlge/Kconfig
>> > > index a3cb25a3ab80..6d831ed67965 100644
>> > > --- a/drivers/staging/qlge/Kconfig
>> > > +++ b/drivers/staging/qlge/Kconfig
>> > > @@ -3,6 +3,7 @@
>> > >  config QLGE
>> > >  	tristate "QLogic QLGE 10Gb Ethernet Driver Support"
>> > >  	depends on ETHERNET && PCI
>> > > +	select NET_DEVLINK
>> > >  	help
>> > >  	This driver supports QLogic ISP8XXX 10Gb Ethernet cards.
>> > >
>> > > diff --git a/drivers/staging/qlge/Makefile b/drivers/staging/qlge/Makefile
>> > > index 1dc2568e820c..07c1898a512e 100644
>> > > --- a/drivers/staging/qlge/Makefile
>> > > +++ b/drivers/staging/qlge/Makefile
>> > > @@ -5,4 +5,4 @@
>> > >
>> > >  obj-$(CONFIG_QLGE) += qlge.o
>> > >
>> > > -qlge-objs := qlge_main.o qlge_dbg.o qlge_mpi.o qlge_ethtool.o
>> > > +qlge-objs := qlge_main.o qlge_dbg.o qlge_mpi.o qlge_ethtool.o qlge_devlink.o
>> > > diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
>> > > index b295990e361b..290e754450c5 100644
>> > > --- a/drivers/staging/qlge/qlge.h
>> > > +++ b/drivers/staging/qlge/qlge.h
>> > > @@ -2060,6 +2060,14 @@ struct nic_operations {
>> > >  	int (*port_initialize)(struct ql_adapter *qdev);
>> > >  };
>> > >
>> > > +
>> > > +
>> > > +struct qlge_devlink {
>> > > +        struct ql_adapter *qdev;
>> > > +        struct net_device *ndev;
>> >
>> > This member should be removed, it is unused throughout the rest of the
>> > series. Indeed, it's simple to use qdev->ndev and that's what
>> > qlge_reporter_coredump() does.
>>
>> It reminds me that I forgot to reply to one of your comments in RFC and
>> sorry for that,
>> > > +
>> > > +
>> > > +struct qlge_devlink {
>> > > +        struct ql_adapter *qdev;
>> > > +        struct net_device *ndev;
>> >
>> > I don't have experience implementing devlink callbacks but looking at
>> > some other devlink users (mlx4, ionic, ice), all of them use devlink
>> > priv space for their main private structure. That would be struct
>> > ql_adapter in this case. Is there a good reason to stray from that
>> > pattern?
>
>Thanks for getting back to that comment.
>
>>
>> struct ql_adapter which is created via alloc_etherdev_mq is the
>> private space of struct net_device so we can't use ql_adapter as the
>> the devlink private space simultaneously. Thus struct qlge_devlink is
>> required.
>
>Looking at those drivers (mlx4, ionic, ice), the usage of
>alloc_etherdev_mq() is not really an obstacle. Definitely, some members
>would need to be moved from ql_adapter to qlge_devlink to use that
>pattern.
>
I see what you mean now. I thought we were going to use struct ql_adapter
as the shared private structure of devlink and net_device. If we are
going to use ql_adapter as the private structure of devlink then we have
to define another private structure for net_device.

>I think, but didn't check in depth, that in those drivers, the devlink
>device is tied to the pci device and can exist independently of the
>netdev, at least in principle.
>
You are right. Take drivers/net/ethernet/mellanox/mlxsw as an example,
devlink reload would first first unregister_netdev and then
register_netdev but struct devlink stays put. But I have yet to
understand when unregister/register_netdev is needed. Do we need to
add "devlink reload" for qlge?

>In any case, I see now that some other drivers (bnxt, liquidio) have a
>pattern similar to what you use so I guess that's acceptable too.

--
Best regards,
Coiby

Re: [PATCH v1 1/6] staging: qlge: Initialize devlink health dump framework for the dlge driver

[Benjamin Poirier]

On 2020-10-12 19:24 +0800, Coiby Xu wrote:
[...]
> > I think, but didn't check in depth, that in those drivers, the devlink
> > device is tied to the pci device and can exist independently of the
> > netdev, at least in principle.
> > 
> You are right. Take drivers/net/ethernet/mellanox/mlxsw as an example,
> devlink reload would first first unregister_netdev and then
> register_netdev but struct devlink stays put. But I have yet to
> understand when unregister/register_netdev is needed.

Maybe it can be useful to manually recover if the hardware or driver
gets in an erroneous state. I've used `modprobe -r qlge && modprobe
qlge` for the same in the past.

> Do we need to
> add "devlink reload" for qlge?

Not for this patchset. That would be a new feature.

Re: [PATCH v1 1/6] staging: qlge: Initialize devlink health dump framework for the dlge driver

[Coiby Xu]

On Tue, Oct 13, 2020 at 09:37:04AM +0900, Benjamin Poirier wrote:
>On 2020-10-12 19:24 +0800, Coiby Xu wrote:
>[...]
>> > I think, but didn't check in depth, that in those drivers, the devlink
>> > device is tied to the pci device and can exist independently of the
>> > netdev, at least in principle.
>> >
>> You are right. Take drivers/net/ethernet/mellanox/mlxsw as an example,
>> devlink reload would first first unregister_netdev and then
>> register_netdev but struct devlink stays put. But I have yet to
>> understand when unregister/register_netdev is needed.
>
>Maybe it can be useful to manually recover if the hardware or driver
>gets in an erroneous state. I've used `modprobe -r qlge && modprobe
>qlge` for the same in the past.

Thank you for providing this user case!
>
>> Do we need to
>> add "devlink reload" for qlge?
>
>Not for this patchset. That would be a new feature.

To implement this feature, it seems I need to understand how qlge work
under the hood. For example, what's the difference between
qlge_soft_reset_mpi_risc and qlge_hard_reset_mpi_risc? Or should we use
a brute-force way like do the tasks in qlge_remove and then re-do the
tasks in qlge_probe? Is a hardware reference manual for qlge device?

--
Best regards,
Coiby

Re: [PATCH v1 1/6] staging: qlge: Initialize devlink health dump framework for the dlge driver

[Benjamin Poirier]

On 2020-10-15 11:37 +0800, Coiby Xu wrote:
> On Tue, Oct 13, 2020 at 09:37:04AM +0900, Benjamin Poirier wrote:
> > On 2020-10-12 19:24 +0800, Coiby Xu wrote:
> > [...]
> > > > I think, but didn't check in depth, that in those drivers, the devlink
> > > > device is tied to the pci device and can exist independently of the
> > > > netdev, at least in principle.
> > > >
> > > You are right. Take drivers/net/ethernet/mellanox/mlxsw as an example,
> > > devlink reload would first first unregister_netdev and then
> > > register_netdev but struct devlink stays put. But I have yet to
> > > understand when unregister/register_netdev is needed.
> > 
> > Maybe it can be useful to manually recover if the hardware or driver
> > gets in an erroneous state. I've used `modprobe -r qlge && modprobe
> > qlge` for the same in the past.
> 
> Thank you for providing this user case!
> > 
> > > Do we need to
> > > add "devlink reload" for qlge?
> > 
> > Not for this patchset. That would be a new feature.
> 
> To implement this feature, it seems I need to understand how qlge work
> under the hood. For example, what's the difference between
> qlge_soft_reset_mpi_risc and qlge_hard_reset_mpi_risc? Or should we use
> a brute-force way like do the tasks in qlge_remove and then re-do the
> tasks in qlge_probe?

I don't know. Like I've said before, I'd recommend testing on actual
hardware. I don't have access to it anymore.

> Is a hardware reference manual for qlge device?

I've never gotten access to one.

The only noteworthy thing from Qlogic that I know of is the firmware
update:
http://driverdownloads.qlogic.com/QLogicDriverDownloads_UI/SearchByProduct.aspx?ProductCategory=322&Product=1104&Os=190

It did fix some weird behavior when I applied it so I'd recommend doing
the same if you get an adapter.

Re: [PATCH v1 1/6] staging: qlge: Initialize devlink health dump framework for the dlge driver

[Coiby Xu]

On Thu, Oct 15, 2020 at 08:06:06PM +0900, Benjamin Poirier wrote:
>On 2020-10-15 11:37 +0800, Coiby Xu wrote:
>> On Tue, Oct 13, 2020 at 09:37:04AM +0900, Benjamin Poirier wrote:
>> > On 2020-10-12 19:24 +0800, Coiby Xu wrote:
>> > [...]
>> > > > I think, but didn't check in depth, that in those drivers, the devlink
>> > > > device is tied to the pci device and can exist independently of the
>> > > > netdev, at least in principle.
>> > > >
>> > > You are right. Take drivers/net/ethernet/mellanox/mlxsw as an example,
>> > > devlink reload would first first unregister_netdev and then
>> > > register_netdev but struct devlink stays put. But I have yet to
>> > > understand when unregister/register_netdev is needed.
>> >
>> > Maybe it can be useful to manually recover if the hardware or driver
>> > gets in an erroneous state. I've used `modprobe -r qlge && modprobe
>> > qlge` for the same in the past.
>>
>> Thank you for providing this user case!
>> >
>> > > Do we need to
>> > > add "devlink reload" for qlge?
>> >
>> > Not for this patchset. That would be a new feature.
>>
>> To implement this feature, it seems I need to understand how qlge work
>> under the hood. For example, what's the difference between
>> qlge_soft_reset_mpi_risc and qlge_hard_reset_mpi_risc? Or should we use
>> a brute-force way like do the tasks in qlge_remove and then re-do the
>> tasks in qlge_probe?
>
>I don't know. Like I've said before, I'd recommend testing on actual
>hardware. I don't have access to it anymore.

Yeah, as I'm changing more code, it's more and more important to test
it on actual hardware. Have you heard anyone installing qle8142 to
Raspberry Pi which has a PCIe bus.
>
>> Is a hardware reference manual for qlge device?
>
>I've never gotten access to one.
>
My experience of wrestling with an AMD GPIO chip [1] shows it would
be a bit annoying to deal with a device without a reference manual.
I have to treat it like a blackbox and try different kinds of input
to see what would happen.

Btw, it seems resetting the device is a kind of panacea. For example,
according to the specs of my touchpad (Synaptics RMI4 Specification),
it even has the feature of spontaneous reset. devlink health [2] also
has the so-called auto-recovery. So resetting is a common phenomenon. I
wonder if there are some common guidelines to do resetting which also
apply to the qlge8*** devices.

>The only noteworthy thing from Qlogic that I know of is the firmware
>update:
>http://driverdownloads.qlogic.com/QLogicDriverDownloads_UI/SearchByProduct.aspx?ProductCategory=322&Product=1104&Os=190
>
>It did fix some weird behavior when I applied it so I'd recommend doing
>the same if you get an adapter.

Thank you for sharing the info!


[1] https://www.spinics.net/lists/linux-gpio/msg53901.html
[2] https://www.kernel.org/doc/html/latest/networking/devlink/devlink-health.html

--
Best regards,
Coiby

[PATCH v1 2/6] staging: qlge: coredump via devlink health reporter

[Coiby Xu]

    $ devlink health dump show DEVICE reporter coredump -p -j
    {
        "Core Registers": {
            "segment": 1,
            "values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0
,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ]
        },
        "Test Logic Regs": {
            "segment": 2,
            "values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ]
        },
        "RMII Registers": {
            "segment": 3,
            "values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ]
        },
        ...
        "Sem Registers": {
            "segment": 50,
            "values": [ 0,0,0,0 ]
        }
    }

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/qlge_devlink.c | 131 ++++++++++++++++++++++++++--
 1 file changed, 125 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/qlge/qlge_devlink.c b/drivers/staging/qlge/qlge_devlink.c
index aa45e7e368c0..91b6600b94a9 100644
--- a/drivers/staging/qlge/qlge_devlink.c
+++ b/drivers/staging/qlge/qlge_devlink.c
@@ -1,16 +1,135 @@
 #include "qlge.h"
 #include "qlge_devlink.h"
 
-static int
-qlge_reporter_coredump(struct devlink_health_reporter *reporter,
-			struct devlink_fmsg *fmsg, void *priv_ctx,
-			struct netlink_ext_ack *extack)
+static int fill_seg_(struct devlink_fmsg *fmsg,
+		    struct mpi_coredump_segment_header *seg_header,
+		    u32 *reg_data)
 {
-	return 0;
+	int i;
+	int header_size = sizeof(struct mpi_coredump_segment_header);
+	int regs_num = (seg_header->seg_size - header_size) / sizeof(u32);
+	int err;
+
+	err = devlink_fmsg_pair_nest_start(fmsg, seg_header->description);
+	if (err)
+		return err;
+	err = devlink_fmsg_obj_nest_start(fmsg);
+	if (err)
+		return err;
+	err = devlink_fmsg_u32_pair_put(fmsg, "segment", seg_header->seg_num);
+	if (err)
+		return err;
+	err = devlink_fmsg_arr_pair_nest_start(fmsg, "values");
+	if (err)
+		return err;
+	for (i = 0; i < regs_num; i++) {
+		err = devlink_fmsg_u32_put(fmsg, *reg_data);
+		if (err)
+			return err;
+		reg_data++;
+	}
+	err = devlink_fmsg_obj_nest_end(fmsg);
+	if (err)
+		return err;
+	err = devlink_fmsg_arr_pair_nest_end(fmsg);
+	if (err)
+		return err;
+	err = devlink_fmsg_pair_nest_end(fmsg);
+	return err;
+}
+
+#define fill_seg(seg_hdr, seg_regs)			               \
+	do {                                                           \
+		err = fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \
+		if (err) {					       \
+			kvfree(dump);                                  \
+			return err;				       \
+		}                                                      \
+	} while (0)
+
+static int qlge_reporter_coredump(struct devlink_health_reporter *reporter,
+				  struct devlink_fmsg *fmsg, void *priv_ctx,
+				  struct netlink_ext_ack *extack)
+{
+	int err = 0;
+
+	struct qlge_devlink *dev = devlink_health_reporter_priv(reporter);
+	struct ql_adapter *qdev = dev->qdev;
+	struct ql_mpi_coredump *dump;
+
+	if (!netif_running(qdev->ndev))
+		return 0;
+
+	dump = kvmalloc(sizeof(*dump), GFP_KERNEL);
+	if (!dump)
+		return -ENOMEM;
+
+	err = ql_core_dump(qdev, dump);
+	if (err) {
+		kvfree(dump);
+		return err;
+	}
+
+	fill_seg(core_regs_seg_hdr, mpi_core_regs);
+	fill_seg(test_logic_regs_seg_hdr, test_logic_regs);
+	fill_seg(rmii_regs_seg_hdr, rmii_regs);
+	fill_seg(fcmac1_regs_seg_hdr, fcmac1_regs);
+	fill_seg(fcmac2_regs_seg_hdr, fcmac2_regs);
+	fill_seg(fc1_mbx_regs_seg_hdr, fc1_mbx_regs);
+	fill_seg(ide_regs_seg_hdr, ide_regs);
+	fill_seg(nic1_mbx_regs_seg_hdr, nic1_mbx_regs);
+	fill_seg(smbus_regs_seg_hdr, smbus_regs);
+	fill_seg(fc2_mbx_regs_seg_hdr, fc2_mbx_regs);
+	fill_seg(nic2_mbx_regs_seg_hdr, nic2_mbx_regs);
+	fill_seg(i2c_regs_seg_hdr, i2c_regs);
+	fill_seg(memc_regs_seg_hdr, memc_regs);
+	fill_seg(pbus_regs_seg_hdr, pbus_regs);
+	fill_seg(mde_regs_seg_hdr, mde_regs);
+	fill_seg(nic_regs_seg_hdr, nic_regs);
+	fill_seg(nic2_regs_seg_hdr, nic2_regs);
+	fill_seg(xgmac1_seg_hdr, xgmac1);
+	fill_seg(xgmac2_seg_hdr, xgmac2);
+	fill_seg(code_ram_seg_hdr, code_ram);
+	fill_seg(memc_ram_seg_hdr, memc_ram);
+	fill_seg(xaui_an_hdr, serdes_xaui_an);
+	fill_seg(xaui_hss_pcs_hdr, serdes_xaui_hss_pcs);
+	fill_seg(xfi_an_hdr, serdes_xfi_an);
+	fill_seg(xfi_train_hdr, serdes_xfi_train);
+	fill_seg(xfi_hss_pcs_hdr, serdes_xfi_hss_pcs);
+	fill_seg(xfi_hss_tx_hdr, serdes_xfi_hss_tx);
+	fill_seg(xfi_hss_rx_hdr, serdes_xfi_hss_rx);
+	fill_seg(xfi_hss_pll_hdr, serdes_xfi_hss_pll);
+
+	err = fill_seg_(fmsg, &dump->misc_nic_seg_hdr,
+			(u32 *)&dump->misc_nic_info);
+	if (err) {
+		kvfree(dump);
+		return err;
+	}
+
+	fill_seg(intr_states_seg_hdr, intr_states);
+	fill_seg(cam_entries_seg_hdr, cam_entries);
+	fill_seg(nic_routing_words_seg_hdr, nic_routing_words);
+	fill_seg(ets_seg_hdr, ets);
+	fill_seg(probe_dump_seg_hdr, probe_dump);
+	fill_seg(routing_reg_seg_hdr, routing_regs);
+	fill_seg(mac_prot_reg_seg_hdr, mac_prot_regs);
+	fill_seg(xaui2_an_hdr, serdes2_xaui_an);
+	fill_seg(xaui2_hss_pcs_hdr, serdes2_xaui_hss_pcs);
+	fill_seg(xfi2_an_hdr, serdes2_xfi_an);
+	fill_seg(xfi2_train_hdr, serdes2_xfi_train);
+	fill_seg(xfi2_hss_pcs_hdr, serdes2_xfi_hss_pcs);
+	fill_seg(xfi2_hss_tx_hdr, serdes2_xfi_hss_tx);
+	fill_seg(xfi2_hss_rx_hdr, serdes2_xfi_hss_rx);
+	fill_seg(xfi2_hss_pll_hdr, serdes2_xfi_hss_pll);
+	fill_seg(sem_regs_seg_hdr, sem_regs);
+
+	kvfree(dump);
+	return err;
 }
 
 static const struct devlink_health_reporter_ops qlge_reporter_ops = {
-		.name = "dummy",
+		.name = "coredump",
 		.dump = qlge_reporter_coredump,
 };
 
-- 
2.28.0

Re: [PATCH v1 2/6] staging: qlge: coredump via devlink health reporter

[Dan Carpenter]

On Thu, Oct 08, 2020 at 07:58:04PM +0800, Coiby Xu wrote:
> -static int
> -qlge_reporter_coredump(struct devlink_health_reporter *reporter,
> -			struct devlink_fmsg *fmsg, void *priv_ctx,
> -			struct netlink_ext_ack *extack)
> +static int fill_seg_(struct devlink_fmsg *fmsg,
> +		    struct mpi_coredump_segment_header *seg_header,
> +		    u32 *reg_data)
>  {
> -	return 0;
> +	int i;
> +	int header_size = sizeof(struct mpi_coredump_segment_header);

Please use the sizeof() directly in the code.  Don't introduce
indirection if you can help it.

> +	int regs_num = (seg_header->seg_size - header_size) / sizeof(u32);
> +	int err;
> +

regards,
dan carpenter

Re: [PATCH v1 2/6] staging: qlge: coredump via devlink health reporter

[Coiby Xu]

On Thu, Oct 08, 2020 at 04:39:40PM +0300, Dan Carpenter wrote:
>On Thu, Oct 08, 2020 at 07:58:04PM +0800, Coiby Xu wrote:
>> -static int
>> -qlge_reporter_coredump(struct devlink_health_reporter *reporter,
>> -			struct devlink_fmsg *fmsg, void *priv_ctx,
>> -			struct netlink_ext_ack *extack)
>> +static int fill_seg_(struct devlink_fmsg *fmsg,
>> +		    struct mpi_coredump_segment_header *seg_header,
>> +		    u32 *reg_data)
>>  {
>> -	return 0;
>> +	int i;
>> +	int header_size = sizeof(struct mpi_coredump_segment_header);
>
>Please use the sizeof() directly in the code.  Don't introduce
>indirection if you can help it.
>
>> +	int regs_num = (seg_header->seg_size - header_size) / sizeof(u32);
>> +	int err;
>> +
>
Thank you for the suggestion!
>regards,
>dan carpenter

--
Best regards,
Coiby

Re: [PATCH v1 2/6] staging: qlge: coredump via devlink health reporter

[Benjamin Poirier]

On 2020-10-08 19:58 +0800, Coiby Xu wrote:
>     $ devlink health dump show DEVICE reporter coredump -p -j
>     {
>         "Core Registers": {
>             "segment": 1,
>             "values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0
> ,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ]
>         },
>         "Test Logic Regs": {
>             "segment": 2,
>             "values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ]
>         },
>         "RMII Registers": {
>             "segment": 3,
>             "values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ]
>         },
>         ...
>         "Sem Registers": {
>             "segment": 50,
>             "values": [ 0,0,0,0 ]
>         }
>     }
> 
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
>  drivers/staging/qlge/qlge_devlink.c | 131 ++++++++++++++++++++++++++--
>  1 file changed, 125 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/qlge/qlge_devlink.c b/drivers/staging/qlge/qlge_devlink.c
> index aa45e7e368c0..91b6600b94a9 100644
> --- a/drivers/staging/qlge/qlge_devlink.c
> +++ b/drivers/staging/qlge/qlge_devlink.c
> @@ -1,16 +1,135 @@
>  #include "qlge.h"
>  #include "qlge_devlink.h"
>  
> -static int
> -qlge_reporter_coredump(struct devlink_health_reporter *reporter,
> -			struct devlink_fmsg *fmsg, void *priv_ctx,
> -			struct netlink_ext_ack *extack)
> +static int fill_seg_(struct devlink_fmsg *fmsg,

Please include the "qlge_" prefix.

> +		    struct mpi_coredump_segment_header *seg_header,
> +		    u32 *reg_data)
>  {
> -	return 0;
> +	int i;
> +	int header_size = sizeof(struct mpi_coredump_segment_header);
> +	int regs_num = (seg_header->seg_size - header_size) / sizeof(u32);
> +	int err;
> +
> +	err = devlink_fmsg_pair_nest_start(fmsg, seg_header->description);
> +	if (err)
> +		return err;
> +	err = devlink_fmsg_obj_nest_start(fmsg);
> +	if (err)
> +		return err;
> +	err = devlink_fmsg_u32_pair_put(fmsg, "segment", seg_header->seg_num);
> +	if (err)
> +		return err;
> +	err = devlink_fmsg_arr_pair_nest_start(fmsg, "values");
> +	if (err)
> +		return err;
> +	for (i = 0; i < regs_num; i++) {
> +		err = devlink_fmsg_u32_put(fmsg, *reg_data);
> +		if (err)
> +			return err;
> +		reg_data++;
> +	}
> +	err = devlink_fmsg_obj_nest_end(fmsg);
> +	if (err)
> +		return err;
> +	err = devlink_fmsg_arr_pair_nest_end(fmsg);
> +	if (err)
> +		return err;
> +	err = devlink_fmsg_pair_nest_end(fmsg);
> +	return err;
> +}
> +
> +#define fill_seg(seg_hdr, seg_regs)			               \

considering that this macro accesses local variables, it is not really
"function-like". I think an all-caps name would be better to tip-off the
reader.

> +	do {                                                           \
> +		err = fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \
> +		if (err) {					       \
> +			kvfree(dump);                                  \
> +			return err;				       \
> +		}                                                      \
> +	} while (0)
> +
> +static int qlge_reporter_coredump(struct devlink_health_reporter *reporter,
> +				  struct devlink_fmsg *fmsg, void *priv_ctx,
> +				  struct netlink_ext_ack *extack)
> +{
> +	int err = 0;
> +
> +	struct qlge_devlink *dev = devlink_health_reporter_priv(reporter);

Please name this variable ql_devlink, like in qlge_probe().

Re: [PATCH v1 2/6] staging: qlge: coredump via devlink health reporter

[Coiby Xu]

On Sat, Oct 10, 2020 at 04:48:09PM +0900, Benjamin Poirier wrote:
>On 2020-10-08 19:58 +0800, Coiby Xu wrote:
>>     $ devlink health dump show DEVICE reporter coredump -p -j
>>     {
>>         "Core Registers": {
>>             "segment": 1,
>>             "values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0
>> ,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ]
>>         },
>>         "Test Logic Regs": {
>>             "segment": 2,
>>             "values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ]
>>         },
>>         "RMII Registers": {
>>             "segment": 3,
>>             "values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ]
>>         },
>>         ...
>>         "Sem Registers": {
>>             "segment": 50,
>>             "values": [ 0,0,0,0 ]
>>         }
>>     }
>>
>> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
>> ---
>>  drivers/staging/qlge/qlge_devlink.c | 131 ++++++++++++++++++++++++++--
>>  1 file changed, 125 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/qlge/qlge_devlink.c b/drivers/staging/qlge/qlge_devlink.c
>> index aa45e7e368c0..91b6600b94a9 100644
>> --- a/drivers/staging/qlge/qlge_devlink.c
>> +++ b/drivers/staging/qlge/qlge_devlink.c
>> @@ -1,16 +1,135 @@
>>  #include "qlge.h"
>>  #include "qlge_devlink.h"
>>
>> -static int
>> -qlge_reporter_coredump(struct devlink_health_reporter *reporter,
>> -			struct devlink_fmsg *fmsg, void *priv_ctx,
>> -			struct netlink_ext_ack *extack)
>> +static int fill_seg_(struct devlink_fmsg *fmsg,
>
>Please include the "qlge_" prefix.
>
>> +		    struct mpi_coredump_segment_header *seg_header,
>> +		    u32 *reg_data)
>>  {
>> -	return 0;
>> +	int i;
>> +	int header_size = sizeof(struct mpi_coredump_segment_header);
>> +	int regs_num = (seg_header->seg_size - header_size) / sizeof(u32);
>> +	int err;
>> +
>> +	err = devlink_fmsg_pair_nest_start(fmsg, seg_header->description);
>> +	if (err)
>> +		return err;
>> +	err = devlink_fmsg_obj_nest_start(fmsg);
>> +	if (err)
>> +		return err;
>> +	err = devlink_fmsg_u32_pair_put(fmsg, "segment", seg_header->seg_num);
>> +	if (err)
>> +		return err;
>> +	err = devlink_fmsg_arr_pair_nest_start(fmsg, "values");
>> +	if (err)
>> +		return err;
>> +	for (i = 0; i < regs_num; i++) {
>> +		err = devlink_fmsg_u32_put(fmsg, *reg_data);
>> +		if (err)
>> +			return err;
>> +		reg_data++;
>> +	}
>> +	err = devlink_fmsg_obj_nest_end(fmsg);
>> +	if (err)
>> +		return err;
>> +	err = devlink_fmsg_arr_pair_nest_end(fmsg);
>> +	if (err)
>> +		return err;
>> +	err = devlink_fmsg_pair_nest_end(fmsg);
>> +	return err;
>> +}
>> +
>> +#define fill_seg(seg_hdr, seg_regs)			               \
>
>considering that this macro accesses local variables, it is not really
>"function-like". I think an all-caps name would be better to tip-off the
>reader.
>
Thank you for this suggestion!

>> +	do {                                                           \
>> +		err = fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \
>> +		if (err) {					       \
>> +			kvfree(dump);                                  \
>> +			return err;				       \
>> +		}                                                      \
>> +	} while (0)
>> +
>> +static int qlge_reporter_coredump(struct devlink_health_reporter *reporter,
>> +				  struct devlink_fmsg *fmsg, void *priv_ctx,
>> +				  struct netlink_ext_ack *extack)
>> +{
>> +	int err = 0;
>> +
>> +	struct qlge_devlink *dev = devlink_health_reporter_priv(reporter);
>
>Please name this variable ql_devlink, like in qlge_probe().

I happened to find the following text in drivers/staging/qlge/TODO
> * in terms of namespace, the driver uses either qlge_, ql_ (used by
>  other qlogic drivers, with clashes, ex: ql_sem_spinlock) or nothing (with
>  clashes, ex: struct ob_mac_iocb_req). Rename everything to use the "qlge_"
>  prefix.

So I will adopt qlge_ instead. Besides I prefer qlge_dl to ql_devlink.

--
Best regards,
Coiby

Re: [PATCH v1 2/6] staging: qlge: coredump via devlink health reporter

[Benjamin Poirier]

On 2020-10-10 18:02 +0800, Coiby Xu wrote:
[...]
> > > +	do {                                                           \
> > > +		err = fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \
> > > +		if (err) {					       \
> > > +			kvfree(dump);                                  \
> > > +			return err;				       \
> > > +		}                                                      \
> > > +	} while (0)
> > > +
> > > +static int qlge_reporter_coredump(struct devlink_health_reporter *reporter,
> > > +				  struct devlink_fmsg *fmsg, void *priv_ctx,
> > > +				  struct netlink_ext_ack *extack)
> > > +{
> > > +	int err = 0;
> > > +
> > > +	struct qlge_devlink *dev = devlink_health_reporter_priv(reporter);
> > 
> > Please name this variable ql_devlink, like in qlge_probe().
> 
> I happened to find the following text in drivers/staging/qlge/TODO
> > * in terms of namespace, the driver uses either qlge_, ql_ (used by
> >  other qlogic drivers, with clashes, ex: ql_sem_spinlock) or nothing (with
> >  clashes, ex: struct ob_mac_iocb_req). Rename everything to use the "qlge_"
> >  prefix.

This comment applies to global identifiers, not local variables.

> 
> So I will adopt qlge_ instead. Besides I prefer qlge_dl to ql_devlink.

Up to you but personally, I think ql_devlink is better. In any case,
"dev" is too general and often used for struct net_device pointers
instead.

Re: [PATCH v1 2/6] staging: qlge: coredump via devlink health reporter

[Coiby Xu]

On Sat, Oct 10, 2020 at 10:22:30PM +0900, Benjamin Poirier wrote:
>On 2020-10-10 18:02 +0800, Coiby Xu wrote:
>[...]
>> > > +	do {                                                           \
>> > > +		err = fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \
>> > > +		if (err) {					       \
>> > > +			kvfree(dump);                                  \
>> > > +			return err;				       \
>> > > +		}                                                      \
>> > > +	} while (0)
>> > > +
>> > > +static int qlge_reporter_coredump(struct devlink_health_reporter *reporter,
>> > > +				  struct devlink_fmsg *fmsg, void *priv_ctx,
>> > > +				  struct netlink_ext_ack *extack)
>> > > +{
>> > > +	int err = 0;
>> > > +
>> > > +	struct qlge_devlink *dev = devlink_health_reporter_priv(reporter);
>> >
>> > Please name this variable ql_devlink, like in qlge_probe().
>>
>> I happened to find the following text in drivers/staging/qlge/TODO
>> > * in terms of namespace, the driver uses either qlge_, ql_ (used by
>> >  other qlogic drivers, with clashes, ex: ql_sem_spinlock) or nothing (with
>> >  clashes, ex: struct ob_mac_iocb_req). Rename everything to use the "qlge_"
>> >  prefix.
>
>This comment applies to global identifiers, not local variables.

Thank you for the explanation! Are you suggesting we should choose
different naming styles so we better tell global identifiers from local
variables?
>
>>
>> So I will adopt qlge_ instead. Besides I prefer qlge_dl to ql_devlink.
>
>Up to you but personally, I think ql_devlink is better. In any case,
>"dev" is too general and often used for struct net_device pointers
>instead.

Thank you for the suggestion. Another reason to use qlge_dl is many
other network drivers supporting devlink interface also adopt this kind
of style.

--
Best regards,
Coiby

Re: [PATCH v1 2/6] staging: qlge: coredump via devlink health reporter

[Benjamin Poirier]

On 2020-10-12 19:51 +0800, Coiby Xu wrote:
> On Sat, Oct 10, 2020 at 10:22:30PM +0900, Benjamin Poirier wrote:
> > On 2020-10-10 18:02 +0800, Coiby Xu wrote:
> > [...]
> > > > > +	do {                                                           \
> > > > > +		err = fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \
> > > > > +		if (err) {					       \
> > > > > +			kvfree(dump);                                  \
> > > > > +			return err;				       \
> > > > > +		}                                                      \
> > > > > +	} while (0)
> > > > > +
> > > > > +static int qlge_reporter_coredump(struct devlink_health_reporter *reporter,
> > > > > +				  struct devlink_fmsg *fmsg, void *priv_ctx,
> > > > > +				  struct netlink_ext_ack *extack)
> > > > > +{
> > > > > +	int err = 0;
> > > > > +
> > > > > +	struct qlge_devlink *dev = devlink_health_reporter_priv(reporter);
> > > >
> > > > Please name this variable ql_devlink, like in qlge_probe().
> > > 
> > > I happened to find the following text in drivers/staging/qlge/TODO
> > > > * in terms of namespace, the driver uses either qlge_, ql_ (used by
> > > >  other qlogic drivers, with clashes, ex: ql_sem_spinlock) or nothing (with
> > > >  clashes, ex: struct ob_mac_iocb_req). Rename everything to use the "qlge_"
> > > >  prefix.
> > 
> > This comment applies to global identifiers, not local variables.
> 
> Thank you for the explanation! Are you suggesting we should choose
> different naming styles so we better tell global identifiers from local
> variables?

That's not the main purpose IMO. Using a consistent prefix for global
identifiers (ex. "qlge_") is to avoid clashes (two drivers using the
same name, as in the examples above). Strictly speaking, it is not a
problem for symbols with internal linkage (ex. static functions) or type
definitions in local header files but it makes the code clearer because
it shows immediately that this identifier was defined in the driver.

For local variables, the name is more a matter of personal taste I think
but it should be consistent within the driver and with other users of
the same api, where applicable. A prefix is not needed but the name is
sometimes a simpler version of a type name which includes a prefix.

> > > So I will adopt qlge_ instead. Besides I prefer qlge_dl to ql_devlink.
> > 
> > Up to you but personally, I think ql_devlink is better. In any case,
> > "dev" is too general and often used for struct net_device pointers
> > instead.
> 
> Thank you for the suggestion. Another reason to use qlge_dl is many
> other network drivers supporting devlink interface also adopt this kind
> of style.

Sounds good. On second thought I regretted suggesting ql_devlink. While
local variable don't need any prefix; if they do have one, better not
mix qlge_ and ql_.

[PATCH v1 3/6] staging: qlge: support force_coredump option for devlink health dump

[Coiby Xu]

With force_coredump module paramter set, devlink health dump will reset
the MPI RISC first which takes 5 secs to be finished.

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/qlge_devlink.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/staging/qlge/qlge_devlink.c b/drivers/staging/qlge/qlge_devlink.c
index 91b6600b94a9..54257468bc7f 100644
--- a/drivers/staging/qlge/qlge_devlink.c
+++ b/drivers/staging/qlge/qlge_devlink.c
@@ -56,10 +56,17 @@ static int qlge_reporter_coredump(struct devlink_health_reporter *reporter,
 	struct qlge_devlink *dev = devlink_health_reporter_priv(reporter);
 	struct ql_adapter *qdev = dev->qdev;
 	struct ql_mpi_coredump *dump;
+	wait_queue_head_t wait;
 
 	if (!netif_running(qdev->ndev))
 		return 0;
 
+	if (test_bit(QL_FRC_COREDUMP, &qdev->flags)) {
+		ql_queue_fw_error(qdev);
+		init_waitqueue_head(&wait);
+		wait_event_timeout(wait, 0, 5 * HZ);
+	}
+
 	dump = kvmalloc(sizeof(*dump), GFP_KERNEL);
 	if (!dump)
 		return -ENOMEM;
-- 
2.28.0

[PATCH v1 4/6] staging: qlge: remove mpi_core_to_log which sends coredump to the kernel ring buffer

[Coiby Xu]

devlink health could be used to get coredump. No need to send so much
data to the kernel ring buffer.

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/qlge.h         |  3 ---
 drivers/staging/qlge/qlge_dbg.c     | 11 -----------
 drivers/staging/qlge/qlge_ethtool.c |  1 -
 drivers/staging/qlge/qlge_main.c    |  2 --
 drivers/staging/qlge/qlge_mpi.c     |  6 ------
 5 files changed, 23 deletions(-)

diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
index 290e754450c5..0a39801be15a 100644
--- a/drivers/staging/qlge/qlge.h
+++ b/drivers/staging/qlge/qlge.h
@@ -2149,7 +2149,6 @@ struct ql_adapter {
 	u32 port_init;
 	u32 link_status;
 	struct ql_mpi_coredump *mpi_coredump;
-	u32 core_is_dumped;
 	u32 link_config;
 	u32 led_config;
 	u32 max_frame_size;
@@ -2162,7 +2161,6 @@ struct ql_adapter {
 	struct delayed_work mpi_work;
 	struct delayed_work mpi_port_cfg_work;
 	struct delayed_work mpi_idc_work;
-	struct delayed_work mpi_core_to_log;
 	struct completion ide_completion;
 	const struct nic_operations *nic_ops;
 	u16 device_id;
@@ -2253,7 +2251,6 @@ int ql_write_cfg(struct ql_adapter *qdev, void *ptr, int size, u32 bit,
 void ql_queue_fw_error(struct ql_adapter *qdev);
 void ql_mpi_work(struct work_struct *work);
 void ql_mpi_reset_work(struct work_struct *work);
-void ql_mpi_core_to_log(struct work_struct *work);
 int ql_wait_reg_rdy(struct ql_adapter *qdev, u32 reg, u32 bit, u32 ebit);
 void ql_queue_asic_error(struct ql_adapter *qdev);
 void ql_set_ethtool_ops(struct net_device *ndev);
diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
index 42fd13990f3a..989575743718 100644
--- a/drivers/staging/qlge/qlge_dbg.c
+++ b/drivers/staging/qlge/qlge_dbg.c
@@ -1314,17 +1314,6 @@ void ql_get_dump(struct ql_adapter *qdev, void *buff)
 	}
 }
 
-/* Coredump to messages log file using separate worker thread */
-void ql_mpi_core_to_log(struct work_struct *work)
-{
-	struct ql_adapter *qdev =
-		container_of(work, struct ql_adapter, mpi_core_to_log.work);
-
-	print_hex_dump(KERN_DEBUG, "Core is dumping to log file!\n",
-		       DUMP_PREFIX_OFFSET, 32, 4, qdev->mpi_coredump,
-		       sizeof(*qdev->mpi_coredump), false);
-}
-
 #ifdef QL_REG_DUMP
 static void ql_dump_intr_states(struct ql_adapter *qdev)
 {
diff --git a/drivers/staging/qlge/qlge_ethtool.c b/drivers/staging/qlge/qlge_ethtool.c
index d44b2dae9213..eed116d8895e 100644
--- a/drivers/staging/qlge/qlge_ethtool.c
+++ b/drivers/staging/qlge/qlge_ethtool.c
@@ -616,7 +616,6 @@ static void ql_get_regs(struct net_device *ndev,
 	struct ql_adapter *qdev = netdev_priv(ndev);
 
 	ql_get_dump(qdev, p);
-	qdev->core_is_dumped = 0;
 	if (!test_bit(QL_FRC_COREDUMP, &qdev->flags))
 		regs->len = sizeof(struct ql_mpi_coredump);
 	else
diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index 135225530e51..aaca740d46c4 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -3808,7 +3808,6 @@ static void ql_cancel_all_work_sync(struct ql_adapter *qdev)
 	cancel_delayed_work_sync(&qdev->mpi_reset_work);
 	cancel_delayed_work_sync(&qdev->mpi_work);
 	cancel_delayed_work_sync(&qdev->mpi_idc_work);
-	cancel_delayed_work_sync(&qdev->mpi_core_to_log);
 	cancel_delayed_work_sync(&qdev->mpi_port_cfg_work);
 }
 
@@ -4504,7 +4503,6 @@ static int ql_init_device(struct pci_dev *pdev, struct net_device *ndev,
 	INIT_DELAYED_WORK(&qdev->mpi_work, ql_mpi_work);
 	INIT_DELAYED_WORK(&qdev->mpi_port_cfg_work, ql_mpi_port_cfg_work);
 	INIT_DELAYED_WORK(&qdev->mpi_idc_work, ql_mpi_idc_work);
-	INIT_DELAYED_WORK(&qdev->mpi_core_to_log, ql_mpi_core_to_log);
 	init_completion(&qdev->ide_completion);
 	mutex_init(&qdev->mpi_mutex);
 
diff --git a/drivers/staging/qlge/qlge_mpi.c b/drivers/staging/qlge/qlge_mpi.c
index 143a886080c5..1cea24201b17 100644
--- a/drivers/staging/qlge/qlge_mpi.c
+++ b/drivers/staging/qlge/qlge_mpi.c
@@ -1269,11 +1269,5 @@ void ql_mpi_reset_work(struct work_struct *work)
 		return;
 	}
 
-	if (qdev->mpi_coredump && !ql_core_dump(qdev, qdev->mpi_coredump)) {
-		netif_err(qdev, drv, qdev->ndev, "Core is dumped!\n");
-		qdev->core_is_dumped = 1;
-		queue_delayed_work(qdev->workqueue,
-				   &qdev->mpi_core_to_log, 5 * HZ);
-	}
 	ql_soft_reset_mpi_risc(qdev);
 }
-- 
2.28.0

[PATCH v1 5/6] staging: qlge: clean up debugging code in the QL_ALL_DUMP ifdef land

[Coiby Xu]

The debugging code in the following ifdef land
 - QL_ALL_DUMP
 - QL_REG_DUMP
 - QL_DEV_DUMP
 - QL_CB_DUMP
 - QL_IB_DUMP
 - QL_OB_DUMP

becomes unnecessary because,
 - Device status and general registers can be obtained by ethtool.
 - Coredump can be done via devlink health reporter.
 - Structure related to the hardware (struct ql_adapter) can be obtained
   by crash or drgn.

Suggested-by: Benjamin Poirier <benjamin.poirier@gmail.com>
Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/qlge.h         |  82 ----
 drivers/staging/qlge/qlge_dbg.c     | 688 ----------------------------
 drivers/staging/qlge/qlge_ethtool.c |   2 -
 drivers/staging/qlge/qlge_main.c    |   7 +-
 4 files changed, 1 insertion(+), 778 deletions(-)

diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
index 0a39801be15a..8aff3ba77730 100644
--- a/drivers/staging/qlge/qlge.h
+++ b/drivers/staging/qlge/qlge.h
@@ -2285,86 +2285,4 @@ void ql_check_lb_frame(struct ql_adapter *qdev, struct sk_buff *skb);
 int ql_own_firmware(struct ql_adapter *qdev);
 int ql_clean_lb_rx_ring(struct rx_ring *rx_ring, int budget);
 
-/* #define QL_ALL_DUMP */
-/* #define QL_REG_DUMP */
-/* #define QL_DEV_DUMP */
-/* #define QL_CB_DUMP */
-/* #define QL_IB_DUMP */
-/* #define QL_OB_DUMP */
-
-#ifdef QL_REG_DUMP
-void ql_dump_xgmac_control_regs(struct ql_adapter *qdev);
-void ql_dump_routing_entries(struct ql_adapter *qdev);
-void ql_dump_regs(struct ql_adapter *qdev);
-#define QL_DUMP_REGS(qdev) ql_dump_regs(qdev)
-#define QL_DUMP_ROUTE(qdev) ql_dump_routing_entries(qdev)
-#define QL_DUMP_XGMAC_CONTROL_REGS(qdev) ql_dump_xgmac_control_regs(qdev)
-#else
-#define QL_DUMP_REGS(qdev)
-#define QL_DUMP_ROUTE(qdev)
-#define QL_DUMP_XGMAC_CONTROL_REGS(qdev)
-#endif
-
-#ifdef QL_STAT_DUMP
-void ql_dump_stat(struct ql_adapter *qdev);
-#define QL_DUMP_STAT(qdev) ql_dump_stat(qdev)
-#else
-#define QL_DUMP_STAT(qdev)
-#endif
-
-#ifdef QL_DEV_DUMP
-void ql_dump_qdev(struct ql_adapter *qdev);
-#define QL_DUMP_QDEV(qdev) ql_dump_qdev(qdev)
-#else
-#define QL_DUMP_QDEV(qdev)
-#endif
-
-#ifdef QL_CB_DUMP
-void ql_dump_wqicb(struct wqicb *wqicb);
-void ql_dump_tx_ring(struct tx_ring *tx_ring);
-void ql_dump_ricb(struct ricb *ricb);
-void ql_dump_cqicb(struct cqicb *cqicb);
-void ql_dump_rx_ring(struct rx_ring *rx_ring);
-void ql_dump_hw_cb(struct ql_adapter *qdev, int size, u32 bit, u16 q_id);
-#define QL_DUMP_RICB(ricb) ql_dump_ricb(ricb)
-#define QL_DUMP_WQICB(wqicb) ql_dump_wqicb(wqicb)
-#define QL_DUMP_TX_RING(tx_ring) ql_dump_tx_ring(tx_ring)
-#define QL_DUMP_CQICB(cqicb) ql_dump_cqicb(cqicb)
-#define QL_DUMP_RX_RING(rx_ring) ql_dump_rx_ring(rx_ring)
-#define QL_DUMP_HW_CB(qdev, size, bit, q_id) \
-		ql_dump_hw_cb(qdev, size, bit, q_id)
-#else
-#define QL_DUMP_RICB(ricb)
-#define QL_DUMP_WQICB(wqicb)
-#define QL_DUMP_TX_RING(tx_ring)
-#define QL_DUMP_CQICB(cqicb)
-#define QL_DUMP_RX_RING(rx_ring)
-#define QL_DUMP_HW_CB(qdev, size, bit, q_id)
-#endif
-
-#ifdef QL_OB_DUMP
-void ql_dump_tx_desc(struct ql_adapter *qdev, struct tx_buf_desc *tbd);
-void ql_dump_ob_mac_iocb(struct ql_adapter *qdev, struct ob_mac_iocb_req *ob_mac_iocb);
-void ql_dump_ob_mac_rsp(struct ql_adapter *qdev, struct ob_mac_iocb_rsp *ob_mac_rsp);
-#define QL_DUMP_OB_MAC_IOCB(qdev, ob_mac_iocb) ql_dump_ob_mac_iocb(qdev, ob_mac_iocb)
-#define QL_DUMP_OB_MAC_RSP(qdev, ob_mac_rsp) ql_dump_ob_mac_rsp(qdev, ob_mac_rsp)
-#else
-#define QL_DUMP_OB_MAC_IOCB(qdev, ob_mac_iocb)
-#define QL_DUMP_OB_MAC_RSP(qdev, ob_mac_rsp)
-#endif
-
-#ifdef QL_IB_DUMP
-void ql_dump_ib_mac_rsp(struct ql_adapter *qdev, struct ib_mac_iocb_rsp *ib_mac_rsp);
-#define QL_DUMP_IB_MAC_RSP(qdev, ib_mac_rsp) ql_dump_ib_mac_rsp(qdev, ib_mac_rsp)
-#else
-#define QL_DUMP_IB_MAC_RSP(qdev, ib_mac_rsp)
-#endif
-
-#ifdef	QL_ALL_DUMP
-void ql_dump_all(struct ql_adapter *qdev);
-#define QL_DUMP_ALL(qdev) ql_dump_all(qdev)
-#else
-#define QL_DUMP_ALL(qdev)
-#endif
-
 #endif /* _QLGE_H_ */
diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
index 989575743718..a02ecf8fb291 100644
--- a/drivers/staging/qlge/qlge_dbg.c
+++ b/drivers/staging/qlge/qlge_dbg.c
@@ -1313,691 +1313,3 @@ void ql_get_dump(struct ql_adapter *qdev, void *buff)
 		ql_get_core_dump(qdev);
 	}
 }
-
-#ifdef QL_REG_DUMP
-static void ql_dump_intr_states(struct ql_adapter *qdev)
-{
-	int i;
-	u32 value;
-
-	for (i = 0; i < qdev->intr_count; i++) {
-		ql_write32(qdev, INTR_EN, qdev->intr_context[i].intr_read_mask);
-		value = ql_read32(qdev, INTR_EN);
-		netdev_err(qdev->ndev, "Interrupt %d is %s\n", i,
-			   (value & INTR_EN_EN ? "enabled" : "disabled"));
-	}
-}
-
-#define DUMP_XGMAC(qdev, reg)					\
-do {								\
-	u32 data;						\
-	ql_read_xgmac_reg(qdev, reg, &data);			\
-	netdev_err(qdev->ndev, "%s = 0x%.08x\n", #reg, data); \
-} while (0)
-
-void ql_dump_xgmac_control_regs(struct ql_adapter *qdev)
-{
-	if (ql_sem_spinlock(qdev, qdev->xg_sem_mask)) {
-		netdev_err(qdev->ndev, "%s: Couldn't get xgmac sem\n",
-			   __func__);
-		return;
-	}
-	DUMP_XGMAC(qdev, PAUSE_SRC_LO);
-	DUMP_XGMAC(qdev, PAUSE_SRC_HI);
-	DUMP_XGMAC(qdev, GLOBAL_CFG);
-	DUMP_XGMAC(qdev, TX_CFG);
-	DUMP_XGMAC(qdev, RX_CFG);
-	DUMP_XGMAC(qdev, FLOW_CTL);
-	DUMP_XGMAC(qdev, PAUSE_OPCODE);
-	DUMP_XGMAC(qdev, PAUSE_TIMER);
-	DUMP_XGMAC(qdev, PAUSE_FRM_DEST_LO);
-	DUMP_XGMAC(qdev, PAUSE_FRM_DEST_HI);
-	DUMP_XGMAC(qdev, MAC_TX_PARAMS);
-	DUMP_XGMAC(qdev, MAC_RX_PARAMS);
-	DUMP_XGMAC(qdev, MAC_SYS_INT);
-	DUMP_XGMAC(qdev, MAC_SYS_INT_MASK);
-	DUMP_XGMAC(qdev, MAC_MGMT_INT);
-	DUMP_XGMAC(qdev, MAC_MGMT_IN_MASK);
-	DUMP_XGMAC(qdev, EXT_ARB_MODE);
-	ql_sem_unlock(qdev, qdev->xg_sem_mask);
-}
-
-static void ql_dump_ets_regs(struct ql_adapter *qdev)
-{
-}
-
-static void ql_dump_cam_entries(struct ql_adapter *qdev)
-{
-	int i;
-	u32 value[3];
-
-	i = ql_sem_spinlock(qdev, SEM_MAC_ADDR_MASK);
-	if (i)
-		return;
-	for (i = 0; i < 4; i++) {
-		if (ql_get_mac_addr_reg(qdev, MAC_ADDR_TYPE_CAM_MAC, i, value)) {
-			netdev_err(qdev->ndev,
-				   "%s: Failed read of mac index register\n",
-				   __func__);
-			break;
-		}
-		if (value[0])
-			netdev_err(qdev->ndev,
-				   "CAM index %d CAM Lookup Lower = 0x%.08x:%.08x, Output = 0x%.08x\n",
-				   i, value[1], value[0], value[2]);
-	}
-	for (i = 0; i < 32; i++) {
-		if (ql_get_mac_addr_reg
-		    (qdev, MAC_ADDR_TYPE_MULTI_MAC, i, value)) {
-			netdev_err(qdev->ndev,
-				   "%s: Failed read of mac index register\n",
-				   __func__);
-			break;
-		}
-		if (value[0])
-			netdev_err(qdev->ndev,
-				   "MCAST index %d CAM Lookup Lower = 0x%.08x:%.08x\n",
-				   i, value[1], value[0]);
-	}
-	ql_sem_unlock(qdev, SEM_MAC_ADDR_MASK);
-}
-
-void ql_dump_routing_entries(struct ql_adapter *qdev)
-{
-	int i;
-	u32 value;
-
-	i = ql_sem_spinlock(qdev, SEM_RT_IDX_MASK);
-	if (i)
-		return;
-	for (i = 0; i < 16; i++) {
-		value = 0;
-		if (ql_get_routing_reg(qdev, i, &value)) {
-			netdev_err(qdev->ndev,
-				   "%s: Failed read of routing index register\n",
-				   __func__);
-			break;
-		}
-		if (value)
-			netdev_err(qdev->ndev,
-				   "Routing Mask %d = 0x%.08x\n",
-				   i, value);
-	}
-	ql_sem_unlock(qdev, SEM_RT_IDX_MASK);
-}
-
-#define DUMP_REG(qdev, reg)			\
-	netdev_err(qdev->ndev, "%-32s= 0x%x\n", #reg, ql_read32(qdev, reg))
-
-void ql_dump_regs(struct ql_adapter *qdev)
-{
-	netdev_err(qdev->ndev, "reg dump for function #%d\n", qdev->func);
-	DUMP_REG(qdev, SYS);
-	DUMP_REG(qdev, RST_FO);
-	DUMP_REG(qdev, FSC);
-	DUMP_REG(qdev, CSR);
-	DUMP_REG(qdev, ICB_RID);
-	DUMP_REG(qdev, ICB_L);
-	DUMP_REG(qdev, ICB_H);
-	DUMP_REG(qdev, CFG);
-	DUMP_REG(qdev, BIOS_ADDR);
-	DUMP_REG(qdev, STS);
-	DUMP_REG(qdev, INTR_EN);
-	DUMP_REG(qdev, INTR_MASK);
-	DUMP_REG(qdev, ISR1);
-	DUMP_REG(qdev, ISR2);
-	DUMP_REG(qdev, ISR3);
-	DUMP_REG(qdev, ISR4);
-	DUMP_REG(qdev, REV_ID);
-	DUMP_REG(qdev, FRC_ECC_ERR);
-	DUMP_REG(qdev, ERR_STS);
-	DUMP_REG(qdev, RAM_DBG_ADDR);
-	DUMP_REG(qdev, RAM_DBG_DATA);
-	DUMP_REG(qdev, ECC_ERR_CNT);
-	DUMP_REG(qdev, SEM);
-	DUMP_REG(qdev, GPIO_1);
-	DUMP_REG(qdev, GPIO_2);
-	DUMP_REG(qdev, GPIO_3);
-	DUMP_REG(qdev, XGMAC_ADDR);
-	DUMP_REG(qdev, XGMAC_DATA);
-	DUMP_REG(qdev, NIC_ETS);
-	DUMP_REG(qdev, CNA_ETS);
-	DUMP_REG(qdev, FLASH_ADDR);
-	DUMP_REG(qdev, FLASH_DATA);
-	DUMP_REG(qdev, CQ_STOP);
-	DUMP_REG(qdev, PAGE_TBL_RID);
-	DUMP_REG(qdev, WQ_PAGE_TBL_LO);
-	DUMP_REG(qdev, WQ_PAGE_TBL_HI);
-	DUMP_REG(qdev, CQ_PAGE_TBL_LO);
-	DUMP_REG(qdev, CQ_PAGE_TBL_HI);
-	DUMP_REG(qdev, COS_DFLT_CQ1);
-	DUMP_REG(qdev, COS_DFLT_CQ2);
-	DUMP_REG(qdev, SPLT_HDR);
-	DUMP_REG(qdev, FC_PAUSE_THRES);
-	DUMP_REG(qdev, NIC_PAUSE_THRES);
-	DUMP_REG(qdev, FC_ETHERTYPE);
-	DUMP_REG(qdev, FC_RCV_CFG);
-	DUMP_REG(qdev, NIC_RCV_CFG);
-	DUMP_REG(qdev, FC_COS_TAGS);
-	DUMP_REG(qdev, NIC_COS_TAGS);
-	DUMP_REG(qdev, MGMT_RCV_CFG);
-	DUMP_REG(qdev, XG_SERDES_ADDR);
-	DUMP_REG(qdev, XG_SERDES_DATA);
-	DUMP_REG(qdev, PRB_MX_ADDR);
-	DUMP_REG(qdev, PRB_MX_DATA);
-	ql_dump_intr_states(qdev);
-	ql_dump_xgmac_control_regs(qdev);
-	ql_dump_ets_regs(qdev);
-	ql_dump_cam_entries(qdev);
-	ql_dump_routing_entries(qdev);
-}
-#endif
-
-#ifdef QL_STAT_DUMP
-
-#define DUMP_STAT(qdev, stat)	\
-	netdev_err(qdev->ndev, "%s = %ld\n", #stat,  \
-		   (unsigned long)(qdev)->nic_stats.stat)
-
-void ql_dump_stat(struct ql_adapter *qdev)
-{
-	netdev_err(qdev->ndev, "%s: Enter\n", __func__);
-	DUMP_STAT(qdev, tx_pkts);
-	DUMP_STAT(qdev, tx_bytes);
-	DUMP_STAT(qdev, tx_mcast_pkts);
-	DUMP_STAT(qdev, tx_bcast_pkts);
-	DUMP_STAT(qdev, tx_ucast_pkts);
-	DUMP_STAT(qdev, tx_ctl_pkts);
-	DUMP_STAT(qdev, tx_pause_pkts);
-	DUMP_STAT(qdev, tx_64_pkt);
-	DUMP_STAT(qdev, tx_65_to_127_pkt);
-	DUMP_STAT(qdev, tx_128_to_255_pkt);
-	DUMP_STAT(qdev, tx_256_511_pkt);
-	DUMP_STAT(qdev, tx_512_to_1023_pkt);
-	DUMP_STAT(qdev, tx_1024_to_1518_pkt);
-	DUMP_STAT(qdev, tx_1519_to_max_pkt);
-	DUMP_STAT(qdev, tx_undersize_pkt);
-	DUMP_STAT(qdev, tx_oversize_pkt);
-	DUMP_STAT(qdev, rx_bytes);
-	DUMP_STAT(qdev, rx_bytes_ok);
-	DUMP_STAT(qdev, rx_pkts);
-	DUMP_STAT(qdev, rx_pkts_ok);
-	DUMP_STAT(qdev, rx_bcast_pkts);
-	DUMP_STAT(qdev, rx_mcast_pkts);
-	DUMP_STAT(qdev, rx_ucast_pkts);
-	DUMP_STAT(qdev, rx_undersize_pkts);
-	DUMP_STAT(qdev, rx_oversize_pkts);
-	DUMP_STAT(qdev, rx_jabber_pkts);
-	DUMP_STAT(qdev, rx_undersize_fcerr_pkts);
-	DUMP_STAT(qdev, rx_drop_events);
-	DUMP_STAT(qdev, rx_fcerr_pkts);
-	DUMP_STAT(qdev, rx_align_err);
-	DUMP_STAT(qdev, rx_symbol_err);
-	DUMP_STAT(qdev, rx_mac_err);
-	DUMP_STAT(qdev, rx_ctl_pkts);
-	DUMP_STAT(qdev, rx_pause_pkts);
-	DUMP_STAT(qdev, rx_64_pkts);
-	DUMP_STAT(qdev, rx_65_to_127_pkts);
-	DUMP_STAT(qdev, rx_128_255_pkts);
-	DUMP_STAT(qdev, rx_256_511_pkts);
-	DUMP_STAT(qdev, rx_512_to_1023_pkts);
-	DUMP_STAT(qdev, rx_1024_to_1518_pkts);
-	DUMP_STAT(qdev, rx_1519_to_max_pkts);
-	DUMP_STAT(qdev, rx_len_err_pkts);
-};
-#endif
-
-#ifdef QL_DEV_DUMP
-
-#define DUMP_QDEV_FIELD(qdev, type, field)		\
-	netdev_err(qdev->ndev, "qdev->%-24s = " type "\n", #field, (qdev)->field)
-#define DUMP_QDEV_DMA_FIELD(qdev, field)		\
-	netdev_err(qdev->ndev, "qdev->%-24s = %llx\n", #field, \
-		   (unsigned long long)qdev->field)
-#define DUMP_QDEV_ARRAY(qdev, type, array, index, field) \
-	netdev_err(qdev->ndev, "%s[%d].%s = " type "\n",		 \
-	       #array, index, #field, (qdev)->array[index].field)
-void ql_dump_qdev(struct ql_adapter *qdev)
-{
-	int i;
-
-	DUMP_QDEV_FIELD(qdev, "%lx", flags);
-	DUMP_QDEV_FIELD(qdev, "%p", pdev);
-	DUMP_QDEV_FIELD(qdev, "%p", ndev);
-	DUMP_QDEV_FIELD(qdev, "%d", chip_rev_id);
-	DUMP_QDEV_FIELD(qdev, "%p", reg_base);
-	DUMP_QDEV_FIELD(qdev, "%p", doorbell_area);
-	DUMP_QDEV_FIELD(qdev, "%d", doorbell_area_size);
-	DUMP_QDEV_FIELD(qdev, "%x", msg_enable);
-	DUMP_QDEV_FIELD(qdev, "%p", rx_ring_shadow_reg_area);
-	DUMP_QDEV_DMA_FIELD(qdev, rx_ring_shadow_reg_dma);
-	DUMP_QDEV_FIELD(qdev, "%p", tx_ring_shadow_reg_area);
-	DUMP_QDEV_DMA_FIELD(qdev, tx_ring_shadow_reg_dma);
-	DUMP_QDEV_FIELD(qdev, "%d", intr_count);
-	if (qdev->msi_x_entry)
-		for (i = 0; i < qdev->intr_count; i++) {
-			DUMP_QDEV_ARRAY(qdev, "%d", msi_x_entry, i, vector);
-			DUMP_QDEV_ARRAY(qdev, "%d", msi_x_entry, i, entry);
-		}
-	for (i = 0; i < qdev->intr_count; i++) {
-		DUMP_QDEV_ARRAY(qdev, "%p", intr_context, i, qdev);
-		DUMP_QDEV_ARRAY(qdev, "%d", intr_context, i, intr);
-		DUMP_QDEV_ARRAY(qdev, "%d", intr_context, i, hooked);
-		DUMP_QDEV_ARRAY(qdev, "0x%08x", intr_context, i, intr_en_mask);
-		DUMP_QDEV_ARRAY(qdev, "0x%08x", intr_context, i, intr_dis_mask);
-		DUMP_QDEV_ARRAY(qdev, "0x%08x", intr_context, i, intr_read_mask);
-	}
-	DUMP_QDEV_FIELD(qdev, "%d", tx_ring_count);
-	DUMP_QDEV_FIELD(qdev, "%d", rx_ring_count);
-	DUMP_QDEV_FIELD(qdev, "%d", ring_mem_size);
-	DUMP_QDEV_FIELD(qdev, "%p", ring_mem);
-	DUMP_QDEV_FIELD(qdev, "%d", intr_count);
-	DUMP_QDEV_FIELD(qdev, "%p", tx_ring);
-	DUMP_QDEV_FIELD(qdev, "%d", rss_ring_count);
-	DUMP_QDEV_FIELD(qdev, "%p", rx_ring);
-	DUMP_QDEV_FIELD(qdev, "%d", default_rx_queue);
-	DUMP_QDEV_FIELD(qdev, "0x%08x", xg_sem_mask);
-	DUMP_QDEV_FIELD(qdev, "0x%08x", port_link_up);
-	DUMP_QDEV_FIELD(qdev, "0x%08x", port_init);
-	DUMP_QDEV_FIELD(qdev, "%u", lbq_buf_size);
-}
-#endif
-
-#ifdef QL_CB_DUMP
-void ql_dump_wqicb(struct wqicb *wqicb)
-{
-	struct tx_ring *tx_ring = container_of(wqicb, struct tx_ring, wqicb);
-	struct ql_adapter *qdev = tx_ring->qdev;
-
-	netdev_err(qdev->ndev, "Dumping wqicb stuff...\n");
-	netdev_err(qdev->ndev, "wqicb->len = 0x%x\n", le16_to_cpu(wqicb->len));
-	netdev_err(qdev->ndev, "wqicb->flags = %x\n",
-		   le16_to_cpu(wqicb->flags));
-	netdev_err(qdev->ndev, "wqicb->cq_id_rss = %d\n",
-		   le16_to_cpu(wqicb->cq_id_rss));
-	netdev_err(qdev->ndev, "wqicb->rid = 0x%x\n", le16_to_cpu(wqicb->rid));
-	netdev_err(qdev->ndev, "wqicb->wq_addr = 0x%llx\n",
-		   (unsigned long long)le64_to_cpu(wqicb->addr));
-	netdev_err(qdev->ndev, "wqicb->wq_cnsmr_idx_addr = 0x%llx\n",
-		   (unsigned long long)le64_to_cpu(wqicb->cnsmr_idx_addr));
-}
-
-void ql_dump_tx_ring(struct tx_ring *tx_ring)
-{
-	struct ql_adapter *qdev = tx_ring->qdev;
-
-	netdev_err(qdev->ndev, "===================== Dumping tx_ring %d ===============\n",
-		   tx_ring->wq_id);
-	netdev_err(qdev->ndev, "tx_ring->base = %p\n", tx_ring->wq_base);
-	netdev_err(qdev->ndev, "tx_ring->base_dma = 0x%llx\n",
-		   (unsigned long long)tx_ring->wq_base_dma);
-	netdev_err(qdev->ndev, "tx_ring->cnsmr_idx_sh_reg, addr = 0x%p, value = %d\n",
-		   tx_ring->cnsmr_idx_sh_reg,
-		   tx_ring->cnsmr_idx_sh_reg
-			? ql_read_sh_reg(tx_ring->cnsmr_idx_sh_reg) : 0);
-	netdev_err(qdev->ndev, "tx_ring->size = %d\n", tx_ring->wq_size);
-	netdev_err(qdev->ndev, "tx_ring->len = %d\n", tx_ring->wq_len);
-	netdev_err(qdev->ndev, "tx_ring->prod_idx_db_reg = %p\n", tx_ring->prod_idx_db_reg);
-	netdev_err(qdev->ndev, "tx_ring->valid_db_reg = %p\n", tx_ring->valid_db_reg);
-	netdev_err(qdev->ndev, "tx_ring->prod_idx = %d\n", tx_ring->prod_idx);
-	netdev_err(qdev->ndev, "tx_ring->cq_id = %d\n", tx_ring->cq_id);
-	netdev_err(qdev->ndev, "tx_ring->wq_id = %d\n", tx_ring->wq_id);
-	netdev_err(qdev->ndev, "tx_ring->q = %p\n", tx_ring->q);
-	netdev_err(qdev->ndev, "tx_ring->tx_count = %d\n", atomic_read(&tx_ring->tx_count));
-}
-
-void ql_dump_ricb(struct ricb *ricb)
-{
-	int i;
-	struct ql_adapter *qdev =
-		container_of(ricb, struct ql_adapter, ricb);
-
-	netdev_err(qdev->ndev, "===================== Dumping ricb ===============\n");
-	netdev_err(qdev->ndev, "Dumping ricb stuff...\n");
-
-	netdev_err(qdev->ndev, "ricb->base_cq = %d\n", ricb->base_cq & 0x1f);
-	netdev_err(qdev->ndev, "ricb->flags = %s%s%s%s%s%s%s%s%s\n",
-		   ricb->base_cq & RSS_L4K ? "RSS_L4K " : "",
-		   ricb->flags & RSS_L6K ? "RSS_L6K " : "",
-		   ricb->flags & RSS_LI ? "RSS_LI " : "",
-		   ricb->flags & RSS_LB ? "RSS_LB " : "",
-		   ricb->flags & RSS_LM ? "RSS_LM " : "",
-		   ricb->flags & RSS_RI4 ? "RSS_RI4 " : "",
-		   ricb->flags & RSS_RT4 ? "RSS_RT4 " : "",
-		   ricb->flags & RSS_RI6 ? "RSS_RI6 " : "",
-		   ricb->flags & RSS_RT6 ? "RSS_RT6 " : "");
-	netdev_err(qdev->ndev, "ricb->mask = 0x%.04x\n", le16_to_cpu(ricb->mask));
-	for (i = 0; i < 16; i++)
-		netdev_err(qdev->ndev, "ricb->hash_cq_id[%d] = 0x%.08x\n", i,
-			   le32_to_cpu(ricb->hash_cq_id[i]));
-	for (i = 0; i < 10; i++)
-		netdev_err(qdev->ndev, "ricb->ipv6_hash_key[%d] = 0x%.08x\n", i,
-			   le32_to_cpu(ricb->ipv6_hash_key[i]));
-	for (i = 0; i < 4; i++)
-		netdev_err(qdev->ndev, "ricb->ipv4_hash_key[%d] = 0x%.08x\n", i,
-			   le32_to_cpu(ricb->ipv4_hash_key[i]));
-}
-
-void ql_dump_cqicb(struct cqicb *cqicb)
-{
-	struct rx_ring *rx_ring = container_of(cqicb, struct rx_ring, cqicb);
-	struct ql_adapter *qdev = rx_ring->qdev;
-
-	netdev_err(qdev->ndev, "Dumping cqicb stuff...\n");
-
-	netdev_err(qdev->ndev, "cqicb->msix_vect = %d\n", cqicb->msix_vect);
-	netdev_err(qdev->ndev, "cqicb->flags = %x\n", cqicb->flags);
-	netdev_err(qdev->ndev, "cqicb->len = %d\n", le16_to_cpu(cqicb->len));
-	netdev_err(qdev->ndev, "cqicb->addr = 0x%llx\n",
-		   (unsigned long long)le64_to_cpu(cqicb->addr));
-	netdev_err(qdev->ndev, "cqicb->prod_idx_addr = 0x%llx\n",
-		   (unsigned long long)le64_to_cpu(cqicb->prod_idx_addr));
-	netdev_err(qdev->ndev, "cqicb->pkt_delay = 0x%.04x\n",
-		   le16_to_cpu(cqicb->pkt_delay));
-	netdev_err(qdev->ndev, "cqicb->irq_delay = 0x%.04x\n",
-		   le16_to_cpu(cqicb->irq_delay));
-	netdev_err(qdev->ndev, "cqicb->lbq_addr = 0x%llx\n",
-		   (unsigned long long)le64_to_cpu(cqicb->lbq_addr));
-	netdev_err(qdev->ndev, "cqicb->lbq_buf_size = 0x%.04x\n",
-		   le16_to_cpu(cqicb->lbq_buf_size));
-	netdev_err(qdev->ndev, "cqicb->lbq_len = 0x%.04x\n",
-		   le16_to_cpu(cqicb->lbq_len));
-	netdev_err(qdev->ndev, "cqicb->sbq_addr = 0x%llx\n",
-		   (unsigned long long)le64_to_cpu(cqicb->sbq_addr));
-	netdev_err(qdev->ndev, "cqicb->sbq_buf_size = 0x%.04x\n",
-		   le16_to_cpu(cqicb->sbq_buf_size));
-	netdev_err(qdev->ndev, "cqicb->sbq_len = 0x%.04x\n",
-		   le16_to_cpu(cqicb->sbq_len));
-}
-
-static const char *qlge_rx_ring_type_name(struct rx_ring *rx_ring)
-{
-	struct ql_adapter *qdev = rx_ring->qdev;
-
-	if (rx_ring->cq_id < qdev->rss_ring_count)
-		return "RX COMPLETION";
-	else
-		return "TX COMPLETION";
-};
-
-void ql_dump_rx_ring(struct rx_ring *rx_ring)
-{
-	struct ql_adapter *qdev = rx_ring->qdev;
-
-	netdev_err(qdev->ndev,
-		   "===================== Dumping rx_ring %d ===============\n",
-		   rx_ring->cq_id);
-	netdev_err(qdev->ndev,
-		   "Dumping rx_ring %d, type = %s\n", rx_ring->cq_id,
-		   qlge_rx_ring_type_name(rx_ring));
-	netdev_err(qdev->ndev, "rx_ring->cqicb = %p\n", &rx_ring->cqicb);
-	netdev_err(qdev->ndev, "rx_ring->cq_base = %p\n", rx_ring->cq_base);
-	netdev_err(qdev->ndev, "rx_ring->cq_base_dma = %llx\n",
-		   (unsigned long long)rx_ring->cq_base_dma);
-	netdev_err(qdev->ndev, "rx_ring->cq_size = %d\n", rx_ring->cq_size);
-	netdev_err(qdev->ndev, "rx_ring->cq_len = %d\n", rx_ring->cq_len);
-	netdev_err(qdev->ndev,
-		   "rx_ring->prod_idx_sh_reg, addr = 0x%p, value = %d\n",
-		   rx_ring->prod_idx_sh_reg,
-		   rx_ring->prod_idx_sh_reg ? ql_read_sh_reg(rx_ring->prod_idx_sh_reg) : 0);
-	netdev_err(qdev->ndev, "rx_ring->prod_idx_sh_reg_dma = %llx\n",
-		   (unsigned long long)rx_ring->prod_idx_sh_reg_dma);
-	netdev_err(qdev->ndev, "rx_ring->cnsmr_idx_db_reg = %p\n",
-		   rx_ring->cnsmr_idx_db_reg);
-	netdev_err(qdev->ndev, "rx_ring->cnsmr_idx = %d\n", rx_ring->cnsmr_idx);
-	netdev_err(qdev->ndev, "rx_ring->curr_entry = %p\n", rx_ring->curr_entry);
-	netdev_err(qdev->ndev, "rx_ring->valid_db_reg = %p\n", rx_ring->valid_db_reg);
-
-	netdev_err(qdev->ndev, "rx_ring->lbq.base = %p\n", rx_ring->lbq.base);
-	netdev_err(qdev->ndev, "rx_ring->lbq.base_dma = %llx\n",
-		   (unsigned long long)rx_ring->lbq.base_dma);
-	netdev_err(qdev->ndev, "rx_ring->lbq.base_indirect = %p\n",
-		   rx_ring->lbq.base_indirect);
-	netdev_err(qdev->ndev, "rx_ring->lbq.base_indirect_dma = %llx\n",
-		   (unsigned long long)rx_ring->lbq.base_indirect_dma);
-	netdev_err(qdev->ndev, "rx_ring->lbq = %p\n", rx_ring->lbq.queue);
-	netdev_err(qdev->ndev, "rx_ring->lbq.prod_idx_db_reg = %p\n",
-		   rx_ring->lbq.prod_idx_db_reg);
-	netdev_err(qdev->ndev, "rx_ring->lbq.next_to_use = %d\n", rx_ring->lbq.next_to_use);
-	netdev_err(qdev->ndev, "rx_ring->lbq.next_to_clean = %d\n", rx_ring->lbq.next_to_clean);
-
-	netdev_err(qdev->ndev, "rx_ring->sbq.base = %p\n", rx_ring->sbq.base);
-	netdev_err(qdev->ndev, "rx_ring->sbq.base_dma = %llx\n",
-		   (unsigned long long)rx_ring->sbq.base_dma);
-	netdev_err(qdev->ndev, "rx_ring->sbq.base_indirect = %p\n",
-		   rx_ring->sbq.base_indirect);
-	netdev_err(qdev->ndev, "rx_ring->sbq.base_indirect_dma = %llx\n",
-		   (unsigned long long)rx_ring->sbq.base_indirect_dma);
-	netdev_err(qdev->ndev, "rx_ring->sbq = %p\n", rx_ring->sbq.queue);
-	netdev_err(qdev->ndev, "rx_ring->sbq.prod_idx_db_reg addr = %p\n",
-		   rx_ring->sbq.prod_idx_db_reg);
-	netdev_err(qdev->ndev, "rx_ring->sbq.next_to_use = %d\n", rx_ring->sbq.next_to_use);
-	netdev_err(qdev->ndev, "rx_ring->sbq.next_to_clean = %d\n", rx_ring->sbq.next_to_clean);
-	netdev_err(qdev->ndev, "rx_ring->cq_id = %d\n", rx_ring->cq_id);
-	netdev_err(qdev->ndev, "rx_ring->irq = %d\n", rx_ring->irq);
-	netdev_err(qdev->ndev, "rx_ring->cpu = %d\n", rx_ring->cpu);
-	netdev_err(qdev->ndev, "rx_ring->qdev = %p\n", rx_ring->qdev);
-}
-
-void ql_dump_hw_cb(struct ql_adapter *qdev, int size, u32 bit, u16 q_id)
-{
-	void *ptr;
-
-	netdev_err(qdev->ndev, "%s: Enter\n", __func__);
-
-	ptr = kmalloc(size, GFP_ATOMIC);
-	if (!ptr)
-		return;
-
-	if (ql_write_cfg(qdev, ptr, size, bit, q_id)) {
-		netdev_err(qdev->ndev, "%s: Failed to upload control block!\n", __func__);
-		goto fail_it;
-	}
-	switch (bit) {
-	case CFG_DRQ:
-		ql_dump_wqicb((struct wqicb *)ptr);
-		break;
-	case CFG_DCQ:
-		ql_dump_cqicb((struct cqicb *)ptr);
-		break;
-	case CFG_DR:
-		ql_dump_ricb((struct ricb *)ptr);
-		break;
-	default:
-		netdev_err(qdev->ndev, "%s: Invalid bit value = %x\n", __func__, bit);
-		break;
-	}
-fail_it:
-	kfree(ptr);
-}
-#endif
-
-#ifdef QL_OB_DUMP
-void ql_dump_tx_desc(struct ql_adapter *qdev, struct tx_buf_desc *tbd)
-{
-	netdev_err(qdev->ndev, "tbd->addr  = 0x%llx\n",
-		   le64_to_cpu((u64)tbd->addr));
-	netdev_err(qdev->ndev, "tbd->len   = %d\n",
-		   le32_to_cpu(tbd->len & TX_DESC_LEN_MASK));
-	netdev_err(qdev->ndev, "tbd->flags = %s %s\n",
-		   tbd->len & TX_DESC_C ? "C" : ".",
-		   tbd->len & TX_DESC_E ? "E" : ".");
-	tbd++;
-	netdev_err(qdev->ndev, "tbd->addr  = 0x%llx\n",
-		   le64_to_cpu((u64)tbd->addr));
-	netdev_err(qdev->ndev, "tbd->len   = %d\n",
-		   le32_to_cpu(tbd->len & TX_DESC_LEN_MASK));
-	netdev_err(qdev->ndev, "tbd->flags = %s %s\n",
-		   tbd->len & TX_DESC_C ? "C" : ".",
-		   tbd->len & TX_DESC_E ? "E" : ".");
-	tbd++;
-	netdev_err(qdev->ndev, "tbd->addr  = 0x%llx\n",
-		   le64_to_cpu((u64)tbd->addr));
-	netdev_err(qdev->ndev, "tbd->len   = %d\n",
-		   le32_to_cpu(tbd->len & TX_DESC_LEN_MASK));
-	netdev_err(qdev->ndev, "tbd->flags = %s %s\n",
-		   tbd->len & TX_DESC_C ? "C" : ".",
-		   tbd->len & TX_DESC_E ? "E" : ".");
-}
-
-void ql_dump_ob_mac_iocb(struct ql_adapter *qdev, struct ob_mac_iocb_req *ob_mac_iocb)
-{
-	struct ob_mac_tso_iocb_req *ob_mac_tso_iocb =
-	    (struct ob_mac_tso_iocb_req *)ob_mac_iocb;
-	struct tx_buf_desc *tbd;
-	u16 frame_len;
-
-	netdev_err(qdev->ndev, "%s\n", __func__);
-	netdev_err(qdev->ndev, "opcode         = %s\n",
-		   (ob_mac_iocb->opcode == OPCODE_OB_MAC_IOCB) ? "MAC" : "TSO");
-	netdev_err(qdev->ndev, "flags1          = %s %s %s %s %s\n",
-		   ob_mac_tso_iocb->flags1 & OB_MAC_TSO_IOCB_OI ? "OI" : "",
-		   ob_mac_tso_iocb->flags1 & OB_MAC_TSO_IOCB_I ? "I" : "",
-		   ob_mac_tso_iocb->flags1 & OB_MAC_TSO_IOCB_D ? "D" : "",
-		   ob_mac_tso_iocb->flags1 & OB_MAC_TSO_IOCB_IP4 ? "IP4" : "",
-		   ob_mac_tso_iocb->flags1 & OB_MAC_TSO_IOCB_IP6 ? "IP6" : "");
-	netdev_err(qdev->ndev, "flags2          = %s %s %s\n",
-		   ob_mac_tso_iocb->flags2 & OB_MAC_TSO_IOCB_LSO ? "LSO" : "",
-		   ob_mac_tso_iocb->flags2 & OB_MAC_TSO_IOCB_UC ? "UC" : "",
-		   ob_mac_tso_iocb->flags2 & OB_MAC_TSO_IOCB_TC ? "TC" : "");
-	netdev_err(qdev->ndev, "flags3          = %s %s %s\n",
-		   ob_mac_tso_iocb->flags3 & OB_MAC_TSO_IOCB_IC ? "IC" : "",
-		   ob_mac_tso_iocb->flags3 & OB_MAC_TSO_IOCB_DFP ? "DFP" : "",
-		   ob_mac_tso_iocb->flags3 & OB_MAC_TSO_IOCB_V ? "V" : "");
-	netdev_err(qdev->ndev, "tid = %x\n", ob_mac_iocb->tid);
-	netdev_err(qdev->ndev, "txq_idx = %d\n", ob_mac_iocb->txq_idx);
-	netdev_err(qdev->ndev, "vlan_tci      = %x\n", ob_mac_tso_iocb->vlan_tci);
-	if (ob_mac_iocb->opcode == OPCODE_OB_MAC_TSO_IOCB) {
-		netdev_err(qdev->ndev, "frame_len      = %d\n",
-			   le32_to_cpu(ob_mac_tso_iocb->frame_len));
-		netdev_err(qdev->ndev, "mss      = %d\n",
-			   le16_to_cpu(ob_mac_tso_iocb->mss));
-		netdev_err(qdev->ndev, "prot_hdr_len   = %d\n",
-			   le16_to_cpu(ob_mac_tso_iocb->total_hdrs_len));
-		netdev_err(qdev->ndev, "hdr_offset     = 0x%.04x\n",
-			   le16_to_cpu(ob_mac_tso_iocb->net_trans_offset));
-		frame_len = le32_to_cpu(ob_mac_tso_iocb->frame_len);
-	} else {
-		netdev_err(qdev->ndev, "frame_len      = %d\n",
-			   le16_to_cpu(ob_mac_iocb->frame_len));
-		frame_len = le16_to_cpu(ob_mac_iocb->frame_len);
-	}
-	tbd = &ob_mac_iocb->tbd[0];
-	ql_dump_tx_desc(qdev, tbd);
-}
-
-void ql_dump_ob_mac_rsp(struct ql_adapter *qdev, struct ob_mac_iocb_rsp *ob_mac_rsp)
-{
-	netdev_err(qdev->ndev, "%s\n", __func__);
-	netdev_err(qdev->ndev, "opcode         = %d\n", ob_mac_rsp->opcode);
-	netdev_err(qdev->ndev, "flags          = %s %s %s %s %s %s %s\n",
-		   ob_mac_rsp->flags1 & OB_MAC_IOCB_RSP_OI ?
-			"OI" : ".", ob_mac_rsp->flags1 & OB_MAC_IOCB_RSP_I ? "I" : ".",
-		   ob_mac_rsp->flags1 & OB_MAC_IOCB_RSP_E ? "E" : ".",
-		   ob_mac_rsp->flags1 & OB_MAC_IOCB_RSP_S ? "S" : ".",
-		   ob_mac_rsp->flags1 & OB_MAC_IOCB_RSP_L ? "L" : ".",
-		   ob_mac_rsp->flags1 & OB_MAC_IOCB_RSP_P ? "P" : ".",
-		   ob_mac_rsp->flags2 & OB_MAC_IOCB_RSP_B ? "B" : ".");
-	netdev_err(qdev->ndev, "tid = %x\n", ob_mac_rsp->tid);
-}
-#endif
-
-#ifdef QL_IB_DUMP
-void ql_dump_ib_mac_rsp(struct ql_adapter *qdev, struct ib_mac_iocb_rsp *ib_mac_rsp)
-{
-	netdev_err(qdev->ndev, "%s\n", __func__);
-	netdev_err(qdev->ndev, "opcode         = 0x%x\n", ib_mac_rsp->opcode);
-	netdev_err(qdev->ndev, "flags1 = %s%s%s%s%s%s\n",
-		   ib_mac_rsp->flags1 & IB_MAC_IOCB_RSP_OI ? "OI " : "",
-		   ib_mac_rsp->flags1 & IB_MAC_IOCB_RSP_I ? "I " : "",
-		   ib_mac_rsp->flags1 & IB_MAC_IOCB_RSP_TE ? "TE " : "",
-		   ib_mac_rsp->flags1 & IB_MAC_IOCB_RSP_NU ? "NU " : "",
-		   ib_mac_rsp->flags1 & IB_MAC_IOCB_RSP_IE ? "IE " : "",
-		   ib_mac_rsp->flags1 & IB_MAC_IOCB_RSP_B ? "B " : "");
-
-	if (ib_mac_rsp->flags1 & IB_MAC_IOCB_RSP_M_MASK)
-		netdev_err(qdev->ndev, "%s%s%s Multicast\n",
-			   (ib_mac_rsp->flags1 & IB_MAC_IOCB_RSP_M_MASK) ==
-			   IB_MAC_IOCB_RSP_M_HASH ? "Hash" : "",
-			   (ib_mac_rsp->flags1 & IB_MAC_IOCB_RSP_M_MASK) ==
-			   IB_MAC_IOCB_RSP_M_REG ? "Registered" : "",
-			   (ib_mac_rsp->flags1 & IB_MAC_IOCB_RSP_M_MASK) ==
-			   IB_MAC_IOCB_RSP_M_PROM ? "Promiscuous" : "");
-
-	netdev_err(qdev->ndev, "flags2 = %s%s%s%s%s\n",
-		   (ib_mac_rsp->flags2 & IB_MAC_IOCB_RSP_P) ? "P " : "",
-		   (ib_mac_rsp->flags2 & IB_MAC_IOCB_RSP_V) ? "V " : "",
-		   (ib_mac_rsp->flags2 & IB_MAC_IOCB_RSP_U) ? "U " : "",
-		   (ib_mac_rsp->flags2 & IB_MAC_IOCB_RSP_T) ? "T " : "",
-		   (ib_mac_rsp->flags2 & IB_MAC_IOCB_RSP_FO) ? "FO " : "");
-
-	if (ib_mac_rsp->flags2 & IB_MAC_IOCB_RSP_ERR_MASK)
-		netdev_err(qdev->ndev, "%s%s%s%s%s error\n",
-			   (ib_mac_rsp->flags2 & IB_MAC_IOCB_RSP_ERR_MASK) ==
-			   IB_MAC_IOCB_RSP_ERR_OVERSIZE ? "oversize" : "",
-			   (ib_mac_rsp->flags2 & IB_MAC_IOCB_RSP_ERR_MASK) ==
-			   IB_MAC_IOCB_RSP_ERR_UNDERSIZE ? "undersize" : "",
-			   (ib_mac_rsp->flags2 & IB_MAC_IOCB_RSP_ERR_MASK) ==
-			   IB_MAC_IOCB_RSP_ERR_PREAMBLE ? "preamble" : "",
-			   (ib_mac_rsp->flags2 & IB_MAC_IOCB_RSP_ERR_MASK) ==
-			   IB_MAC_IOCB_RSP_ERR_FRAME_LEN ? "frame length" : "",
-			   (ib_mac_rsp->flags2 & IB_MAC_IOCB_RSP_ERR_MASK) ==
-			   IB_MAC_IOCB_RSP_ERR_CRC ? "CRC" : "");
-
-	netdev_err(qdev->ndev, "flags3 = %s%s\n",
-		   ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DS ? "DS " : "",
-		   ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL ? "DL " : "");
-
-	if (ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_RSS_MASK)
-		netdev_err(qdev->ndev, "RSS flags = %s%s%s%s\n",
-			   ((ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_RSS_MASK) ==
-			    IB_MAC_IOCB_RSP_M_IPV4) ? "IPv4 RSS" : "",
-			   ((ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_RSS_MASK) ==
-			    IB_MAC_IOCB_RSP_M_IPV6) ? "IPv6 RSS " : "",
-			   ((ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_RSS_MASK) ==
-			    IB_MAC_IOCB_RSP_M_TCP_V4) ? "TCP/IPv4 RSS" : "",
-			   ((ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_RSS_MASK) ==
-			    IB_MAC_IOCB_RSP_M_TCP_V6) ? "TCP/IPv6 RSS" : "");
-
-	netdev_err(qdev->ndev, "data_len	= %d\n",
-		   le32_to_cpu(ib_mac_rsp->data_len));
-	netdev_err(qdev->ndev, "data_addr    = 0x%llx\n",
-		   (unsigned long long)le64_to_cpu(ib_mac_rsp->data_addr));
-	if (ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_RSS_MASK)
-		netdev_err(qdev->ndev, "rss    = %x\n",
-			   le32_to_cpu(ib_mac_rsp->rss));
-	if (ib_mac_rsp->flags2 & IB_MAC_IOCB_RSP_V)
-		netdev_err(qdev->ndev, "vlan_id    = %x\n",
-			   le16_to_cpu(ib_mac_rsp->vlan_id));
-
-	netdev_err(qdev->ndev, "flags4 = %s%s%s\n",
-		   ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV ? "HV " : "",
-		   ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS ? "HS " : "",
-		   ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HL ? "HL " : "");
-
-	if (ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV) {
-		netdev_err(qdev->ndev, "hdr length	= %d\n",
-			   le32_to_cpu(ib_mac_rsp->hdr_len));
-		netdev_err(qdev->ndev, "hdr addr    = 0x%llx\n",
-			   (unsigned long long)le64_to_cpu(ib_mac_rsp->hdr_addr));
-	}
-}
-#endif
-
-#ifdef QL_ALL_DUMP
-void ql_dump_all(struct ql_adapter *qdev)
-{
-	int i;
-
-	QL_DUMP_REGS(qdev);
-	QL_DUMP_QDEV(qdev);
-	for (i = 0; i < qdev->tx_ring_count; i++) {
-		QL_DUMP_TX_RING(&qdev->tx_ring[i]);
-		QL_DUMP_WQICB((struct wqicb *)&qdev->tx_ring[i]);
-	}
-	for (i = 0; i < qdev->rx_ring_count; i++) {
-		QL_DUMP_RX_RING(&qdev->rx_ring[i]);
-		QL_DUMP_CQICB((struct cqicb *)&qdev->rx_ring[i]);
-	}
-}
-#endif
diff --git a/drivers/staging/qlge/qlge_ethtool.c b/drivers/staging/qlge/qlge_ethtool.c
index eed116d8895e..d6b7f60a84da 100644
--- a/drivers/staging/qlge/qlge_ethtool.c
+++ b/drivers/staging/qlge/qlge_ethtool.c
@@ -327,8 +327,6 @@ static void ql_update_stats(struct ql_adapter *qdev)
 	ql_sem_unlock(qdev, qdev->xg_sem_mask);
 quit:
 	spin_unlock(&qdev->stats_lock);
-
-	QL_DUMP_STAT(qdev);
 }
 
 static void ql_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index aaca740d46c4..898c50f553c6 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -1857,8 +1857,6 @@ static void ql_process_mac_split_rx_intr(struct ql_adapter *qdev,
 	struct net_device *ndev = qdev->ndev;
 	struct sk_buff *skb = NULL;
 
-	QL_DUMP_IB_MAC_RSP(qdev, ib_mac_rsp);
-
 	skb = ql_build_rx_skb(qdev, rx_ring, ib_mac_rsp);
 	if (unlikely(!skb)) {
 		netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
@@ -1955,8 +1953,6 @@ static unsigned long ql_process_mac_rx_intr(struct ql_adapter *qdev,
 			((le16_to_cpu(ib_mac_rsp->vlan_id) &
 			IB_MAC_IOCB_RSP_VLAN_MASK)) : 0xffff;
 
-	QL_DUMP_IB_MAC_RSP(qdev, ib_mac_rsp);
-
 	if (ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV) {
 		/* The data and headers are split into
 		 * separate buffers.
@@ -2002,7 +1998,6 @@ static void ql_process_mac_tx_intr(struct ql_adapter *qdev,
 	struct tx_ring *tx_ring;
 	struct tx_ring_desc *tx_ring_desc;
 
-	QL_DUMP_OB_MAC_RSP(qdev, mac_rsp);
 	tx_ring = &qdev->tx_ring[mac_rsp->txq_idx];
 	tx_ring_desc = &tx_ring->q[mac_rsp->tid];
 	ql_unmap_send(qdev, tx_ring_desc, tx_ring_desc->map_cnt);
@@ -2593,7 +2588,7 @@ static netdev_tx_t qlge_send(struct sk_buff *skb, struct net_device *ndev)
 		tx_ring->tx_errors++;
 		return NETDEV_TX_BUSY;
 	}
-	QL_DUMP_OB_MAC_IOCB(qdev, mac_iocb_ptr);
+
 	tx_ring->prod_idx++;
 	if (tx_ring->prod_idx == tx_ring->wq_len)
 		tx_ring->prod_idx = 0;
-- 
2.28.0

Re: [PATCH v1 5/6] staging: qlge: clean up debugging code in the QL_ALL_DUMP ifdef land

[Benjamin Poirier]

On 2020-10-08 19:58 +0800, Coiby Xu wrote:
> The debugging code in the following ifdef land
>  - QL_ALL_DUMP
>  - QL_REG_DUMP
>  - QL_DEV_DUMP
>  - QL_CB_DUMP
>  - QL_IB_DUMP
>  - QL_OB_DUMP
> 
> becomes unnecessary because,
>  - Device status and general registers can be obtained by ethtool.
>  - Coredump can be done via devlink health reporter.
>  - Structure related to the hardware (struct ql_adapter) can be obtained
>    by crash or drgn.
> 
> Suggested-by: Benjamin Poirier <benjamin.poirier@gmail.com>
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
>  drivers/staging/qlge/qlge.h         |  82 ----
>  drivers/staging/qlge/qlge_dbg.c     | 688 ----------------------------
>  drivers/staging/qlge/qlge_ethtool.c |   2 -
>  drivers/staging/qlge/qlge_main.c    |   7 +-

Please also update drivers/staging/qlge/TODO accordingly. There is still
a lot of debugging code IMO (the netif_printk statements - kernel
tracing can be used instead of those) but this patch is a substantial
improvement.

Re: [PATCH v1 5/6] staging: qlge: clean up debugging code in the QL_ALL_DUMP ifdef land

[Coiby Xu]

On Sat, Oct 10, 2020 at 05:01:26PM +0900, Benjamin Poirier wrote:
>On 2020-10-08 19:58 +0800, Coiby Xu wrote:
>> The debugging code in the following ifdef land
>>  - QL_ALL_DUMP
>>  - QL_REG_DUMP
>>  - QL_DEV_DUMP
>>  - QL_CB_DUMP
>>  - QL_IB_DUMP
>>  - QL_OB_DUMP
>>
>> becomes unnecessary because,
>>  - Device status and general registers can be obtained by ethtool.
>>  - Coredump can be done via devlink health reporter.
>>  - Structure related to the hardware (struct ql_adapter) can be obtained
>>    by crash or drgn.
>>
>> Suggested-by: Benjamin Poirier <benjamin.poirier@gmail.com>
>> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
>> ---
>>  drivers/staging/qlge/qlge.h         |  82 ----
>>  drivers/staging/qlge/qlge_dbg.c     | 688 ----------------------------
>>  drivers/staging/qlge/qlge_ethtool.c |   2 -
>>  drivers/staging/qlge/qlge_main.c    |   7 +-
>
>Please also update drivers/staging/qlge/TODO accordingly. There is still
>a lot of debugging code IMO (the netif_printk statements - kernel
>tracing can be used instead of those) but this patch is a substantial
>improvement.

Thank you for the reminding! To move qlge out of staging tree would be
interesting exercise for me:)

--
Best regards,
Coiby

Re: [PATCH v1 5/6] staging: qlge: clean up debugging code in the QL_ALL_DUMP ifdef land

[Benjamin Poirier]

On 2020-10-10 18:00 +0800, Coiby Xu wrote:
[...]
> > 
> > Please also update drivers/staging/qlge/TODO accordingly. There is still
> > a lot of debugging code IMO (the netif_printk statements - kernel
> > tracing can be used instead of those) but this patch is a substantial
> > improvement.
> 
> Thank you for the reminding! To move qlge out of staging tree would be
> interesting exercise for me:)

If you would like to work more on the driver, I would highly suggest
getting one or two adapters to be able to test your changes. They can be
had for relatively cheap on ebay. Just search for "qle8142".

Re: [PATCH v1 5/6] staging: qlge: clean up debugging code in the QL_ALL_DUMP ifdef land

[Coiby Xu]

On Sat, Oct 10, 2020 at 10:40:55PM +0900, Benjamin Poirier wrote:
>On 2020-10-10 18:00 +0800, Coiby Xu wrote:
>[...]
>> >
>> > Please also update drivers/staging/qlge/TODO accordingly. There is still
>> > a lot of debugging code IMO (the netif_printk statements - kernel
>> > tracing can be used instead of those) but this patch is a substantial
>> > improvement.
>>
>> Thank you for the reminding! To move qlge out of staging tree would be
>> interesting exercise for me:)
>
>If you would like to work more on the driver, I would highly suggest
>getting one or two adapters to be able to test your changes. They can be
>had for relatively cheap on ebay. Just search for "qle8142".

Thank you for the info! Right now I don't have a desktop to install
this kind of adapter. I'll get one after settling the plan for a desktop.

--
Best regards,
Coiby

[PATCH v1 6/6] staging: qlge: add documentation for debugging qlge

[Coiby Xu]

Instructions and examples on kernel data structures dumping and coredump.

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 .../networking/device_drivers/index.rst       |   1 +
 .../device_drivers/qlogic/index.rst           |  18 +++
 .../networking/device_drivers/qlogic/qlge.rst | 118 ++++++++++++++++++
 MAINTAINERS                                   |   6 +
 4 files changed, 143 insertions(+)
 create mode 100644 Documentation/networking/device_drivers/qlogic/index.rst
 create mode 100644 Documentation/networking/device_drivers/qlogic/qlge.rst

diff --git a/Documentation/networking/device_drivers/index.rst b/Documentation/networking/device_drivers/index.rst
index a3113ffd7a16..d8279de7bf25 100644
--- a/Documentation/networking/device_drivers/index.rst
+++ b/Documentation/networking/device_drivers/index.rst
@@ -15,6 +15,7 @@ Contents:
    ethernet/index
    fddi/index
    hamradio/index
+   qlogic/index
    wan/index
    wifi/index
 
diff --git a/Documentation/networking/device_drivers/qlogic/index.rst b/Documentation/networking/device_drivers/qlogic/index.rst
new file mode 100644
index 000000000000..ad05b04286e4
--- /dev/null
+++ b/Documentation/networking/device_drivers/qlogic/index.rst
@@ -0,0 +1,18 @@
+.. SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+
+QLogic QLGE Device Drivers
+===============================================
+
+Contents:
+
+.. toctree::
+   :maxdepth: 2
+
+   qlge
+
+.. only::  subproject and html
+
+   Indices
+   =======
+
+   * :ref:`genindex`
diff --git a/Documentation/networking/device_drivers/qlogic/qlge.rst b/Documentation/networking/device_drivers/qlogic/qlge.rst
new file mode 100644
index 000000000000..0b888253d152
--- /dev/null
+++ b/Documentation/networking/device_drivers/qlogic/qlge.rst
@@ -0,0 +1,118 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=======================================
+QLogic QLGE 10Gb Ethernet device driver
+=======================================
+
+This driver use drgn and devlink for debugging.
+
+Dump kernel data structures in drgn
+-----------------------------------
+
+To dump kernel data structures, the following Python script can be used
+in drgn:
+
+.. code-block:: python
+
+	def align(x, a):
+	    """the alignment a should be a power of 2
+	    """
+	    mask = a - 1
+	    return (x+ mask) & ~mask
+
+	def struct_size(struct_type):
+	    struct_str = "struct {}".format(struct_type)
+	    return sizeof(Object(prog, struct_str, address=0x0))
+
+	def netdev_priv(netdevice):
+	    NETDEV_ALIGN = 32
+	    return netdevice.value_() + align(struct_size("net_device"), NETDEV_ALIGN)
+
+	name = 'xxx'
+	qlge_device = None
+	netdevices = prog['init_net'].dev_base_head.address_of_()
+	for netdevice in list_for_each_entry("struct net_device", netdevices, "dev_list"):
+	    if netdevice.name.string_().decode('ascii') == name:
+	        print(netdevice.name)
+
+	ql_adapter = Object(prog, "struct ql_adapter", address=netdev_priv(qlge_device))
+
+The struct ql_adapter will be printed in drgn as follows,
+
+    >>> ql_adapter
+    (struct ql_adapter){
+            .ricb = (struct ricb){
+                    .base_cq = (u8)0,
+                    .flags = (u8)120,
+                    .mask = (__le16)26637,
+                    .hash_cq_id = (u8 [1024]){ 172, 142, 255, 255 },
+                    .ipv6_hash_key = (__le32 [10]){},
+                    .ipv4_hash_key = (__le32 [4]){},
+            },
+            .flags = (unsigned long)0,
+            .wol = (u32)0,
+            .nic_stats = (struct nic_stats){
+                    .tx_pkts = (u64)0,
+                    .tx_bytes = (u64)0,
+                    .tx_mcast_pkts = (u64)0,
+                    .tx_bcast_pkts = (u64)0,
+                    .tx_ucast_pkts = (u64)0,
+                    .tx_ctl_pkts = (u64)0,
+                    .tx_pause_pkts = (u64)0,
+                    ...
+            },
+            .active_vlans = (unsigned long [64]){
+                    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 52780853100545, 18446744073709551615,
+                    18446619461681283072, 0, 42949673024, 2147483647,
+            },
+            .rx_ring = (struct rx_ring [17]){
+                    {
+                            .cqicb = (struct cqicb){
+                                    .msix_vect = (u8)0,
+                                    .reserved1 = (u8)0,
+                                    .reserved2 = (u8)0,
+                                    .flags = (u8)0,
+                                    .len = (__le16)0,
+                                    .rid = (__le16)0,
+                                    ...
+                            },
+                            .cq_base = (void *)0x0,
+                            .cq_base_dma = (dma_addr_t)0,
+                    }
+                    ...
+            }
+    }
+
+coredump via devlink
+--------------------
+
+
+And the coredump obtained via devlink in json format looks like,
+
+.. code:: shell
+
+	$ devlink health dump show DEVICE reporter coredump -p -j
+	{
+	    "Core Registers": {
+	        "segment": 1,
+	        "values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ]
+	    },
+	    "Test Logic Regs": {
+	        "segment": 2,
+	        "values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ]
+	    },
+	    "RMII Registers": {
+	        "segment": 3,
+	        "values": [ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 ]
+	    },
+	    ...
+	    "Sem Registers": {
+	        "segment": 50,
+	        "values": [ 0,0,0,0 ]
+	    }
+	}
+
+When the module parameter qlge_force_coredump is set to be true, the MPI
+RISC reset before coredumping. So coredumping will much longer since
+devlink tool has to wait for 5 secs for the resetting to be
+finished.
diff --git a/MAINTAINERS b/MAINTAINERS
index 9ecb727f0a8f..d482078e3e88 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14411,6 +14411,12 @@ L:	netdev@vger.kernel.org
 S:	Supported
 F:	drivers/staging/qlge/
 
+QLOGIC QLGE 10Gb ETHERNET DRIVER
+M:	Coiby Xu <coiby.xu@gmail.com>
+L:	netdev@vger.kernel.org
+S:	Maintained
+F:	Documentation/networking/device_drivers/qlogic/qlge.rst
+
 QM1D1B0004 MEDIA DRIVER
 M:	Akihiro Tsukada <tskd08@gmail.com>
 L:	linux-media@vger.kernel.org
-- 
2.28.0