On Tue, 2009-02-10 at 22:53 -0700, Matthew Wilcox wrote:
> On Wed, Feb 11, 2009 at 03:37:43PM +1100, Michael Ellerman wrote:
> > > Would it be fair to say that those platforms are the same ones which
> > > implement arch_msi_check_device(), which already works for this case?
> >
> > Yes and yes but ..
> >
> > We can do some checking in arch_msi_check_device() and return a
> > "could-be-allocated" value from there[1].
> >
> > But we still want to be able to return a "could-be-allocated" value from
> > arch_setup_msi_irqs(), without going through the business of setting up
> > msi_descs.
> >
> > Simply because when we do the firmware call to actually allocate the
> > MSIs, it might give us less than we ask for. At that point we can just
> > return that number.
>
> Hm, yes, OK.
>
> > We /could/ try to allocate in arch_msi_check_device(), see what we get,
> > deallocate, and return the number. But it's a) ugly as hell, and b)
> > still possible that when we come back to do the real allocation we get a
> > different number.
>
> Yes. I also considered that you could actually do the allocation in
> arch_msi_check_device() and then fill in during arch_setup_msi_irqs(),
> but you could leak if, say, ioremap_nocache() fails.
Worse, we'd leave the device with MSI(X) enabled, even though
pci_enable_msi/x() failed. Which would confuse drivers a little.
To do that we'd need an arch_msi_uncheck_device() ... :P
> So, I'm OK with this patch, given the change I suggested below is
> implemented. Now, onto more fun matters ...
Cool, I'll resend with that change.
And comment on your other stuff in another mail.
> > > > @@ -43,7 +43,8 @@ int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
> > > > list_for_each_entry(entry, &dev->msi_list, list) {
> > > > ret = arch_setup_msi_irq(dev, entry);
> > > > if (ret)
> > > > - return ret;
> > > > + /* Ensure the error code is always < 0 */
> > > > + return min(ret, -ret);
> > > > }
> > > >
> > > > return 0;
> > >
> > > I really don't like this. If we're returning an error, we should return
> > > a sensible error value. Something like:
> > >
> > > if (ret < 0)
> > > return ret;
> > > if (ret > 0)
> > > return -ENOSPC;
> >
> > Fair enough. My reasoning was that negating it at least left some chance
> > that you could work out where it came from. But I'm not fussed, and I
> > admit it's a bit ugly.
>
> I have three points which contradict each other. Bear with me ;-)
>
> First, I just noticed that this was a patch to arch_setup_msi_irqs() so
> really we should return a count of how far we got through the list.
That's what the code to calculate avail in msix_capability_init() does.
> Second, arch_setup_msi_irq() isn't permitted to return a number >0, so
> we don't really need this hunk at all.
Permitted by who?
The point of the patch is that if it does return a number > 0, that's
probably because some arch routine returned > 0 and the arch MSI code
just returned that value without checking. I looked at auditing all the
arch MSI code to make sure no one did that, but it's a lot of code and
it's prone to breaking in future.
> Third, I did push a git tree while on holiday that included eliminating
> arch_setup_msi_irqs() from this file and pushing it to all architectures.
>
> I told Jesse, but didn't send email about it (connectivity was
> intermittent and I couldn't really send email). I _intended_ to tell
> Jesse to pass it along, but clearly I didn't do that. Here's the git
> tree:
>
> http://git.kernel.org/?p=linux/kernel/git/willy/pci.git;a=shortlog;h=msi-20090126
>
> There's several lines of development in there, so it's a bit messy. Oh
> well.
Most of that (on master) looks good. I'm not sure why you're keen on
getting rid of the "irq" interface, other than simplicity in the generic
code - but it means we end up reproducing logic in each arch.
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
Attachment:
signature.asc
Description: This is a digitally signed message part
