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

[Eric W. Biederman]

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.

> 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.

Eric
-
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/