diff mbox

[v3,00/32] provide interfaces to access PCIe capabilities registers

Message ID CAErSpo5ftvKLF2=1ixzp1KroNLKh_s-b5hi9aFv3X7PpqisGwg@mail.gmail.com
State Not Applicable
Headers show

Commit Message

Bjorn Helgaas Aug. 22, 2012, 4:28 p.m. UTC 
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.

        return 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Cui, Dexuan Aug. 23, 2012, 1 a.m. UTC | #1 
Bjorn Helgaas wrote on 2012-08-23:
> 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.
I agree.

> 
> 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.
Hi Bjorn,
Thanks for the patch!
It seems good to me (BTW, I don't have such a device at
hand to test)

Thanks,
-- Dexuan

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Don Dutile Aug. 23, 2012, 1:51 a.m. UTC | #2 
On 08/22/2012 12:28 PM, Bjorn Helgaas wrote:
> 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.
>
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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);