linux-kernel - Re: [tpmdd-devel] [PATCH 1/1] TPM: STMicroelectronics ST33 I2C KERNEL 3.x.x

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: <201212052214.48840.PeterHuewe@gmx.de>
Date:	Wed, 5 Dec 2012 22:14:48 +0100
From:	Peter Hüwe <PeterHuewe@....de>
To:	Kent Yoder <key@...ux.vnet.ibm.com>
Cc:	Mathias LEBLANC <Mathias.LEBLANC@...com>,
	"Jean-Luc BLANC" <jean-luc.blanc@...com>,
	"Sirrix@...per.es" <Sirrix@...per.es>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Rajiv Andrade <mail@...jiv.net>,
	"tpmdd-devel@...ts.sourceforge.net" 
	<tpmdd-devel@...ts.sourceforge.net>,
	Kent Yoder <shpedoikal@...il.com>
Subject: Re: [tpmdd-devel] [PATCH 1/1] TPM: STMicroelectronics ST33 I2C KERNEL 3.x.x

Hi Kent, Matthias,

Am Mittwoch, 5. Dezember 2012, 19:07:20 schrieb Kent Yoder:
>   Heh, duh, well of course it is. I've now staged everything I'm
> planning on pushing at:
> 
> git://github.com/shpedoikal/linux.git tpmdd-12-05-12
> 
> Please test and let me know if I missed anything.

I just checked out your commit from github.

Here's a part of the review:

Smatch still complains a bit, sparse is fine.:

 make -C /data/data-old/linux-2.6/ M=`pwd` C=1 CHECK=smatch modules
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
  Building modules, stage 2.
  MODPOST 6 modules
  LD [M]  /data/data-old/linux-2.6/drivers/char/tpm/tpm_stm_st33_i2c.ko
make: Leaving directory `/data/data-old/linux-2.6'

Please check.

(maybe also fix the checkpatch -strict stuff as well? Would be nice ;)

drivers/char/tpm/tpm_stm_st33_i2c.h 
- Does the driver really need a seperate headerfile in drivers/char/tpm/ ? for me it seems everything can be included into the c-file.

> struct st_tpm_hash {
>	int size;
>	u8 *data;
> };
is unused - please remove.

> #define MINOR_NUM_I2C		224
Please remove, it's unused and if you really need it use the one from tpm.h

include/linux/i2c/tpm_stm_st33_i2c.h
I'm not sure if this is needed publicly? Or does only your driver need this?

>struct st33zp24_platform_data {
Telling from the name I have no idea what this device is.

drivers/char/tpm/tpm_stm_st33_i2c.c

>enum stm33zp24_int_flags {
>	TPM_GLOBAL_INT_ENABLE = 0x80,
>	TPM_INTF_CMD_READY_INT = 0x080,
>	TPM_INTF_FIFO_AVALAIBLE_INT = 0x040,
>	TPM_INTF_WAKE_UP_READY_INT = 0x020,
>	TPM_INTF_LOCALITY_CHANGE_INT = 0x004,
>	TPM_INTF_STS_VALID_INT = 0x002,
>	TPM_INTF_DATA_AVAIL_INT = 0x001,
>};

Why the leading zeros? please remove.

> static int tpm_st33_i2c_pm_suspend(struct i2c_client *client, pm_message_t mesg)
>...
> static int tpm_st33_i2c_pm_resume(struct i2c_client *client)
>,,,
>static struct i2c_driver tpm_st33_i2c_driver = {
>	.driver = {
>		   .owner = THIS_MODULE,
>		   .name = TPM_ST33_I2C,
>		   },
>	.probe = tpm_st33_i2c_probe,
>	.remove = tpm_st33_i2c_remove,
>	.resume = tpm_st33_i2c_pm_resume,
>	.suspend = tpm_st33_i2c_pm_suspend,
>	.id_table = tpm_st33_i2c_id
>};

Please convert resume/suspend  to .driver.pm 

It's pretty easy.
See this post  for details
http://sourceforge.net/mailarchive/message.php?msg_id=29516784
Rafael did spent quite a lot of effort to convert almost every driver back then, so we should 'fix' new ones.

> /*
>  * tpm_st33_i2c_init initialize driver
>  * @return: 0 if successful, else non zero value.
>  */
> static int __init tpm_st33_i2c_init(void)
> {
> 	return i2c_add_driver(&tpm_st33_i2c_driver);
> }
> 
> /*
>  * tpm_st33_i2c_exit The kernel calls this function during unloading the
>  * module or during shut down process
>  */
> static void __exit tpm_st33_i2c_exit(void)
> {
> 	i2c_del_driver(&tpm_st33_i2c_driver);
> }
> 
> module_init(tpm_st33_i2c_init);
> module_exit(tpm_st33_i2c_exit);

Hooray for oneliners ;)
+ module_i2c_driver(tpm_st33_i2c_driver);

Keep on hacking ;)

Thanks,
PeterH

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