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

[Don Dutile <ddutile@redhat.com>]

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;