Bjorn Helgaas wrote on 2012-08-23:
> 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.
>
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
