Re: [tpmdd-devel] [PATCH 1/1] TPM: STMicroelectronics st33 driver SPI

[Kent Yoder]

On Sun, Apr 28, 2013 at 03:16:33AM +0200, Peter Hüwe wrote:
> Hi Matthias,
> 
> it's nice to see that you consider most of the comments, unfortunately I still 
> have some left ;)
> 
> > +/*
> > + * tpm_st33_spi_init initialize driver
> > + * @return: 0 if successful, else non zero value.
> > + */
> > +static int __init tpm_st33_spi_init(void)
> > +{
> > +	return spi_register_driver(&tpm_st33_spi_driver);
> > +}
> > +
> > +/*
> > + * tpm_st33_spi_exit The kernel calls this function during unloading the
> > + * module or during shut down process
> > + */
> > +static void __exit tpm_st33_spi_exit(void)
> > +{
> > +	spi_unregister_driver(&tpm_st33_spi_driver);
> > +}
> > +
> > +module_init(tpm_st33_spi_init);
> > +module_exit(tpm_st33_spi_exit);
> 
> Please use module_spi_driver(&tpm_st33_spi_driver); instead
> Boilerplate code sucks.
> 
> 
> > +	u8 *data_buffer = platform_data->tpm_spi_buffer[0];
> > +	struct spi_transfer xfer = {
> > +		.tx_buf	 = data_buffer,
> > +		.rx_buf	 = platform_data->tpm_spi_buffer[1],
> > +	};
> > +	struct spi_message msg;
> >...
> > +	xfer.len = total_length;
> > +	spi_message_init(&msg);
> > +	spi_message_add_tail(&xfer, &msg);
> > +	value = spi_sync(dev, &msg);
> 
> 
> Doesn't spi_write fit here ?
> 
> static inline int
> spi_write(struct spi_device *spi, const void *buf, size_t len)
> {
> 	struct spi_transfer	t = {
> 			.tx_buf		= buf,
> 			.len		= len,
> 		};
> 	struct spi_message	m;
> 
> 	spi_message_init(&m);
> 	spi_message_add_tail(&t, &m);
> 	return spi_sync(spi, &m);
> }
> 
> Seems pretty identical.
> -> This would save some lines of code,
> and again - boilerplate code sucks.
> Same applies to spi_read,
> 
> 
> 
> The driver generates some gcc warnings:
>  drivers/char/tpm/tpm_spi_stm_st33.c: In function 'read8_reg': 
>  drivers/char/tpm/tpm_spi_stm_st33.c:180:5: warning: unused variable 'latency'  
> [-Wunused-variable]
>  drivers/char/tpm/tpm_spi_stm_st33.c: In function 'tpm_stm_spi_status':
>  drivers/char/tpm/tpm_spi_stm_st33.c:353:2: warning: 'data' is used  
> uninitialized in this function [-Wuninitialized]
>  drivers/char/tpm/tpm_spi_stm_st33.c: In function 'get_burstcount':
>  drivers/char/tpm/tpm_spi_stm_st33.c:450:12: warning: 'temp' is used  
> uninitialized in this function [-Wuninitialized]
>  drivers/char/tpm/tpm_spi_stm_st33.c: In function 'check_locality':
>  drivers/char/tpm/tpm_spi_stm_st33.c:369:22: warning: 'data' may be used  
> uninitialized in this function [-Wuninitialized]

 Thanks for pointing this out Peter -- it made me realize my bash
aliases for checking were not working. :-)

Please take a look at these too:

drivers/char/tpm/tpm_spi_stm_st33.c:446 get_burstcount() warn: unsigned
'status' is never less than zero.
drivers/char/tpm/tpm_spi_stm_st33.c:452 get_burstcount() warn: unsigned
'status' is never less than zero.
drivers/char/tpm/tpm_spi_stm_st33.c:523 recv_data() warn: unsigned
'status' is never less than zero.
drivers/char/tpm/tpm_spi_stm_st33.c:572 tpm_stm_spi_send() warn:
unsigned 'ret' is never less than zero.
drivers/char/tpm/tpm_spi_stm_st33.c:590 tpm_stm_spi_send() warn:
unsigned 'ret' is never less than zero.
drivers/char/tpm/tpm_spi_stm_st33.c:613 tpm_stm_spi_send() warn:
unsigned 'ret' is never less than zero.
drivers/char/tpm/tpm_spi_stm_st33.c:836 tpm_st33_spi_probe() warn:
unsigned 'err' is never less than zero.
drivers/char/tpm/tpm_spi_stm_st33.c:842 tpm_st33_spi_probe() warn:
unsigned 'err' is never less than zero.
drivers/char/tpm/tpm_spi_stm_st33.c:854 tpm_st33_spi_probe() warn:
unsigned 'err' is never less than zero.
drivers/char/tpm/tpm_spi_stm_st33.c:859 tpm_st33_spi_probe() warn:
unsigned 'err' is never less than zero.
drivers/char/tpm/tpm_spi_stm_st33.c:863 tpm_st33_spi_probe() warn:
unsigned 'err' is never less than zero.

