lkml.org 
[lkml]   [2012]   [Aug]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 00/32] provide interfaces to access PCIe capabilities registers
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;



\
 
 \ /
  Last update: 2012-08-23 04:41    [W:0.272 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site