Re: [PATCH 9/21] MSI: Expand pci_msi_supported()

[Michael Ellerman]

On Tue, 2007-03-27 at 23:20 -0600, Eric W. Biederman wrote:
> Michael Ellerman <michael@xxxxxxxxxxxxxx> writes:
> 
> > I don't think it's that confusing. I agree it was a bit weird that
> > previously it was explicitly checking for < 0, so I fixed that.
> 
> The previous case was clearer.  This isn't a please do some work
> for me function, where we expect occasional failure and we need
> to return an error code when there are different types.
> 
> There is only one time of failure here, that we don't support MSI
> on this device.
> 
> >> After this patch we are simply checking to see if there was a
> >> return value at all.
> >> 
> >> Can we please change the return value here so it is actually boolean.
> >> 1 for supported 0 for not supported.
> >> 
> >> There aren't any useful return values anyway so this would just make
> >> the code easier to read and maintain.
> >
> > There aren't any useful return values as it's currently written, but
> > there code be. And I'd like to keep that possibility.
> >
> > My next patch allows the arch routine to propagate its return value out
> > to the caller, which is useful.
> >
> > And I don't think making it return 0/1 makes it any clearer. As it is
> > now it's just:
> >
> > If MSI is supported we return 0.
> > If MSI is not supported we return some error code which is != 0.
> >
> > The caller just does:
> >
> > if (pci_msi_supported(blah ..))
> > 	error;
> 
> Exactly.  Which just reading through is non-obvious.
> 
> if (supported())
>    fail();
> 
> Where if we said
> if (!supported())
>    fail();
> 
> The code would be clearer.

Yeah OK I see what you mean. The name of the function is slightly
counter intutive given it's return semantics. I can't really think of a
better one though, perhaps msi_check_device() ?

> > Which is exactly the same whether it's 0/1 or 0/<error code>. And we
> > have the option of returning a useful return value.
> 
> But the callers all ignore so it still isn't useful, and I'm not 
> at all certain it makes any sense for it to be useful.

See my next patch, which makes the callers pass it back to the drivers,
(nearly) all of which print it to the console.

I think having a meaningful return value is very useful there, having
just spent a while debugging the powerpc backends. I actually have a
debug patch which changes pci_msi_supported() to return -1/-2/-3 etc for
the different cases.

And with my next patch that hooks the arch up, it's even more useful.
Otherwise you're left with no option other than printk debugging.

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