Kent

> 
> 
> The driver does give me some smatch errors:
> 
> $ LANG=C make -j16 C=1 CHECK=smatch
>  CHECK   drivers/char/tpm/tpm_spi_stm_st33.c
>  drivers/char/tpm/tpm_spi_stm_st33.c:882 tpm_st33_spi_probe() warn: variable  
> dereferenced before check 'platform_data' (see line 781)
>  drivers/char/tpm/tpm_spi_stm_st33.c:980 tpm_st33_spi_pm_resume() warn: should  
> this be a bitwise op?
>  drivers/char/tpm/tpm_spi_stm_st33.c:980 tpm_st33_spi_pm_resume() warn: should  
> this be a bitwise op?
> 
> 
> 
> and some sparse errors:
>  drivers/char/tpm/tpm_spi_stm_st33.c:102:35: warning: cast removes address 
> space of expression
>  drivers/char/tpm/tpm_spi_stm_st33.c:171:35: warning: cast removes address 
> space of expression
>  drivers/char/tpm/tpm_spi_stm_st33.c:299:19: warning: cast removes address 
> space of expression
>  drivers/char/tpm/tpm_spi_stm_st33.c:311:15: warning: symbol 
> 'wait_for_serirq_timeout' was not declared. Should it be static?
>  drivers/char/tpm/tpm_spi_stm_st33.c:546:19: warning: cast removes address 
> space of expression
> 
> 
> 
> 
> 
> And here are some other, rather subjective remarks:
> 
> > +		for (; nbr_dummy_bytes < total_length &&
> > +			 ((u8 *)xfer.rx_buf)[nbr_dummy_bytes] == 0;
> > +			 nbr_dummy_bytes++)
> > +			;
> 
> Not sure if it would be easier to read using a while and putting the increment 
> into the loop body
> 
> >+		while (nbr_dummy_bytes < total_length && 
> >+			((u8 *)xfer.rx_buf)[nbr_dummy_bytes] == 0)
> >+			 nbr_dummy_bytes++;
> 
> I usually don't like empty loop bodies, as they tend to be overlooked.
> 
> 
> 
> > +static u32 SPI_WRITE_DATA(struct tpm_chip *tpm, u8 tpm_register,
> > +			  u8 *tpm_data, u16 tpm_size)
> > +{
> > +	u8 value = 0;
> > +	value = write8_reg(tpm, tpm_register, tpm_data, tpm_size);
> > +
> > +	switch (value) {
> > +	case TPM_ST_SPI_OK:
> > +		return TPM_ST_SPI_OK;
> > +	case 0:
> > +		return 0;
> > +	default:
> > +		return -EBUSY;
> > +	}
> > +} /* SPI_WRITE_DATA() */
> 
> Why do you need a wrapper function here for the return code?
> write8_reg is only called from this location, so why doesn't it return the 
> correct error code directly?
> *confused*
> 
> 
> Same applies to read8_reg.
> 
> 
> 
> 
> > +static int _wait_for_interrupt_serirq_timeout(struct tpm_chip *chip,
> > +					unsigned long timeout)
> Is only called from wait_for_serirq_timeout - I'm not really sure if it helps 
> readability to have a separate function.
> 
> 
> 
> 
> > +	.resume = tpm_st33_spi_pm_resume,
> > +	.suspend = tpm_st33_spi_pm_suspend,
> Maybe use 
> SIMPLE_DEV_PM_OPS 
> instead as it is (afaik) the new standard way for PM_OPS.
> The conversion should be pretty similar to the one in your I2C TPM driver:
> https://github.com/shpedoikal/linux/commit/d459335381eca1cb91fefb87021d3d172342e55a
> 
> 
> Regards,
> 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/