| Message ID | 20131002023337.GB22748@concordia |
|---|---|
| State | Superseded |
| Headers | show |
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.
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;