netdev - Re: [Intel-wired-lan] [PATCH iwl-next v2] i40e: Add support for fw health report

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <844e2248-c842-40a0-b4f5-8e71f3d06c03@intel.com>
Date: Mon, 29 Jul 2024 14:04:54 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Kamal Heib <kheib@...hat.com>
CC: Ivan Vecera <ivecera@...hat.com>, <netdev@...r.kernel.org>, Tony Nguyen
	<anthony.l.nguyen@...el.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, "David S . Miller" <davem@...emloft.net>,
	<intel-wired-lan@...ts.osuosl.org>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v2] i40e: Add support for fw
 health report

On 7/18/24 20:13, Kamal Heib wrote:
> Add support for reporting fw status via the devlink health report.
> 
> Example:
>   # devlink health show pci/0000:02:00.0 reporter fw
>   pci/0000:02:00.0:
>     reporter fw
>       state healthy error 0 recover 0
>   # devlink health diagnose pci/0000:02:00.0 reporter fw
>   Mode: normal
> 
> Signed-off-by: Kamal Heib <kheib@...hat.com>
> ---
> v2:
> - Address comments from Jiri.
> - Move the creation of the health report.
> ---
>   drivers/net/ethernet/intel/i40e/i40e.h        |  1 +
>   .../net/ethernet/intel/i40e/i40e_devlink.c    | 57 +++++++++++++++++++
>   .../net/ethernet/intel/i40e/i40e_devlink.h    |  2 +
>   drivers/net/ethernet/intel/i40e/i40e_main.c   | 14 +++++
>   4 files changed, 74 insertions(+)
> 

[...]

> +int i40e_devlink_create_health_reporter(struct i40e_pf *pf)
> +{
> +	struct devlink *devlink = priv_to_devlink(pf);
> +	struct device *dev = &pf->pdev->dev;
> +	int rc = 0;
> +
> +	devl_lock(devlink);

if you are going to have just one health reporter, you could just use
devlink_health_reporter_create() and avoid lock+unlock calls here

> +	pf->fw_health_report =
> +		devl_health_reporter_create(devlink, &i40e_fw_reporter_ops, 0, pf);
> +	if (IS_ERR(pf->fw_health_report)) {
> +		rc = PTR_ERR(pf->fw_health_report);
> +		dev_err(dev, "Failed to create fw reporter, err = %d\n", rc);

you are not zeroing pf->fw_health_report here, so ...

> +	}
> +	devl_unlock(devlink);
> +
> +	return rc;
> +}

[snip]

> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -15370,6 +15370,9 @@ static bool i40e_check_recovery_mode(struct i40e_pf *pf)
>   		dev_crit(&pf->pdev->dev, "Firmware recovery mode detected. Limiting functionality.\n");
>   		dev_crit(&pf->pdev->dev, "Refer to the Intel(R) Ethernet Adapters and Devices User Guide for details on firmware recovery mode.\n");
>   		set_bit(__I40E_RECOVERY_MODE, pf->state);
> +		if (pf->fw_health_report)
> +			devlink_health_report(pf->fw_health_report,

... you could possibly call devlink_health_report() with ERR_PTR as
the first argument

> +					      "recovery mode detected", pf);
>   
>   		return true;
>   	}