On Wednesday 24 September 2008 03:16:40 Matthew Wilcox wrote:
> 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.
> */
>
Matthew, thanks to be so detail...
> > +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;
I will modify this. But I still don't understand why -ENOTTY... I think there
are some history about it?
> > +
> > + 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?
>From the implemention note of FLR, "Software clears the entire Command
register, disabling the Function from issuing any new Requests." So here we
are.
And from PCI spec 6.2.2. Device Control
"The Command register provides coarse control over a device's ability to
generate and respond to PCI cycles. When a 0 is written to this register, the
device is logically disconnected from the PCI bus for all accesses except
configuration accesses. "
So, if 0 is written, I think the disconnection should affect rather than
individual bits.
But I have checked about PCI_EXP_DEVCTL, seems we don't need to clear it...
> > + /* 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?
Um... I just think that message is too ambiguous to know the exactly reason,
e.g. "still busy", user can ask "what are you waiting for?" So I want to show
what the function is doing here, and didn't aware the acronyms problem...
What's about "Still busy after 100ms when resetting function. Waiting 5
seconds\n"?
> > + 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.
I think the device didn't suppose to be completely quiesced before doing flr.
The spec also said
"Note that the controls that enable the Function to initiate requests on PCI
Express are cleared, including Bus Master Enable, MSI Enable, and the like,
effectively causing the Function to *become quiescent* on the Link."
So I think the caller should take care of INTx, and this routine cover others.
If the interrupts are shared, seems we can't prevent other user from
suffering a little. But luckily, the most modern device which need to be
reset should have used MSI/MSI-X, so I think it's reasonable.
But what's the missing bits your mentioned?
--
regards
Yang, Sheng
>
> --
> 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
