linux-kernel - Re: [PATCH 9/21] MSI: Expand pci_msi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1175058070.6386.12.camel@concordia.ozlabs.ibm.com>
Date:	Wed, 28 Mar 2007 15:01:10 +1000
From:	Michael Ellerman <michael@...erman.id.au>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	linux-pci@...ey.karlin.mff.cuni.cz,
	Greg Kroah-Hartman <greg@...ah.com>,
	"David S. Miller" <davem@...emloft.net>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	linux-kernel@...r.kernel.org, Andrew Morton <akpm@...l.org>,
	daniel.e.wolstenholme@...el.com
Subject: Re: [PATCH 9/21] MSI: Expand pci_msi_supported()

On Tue, 2007-03-27 at 22:45 -0600, Eric W. Biederman wrote:
> Michael Ellerman <michael@...erman.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

Download attachment "signature.asc" of type "application/pgp-signature" (190 bytes)