Re: [PATCH] PCI: Add device-specific reset for Kunpeng virtual functions

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Jul 18, 2025 at 03:21:30PM +0800, Weili Qian wrote:
> On 2025/7/15 3:19, Bjorn Helgaas wrote:
> > On Sat, Jul 12, 2025 at 07:30:28PM +0800, Weili Qian wrote:
> >> Prior to commit d591f6804e7e ("PCI: Wait for device readiness with
> >> Configuration RRS"), pci_dev_wait() polls PCI_COMMAND register until
> >> its value is not ~0(i.e., PCI_ERROR_RESPONSE). After d591f6804e7e,
> >> if the Configuration Request Retry Status Software Visibility (RRS SV)
> >> is enabled, pci_dev_wait() polls PCI_VENDOR_ID register until its value
> >> is not the reserved Vendor ID value 0x0001.
> >>
> >> On Kunpeng accelerator devices, RRS SV is enabled. However,
> >> when the virtual function's FLR (Function Level Reset) is not
> >> ready, the pci_dev_wait() reads the PCI_VENDOR_ID register and gets
> >> the value 0xffff instead of 0x0001. It then incorrectly assumes this
> >> is a valid Vendor ID and concludes the device is ready, returning
> >> successfully. In reality, the function may not be fully ready, leading
> >> to the device becoming unavailable.
> >>
> >> A 100ms wait period is already implemented before calling pci_dev_wait().
> >> In most cases, FLR completes within 100ms. However, to eliminate the
> >> risk of function being unavailable due to an incomplete FLR, a
> >> device-specific reset is added. After pcie_flr(), the function continues
> >> to poll PCI_COMMAND register until its value is no longer ~0.
> > 
> > As far as I can tell, there's nothing specific to Kungpeng devices
> > here.  We've seen a similar issue with Intel NVMe devices [1], and I
> > don't want a whole mess of quirks and device-specific reset methods.
> > 
> > We need some sort of generic solution for this.  My understanding was
> > that if devices are not ready 100ms after a reset, they are required
> > to respond with RRS.  Maybe these devices are defective.  Or maybe my
> > understanding is incorrect.  Either way, I think we should at least
> > check for a PCIe error before assuming that 0xffff is a valid
> > response.
> 
> A generic solution for this would be even better.
> 
> I tested the patch in [1], but it did not resolve the issue.
> According to the PCIe specification(e.g., Section 7.5.1
> PCI-Compatible Configuration Registers), the Vendor ID and Device ID
> for VFs are 0xffff.  However, if VFs cannot respond using the RRS
> mechanism, the pci_dev_wait() function reads the PCI_VENDOR_ID
> register and gets the value 0xffffffff. This value is considered an
> error on PCIe, causing pci_dev_wait() to continuously poll the
> register until a timeout occurs.

This is one of the problems: reading the VF Vendor ID gets ~0 in two
cases: (1) the config read was successful (the VF Vendor ID is
actually 0xffff), and (2) the config read failed and the RC
synthesized ~0 return data.  I hoped to be able to distinguish these
with the debug patch at [2], but so far it doesn't work as I expected,
so more debug is required.

[1] is an example of another way of doing a device-specific quirk.  It
doesn't really matter whether that way works for the Kunpeng device,
because I want to avoid any of these device-specific things if
possible.

> > [1] https://lore.kernel.org/linux-pci/20250611101442.387378-1-hui.wang@canonical.com/

[2] https://lore.kernel.org/r/20250701232341.GA1859056@bhelgaas

> >> Fixes: d591f6804e7e ("PCI: Wait for device readiness with Configuration RRS")
> >> Signed-off-by: Weili Qian <qianweili@huawei.com>
> >> ---
> >>  drivers/pci/quirks.c | 36 ++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 36 insertions(+)
> >>
> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> >> index d7f4ee634263..1df1756257d2 100644
> >> --- a/drivers/pci/quirks.c
> >> +++ b/drivers/pci/quirks.c
> >> @@ -4205,6 +4205,36 @@ static int reset_hinic_vf_dev(struct pci_dev *pdev, bool probe)
> >>  	return 0;
> >>  }
> >>  
> >> +#define KUNPENG_OPERATION_WAIT_CNT	3000
> >> +#define KUNPENG_RESET_WAIT_TIME		20
> >> +
> >> +/* Device-specific reset method for Kunpeng accelerator virtual functions */
> >> +static int reset_kunpeng_acc_vf_dev(struct pci_dev *pdev, bool probe)
> >> +{
> >> +	u32 wait_cnt = 0;
> >> +	u32 cmd;
> >> +
> >> +	if (probe)
> >> +		return 0;
> >> +
> >> +	pcie_flr(pdev);
> >> +
> >> +	do {
> >> +		pci_read_config_dword(pdev, PCI_COMMAND, &cmd);
> >> +		if (!PCI_POSSIBLE_ERROR(cmd))
> >> +			break;
> >> +
> >> +		if (++wait_cnt > KUNPENG_OPERATION_WAIT_CNT) {
> >> +			pci_warn(pdev, "wait for FLR ready timeout; giving up\n");
> >> +			return -ENOTTY;
> >> +		}
> >> +
> >> +		msleep(KUNPENG_RESET_WAIT_TIME);
> >> +	} while (true);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
> >>  	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82599_SFP_VF,
> >>  		 reset_intel_82599_sfp_virtfn },
> >> @@ -4220,6 +4250,12 @@ static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
> >>  		reset_chelsio_generic_dev },
> >>  	{ PCI_VENDOR_ID_HUAWEI, PCI_DEVICE_ID_HINIC_VF,
> >>  		reset_hinic_vf_dev },
> >> +	{ PCI_VENDOR_ID_HUAWEI, PCI_DEVICE_ID_HUAWEI_ZIP_VF,
> >> +		reset_kunpeng_acc_vf_dev },
> >> +	{ PCI_VENDOR_ID_HUAWEI, PCI_DEVICE_ID_HUAWEI_SEC_VF,
> >> +		reset_kunpeng_acc_vf_dev },
> >> +	{ PCI_VENDOR_ID_HUAWEI, PCI_DEVICE_ID_HUAWEI_HPRE_VF,
> >> +		reset_kunpeng_acc_vf_dev },
> >>  	{ 0 }
> >>  };
> >>  
> >> -- 
> >> 2.33.0
> >>
> > .
> >