RE: [tpmdd-devel] [PATCH 1/1] TPM: STMicroelectronics ST33 I2CKERNEL 3.x.x

[Mathias LEBLANC]

Hi Peter,

Thanks for your contribution.
I have modified the driver files name and descriptions.
Regarding the warnings, it's strange. 

@Kent, could you confirm that you have the same?

Regards,

Mathias Leblanc

-----Original Message-----
From: Peter Hüwe [mailto:PeterHuewe@xxxxxx] 
Sent: 29 November, 2012 01:05
To: Mathias LEBLANC
Cc: Kent Yoder; Kent Yoder; Jean-Luc BLANC; linux-kernel@xxxxxxxxxxxxxxx; Rajiv Andrade; tpmdd-devel@xxxxxxxxxxxxxxxxxxxxx; Sirrix@xxxxxxxxx
Subject: Re: [tpmdd-devel] [PATCH 1/1] TPM: STMicroelectronics ST33 I2C KERNEL 3.x.x

Hi Mathias,

please note: 
I'm writing this email on behalf of myself only and nobody else, especially not my employer - and I'm doing this in my spare time.
I'm working for a direct competitor of yours, but I'm not using any knowledge that I've picked up at work or that is considered secret in any way.
I have a personal interest in the TPM subsystem and want to keep it as clean as possible.
So please don't see my review as something negative, but rather something positive.


Am Mittwoch, 28. November 2012, 18:48:57 schrieb Mathias LEBLANC:
> Ok, so i have patch the ST33 I2C driver on this branch and correct 
> some errors. I send you the patch for the kernel 3.x I have no error 
> on compilation, tell me if you have problems.
> I have implemented the tpm_do_selftest function to get the tpm ready, 
> but it can be removed ________________________________________

Unfortunately you attached the patch instead of sending it in plaintext which is the usual practice -> care to resend in plain text? 
Makes the review far easier.

(btw.: Please also have a look at
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/SubmittingPatches;hb=HEAD
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/SubmitChecklist;hb=HEAD
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/SubmittingDrivers;hb=HEAD
which describes the process in detail)

When you resend the patch, can you please include the "metadata" as well - i.e. your modifications to the Kconfig / Makefile etc.
I do not see a reason why to keep it in a seperate patch.




I tried the patch you've posted and it applies cleanly and now (finally) compiles as well - so now I can start with my review:

= The name =
There's already one i2c tpm driver in the tree, so maybe it would be a good idea to keep the naming scheme consistent? 
-> How about tpm_i2c_stm_st33.c ?
Eventually this is something Kent as a maintainer has to decice - but I would really like to see the name change.
I hope we can eventually consolidate all the 'tis' based drivers.


= Compiling / License =
When compiling the driver I get the following warning
   WARNING: modpost: missing MODULE_LICENSE() in /data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.o
Please include the appropriate MODULE_LICENSE as my kernel otherwise gets tainted by your driver.

Also this:
+ * STMicroelectronics TPM I2C Linux driver for TPM ST33ZP24
+ * Copyright (C) 2009, 2010  STMicroelectronics
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.

is not possible afaik - kernel code must be under GPL v2 _only_ without the "or (at your option) any later version." addition.



= sparse warnings =
When running sparse against your code I get the following warnings:
 make -C /data/data-old/linux-2.6/ M=`pwd`  modules C=1
make: Entering directory `/data/data-old/linux-2.6'
  CHECK   /data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:167:19: warning: cast removes address space of expression
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:187:19: warning: cast removes address space of expression
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:180:5: warning: symbol 'wait_for_serirq_timeout' was not declared. Should it be static?
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:210:19: warning: cast removes address space of expression
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:227:19: warning: cast removes address space of expression
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:245:19: warning: cast removes address space of expression
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:269:19: warning: cast removes address space of expression
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:307:19: warning: cast removes address space of expression
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:324:38: warning: cast removes address space of expression
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:394:19: warning: cast removes address space of expression
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:424:19: warning: cast removes address space of expression
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:456:19: warning: cast removes address space of expression
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:531:19: warning: cast removes address space of expression
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:672:29: warning: incorrect type in assignment (different address spaces)
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:672:29:    expected void [noderef] <asn:2>*iobase
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:672:29:    got struct i2c_client *client
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:781:19: warning: cast removes address space of expression
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:818:19: warning: cast removes address space of expression
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:841:19: warning: cast removes address space of expression
  CC [M]  /data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.o
  Building modules, stage 2.
  MODPOST 8 modules


Please fix these if applicable - otherwise you'll probably get a friendly reminder to do so by fengguang's build test ;)


= smatch =
Same applies to smatch
make -C /data/data-old/linux-2.6/ M=`pwd`  modules C=1 CHECK=smatch
make: Entering directory `/data/data-old/linux-2.6'
  CHECK   /data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:535 tpm_stm_i2c_recv() warn: variable dereferenced before check 'chip' (see line 531)
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:748 tpm_st33_i2c_probe() warn: variable dereferenced before check 'platform_data' (see line 659)
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:848 tpm_st33_i2c_pm_resume() warn: should this be a bitwise op?
/data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.c:848 tpm_st33_i2c_pm_resume() warn: should this be a bitwise op?
  CC [M]  /data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.o

Please fix these if applicable - otherwise you'll probably get a friendly reminder to do so by fengguang's build test ;)

= checkpatch =
Also please run .../scripts/checkpatch.pl -strict before submission - not everything that is reported might be applicable, but quite often it is.



Looking forward to your v2 so I can give a more detailed code review of your code.


Thanks,
Peter


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