linux-kernel - Re: [tpmdd-devel] [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] [thread-next>] [day] [month] [year] [list]

Message-Id: <201305160029.07073.PeterHuewe@gmx.de>
Date:	Thu, 16 May 2013 00:29:06 +0200
From:	Peter Hüwe <PeterHuewe@....de>
To:	tpmdd-devel@...ts.sourceforge.net
Cc:	Matthias Leblanc <mathias.leblanc@...com>,
	Kent Yoder <key@...ux.vnet.ibm.com>,
	Rajiv Andrade <mail@...jiv.net>,
	Marcel Selhorst <tpmdd@...horst.net>,
	Sirrix AG <tpmdd@...rix.com>,
	"Jean-Luc Blanc" <jean-luc.blanc@...com>,
	linux-kernel@...r.kernel.org
Subject: Re: [tpmdd-devel] [PATCH 1/1] TPM: STMicroelectronics st33 driver SPI

Hi Matthias,

Am Mittwoch, 15. Mai 2013, 15:53:19 schrieb Matthias Leblanc:

> +static inline int spi_read_write(struct spi_device *spi,
> +	 struct spi_transfer xfer) {
> +	struct spi_message msg;
> +	spi_message_init(&msg);
> +	spi_message_add_tail(&xfer, &msg);
> +	return spi_sync(spi, &msg);
> +}

This looks extremely like the predefined spi_sync_transfer in include/linux/spi.h

/**
 * spi_sync_transfer - synchronous SPI data transfer
 * @spi: device with which data will be exchanged
 * @xfers: An array of spi_transfers
 * @num_xfers: Number of items in the xfer array
 * Context: can sleep
 *
 * Does a synchronous SPI data transfer of the given spi_transfer array.
 *
 * For more specific semantics see spi_sync().
 *
 * It returns zero on success, else a negative error code.
 */
static inline int
spi_sync_transfer(struct spi_device *spi, struct spi_transfer *xfers,
	unsigned int num_xfers)
{
	struct spi_message msg;

	spi_message_init_with_transfers(&msg, xfers, num_xfers);

	return spi_sync(spi, &msg);
}


where spi_message_init_with_transfers is defined as

static inline void
spi_message_init_with_transfers(struct spi_message *m,
struct spi_transfer *xfers, unsigned int num_xfers)
{
	unsigned int i;

	spi_message_init(m);
	for (i = 0; i < num_xfers; ++i)
		spi_message_add_tail(&xfers[i], m);
}


If you combine these functions you end up (more or less) with  your implementation

So please use spi_sync_transfer
OR as an alternative use spi_write / spi_read as an alternative as mentioned in a previous email



And here is a rather subjective remark:

> +             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.



> +module_spi_driver(tpm_st33_spi_driver);
> +
> +MODULE_AUTHOR("Christophe Ricard (tpmsupport@...com)");
> +MODULE_DESCRIPTION("STM TPM SPI ST33 Driver");
> +MODULE_VERSION("1.2.0");
> +MODULE_LICENSE("GPL");

Something is strange here - if I apply your patch, the module license gets lost - so I get a compile warning:

WARNING: modpost: missing MODULE_LICENSE() in /data/data-old/linux-2.6/drivers/char/tpm/tpm_spi_stm_st33.o

Do you edit the patch by hand before sending?

Steps to reproduce:
drivers/char/tpm $ git reset --hard tpmdd/tpmdd-04-26-13
HEAD is now at d224213 tpm_tis: missing platform_driver_unregister() on error in init_tis()
drivers/char/tpm $ git am /tmp/\[tpmdd-devel\]\ \[PATCH\ 1_1\]\ TPM_STMicroelectronics\ st33\ driver\ SPI.mbox 
Applying: TPM: STMicroelectronics st33 driver SPI

drivers/char/tpm $ tail tpm_spi_stm_st33.c                    },
        .probe = tpm_st33_spi_probe,
        .remove = tpm_st33_spi_remove,
};

module_spi_driver(tpm_st33_spi_driver);

MODULE_AUTHOR("Christophe Ricard (tpmsupport@...com)");
MODULE_DESCRIPTION("STM TPM SPI ST33 Driver");
MODULE_VERSION("1.2.0");


And the usual sparse warnings ;)
drivers/char/tpm $ make M=`pwd` -C /data/data-old/linux-2.6/ modules C=1 CHECK=sparse
make: Entering directory `/data/data-old/linux-2.6'
  CHECK   /data/data-old/linux-2.6/drivers/char/tpm/tpm_spi_stm_st33.c
/data/data-old/linux-2.6/drivers/char/tpm/tpm_spi_stm_st33.c:104:35: warning: cast removes address space of expression
/data/data-old/linux-2.6/drivers/char/tpm/tpm_spi_stm_st33.c:170:35: warning: cast removes address space of expression
/data/data-old/linux-2.6/drivers/char/tpm/tpm_spi_stm_st33.c:242:19: warning: cast removes address space of expression
/data/data-old/linux-2.6/drivers/char/tpm/tpm_spi_stm_st33.c:235:15: warning: symbol 'wait_for_serirq_timeout' was not declared. Should it be static?
/data/data-old/linux-2.6/drivers/char/tpm/tpm_spi_stm_st33.c:482:19: warning: cast removes address space of expression



Regards,
Peter
--
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/