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

[Tejun Heo]

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.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/