Re: [Penance PATCH] PCI: clean up the MSI code a bit

[Stefan Smietanowski <stesmi@stesmi.com>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Tom.

>>pci_enable_msix(dev)
>>{
>> if (is_dev_msi(dev))
>>   pci_disable_msi(dev);
>> else if (is_dev_msix(dev))
>>   return(ALREADY_MSIX);
>> else
>>   return(MSIX_NOT_AVAILABLE);
>> if (!__pci_enable_msix(dev))
>>   pci_enable_msi(dev);
>>}
>>
>>That way noone needs to explicitly turn off msi as it's done
>>automatically instead and the device will after this call
>>always be in either MSIX, MSI or NORMAL IRQ mode, and
>>always in the "best" mode the device, motherboard, bios, NB,
>>whatever combination is available.
> 
> 
> Your logic does not work because existing MSI/MSI-X code does not allow
> a driver to switch back and forth between MSI mode and MSI-X mode. A
> driver can switch interrupt mode between NORMAL IRQ mode and MSI mode or
> between NORMAL IRQ mode and MSI-X mode but NOT between MSI mode and
> MSI-X mode. A device driver should know well which MSI mode or MSI-X
> mode it wants to run when its device supports both MSI and MSI-X
> capability structures. Please read MSI-HOWTO before any attempt. If you
> like to continue this path, then think of a better policy of how to
> manage vector sources for MSI and MSI-X allocation before making
> changes.

I know about the different capability structures, but the function
as listed above really "only" makes a few changes.

Currently, with legacy irq mode, the driver calls
pci_enable_msix(dev, .., ..);
and that's it.

If we default to MSI mode, then that is replaced by
pci_disable_msi(dev); pci_enable_msix(dev, .., ..);
with any error checking required, etc.

If we implement an error code for each of the cases, so that
the driver KNOWS which mode it's in after pci_enable_msix() is called
I don't see a difference. I'm not an expert on the subject and likely
missing things but ..

If we could successfully change from msi to msix mode we get a normal
zero return code from pci_enable_msix(), however if we ended up in
msi mode we could have a new return code indicating that we're now in
msi mode and if we ended up with legacy IRQ mode we have another one
telling us that. The problem as I see it is that we then don't get the
other error codes from pci_enable_msix(). Takes some thinking I guess.
Only solution I see, if we want to be able to pass the error code as
well as which mode we ended up in (if not msix), is either another
argument to pci_enable_msix, changing
int pci_enable_msix(struct pci_dev *dev, u32 *entries, int nvec)
to
int pci_enable_msix(struct pci_dev *dev, u32 *entries, int nvec, int *mode)
where mode is for instance 0 for msix, 1 for msi and 2 for legacy irq.
We could also have "mode" be a mask of what we support at all.

for instance the device driver would do:

mode = MSIX|LEGACY_IRQ;
pci_enable_msix(dev, entries, nvec, &mode);
where mode would be overwritten with the mode we ended up in.

Tell me I'm missing something here. This doesn't feel like too different
to the logic the driver must ALREADY have, since it ALREADY must know
how to fall back to legacy irq mode.

if (pci_enable_msix(dev, entries, nvec) < 0) {
  if (pci_enable_msi(dev) < 0)
   ...
  ...
}
This is all changed to

if (pci_enable_msix(dev, entries, nvec, mode) < 0) {
  if (mode==MSI)
    ..
  else if (mode==LEGACY_IRQ)
    ..
}

Or am I really not thinking straight?

The logic should be already there in the driver beforehand, so it's just
slightly different way of knowing it.

// Stefan
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (MingW32)

iD8DBQFCqbgMBrn2kJu9P78RArWwAKCPk+xZcqhMfFybHztNUqwvSrOJagCgn0O2
EhWG6p+wb/b1cKtPLJU1vzc=
=AgYC
-----END PGP SIGNATURE-----