On Mon, Sep 08, 2008 at 06:52:31PM +0800, Sheng Yang wrote:
> /**
> + * pcie_do_flr() - execute Function Level Reset
> + * @dev: PCI device to execute FLR
> + *
> + * Returns 0 on success, or -EOPNOTSUPP if device don't support the feature.
> + *
> + * The caller should maintained the save/restore of PCI configuration space.
> + *
> + * Notice that on rare case, the Transaction Pending bit does not clear before
> + * FLR, we forced to continue the process, then bless...
> + **/
/**
* pcie_do_flr() - execute Function Level Reset
* @dev: Device to reset
*
* Function Level Reset is a feature introduced by the PCIe spec version
* XXX. It allows one function of a multifunction PCI device to be
* reset without affecting the other functions in the same device.
* Resetting the device will make the contents of PCI configuration space
* random, so any caller of this must be prepared to reinitialise the
* device including MSI, bus mastering, BARs, decoding IO and memory spaces,
* etc.
*
* Returns 0 if the device was successfully reset or an error code if
* the device doesn't support FLR or we failed to reset the device
* properly.
*/
> +int pcie_do_flr(struct pci_dev *dev)
> +{
> + u16 status;
> + u32 cap;
> + int exppos = pci_find_capability(dev, PCI_CAP_ID_EXP);
> +
> + if (!exppos)
> + return -ENOTSUPP;
return -ENOTTY;
> + pci_read_config_dword(dev, exppos + PCI_EXP_DEVCAP, &cap);
> + if (!(cap & PCI_EXP_DEVCAP_FLR))
> + return -ENOTSUPP;
return -ENOTTY;
> +
> + pci_block_user_cfg_access(dev);
> +
> + /* Clear entire Command register */
> + pci_write_config_word(dev, PCI_COMMAND, 0);
> + pci_write_config_word(dev, exppos + PCI_EXP_DEVCTL, 0);
Is this wise? Clearing the 'disable INTx' bit (10) might cause the
device to generate interrupts on its interrupt pin which could cause
lockups. I'm also not sure we need to clear any of the bits in
PCI_EXP_DEVCTL -- can you explain?
> + /* Wait for Transaction Pending bit clean */
> + msleep(100);
> + pci_read_config_word(dev, exppos + PCI_EXP_DEVSTA, &status);
> + if (status & PCI_EXP_DEVSTA_TRPND) {
> + dev_info(&dev->dev, "trasaction pending when doing FLR, "
> + "waiting for another 5 seconds\n");
I don't ever want to see 'FLR' in a message the user might see. If you
have to, spell it out as 'Function Level Reset' or just say "resetting
function" or something. We use far too many acronyms as it is. What
was wrong with the text I proposed in my first review?
> + ssleep(5);
> + pci_read_config_word(dev, exppos + PCI_EXP_DEVSTA, &status);
> + if (status & PCI_EXP_DEVSTA_TRPND)
> + dev_info(&dev->dev, "still busy after 5s when doing "
> + "FLR, reset anyway\n");
as above.
> + }
> +
> + pci_write_config_word(dev, exppos + PCI_EXP_DEVCTL,
> + PCI_EXP_DEVCTL_BCR_FLR);
> + mdelay(100);
> +
> + pci_unblock_user_cfg_access(dev);
> + return 0;
> +}
> +EXPORT_SYMBOL(pcie_do_flr);
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.
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.
--
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
