lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 12 Oct 2020 19:24:06 +0800 From: Coiby Xu <coiby.xu@...il.com> To: Benjamin Poirier <benjamin.poirier@...il.com> Cc: devel@...verdev.osuosl.org, Shung-Hsi Yu <shung-hsi.yu@...e.com>, Manish Chopra <manishc@...vell.com>, "supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER" <GR-Linux-NIC-Dev@...vell.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, open list <linux-kernel@...r.kernel.org>, "open list:QLOGIC QLGE 10Gb ETHERNET DRIVER" <netdev@...r.kernel.org> Subject: Re: [PATCH v1 1/6] staging: qlge: Initialize devlink health dump framework for the dlge driver 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@...il.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
Powered by blists - more mailing lists