Re: [PATCH v3 01/11] PCI: Propagate FLR return values to callers

[Baolu Lu <baolu.lu@linux.intel.com>]

On 4/17/26 07:28, Nicolin Chen wrote:
> A reset failure implies that the device might be unreliable. E.g. its ATC
> might still retain stale entries. Thus, the IOMMU layer cannot trust this
> device to resume its ATS function that can lead to memory corruption. So,
> the pci_dev_reset_iommu_done() won't recover the device's IOMMU pathway if
> the device reset fails.
> 
> Those functions in the pci_dev_reset_methods array invoke pcie_flr(), but
> do not check the return value. Propagate them correctly.
> 
> Given that these functions have been running okay, and the return values
> will be only needed for an incoming work. This is not treated as bug fix.
> 
> Suggested-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>   drivers/pci/quirks.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 48946cca4be72..05ce12b6b2f76 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -3957,7 +3957,7 @@ static int reset_intel_82599_sfp_virtfn(struct pci_dev *dev, bool probe)
>   	 * supported.
>   	 */
>   	if (!probe)
> -		pcie_flr(dev);
> +		return pcie_flr(dev);
>   	return 0;
>   }
>   
> @@ -4015,6 +4015,7 @@ static int reset_chelsio_generic_dev(struct pci_dev *dev, bool probe)
>   {
>   	u16 old_command;
>   	u16 msix_flags;
> +	int ret;
>   
>   	/*
>   	 * If this isn't a Chelsio T4-based device, return -ENOTTY indicating
> @@ -4060,7 +4061,7 @@ static int reset_chelsio_generic_dev(struct pci_dev *dev, bool probe)
>   				      PCI_MSIX_FLAGS_ENABLE |
>   				      PCI_MSIX_FLAGS_MASKALL);
>   
> -	pcie_flr(dev);
> +	ret = pcie_flr(dev);

It makes more sense to return early here on failure. There is no need to
perform the subsequent steps if pcie_flr() fails. Would something like
the following work?

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 757a296eae41..9e0f29ac9f95 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4015,6 +4015,7 @@ static int reset_chelsio_generic_dev(struct 
pci_dev *dev, bool probe)
  {
         u16 old_command;
         u16 msix_flags;
+       int ret;

         /*
          * If this isn't a Chelsio T4-based device, return -ENOTTY 
indicating
@@ -4060,7 +4061,9 @@ static int reset_chelsio_generic_dev(struct 
pci_dev *dev, bool probe)
                                       PCI_MSIX_FLAGS_ENABLE |
                                       PCI_MSIX_FLAGS_MASKALL);

-       pcie_flr(dev);
+       ret = pcie_flr(dev);
+       if (ret)
+               return ret;

         /*
          * Restore the configuration information (BAR values, etc.) 
including

>   
>   	/*
>   	 * Restore the configuration information (BAR values, etc.) including
> @@ -4069,7 +4070,7 @@ static int reset_chelsio_generic_dev(struct pci_dev *dev, bool probe)
>   	 */
>   	pci_restore_state(dev);
>   	pci_write_config_word(dev, PCI_COMMAND, old_command);
> -	return 0;
> +	return ret;
>   }
>   
>   #define PCI_DEVICE_ID_INTEL_82599_SFP_VF   0x10ed
> @@ -4152,9 +4153,7 @@ static int nvme_disable_and_flr(struct pci_dev *dev, bool probe)
>   
>   	pci_iounmap(dev, bar);
>   
> -	pcie_flr(dev);
> -
> -	return 0;
> +	return pcie_flr(dev);
>   }
>   
>   /*
> @@ -4166,14 +4165,16 @@ static int nvme_disable_and_flr(struct pci_dev *dev, bool probe)
>    */
>   static int delay_250ms_after_flr(struct pci_dev *dev, bool probe)
>   {
> +	int ret;
> +
>   	if (probe)
>   		return pcie_reset_flr(dev, PCI_RESET_PROBE);
>   
> -	pcie_reset_flr(dev, PCI_RESET_DO_RESET);
> +	ret = pcie_reset_flr(dev, PCI_RESET_DO_RESET);
>   
>   	msleep(250);

The same there, ...

>   
> -	return 0;
> +	return ret;
>   }
>   
>   #define PCI_DEVICE_ID_HINIC_VF      0x375E
> @@ -4189,6 +4190,7 @@ static int reset_hinic_vf_dev(struct pci_dev *pdev, bool probe)
>   	unsigned long timeout;
>   	void __iomem *bar;
>   	u32 val;
> +	int ret;
>   
>   	if (probe)
>   		return 0;
> @@ -4209,7 +4211,7 @@ static int reset_hinic_vf_dev(struct pci_dev *pdev, bool probe)
>   	val = val | HINIC_VF_FLR_PROC_BIT;
>   	iowrite32be(val, bar + HINIC_VF_OP);
>   
> -	pcie_flr(pdev);
> +	ret = pcie_flr(pdev);


... and here.

>   
>   	/*
>   	 * The device must recapture its Bus and Device Numbers after FLR
> @@ -4236,7 +4238,7 @@ static int reset_hinic_vf_dev(struct pci_dev *pdev, bool probe)
>   reset_complete:
>   	pci_iounmap(pdev, bar);
>   
> -	return 0;
> +	return ret;
>   }
>   
>   static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {

Thanks,
baolu