[RFC/PATCH 4/16] Abstract MSI suspend

[Eric W. Biederman]

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 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.

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.

In summary I think there is still room for cleanup in msi.c, but it
think it is at least reaching the point where you just don't stumble
over opportunities.

Eric