[flashrom] [PATCH] Prevent submission of empty read requests in linux_spi

[Stefan Tauner]

On Sat,  3 Mar 2012 16:28:47 +0100
Michael Karcher <flashrom at mkarcher.dialup.fu-berlin.de> wrote:

> The submission of zero-sized read requests in a write-only transaction
> fails at least for omap2_mcspi drivers and is pointless in general.
> 
> Even with this patch, zero-sized write requests might be submitted for
> read-only transactions, but for most SPI chips, there always is a command
> transferred (written) before reading data, so there is no such thing as
> a pure-read transaction.

maybe we should nevertheless guard against that with an
if (readcnt != 0 && writecnt == 0)
	return SPI_INVALID_LENGTH;
or so?

> 
> Signed-off-by: Michael Karcher <flashrom at mkarcher.dialup.fu-berlin.de>
> ---
>  linux_spi.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/linux_spi.c b/linux_spi.c
> index d994389..3d1ca3e 100644
> --- a/linux_spi.c
> +++ b/linux_spi.c
> @@ -114,6 +114,7 @@ static int linux_spi_send_command(struct flashctx *flash, unsigned int writecnt,
>  				  const unsigned char *txbuf,
>  				  unsigned char *rxbuf)
>  {
> +	unsigned iocontrol_code;

hm... afaics ioctl commands are always exactly 32b wide. naive kernel
newbie question: why not use uint32_t?
if there is a reason against that, please add "int" at least to match
the rest of our code base.

>  	struct spi_ioc_transfer msg[2] = {
>  		{
>  			.tx_buf = (uint64_t)(ptrdiff_t)txbuf,
> @@ -128,7 +129,14 @@ static int linux_spi_send_command(struct flashctx *flash, unsigned int writecnt,
>  	if (fd == -1)
>  		return -1;
>  
> -	if (ioctl(fd, SPI_IOC_MESSAGE(2), msg) == -1) {
> +	/* Just submit the first (write) request in case there is nothing
> +	   to read. Otherwise submit both requests */

missing full stop, if one likes to nitpick... :) (in the subject too
btw).

> +	if (readcnt == 0)
> +		iocontrol_code = SPI_IOC_MESSAGE(1);
> +	else
> +		iocontrol_code = SPI_IOC_MESSAGE(2);
> +
> +	if (ioctl(fd, iocontrol_code, msg) == -1) {
>  		msg_cerr("%s: ioctl: %s\n", __func__, strerror(errno));
>  		return -1;
>  	}

if (ioctl(fd, SPI_IOC_MESSAGE((readcnt == 0) ? 1 : 2), msg) == -1)
would work too i guess (it compiles on my machine at least), but it has
disadvantages too.

one could get rid of the first problem (correct type of IOC) by
assigning 1 or 2 instead of the expanded SPI_IOC_MESSAGE(x) value to
the local variable and use the macro in the argument list with the
variable as parameter.

i have no preference for any of those solutions... the posted one is
fine too.

regarding correctness i think it is ok too, although i have to admit
that my review is probably not as thorough as carldani's would have
been. i have looked at most of the macros involved; i have looked at
spidev_ioctl and spidev_message and everything makes sense...
since it's also tested on hardware i think it is good to go so it is...
Acked-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner