Message-ID: <20151119233924.GN64635@google.com>
Date:	Thu, 19 Nov 2015 15:39:24 -0800
From:	Brian Norris <computersforpeace@...il.com>
To:	Michal Suchanek <hramrach@...il.com>
Cc:	Hou Zhiqiang <B48286@...escale.com>, shijie.huang@...el.com,
	David Woodhouse <dwmw2@...radead.org>,
	Han Xu <han.xu@...escale.com>,
	Rafał Miłecki <zajec5@...il.com>,
	Ben Hutchings <ben@...adent.org.uk>,
	Marek Vasut <marex@...x.de>, Gabor Juhos <juhosg@...nwrt.org>,
	Bean Huo 霍斌斌, <beanhuo@...ron.com>,
	linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Huang Shijie <shijie.huang@....com>,
	Heiner Kallweit <hkallweit1@...il.com>
Subject: Re: [PATCH v4 7/7] mtd: spi-nor: add read loop

+ Heiner

On Fri, Aug 14, 2015 at 09:23:09AM -0000, Michal Suchanek wrote:
> mtdblock and ubi do not handle the situation when read returns less data
> than requested. Loop in spi-nor until buffer is filled or an error is
> returned.

I'm slightly torn on this patch. I believe this is inspired by issues in
the SPI layer. But I believe there is some agreement from the SPI layer
that protocol drivers (such as m25p80.c/spi-nor.c) should not have to
worry about spi_messages getting truncated [1]. Heiner is looking at
that.

But on the other hand, it's possible that some non-SPI driver would have
similar limitations, and so spi-nor.c should be handling the issue.

> Signed-off-by: Michal Suchanek <hramrach@...il.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index e0ae9cf..246fac7 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -738,14 +738,22 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>  	if (ret)
>  		return ret;
>  
> -	ret = nor->read(nor, from, len, buf);
> +	while (len) {
> +		ret = nor->read(nor, from, len, buf);
> +		if (ret <= 0)
> +			goto read_err;

Do we really want to exit silently when ret==0, but len!=0?

> +
> +		BUG_ON(ret > len);

Maybe the condition here should be 'ret > len || len == 0', since this
shouldn't happen.

Also, please don't use BUG_ON(). Even though this is really unexpected,
we don't need to crash the system. Perhaps:

		if (WARN_ON(ret > len || ret == 0)) {
			ret = -EIO;
			goto read_err;
		}

> +		*retlen += ret;
> +		buf += ret;
> +		from += ret;
> +		len -= ret;
> +	}
> +	ret = 0;
>  
> +read_err:
>  	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_READ);
> -	if (ret < 0)
> -		return ret;
> -
> -	*retlen += ret;
> -	return 0;
> +	return ret;
>  }
>  
>  static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,

Brian

[1] http://www.spinics.net/lists/linux-spi/msg05877.html
    "RfC: Handle SPI controller limitations like maximum message length"

    I'm honestly not really sure what the conclusion from that thread
    is, so far. It seems like the SPI master "should" be exposing its
    max length to the SPI core, but it's unclear whether that would get
    propagated as a short write/read (i.e., shorter-than-expected
    spi_message::actual_length), or whether the SPI core should be
    somehow splitting up the messages into manageable chunks for us. I'm
    not even sure if the latter is legal for things like
    read-from-flash; might this cause the chip select to get toggled,
    potentially disrupting the operation?

    Anyway, in the former case, we need something like your patch. But
    in the latter case, we actually don't need anything, other than
    maybe an assertion that ret == len.
--
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/

Powered by blists - more mailing lists