On Wednesday, September 24, 2008 3:17 AM, Matthew Wilcox wrote:
>On Mon, Sep 08, 2008 at 06:52:31PM +0800, Sheng Yang wrote:
>> +int pcie_do_flr(struct pci_dev *dev)
>> +{
>> + u16 status;
>> + u32 cap;
...
>> + ssleep(5);
5s is too long, 1s should be enough.
...
>
>Let's talk about what else we might need to do here. The spec says:
>
> Any outstanding INTx interrupt asserted by the Function must be
> de-asserted by sending the corresponding Deassert_INTx Message prior
> to starting the FLR.
>
I believe this is requirement of the hardware rather than the software. PCIe endpoint using legacy INTx should deassert the interrupt when either the Interrupt Disable bit in the Command register is set or FLR bit is triggered. Software needs not to worry about acking the asserted INTx when doing these operations.
>So we need to make sure there are no pending interrupts for this
>function before issuing the FLR. Is that the responsibility of the
>caller? We should say so if it is. If we're going to do it in this
>function, we should use disable_irq() to be sure that the interrupt
>isn't currently executing on some other CPU (having first checked that
>the device isn't using MSI-X, because if it is, we don't use the
>interrupt in pdev->irq). If we're using a shared interrupt, other users
>are going to suffer.
>
>I still don't really understand how this flr fits in with every other
>part of the system. Is the device supposed to be quiesced before this
>is called? If so, a lot of what it does is pointless. Or is it
>supposed to do the quiescing? If so, it's missing some bits.
Though the original purpose of this function is for KVM, having an API to reset device using standardized methods obviously benefits the driver developers a lot -- they don't have to write their own ones or use device specific mechanisms to reset the hardware at initialization or error recovery stage if devices support one of these methods. The standardized methods include: FLR, PM D3-D0 and secondary bus reset, and all of them should be implemented in this function. Only setting/clearing FLR bit is almost useless since it's neither supported by PCI device nor most of PCIe devices. The code above also should call pci_set_pcie_reset_state too, to check if PCIe conventional reset is supported.
>
>--
>Matthew Wilcox Intel Open Source Technology Centre
>"Bill, look, we understand that you're interested in selling us this
>operating system, but compare it to ours. We can't possibly take such
>a retrograde step."
>--
>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
--
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
