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

On 2020-10-10 18:24 +0800, Coiby Xu wrote:

On Sat, Oct 10, 2020 at 04:35:14PM +0900, Benjamin Poirier wrote:

quoted

On 2020-10-08 19:58 +0800, Coiby Xu wrote:

quoted

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

Signed-off-by: Coiby Xu <redacted>
---
 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,

quoted

quoted

+
+
+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.