diff mbox series

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

Message ID 20201008115808.91850-2-coiby.xu@gmail.com
State New
Headers show
Series [v1,1/6] staging: qlge: Initialize devlink health dump framework for the dlge driver | expand

Commit Message

Coiby Xu Oct. 8, 2020, 11:58 a.m. UTC 
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

Comments

Willem de Bruijn Oct. 8, 2020, 12:22 p.m. UTC | #1 
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;
>  }
Coiby Xu Oct. 8, 2020, 12:54 p.m. UTC | #2 
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
Dan Carpenter Oct. 8, 2020, 1:31 p.m. UTC | #3 
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
kernel test robot Oct. 8, 2020, 5:45 p.m. UTC | #4 
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
Coiby Xu Oct. 9, 2020, 12:12 a.m. UTC | #5 
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
Benjamin Poirier Oct. 10, 2020, 7:35 a.m. UTC | #6 
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.
Coiby Xu Oct. 10, 2020, 10:24 a.m. UTC | #7 
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
Benjamin Poirier Oct. 10, 2020, 1:48 p.m. UTC | #8 
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.
Coiby Xu Oct. 12, 2020, 8:08 a.m. UTC | #9 
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
Coiby Xu Oct. 12, 2020, 11:24 a.m. UTC | #10 
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
Benjamin Poirier Oct. 13, 2020, 12:37 a.m. UTC | #11 
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.
Coiby Xu Oct. 15, 2020, 3:37 a.m. UTC | #12 
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
Benjamin Poirier Oct. 15, 2020, 11:06 a.m. UTC | #13 
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.
Coiby Xu Oct. 16, 2020, 11:08 p.m. UTC | #14 
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
diff mbox series

Patch

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);
 }