linux-kernel - Re: [PATCH 1/1] TPM: STMicroelectronics st33 driver SPI

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:	Thu, 23 May 2013 11:05:33 +0300
From:	Andy Shevchenko <andy.shevchenko@...il.com>
To:	Mathias LEBLANC <Mathias.LEBLANC@...com>
Cc:	Kent Yoder <key@...ux.vnet.ibm.com>,
	Rajiv Andrade <mail@...jiv.net>,
	Marcel Selhorst <tpmdd@...horst.net>,
	Sirrix AG <tpmdd@...rix.com>,
	"tpmdd-devel@...ts.sourceforge.net" 
	<tpmdd-devel@...ts.sourceforge.net>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Jean-Luc BLANC <jean-luc.blanc@...com>,
	Mark Brown <broonie@...nel.org>, linux-spi@...r.kernel.org
Subject: Re: [PATCH 1/1] TPM: STMicroelectronics st33 driver SPI

On Thu, May 23, 2013 at 10:43 AM, Mathias LEBLANC
<Mathias.LEBLANC@...com> wrote:

> Thanks for your support, I will fix these code style problem.

I left below the comments I think should be addressed besides style.
Please, comment what you think about them.

> However in a first time, can we publish this SPI driver?

It's up to SPI subsystem maintainer, though I couldn't consider the
quality of the driver is enough to include to upstream. You may try to
ask Greg to go to staging if you have real demand of this.

> I think that it will be preferable to submit it and apply some patch if it's only coding style error.

I don't support the way of submitting patch on top of submiting
something that must be just fixed.

> I have fix errors in this patch that has been discovered in the I2C patch, so I don't know what's stop this submission.
> I think that's driver is more criticized than the I2C driver although it's the same base.

I hope you understand it's not an exuce.

> I know that's important to have good code in the kernel source and I'm agree about that,

Good.

> I propose it be published as a first release and I fix coding style problem in a second time.

See above.

>> +++ b/drivers/char/tpm/tpm_spi_stm_st33.c

>> +enum stm33zp24_int_flags {
>> +       TPM_GLOBAL_INT_ENABLE = 0x80,
>> +       TPM_INTF_CMD_READY_INT = 0x080,
>
> What the difference? It looks like first constant is not belong to this enum.
>
>> +static int spi_write8_reg(struct tpm_chip *tpm, u8 tpm_register,
>> +                     u8 *tpm_data, u16 tpm_size) {
>> +       u8 data = 0;
>> +       int total_length = 0, nbr_dummy_bytes;
>> +       int value = 0;
>> +       struct spi_device *dev =
>> +                (struct spi_device __force *)tpm->vendor.iobase;
>> +       struct st33zp24_platform_data *platform_data = dev->dev.platform_data;
>> +       u8 *data_buffer = platform_data->tpm_spi_buffer[0];
>
> It seems a bad idea to have buffers in platform_data. I bet the buffers should be part of other struct. What did I miss?
>
>> +       struct spi_transfer xfer = {
>> +               .tx_buf  = data_buffer,
>> +               .rx_buf  = platform_data->tpm_spi_buffer[1],
>> +       };
>
> ... even this entire structure.
> Can you consider to use spi_message API ?

>> +static unsigned long wait_for_serirq_timeout(struct tpm_chip *chip,
>> +        bool condition, unsigned long timeout) {
>> +       long status = 0;
>> +       struct spi_device *client;
>> +       struct st33zp24_platform_data *pin_infos;
>> +
>> +       client = (struct spi_device __force *)chip->vendor.iobase;
>
> Is there any better storage for this pointer? It seems an abuse of iobase member.

>> +       chip->vendor.iobase = (void __iomem *)dev;
>
> Don't like this one. Try to find better way to drag pointer.


--
With Best Regards,
Andy Shevchenko
--
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/