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

[Benjamin Poirier <benjamin.poirier@gmail.com>]

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.