On 08/22/2012 12:28 PM, Bjorn Helgaas wrote:
On Mon, Aug 20, 2012 at 9:40 PM, Cui, Dexuan<dexuan.cui@xxxxxxxxx> 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.
It should not exist in any other VF device. The SRIOV spec states
that all VFs must support FLR. The 82599 quirk is just that...
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;
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
