Re: [PATCH v3 00/32] provide interfaces to access PCIe capabilities registers

[Bjorn Helgaas <bhelgaas@google.com>]

On Mon, Aug 20, 2012 at 9:40 PM, Cui, Dexuan <dexuan.cui@intel.com> wrote:
> Bjorn Helgaas wrote on 2012-08-21:

>> I am still concerned about reset_intel_82599_sfp_virtfn().  It looks
>> wrong and possibly unnecessary.  It looks wrong because it sets
>> PCI_EXP_DEVCTL_BCR_FLR and blindly clears all other bits in
>> PCI_EXP_DEVCTL.  Most of the bits are probably cleared by the FLR
>> anyway, but Aux Power PM Enable is RWS ("sticky"), so it is *not*
>> modified by FLR.  Therefore, using reset_intel_82599_sfp_virtfn() has
>> the probably unintended side effect of clearing Aux Power PM Enable.
>>
>> It looks possibly unnecessary because the generic pcie_flr() does
>> essentially the same thing, so it's not clear why we need a special
>> case for 82599.

> I think reset_intel_82599_sfp_virtfn() is correct AND necessary.
> (pcie_flr() doesn't work here)
>
> Please note the 82599 VF is a special PCIe device that doesn't report
> PCIe FLR capability though actually it does support that.
> That is why we put it in quirks.c. :-)
>
> The PCI_EXP_DEVCTL_AUX_PME bit of the 82599 VF is read-only and
> always zero.
>
> Please see
> http://www.intel.com/content/dam/doc/datasheet/82599-10-gbe-controller-datasheet.pdf
> 9.5 Virtual Functions Configuration Space
>   Table 9.7 VF PCIe Configuration Space
>   9.5.2.2.1 VF Device Control Register (0xA8; RW)

Thanks, Dexuan!

The 82599 does report FLR in the DEVCAP for the PF (sec 9.3.10.4), but
not in the DEVCAP for the VF (sec 9.5), which indeed means we can't
use pcie_flr() on the VFs.  I wonder whether this error appears in any
other devices.

The VF DEVCTL register (sec 9.5.2.2.1) is RO and zero except for
"Initiate FLR" unlike the PF DEVCTL (sec 9.3.10.5).  The
read/modify/write done by pcie_flr() would work on the VF but is not
necessary.

The VF DEVSTA register (sec 9.5.2.2.2) does have an active
"Transaction Pending" bit.  That suggests to me that we should wait
for it to be clear, as pcie_flr() does.

What would you think of a patch like the following?  My idea is to
make it the same as pcie_flr() except for the absolutely necessary
differences.  With this patch, the only difference is that we don't
look at the 82599 DEVCAP FLR bit.

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index aa77538..7a451ff 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3081,10 +3081,36 @@ static int reset_intel_generic_dev(struct
pci_dev *dev, int probe)

 static int reset_intel_82599_sfp_virtfn(struct pci_dev *dev, int probe)
 {
+       int i;
+       u16 status;
+
+       /*
+        * http://www.intel.com/content/dam/doc/datasheet/82599-10-gbe-controller-datasheet.pdf
+        *
+        * The 82599 supports FLR on VFs, but FLR support is reported only
+        * in the PF DEVCAP (sec 9.3.10.4), not in the VF DEVCAP (sec 9.5).
+        * Therefore, we can't use pcie_flr(), which checks the VF DEVCAP.
+        */
+
        if (probe)
                return 0;

-       pcie_capability_write_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
+       /* Wait for Transaction Pending bit clean */
+       for (i = 0; i < 4; i++) {
+               if (i)
+                       msleep((1 << (i - 1)) * 100);
+
+               pcie_capability_read_word(dev, PCI_EXP_DEVSTA, &status);
+               if (!(status & PCI_EXP_DEVSTA_TRPND))
+                       goto clear;
+       }
+
+       dev_err(&dev->dev, "transaction is not cleared; "
+                       "proceeding with reset anyway\n");
+
+clear:
+       pcie_capability_set_word(dev, PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
+
        msleep(100);

        return 0;