| Message ID | 1477695497-6207-4-git-send-email-keith.busch@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Fri, Oct 28, 2016 at 06:58:17PM -0400, Keith Busch wrote: > There is no need to disable MSIx interrupts or write message data on a > device that isn't present. This patch checks the device removed state > prior to executing device shutdown operations so that tear down on > removed devices completes quicker. > > Signed-off-by: Keith Busch <keith.busch@intel.com> > Cc: Lukas Wunner <lukas@wunner.de> > --- > drivers/pci/msi.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > index bfdd074..9249ec1 100644 > --- a/drivers/pci/msi.c > +++ b/drivers/pci/msi.c > @@ -317,7 +317,7 @@ void __pci_write_msi_msg(struct msi_desc *entry, struct msi_msg *msg) > { > struct pci_dev *dev = msi_desc_to_pci_dev(entry); > > - if (dev->current_state != PCI_D0) { > + if (dev->current_state != PCI_D0 || dev->is_removed) { > /* Don't touch the hardware now */ > } else if (entry->msi_attrib.is_msix) { > void __iomem *base = pci_msix_desc_addr(entry); Hm, I don't see __pci_write_msi_msg() being called anywhere on device removal. Am I missing something? Rest of the patch LGTM. Thanks, Lukas > @@ -1017,6 +1017,11 @@ void pci_msix_shutdown(struct pci_dev *dev) > if (!pci_msi_enable || !dev || !dev->msix_enabled) > return; > > + if (dev->is_removed) { > + dev->msix_enabled = 0; > + return; > + } > + > /* Return the device with MSI-X masked as initial states */ > for_each_pci_msi_entry(entry, dev) { > /* Keep cached states to be restored */ > -- > 2.7.2 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 31, 2016 at 12:00:40PM +0100, Lukas Wunner wrote: > On Fri, Oct 28, 2016 at 06:58:17PM -0400, Keith Busch wrote: > > There is no need to disable MSIx interrupts or write message data on a > > device that isn't present. This patch checks the device removed state > > prior to executing device shutdown operations so that tear down on > > removed devices completes quicker. > > > > Signed-off-by: Keith Busch <keith.busch@intel.com> > > Cc: Lukas Wunner <lukas@wunner.de> > > --- > > drivers/pci/msi.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > > index bfdd074..9249ec1 100644 > > --- a/drivers/pci/msi.c > > +++ b/drivers/pci/msi.c > > @@ -317,7 +317,7 @@ void __pci_write_msi_msg(struct msi_desc *entry, struct msi_msg *msg) > > { > > struct pci_dev *dev = msi_desc_to_pci_dev(entry); > > > > - if (dev->current_state != PCI_D0) { > > + if (dev->current_state != PCI_D0 || dev->is_removed) { > > /* Don't touch the hardware now */ > > } else if (entry->msi_attrib.is_msix) { > > void __iomem *base = pci_msix_desc_addr(entry); > > Hm, I don't see __pci_write_msi_msg() being called anywhere on device > removal. Am I missing something? Rest of the patch LGTM. Prior to calling pci_disable_msix as part of typical driver unbinding, the driver needs to call free_irq for every registerded action. That will get to this __pci_write_msi_msg when it tries to mask the vector. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 28, 2016 at 06:58:17PM -0400, Keith Busch wrote: > There is no need to disable MSIx interrupts or write message data on a > device that isn't present. This patch checks the device removed state > prior to executing device shutdown operations so that tear down on > removed devices completes quicker. > > Signed-off-by: Keith Busch <keith.busch@intel.com> > Cc: Lukas Wunner <lukas@wunner.de> > --- > drivers/pci/msi.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > index bfdd074..9249ec1 100644 > --- a/drivers/pci/msi.c > +++ b/drivers/pci/msi.c > @@ -317,7 +317,7 @@ void __pci_write_msi_msg(struct msi_desc *entry, struct msi_msg *msg) > { > struct pci_dev *dev = msi_desc_to_pci_dev(entry); > > - if (dev->current_state != PCI_D0) { > + if (dev->current_state != PCI_D0 || dev->is_removed) { > /* Don't touch the hardware now */ > } else if (entry->msi_attrib.is_msix) { > void __iomem *base = pci_msix_desc_addr(entry); > @@ -1017,6 +1017,11 @@ void pci_msix_shutdown(struct pci_dev *dev) > if (!pci_msi_enable || !dev || !dev->msix_enabled) > return; > > + if (dev->is_removed) { > + dev->msix_enabled = 0; > + return; > + } > + > /* Return the device with MSI-X masked as initial states */ > for_each_pci_msi_entry(entry, dev) { > /* Keep cached states to be restored */ Do we need the same thing in pci_msi_shutdown()? It's too bad we have the data structure maintenance stuff all intermingled with the hardware-touching code. I don't know how they could really be disentangled, but it feels a little ad hoc to sprinkle is_removed checks in random places where we observe problems. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 13, 2016 at 03:18:10PM -0600, Bjorn Helgaas wrote: > On Fri, Oct 28, 2016 at 06:58:17PM -0400, Keith Busch wrote: > > @@ -1017,6 +1017,11 @@ void pci_msix_shutdown(struct pci_dev *dev) > > if (!pci_msi_enable || !dev || !dev->msix_enabled) > > return; > > > > + if (dev->is_removed) { > > + dev->msix_enabled = 0; > > + return; > > + } > > + > > /* Return the device with MSI-X masked as initial states */ > > for_each_pci_msi_entry(entry, dev) { > > /* Keep cached states to be restored */ > > Do we need the same thing in pci_msi_shutdown()? We get MSI automatically from the previous two patches since MSI masking is config access. MSIx is MMIO so it needs to explicitly check the removed flag to fence off those unnecessary read-modify-writes. > It's too bad we have the data structure maintenance stuff all > intermingled with the hardware-touching code. I don't know how they > could really be disentangled, but it feels a little ad hoc to sprinkle > is_removed checks in random places where we observe problems. Yeah, I unfortunately don't see a better way to do it either. It may look a bit arbitrary considering there's plenty of other places we could possibly check for removed, but this is the one we observe to be the highest contributor. I'm mainly interested in NVM-Express where controllers have dozens to hundreds of vectors, so this is potentially shaving off another ~100 non-posted commands. I have heard interest in leveraging this flag for other device drivers, though, and suspect new usage may come later. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index bfdd074..9249ec1 100644 --- a/drivers/pci/msi.c +++ b/drivers/pci/msi.c @@ -317,7 +317,7 @@ void __pci_write_msi_msg(struct msi_desc *entry, struct msi_msg *msg) { struct pci_dev *dev = msi_desc_to_pci_dev(entry); - if (dev->current_state != PCI_D0) { + if (dev->current_state != PCI_D0 || dev->is_removed) { /* Don't touch the hardware now */ } else if (entry->msi_attrib.is_msix) { void __iomem *base = pci_msix_desc_addr(entry); @@ -1017,6 +1017,11 @@ void pci_msix_shutdown(struct pci_dev *dev) if (!pci_msi_enable || !dev || !dev->msix_enabled) return; + if (dev->is_removed) { + dev->msix_enabled = 0; + return; + } + /* Return the device with MSI-X masked as initial states */ for_each_pci_msi_entry(entry, dev) { /* Keep cached states to be restored */
There is no need to disable MSIx interrupts or write message data on a device that isn't present. This patch checks the device removed state prior to executing device shutdown operations so that tear down on removed devices completes quicker. Signed-off-by: Keith Busch <keith.busch@intel.com> Cc: Lukas Wunner <lukas@wunner.de> --- drivers/pci/msi.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)