[RFC/PATCH 4/16] Abstract MSI suspend

[Michael Ellerman]

On Mon, 2007-01-29 at 01:45 -0700, Eric W. Biederman wrote:
> Michael Ellerman <michael at ellerman.id.au> writes:
> 
> > On Sun, 2007-01-28 at 01:27 -0700, Eric W. Biederman wrote:
> >> Michael Ellerman <michael at ellerman.id.au> 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 <michael at ellerman.id.au>
> >> > ---
> >> >
> >> >  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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20070129/7b2ab4c1/attachment.pgp>