linux-kernel - Re: [PATCH 4/8]drivers:tmp.c Fix warning: variable 'rc' set but not used

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: <4C173216.3040007@s5r6.in-berlin.de>
Date:	Tue, 15 Jun 2010 09:56:06 +0200
From:	Stefan Richter <stefanr@...6.in-berlin.de>
To:	"Justin P. Mattock" <justinmattock@...il.com>
CC:	Valdis.Kletnieks@...edu, linux-kernel@...r.kernel.org,
	Peter Stuge <peter@...ge.se>,
	tpmdd-devel@...ts.sourceforge.net,
	Debora Velarde <debora@...ux.vnet.ibm.com>,
	Rajiv Andrade <srajiv@...ux.vnet.ibm.com>,
	Marcel Selhorst <m.selhorst@...rix.com>
Subject: Re: [PATCH 4/8]drivers:tmp.c Fix warning: variable 'rc' set but	not
 used

(adding Cc: tpm maintainers, after I had cut the Cc list earlier...)

Justin P. Mattock wrote:
> On 06/14/2010 11:57 PM, Stefan Richter wrote:
>> Justin P. Mattock wrote:
>>> On 06/14/2010 10:29 PM, Peter Stuge wrote:
>>>> Justin P. Mattock wrote:
>>>>> I have no manual in front of me. Did a quick google, but came up with
>>>>> (no hits) info on what that function does. grep showed too many
>>>>> entries
>>>>> to really see why/what this is.
>>>>
>>>> Check out the tool cscope. (Or kscope, if you prefer a GUI.)
>>>
>>> thanks for this tool.. I think this is what I need.. running around not
>>> knowing what/where the manual is for a call is a bit daunting.
>>
>> Similar to cscope/kscope but online:
>> http://lxr.free-electrons.com/
>> http://lxr.linux.no/linux
>>
>> E.g. http://lxr.free-electrons.com/ident?i=transmit_cmd
> 
> hmm.. yeah I use those guys a lot.. with grep also.. I think what might
> throw me off is things like def.orig = def.new then def.orig gets lost
> in the mix(or it's there, but the grep becomes gigantic in code
> results.) main thing I look for is googling "somedefinition linux man"
> for some idea of what that function does.
> 
> Justin P. Mattock

I'm afraid that even if there was inline documentation of the function,
it would still be near impossible to guess what to do with unused return
codes if one is not familiar with the hardware and with the driver.

To quote your original patch again:

> Im getting this warning when compiling:
>  CC      drivers/char/tpm/tpm.o
> drivers/char/tpm/tpm.c: In function 'tpm_gen_interrupt':
> drivers/char/tpm/tpm.c:508:10: warning: variable 'rc' set but not used
> 
> The below patch gets rid of the warning,
> but I'm not sure if it's the best solution.
> 
>  Signed-off-by: Justin P. Mattock <justinmattock@...il.com>
> 
> ---
>  drivers/char/tpm/tpm.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c
> index 05ad4a1..3d685dc 100644
> --- a/drivers/char/tpm/tpm.c
> +++ b/drivers/char/tpm/tpm.c
> @@ -514,6 +514,8 @@ void tpm_gen_interrupt(struct tpm_chip *chip)
>  
>  	rc = transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
>  			"attempting to determine the timeouts");
> +	if (!rc)
> +		rc = 0;
>  }
>  EXPORT_SYMBOL_GPL(tpm_gen_interrupt);

So, void tpm_gen_interrupt() itself clearly is not prepared to do
anything about an error return from transmit_cmd.  The question is, is
this OK and can the "rc = " simply be removed, or should
tpm_gen_interrupt gain some error handling, or should tpm_gen_interrupt
be changed to a non-void function and its callers check for errors in
tpm_gen_interrupt?

It could very well be that there is really no need or no possibility for
error handling.  But until somebody familiar with the matter has figured
this out, it is in fact better to leave the compiler warning in place.

Just silencing the compiler if the possibility exists that there is an
actual flaw here is not the way to go.  Still, good that you reported it
even with an RFC patch, since the authors were obviously unaware of such
a warning until now.
-- 
Stefan Richter
-=====-==-=- -==- -====
http://arcgraph.de/sr/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/