Messages in this thread | | | Date | Wed, 18 Sep 2013 11:48:00 +0200 | From | Alexander Gordeev <> | Subject | Re: [PATCH v2 2/6] PCI/MSI: Factor out pci_get_msi_cap() interface |
| |
On Wed, Sep 18, 2013 at 12:30:23AM +1000, Michael Ellerman wrote: > How about no? > > We have a small number of MSIs available, limited by hardware & > firmware, if we don't impose a quota then the first device that probes > will get most/all of the MSIs and other devices miss out.
Out of curiosity - how pSeries has had done it without quotas before 448e2ca ("powerpc/pseries: Implement a quota system for MSIs")?
> Anyway I don't see what problem you're trying to solve? I agree the > -ve/0/+ve return value pattern is ugly, but it's hardly the end of the > world.
Well, the interface recently has been re-classified from "ugly" to "unnecessarily complex and actively harmful" in Tejun's words ;)
Indeed, I checked most of the drivers and it is incredible how people are creative in misusing the interface: from innocent pci_disable_msix() calls when if pci_enable_msix() failed to assuming MSI-Xs were enabled if pci_enable_msix() returned a positive value (apparently untested).
Roughly third of the drivers just do not care and bail out once pci_enable_msix() has not succeeded. Not sure how many of these are mandated by the hardware.
Another quite common pattern is a call to pci_enable_msix() to figure out the number of MSI-Xs available and a repeated call of pci_enable_msix() to enable those MSI-Xs, this time.
The last pattern makes most of sense to me and could be updated with a more clear sequence - a call to (bit modified) pci_msix_table_size() followed by a call to pci_enable_msix(). I think this pattern can effectively supersede the currently recommended "loop" practice.
But as pSeries quota is still required I propose to introduce a new interface pci_get_msix_limit() that combines pci_msix_table_size() and (also new) arch_get_msix_limit(). The latter would check the quota thing in case of pSeries and none in case of all other architectures.
The recommended practice would be:
/* * Retrieving 'nvec' by means other than pci_msix_table_size() */
rc = pci_get_msix_limit(pdev); if (rc < 0) return rc;
/* * nvec = min(rc, nvec); */
for (i = 0; i < nvec; i++) msix_entry[i].entry = i;
rc = pci_enable_msix(pdev, msix_entry, nvec); if (rc) return rc;
Thoughts?
> cheers
-- Regards, Alexander Gordeev agordeev@redhat.com
| |