[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <714694aa-a6c3-4e89-ae12-a6a87bdf430a@intel.com>
Date: Wed, 13 Dec 2023 13:59:51 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Andrii Staikov <andrii.staikov@...el.com>,
<intel-wired-lan@...ts.osuosl.org>
CC: <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>, Karen Ostrowska
<karen.ostrowska@...el.com>, Mateusz Palczewski
<mateusz.palczewski@...el.com>, Drewek Wojciech <wojciech.drewek@...el.com>,
Kitszel Przemyslaw <przemyslaw.kitszel@...el.com>
Subject: Re: [PATCH iwl-net v4] i40e: Restore VF MSI-X state during PCI reset
On 12/12/2023 4:24 AM, Andrii Staikov wrote:
> During a PCI FLR the MSI-X Enable flag in the VF PCI MSI-X capability
> register will be cleared. This can lead to issues when a VF is
> assigned to a VM because in these cases the VF driver receives no
> indication of the PF PCI error/reset and additionally it is incapable
> of restoring the cleared flag in the hypervisor configuration space
> without fully reinitializing the driver interrupt functionality.
>
> Since the VF driver is unable to easily resolve this condition on its own,
> restore the VF MSI-X flag during the PF PCI reset handling.
>
> Fixes: 19b7960b2da1 ("i40e: implement split PCI error reset handler")
> Co-developed-by: Karen Ostrowska <karen.ostrowska@...el.com>
> Signed-off-by: Karen Ostrowska <karen.ostrowska@...el.com>
> Co-developed-by: Mateusz Palczewski <mateusz.palczewski@...el.com>
> Signed-off-by: Mateusz Palczewski <mateusz.palczewski@...el.com>
> Reviewed-by: Drewek Wojciech <wojciech.drewek@...el.com>
> Reviewed-by: Kitszel Przemyslaw <przemyslaw.kitszel@...el.com>
> Signed-off-by: Andrii Staikov <andrii.staikov@...el.com>
The ice driver recently started caching the PCI device structure
pointers in their VF structure instead of having to do this sort of
lookup on the fly.
See 31642d2854e2 ("ice: store VF's pci_dev ptr in ice_vf") [1][2]
[1]:
https://lore.kernel.org/intel-wired-lan/20230912115626.105828-1-mateusz.polchlopek@intel.com/
[2]:
https://lore.kernel.org/netdev/20231019173227.3175575-4-jacob.e.keller@intel.com/
Can we do something similar for i40e?
> ---
> v1 -> v2: Fix signed-off tags
> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20231204131041.3369693-1-andrii.staikov@intel.com/
>
> v2 -> v3: use @vf_dev in pci_get_device() instead of NULL and remove unnecessary call
> https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20231206125127.218350-1-andrii.staikov@intel.com/
>
> v3 -> v4: wrap the added functionality into the CONFIG_PCI_IOV define as
> this is VF-related functionality
> ---
> drivers/net/ethernet/intel/i40e/i40e_main.c | 3 +++
> .../ethernet/intel/i40e/i40e_virtchnl_pf.c | 26 +++++++++++++++++++
> .../ethernet/intel/i40e/i40e_virtchnl_pf.h | 3 +++
> 3 files changed, 32 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 7bb1f64833eb..bbe2d115fb15 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -16513,6 +16513,9 @@ static void i40e_pci_error_reset_done(struct pci_dev *pdev)
> return;
>
> i40e_reset_and_rebuild(pf, false, false);
> +#ifdef CONFIG_PCI_IOV
> + i40e_restore_all_vfs_msi_state(pdev);
> +#endif /* CONFIG_PCI_IOV */
> }
>
> /**
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> index 3f99eb198245..d60f5419d6bd 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> @@ -154,6 +154,32 @@ void i40e_vc_notify_reset(struct i40e_pf *pf)
> (u8 *)&pfe, sizeof(struct virtchnl_pf_event));
> }
>
> +#ifdef CONFIG_PCI_IOV
Also noticed that i40e_virtchnl_pf.c is compiled always instead of only
conditionally when CONFIG_PCI_IOV is enabled. That's not really the
fault of this change, but I think it would be good cleanup to avoid
needing to compile any of this code if CONFIG_PCI_IOV is disabled.
> +void i40e_restore_all_vfs_msi_state(struct pci_dev *pdev)
> +{
> + u16 vf_id;
> + u16 pos;
> +
> + /* Continue only if this is a PF */
> + if (!pdev->is_physfn)
> + return;
> +
The function could also just pass the pf instead of pdev, and we'd know
its the physical function.
> + if (!pci_num_vf(pdev))
> + return;
If we switch to saving pdevs in our VF structure, this could be a simple
iteration loop that does nothing if the number of VFs is 0.
> +
> + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_SRIOV);
> + if (pos) {
> + struct pci_dev *vf_dev = NULL;
> +
> + pci_read_config_word(pdev, pos + PCI_SRIOV_VF_DID, &vf_id);
> + while ((vf_dev = pci_get_device(pdev->vendor, vf_id, vf_dev))) {
> + if (vf_dev->is_virtfn && vf_dev->physfn == pdev)
> + pci_restore_msi_state(vf_dev);
> + }
> + }
> +}
> +#endif /* CONFIG_PCI_IOV */
> +
Thanks,
Jake
Powered by blists - more mailing lists