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] [day] [month] [year] [list]

Date:	Wed, 28 Mar 2007 15:39:11 +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 23:20 -0600, Eric W. Biederman wrote:
> Michael Ellerman <michael@...erman.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

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