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: Fri, 9 Oct 2020 08:12:45 +0800 From: Coiby Xu <coiby.xu@...il.com> To: Dan Carpenter <dan.carpenter@...cle.com> Cc: devel@...verdev.osuosl.org, "supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER" <GR-Linux-NIC-Dev@...vell.com>, Manish Chopra <manishc@...vell.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Shung-Hsi Yu <shung-hsi.yu@...e.com>, open list <linux-kernel@...r.kernel.org>, Benjamin Poirier <benjamin.poirier@...il.com>, "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 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@...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); >> }; >> >> + >> + > >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
Powered by blists - more mailing lists