On Wed, Sep 24, 2008 at 10:39:12PM +0800, Zhao, Yu wrote: > >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. Oh, that makes sense. Sorry, for some reason I got confused and thought the Deassert_INTx was sent by the root port back to the device. It isn't, of course, it's sent by the device. You're right, we don't need to do anything to support this case. I believe my comment about shared interrupts still holds true, though, so we still need to do _something_ for interrupts -- either disable_irq() or make sure no interrupt handler currently registered will try to touch this device. > >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. I'm not quite sure what you mean there, but if we've asked for a function to be reset, we clearly don't want to reset the entire device. Better to fail, and return an error to the caller, that way they can decide whether to try to reset the entire device or not. I'd quite like to see this tied into the Linux PCI error handling framework as part of the steps involved in recovery. It would be nice (if the problem is only with one function of a device) to only reset that one function, not all of them. I'm not quite sure how PCI error recovery interacts with v12n. It seems more than slightly rude to reset a device that includes a function in another machine's address space. Anyway, I've fairly aggressively not been paying attention to v12n and don't intend to start now. -- 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
