On Mon, 2007-01-29 at 01:45 -0700, Eric W. Biederman wrote: > Michael Ellerman writes: > > > On Sun, 2007-01-28 at 01:27 -0700, Eric W. Biederman wrote: > >> Michael Ellerman writes: > >> > >> > Currently pci_disable_device() disables MSI on a device by twiddling > >> > bits in config space via disable_msi_mode(). > >> > > >> > On some platforms that may not be appropriate, so abstract the MSI > >> > suspend logic into pci_disable_device_msi(). > >> > >> > > >> > Signed-off-by: Michael Ellerman > >> > --- > >> > > >> > drivers/pci/msi.c | 11 +++++++++++ > >> > drivers/pci/pci.c | 7 +------ > >> > drivers/pci/pci.h | 2 ++ > >> > 3 files changed, 14 insertions(+), 6 deletions(-) > >> > > >> > Index: msi/drivers/pci/msi.c > >> > =================================================================== > >> > --- msi.orig/drivers/pci/msi.c > >> > +++ msi/drivers/pci/msi.c > >> > @@ -271,6 +271,17 @@ void disable_msi_mode(struct pci_dev *de > >> > pci_intx(dev, 1); /* enable intx */ > >> > } > >> > > >> > +void pci_disable_device_msi(struct pci_dev *dev) > >> > +{ > >> > + if (dev->msi_enabled) > >> > + disable_msi_mode(dev, pci_find_capability(dev, PCI_CAP_ID_MSI), > >> > + PCI_CAP_ID_MSI); > >> > + > >> > + if (dev->msix_enabled) > >> > + disable_msi_mode(dev, pci_find_capability(dev, PCI_CAP_ID_MSI), > >> > + PCI_CAP_ID_MSIX); > >> > >> Just a quick note. This is wrong. It should be PCI_CAP_ID_MSIX. > >> The code that is being moved is buggy. So the patch itself doesn't > >> make the situation any worse. > > > > Greg, if you want to drop that patch I'll prepare two patches to fix it > > and then move it. I don't have any hardware to test, although I'm > > guessing no one does given that it's been broken since its inception. > > The mthca IB driver was one of the early adopters of MSI, and it uses > MSI-X. So it isn't that no one is using MSI-X and the MSI-X code > paths don't get exercised. I meant the MSI-X suspend/resume path specifically, I'm guessing most laptops don't come with IB cards yet ;) > I expect what is closer to the truth is that the code authors have so > far simply disabled msi separately instead of assuming pci_disable_device > will do it magically for them. So it may be the case that we can > just kill this code path altogether. I recall reading comments to that effect in one driver, although it wasn't obvious exactly what the problem was - but it's probably worth doing a thorough review while the number of MSI/MSI-X drivers is small. > Possibly we can just reduce it to WARN_ON(dev->msi_enabled || dev->msix_enabled); > > I suspect msi_remove_pci_irq_vectors may similarly not actually make a > difference. I think the function dates from a time when the code > attempted to cache the irq number so if you removed and re-added a module > or at least disabled and enabled msi you would get the same linux irq > number. I remember killing that caching because it made the code > unmaintainable and wasn't really useful. That was my suspicion as well, I was hoping someone who knew the code better than me would pipe up and let me know why it was a special case. Have the original MSI authors vanished without a trace? It seems to date from the initial MSI submission, and has only ever been called from pci_free_resources(). The rest of the pci hotunplug code paths are not clear to me though, so I don't know whether we can rely on pci_disable_msi() already being called for us. cheers -- Michael Ellerman OzLabs, IBM Australia Development Lab wwweb: http://michael.ellerman.id.au phone: +61 2 6212 1183 (tie line 70 21183) We do not inherit the earth from our ancestors, we borrow it from our children. - S.M.A.R.T Person