Ok.  Looking at this one I don't see a bug exactly but there
is one very confusing piece.

Currently we have a function pci_msi_supported that sounds like it
sounds like it should return a boolean value.

However instead it returns 0 for success and -EINVAL for failure.

Reading through the code before this patch it is clear it does this
weird thing because we check for < 0.

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.

-

[ message continues ]

I don't think it's that confusing. I agree it was a bit weird that

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 !=3D 0.

The caller just does:

if (pci_msi_supported(blah ..))
	error;

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.

cheers

--=20
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
[ message continues ]

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

Exactly.  Which just reading through is non-obvious.

if (supported())
   fail();

Where if we said
if (!supported())
   fail();


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.

Eric
-

[ message continues ]

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

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

--=20
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
[ message continues ]

On another thought if we want to keep the return code we should
probably rename it pci_msi_verify().  Or something like that
so it is clear we are not asking if it is supported a question
with a boolean answer.  But merely verifying that it is supported.

At which point if the verification function fails it would
be expected to return an error code, and we could then be
expected to propagate when we receive it.

Eric
-

[ message continues ]