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

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

[Eric W. Biederman]

Michael Ellerman <michael@ellerman.id.au> writes:

> pci_enable_msi() and pci_enable_msix() both search for the MSI/MSI-X
> capability, we can fold this into pci_msi_supported() by passing the
> type in.
>
> Update the code to match the comment for pci_msi_supported(). That is
> it returns 0 on success, and anything else indicates an error.

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.

Eric


>
> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> ---
>
>  drivers/pci/msi.c |   19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
>
> Index: msi-new/drivers/pci/msi.c
> ===================================================================
> --- msi-new.orig/drivers/pci/msi.c
> +++ msi-new/drivers/pci/msi.c
> @@ -433,12 +433,13 @@ static int msix_capability_init(struct p
>  /**
>   * pci_msi_supported - check whether MSI may be enabled on device
>   * @dev: pointer to the pci_dev data structure of MSI device function
> + * @type: are we checking for MSI or MSI-X ?
>   *
>   * Look at global flags, the device itself, and its parent busses
>   * to return 0 if MSI are supported for the device.
>   **/
>  static
> -int pci_msi_supported(struct pci_dev * dev)
> +int pci_msi_supported(struct pci_dev * dev, int type)
>  {
>  	struct pci_bus *bus;
>  
> @@ -456,6 +457,9 @@ int pci_msi_supported(struct pci_dev * d
>  		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
>  			return -EINVAL;
>  
> +	if (!pci_find_capability(dev, type))
> +		return -EINVAL;
> +
>  	return 0;
>  }
>  
> @@ -471,13 +475,9 @@ int pci_msi_supported(struct pci_dev * d
>   **/
>  int pci_enable_msi(struct pci_dev* dev)
>  {
> -	int pos, status;
> -
> -	if (pci_msi_supported(dev) < 0)
> -		return -EINVAL;
> +	int status;
>  
> -	pos = pci_find_capability(dev, PCI_CAP_ID_MSI);
> -	if (!pos)
> +	if (pci_msi_supported(dev, PCI_CAP_ID_MSI))
>  		return -EINVAL;
>  
>  	WARN_ON(!!dev->msi_enabled);
> @@ -580,13 +580,10 @@ int pci_enable_msix(struct pci_dev* dev,
>  	int i, j;
>  	u16 control;
>  
> -	if (!entries || pci_msi_supported(dev) < 0)
> +	if (!entries || pci_msi_supported(dev, PCI_CAP_ID_MSIX))
>   		return -EINVAL;
>  
>  	pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
> -	if (!pos)
> - 		return -EINVAL;
> -
>  	pci_read_config_word(dev, msi_control_reg(pos), &control);
>  	nr_entries = multi_msix_capable(control);
>  	if (nvec > nr_entries)

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

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2181 bytes --]

On Tue, 2007-03-27 at 22:45 -0600, Eric W. Biederman wrote:
> Michael Ellerman <michael@ellerman.id.au> writes:
> 
> > pci_enable_msi() and pci_enable_msix() both search for the MSI/MSI-X
> > capability, we can fold this into pci_msi_supported() by passing the
> > type in.
> >
> > Update the code to match the comment for pci_msi_supported(). That is
> > it returns 0 on success, and anything else indicates an error.
> 
> 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.

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.

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

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

-- 
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 #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Eric W. Biederman]

Michael Ellerman <michael@ellerman.id.au> 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

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

[Eric W. Biederman]

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

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

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2866 bytes --]

On Tue, 2007-03-27 at 23:20 -0600, Eric W. Biederman wrote:
> Michael Ellerman <michael@ellerman.id.au> 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 #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]