Re: [PATCH v2 2/6] PCI/MSI: Factor out pci_get_msi_cap() interface

[Tejun Heo <tj@kernel.org>]

Hello,

On Wed, Sep 18, 2013 at 11:48:00AM +0200, Alexander Gordeev wrote:
> 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")?

Hmmm... do we need to treat this any differently?  If the platform
can't allocate full range of requested MSIs, just failing should be
enough regardless of why such allocation can't be met, no?

> > 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 ;)

LOL. :)

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

Yeah, I mean, this type of interface is a trap.  People have to
actively resist to avoid doing silly stuff which is a lot to ask.

> 	/*
> 	 * 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;

I really think what we should do is

* Determine the number of MSIs the controller wants.  Don't worry
  about quotas or limits or anything.  Just determine the number
  necessary to enable enhanced interrupt handling.
	
* Try allocating that number of MSIs.  If it fails, then just revert
  to single interrupt mode.  It's not the end of the world and mostly
  guaranteed to work.  Let's please not even try to do partial
  multiple interrupts.  I really don't think it's worth the risk or
  complexity.

Thanks.

-- 
tejun