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

Commit Message

Michael Ellerman Oct. 2, 2013, 2:33 a.m. UTC

On Tue, Oct 01, 2013 at 07:55:03AM -0400, Tejun Heo wrote:
> Hello,
> 
> On Tue, Oct 01, 2013 at 05:35:48PM +1000, Michael Ellerman wrote:
> > > > 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.
> > 
> > I really think you're overstating the complexity here.
> > 
> > Functions typically return a boolean   -> nothing to see here
> > This function returns a tristate value -> brain explosion!
> 
> It is an interface which forces the driver writers to write
> complicated fallback code which won't usually be excercised.  

It does not force anyone to do anything. That's just bull.

Code which is unwilling or unable to cope with the extra complexity
can simply do:

if (pci_enable_msix(..))
	goto fail;

It's as simple as that.

> > > * 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.
> > 
> > It will potentially break existing setups on our hardware.
> 
> I think it'd be much better to have a separate interface for the
> drivers which actually require it *in practice* rather than forcing
> everyone to go "oh this interface supports that, I don't know if I
> need it but let's implement fallback logic which I won't and have no
> means of testing".  

Sure, that's easy:



> Are we talking about some limited number of device drivers here?  

I don't have a list, but yeah there are certain drivers that folks care about.

> Also, is the quota still necessary for machines in production today?

As far as I know yes. The number of MSIs is growing on modern systems, but so
is the number of cpus and devices.

cheers
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Tejun Heo Oct. 2, 2013, 3:23 a.m. UTC | #1

On Wed, Oct 02, 2013 at 12:33:38PM +1000, Michael Ellerman wrote:
> > It is an interface which forces the driver writers to write
> > complicated fallback code which won't usually be excercised.  
> 
> It does not force anyone to do anything. That's just bull.

Yeah, sure, we don't have shitty code in drivers which don't need any
of that retry logic, right?  What the hell is up with the gratuituous
escalation?  You really wanna go that way?

> Code which is unwilling or unable to cope with the extra complexity
> can simply do:
> 
> if (pci_enable_msix(..))
> 	goto fail;
> 
> It's as simple as that.

You apparently have no clue how people behave.  You give a function
which indicates complex failure mode, driver writers *will* try to
handle that whether they actually understand the implication or not.
That's a natural and correct behavior too because any half-competent
software eng would design API so that it receives and returns
information which is relevant to its users.  If there are special
cases to handle, make the damn interface for it special too so that it
doesn't confuse the common case.

Driver codes already have generally lower quality than core code, if
for nothing else, due to the sheer volume, and there are many driver
writers who aren't too privvy with various kernel subsystems.  They
usually just copy whatever other similar driver is doing, and this one
is a lot worse - this thing affects hardware directly.  If you expect
all the shitty implementations of ahci to handle the different
variations of multiple MSI config cases, you just don't have any
experience dealing with cheap commodity hardware.

Driver APIs should be intuitive, clear in its intentions, and don't
tempt fate with hairy configs for vast majority of cases.

> +int pci_enable_msix_or_fail(struct pci_dev *dev, struct msix_entry *entries,
> +                           int nvec)
> +{
> +       int rc;
> +
> +       rc = pci_enable_msix(dev, entries, nvec);
> +       if (rc > 0)
> +               rc = -ENOSPC;
> +
> +       return rc;
> +}

Make the *default* case simple and give clearly special interface for
the special cases.  What's so hard about that?

> > Are we talking about some limited number of device drivers here?  
> 
> I don't have a list, but yeah there are certain drivers that folks care about.

And here's another problem with the current interface.  Because the
default interface is the unnecessrily complicated one, now we can't
tell which ones actually need the complicated treatment.

> > Also, is the quota still necessary for machines in production today?
> 
> As far as I know yes. The number of MSIs is growing on modern systems, but so
> is the number of cpus and devices.

That's a bummer, but let's please make the default interface simple.
I really don't wanna see partial allocations for ahci.

Patch

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index d5f90d6..48d0252 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -988,6 +988,18 @@  int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
 }
 EXPORT_SYMBOL(pci_enable_msix);
 
+int pci_enable_msix_or_fail(struct pci_dev *dev, struct msix_entry *entries,
+                           int nvec)
+{
+       int rc;
+
+       rc = pci_enable_msix(dev, entries, nvec);
+       if (rc > 0)
+               rc = -ENOSPC;
+
+       return rc;
+}
+
 void pci_msix_shutdown(struct pci_dev *dev)
 {
        struct msi_desc *entry;