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

[Stefan Tauner]

On Sat,  3 Mar 2012 16:28:47 +0100
Michael Karcher  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
>
> 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

[PATCH] Prevent submission of empty read requests in linux_spi

[Michael Karcher]

Am Dienstag, den 06.03.2012, 00:41 +0100 schrieb Stefan Tauner:
> > Signed-off-by: Michael Karcher
>
> Acked-by: Stefan Tauner

Thanks, this is r1513 now, after incorporating your suggestions. I
skipped the ?: idea, as using SPI_IOC_MESSAGE(1+(readcnt!=0)) failed at
run time on the system of "GNUtoo".

Regards,
  Michael Karcher