linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH v4 7/7] mtd: spi-nor: add read loop
       [not found] ` <1592668a061137b33c9a6392dfccc67c69fc1fe6.1439543572.git.hramrach@gmail.com>
@ 2015-08-14 10:02   ` Andrew Murray
  2015-08-14 10:08     ` Michal Suchanek
  2015-11-19 23:39   ` Brian Norris
  1 sibling, 1 reply; 12+ messages in thread
From: Andrew Murray @ 2015-08-14 10:02 UTC (permalink / raw)
  To: Michal Suchanek
  Cc: Hou Zhiqiang, shijie.huang, David Woodhouse, Brian Norris,
	Han Xu, Rafał Miłecki, Huang Shijie, Ben Hutchings,
	Marek Vasut, Gabor Juhos, Bean Huo 霍斌斌,,
	MTD Maling List, LKML

On 14 August 2015 at 10:23, Michal Suchanek <hramrach@gmail.com> 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.
>
> Signed-off-by: Michal Suchanek <hramrach@gmail.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;
> +
> +               BUG_ON(ret > len);
> +               *retlen += ret;

Is *retlen initialized to 0 anywhere?

Andrew Murray

> +               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,
> --
> 2.1.4
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 7/7] mtd: spi-nor: add read loop
  2015-08-14 10:02   ` [PATCH v4 7/7] mtd: spi-nor: add read loop Andrew Murray
@ 2015-08-14 10:08     ` Michal Suchanek
  2015-11-05  3:39       ` Hou Zhiqiang
  0 siblings, 1 reply; 12+ messages in thread
From: Michal Suchanek @ 2015-08-14 10:08 UTC (permalink / raw)
  To: Andrew Murray
  Cc: Hou Zhiqiang, Huang Shijie, David Woodhouse, Brian Norris,
	Han Xu, Rafał Miłecki, Huang Shijie, Ben Hutchings,
	Marek Vasut, Gabor Juhos, Bean Huo 霍斌斌,,
	MTD Maling List, LKML

On 14 August 2015 at 12:02, Andrew Murray <amurray@embedded-bits.co.uk> wrote:
> On 14 August 2015 at 10:23, Michal Suchanek <hramrach@gmail.com> 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.
>>
>> Signed-off-by: Michal Suchanek <hramrach@gmail.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;
>> +
>> +               BUG_ON(ret > len);
>> +               *retlen += ret;
>
> Is *retlen initialized to 0 anywhere?

It's initialized in mtdcore and passed into mtd->_read.

Yes, the interface is really awkward.

int mtd_read(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen,
             u_char *buf)
{
        int ret_code;
        *retlen = 0;


Thanks

Michal

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH v4 0/7] Add spi-nor SPI transfer error handling
       [not found] <cover.1439543572.git.hramrach@gmail.com>
       [not found] ` <1592668a061137b33c9a6392dfccc67c69fc1fe6.1439543572.git.hramrach@gmail.com>
@ 2015-08-15  1:51 ` Bean Huo 霍斌斌 (beanhuo)
  2015-08-16 10:20   ` Michal Suchanek
       [not found] ` <e1251eea4ec9cc8c72d54300f3396919a3afe4b1.1439543572.git.hramrach@gmail.com>
  2015-11-19 23:43 ` [PATCH v4 0/7] Add spi-nor SPI transfer error handling Brian Norris
  3 siblings, 1 reply; 12+ messages in thread
From: Bean Huo 霍斌斌 (beanhuo) @ 2015-08-15  1:51 UTC (permalink / raw)
  To: Michal Suchanek, Hou Zhiqiang, shijie.huang, David Woodhouse,
	Brian Norris, Han Xu, Rafał Miłecki
  Cc: Ben Hutchings, Marek Vasut, Gabor Juhos, linux-mtd, linux-kernel

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 639 bytes --]

>Hello,

>with these patches SPI transfer errors are not silently ignored but rather reported to spi-nor users.

>This should prevent silently dropping data to the floor in cases when the SPI transfer fails and the failure is detected.

>It has been pointed out that MTD users do not handle the case when data is read only partially so this version adds the last patch which handles this in spi-nor.

>Thanks

>Michal
 Seems parallel nand read/write also has the same condition.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 0/7] Add spi-nor SPI transfer error handling
  2015-08-15  1:51 ` [PATCH v4 0/7] Add spi-nor SPI transfer error handling Bean Huo 霍斌斌 (beanhuo)
@ 2015-08-16 10:20   ` Michal Suchanek
  0 siblings, 0 replies; 12+ messages in thread
From: Michal Suchanek @ 2015-08-16 10:20 UTC (permalink / raw)
  To: Bean Huo 霍斌斌 (beanhuo)
  Cc: Hou Zhiqiang, shijie.huang, David Woodhouse, Brian Norris,
	Han Xu, Rafał Miłecki, Ben Hutchings,
	Marek Vasut, Gabor Juhos, linux-mtd, linux-kernel

Hello,

On 15 August 2015 at 03:51, Bean Huo 霍斌斌 (beanhuo) <beanhuo@micron.com> wrote:
>>Hello,
>
>>with these patches SPI transfer errors are not silently ignored but rather reported to spi-nor users.
>
>>This should prevent silently dropping data to the floor in cases when the SPI transfer fails and the failure is detected.
>
>>It has been pointed out that MTD users do not handle the case when data is read only partially so this version adds the last patch which handles this in spi-nor.
>
>>Thanks
>
>>Michal
>  Seems parallel nand read/write also has the same condition.

I am not familiar with parallel NAND drivers so I have no idea if
parallel nand can fail in similar way.

As I understand it the parallel nand controller is dedicated piece of
hardware just for accessing the nand so there may not be any problems
similar to what the generic SPI bus has.

Thanks

Michal

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH v4 7/7] mtd: spi-nor: add read loop
  2015-08-14 10:08     ` Michal Suchanek
@ 2015-11-05  3:39       ` Hou Zhiqiang
  2015-11-20 19:18         ` Michal Suchanek
  0 siblings, 1 reply; 12+ messages in thread
From: Hou Zhiqiang @ 2015-11-05  3:39 UTC (permalink / raw)
  To: Michal Suchanek, Andrew Murray
  Cc: Huang Shijie, David Woodhouse, Brian Norris, Xu Han,
	Rafa? Mi?ecki, Huang Shijie, Ben Hutchings, Marek Vasut,
	Gabor Juhos, Bean Huo 霍斌斌,,
	MTD Maling List, LKML

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2134 bytes --]

Hi Michal,

Does this have any updates?

Thanks,
Zhiqiang

> -----Original Message-----
> From: Michal Suchanek [mailto:hramrach@gmail.com]
> Sent: 2015年8月14日 18:08
> To: Andrew Murray
> Cc: Hou Zhiqiang-B48286; Huang Shijie; David Woodhouse; Brian Norris; Xu
> Han-B45815; Rafał Miłecki; Huang Shijie; Ben Hutchings; Marek Vasut;
> Gabor Juhos; Bean Huo 霍斌斌,; MTD Maling List; LKML
> Subject: Re: [PATCH v4 7/7] mtd: spi-nor: add read loop
> 
> On 14 August 2015 at 12:02, Andrew Murray <amurray@embedded-bits.co.uk>
> wrote:
> > On 14 August 2015 at 10:23, Michal Suchanek <hramrach@gmail.com> 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.
> >>
> >> Signed-off-by: Michal Suchanek <hramrach@gmail.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;
> >> +
> >> +               BUG_ON(ret > len);
> >> +               *retlen += ret;
> >
> > Is *retlen initialized to 0 anywhere?
> 
> It's initialized in mtdcore and passed into mtd->_read.
> 
> Yes, the interface is really awkward.
> 
> int mtd_read(struct mtd_info *mtd, loff_t from, size_t len, size_t
> *retlen,
>              u_char *buf)
> {
>         int ret_code;
>         *retlen = 0;
> 
> 
> Thanks
> 
> Michal
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 6/7] mtd: spi-nor: simplify write loop
       [not found] ` <e1251eea4ec9cc8c72d54300f3396919a3afe4b1.1439543572.git.hramrach@gmail.com>
@ 2015-11-19 23:18   ` Brian Norris
  2015-11-20 18:59     ` Michal Suchanek
  0 siblings, 1 reply; 12+ messages in thread
From: Brian Norris @ 2015-11-19 23:18 UTC (permalink / raw)
  To: Michal Suchanek
  Cc: Hou Zhiqiang, shijie.huang, David Woodhouse, Han Xu,
	Rafał Miłecki, Huang Shijie, Ben Hutchings,
	Marek Vasut, Gabor Juhos, Bean Huo 霍斌斌,,
	linux-mtd, linux-kernel

Hi Michal,

Sorry this has sat so long...

On Fri, Aug 14, 2015 at 09:23:08AM -0000, Michal Suchanek wrote:
> The spi-nor write loop assumes that what is passed to the hardware
> driver write() is what gets written.
> 
> When write() writes less than page size at once data is dropped on the
> floor. Check the amount of data writen.

Have you seen write() return less than the page size? I know you've
struggled with a SPI driver that can't do "very" (for some definition of
very) long transfers, due to unknown bugs, but I thought that "very" was
much larger than the page size.

> This also means that write can start mid-page any time so there is
> no special case for first page.

I think we'd have some problems if we start seeing hardware that can't
write ~256 bytes at a time. If nothing else, this can be a problem
because some SPI NOR flash are known to have inferior reliability if you
regularly write in small chunks. I believe this is because they actually
use some kind of internal ECC.

So, if you're just guarding against a theoretical behavior, perhaps it's
best if this is done with some kind of assertion, as I'd rather not
encourage non-aligned writes if possible. I notice you used BUG_ON() in
another patch, but I'd suggest maybe something less harsh, like WARN()
or WARN_ONCE().

Brian

> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 48 ++++++++++++++-----------------------------
>  1 file changed, 15 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 837e3df..e0ae9cf 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -836,7 +836,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>  	size_t *retlen, const u_char *buf)
>  {
>  	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> -	u32 page_offset, page_size, i;
> +	size_t page_offset, page_remain, i;
>  	int ret;
>  
>  	dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len);
> @@ -845,45 +845,27 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>  	if (ret)
>  		return ret;
>  
> -	write_enable(nor);
> -
> -	page_offset = to & (nor->page_size - 1);
> +	for (i = 0; i < len; ) {
> +		int written;
>  
> -	/* do all the bytes fit onto one page? */
> -	if (page_offset + len <= nor->page_size) {
> -		ret = nor->write(nor, to, len, buf);
> -		if (ret < 0)
> -			goto write_err;
> -		*retlen += ret;
> -	} else {
> +		page_offset = to & (nor->page_size - 1);
>  		/* the size of data remaining on the first page */
> -		page_size = nor->page_size - page_offset;
> -		ret = nor->write(nor, to, page_size, buf);
> +		page_remain = min_t(size_t,
> +				    nor->page_size - page_offset, len - i);
> +
> +		write_enable(nor);
> +		ret = nor->write(nor, to + i, page_remain, buf + i);
>  		if (ret < 0)
>  			goto write_err;
> -		*retlen += ret;
> -
> -		/* write everything in nor->page_size chunks */
> -		for (i = ret; i < len; ) {
> -			page_size = len - i;
> -			if (page_size > nor->page_size)
> -				page_size = nor->page_size;
> -
> -			ret = spi_nor_wait_till_ready(nor);
> -			if (ret)
> -				goto write_err;
> +		written = ret;
>  
> -			write_enable(nor);
> -
> -			ret = nor->write(nor, to + i, page_size, buf + i);
> -			if (ret < 0)
> -				goto write_err;
> -			*retlen += ret;
> -			i += ret;
> -		}
> +		ret = spi_nor_wait_till_ready(nor);
> +		if (ret)
> +			goto write_err;
> +		*retlen += written;
> +		i += written;
>  	}
>  
> -	ret = spi_nor_wait_till_ready(nor);
>  write_err:
>  	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_WRITE);
>  	return ret;
> -- 
> 2.1.4
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 7/7] mtd: spi-nor: add read loop
       [not found] ` <1592668a061137b33c9a6392dfccc67c69fc1fe6.1439543572.git.hramrach@gmail.com>
  2015-08-14 10:02   ` [PATCH v4 7/7] mtd: spi-nor: add read loop Andrew Murray
@ 2015-11-19 23:39   ` Brian Norris
  2015-11-20  6:26     ` Heiner Kallweit
  1 sibling, 1 reply; 12+ messages in thread
From: Brian Norris @ 2015-11-19 23:39 UTC (permalink / raw)
  To: Michal Suchanek
  Cc: Hou Zhiqiang, shijie.huang, David Woodhouse, Han Xu,
	Rafał Miłecki, Ben Hutchings, Marek Vasut, Gabor Juhos,
	Bean Huo 霍斌斌,,
	linux-mtd, linux-kernel, Huang Shijie, Heiner Kallweit

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 0/7] Add spi-nor SPI transfer error handling
       [not found] <cover.1439543572.git.hramrach@gmail.com>
                   ` (2 preceding siblings ...)
       [not found] ` <e1251eea4ec9cc8c72d54300f3396919a3afe4b1.1439543572.git.hramrach@gmail.com>
@ 2015-11-19 23:43 ` Brian Norris
  3 siblings, 0 replies; 12+ messages in thread
From: Brian Norris @ 2015-11-19 23:43 UTC (permalink / raw)
  To: Michal Suchanek
  Cc: Hou Zhiqiang, David Woodhouse, Han Xu, Rafał Miłecki,
	Ben Hutchings, Marek Vasut, Gabor Juhos,
	Bean Huo 霍斌斌,,
	linux-mtd, linux-kernel, Heiner Kallweit, Huang Shijie

Hi Michal,

On Fri, Aug 14, 2015 at 09:23:06AM -0000, Michal Suchanek wrote:
> Hello,
> 
> with these patches SPI transfer errors are not silently ignored but rather
> reported to spi-nor users.
> 
> This should prevent silently dropping data to the floor in cases when the SPI
> transfer fails and the failure is detected.
> 
> It has been pointed out that MTD users do not handle the case when data is read
> only partially so this version adds the last patch which handles this in
> spi-nor.
> 
> Thanks
> 
> Michal
> 
> Michal Suchanek (7):
>   mtd: spi-nor: change return value of read/write
>   mtd: m25p80: return amount of data transferred or error in read/write
>   mtd: fsl-quadspi: return amount of data read/written or error
>   mtd: spi-nor: check return value from read/write
>   mtd: spi-nor: stop passing around retlen

Patches 1 to 5 look good to me, though there is a new spi-nor driver
since you sent this. Can you rebase/fixup?

>   mtd: spi-nor: simplify write loop
>   mtd: spi-nor: add read loop

I posted some comments for these. I get the feeling that patch 6 is
over-complicated and should be just a simple assertion. And I'm still
not 100% sure on the approach for patch 7, since it's not clear whether
the responsibility lies in the SPI layer or in MTD.

Brian

>  drivers/mtd/devices/m25p80.c      | 33 +++++++++------
>  drivers/mtd/spi-nor/fsl-quadspi.c | 29 ++++++-------
>  drivers/mtd/spi-nor/spi-nor.c     | 85 +++++++++++++++++++++++----------------
>  include/linux/mtd/spi-nor.h       |  8 ++--
>  4 files changed, 91 insertions(+), 64 deletions(-)
> 
> -- 
> 2.1.4
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 7/7] mtd: spi-nor: add read loop
  2015-11-19 23:39   ` Brian Norris
@ 2015-11-20  6:26     ` Heiner Kallweit
  0 siblings, 0 replies; 12+ messages in thread
From: Heiner Kallweit @ 2015-11-20  6:26 UTC (permalink / raw)
  To: Brian Norris, Michal Suchanek
  Cc: Hou Zhiqiang, shijie.huang, David Woodhouse, Han Xu,
	Rafał Miłecki, Ben Hutchings, Marek Vasut, Gabor Juhos,
	Bean Huo, linux-mtd, linux-kernel, Huang Shijie

Am 20.11.2015 um 00:39 schrieb Brian Norris:
> + 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@gmail.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?

That's exactly my issue with Freescale ESPI.
You provide a message length (up to 64K) to the chip and it sets / resets
CS internally. Resuming a short read requires to set the (SPI NOR)
start address for the next read properly.
And this can be done by the protocol driver only.
Currently the fsl-espi controller driver includes some ugly
(protocol driver) logic to deal with this.
One consequence is that this controller driver can be used with
SPI NOR devices only.

At a first glance I see two options:

1. The controller driver returns an error like EMSGSIZE and the number
   of read bytes. Then the protocol driver can loop and assemble the chunks.

2. The SPI master exposes the SPI message length constraint and the
   protocol driver can proactively deal with this limitation.

I have a little headache with option 1 because I'm not sure that we can
in general rely on the number of read bytes if an error is returned.
Some controller driver might return whatever value assuming that the
caller ignores it anyway if an error is returned.

Options 2 I like better as it doesn't require the controller driver
to handle an error situation like a short read in a specific way.

And it seems like Mark could be fine with an additional member of
spi_master like size_t max_msg_size.
The idea of a struct encapsulating all constraints he didn't like too much.

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 6/7] mtd: spi-nor: simplify write loop
  2015-11-19 23:18   ` [PATCH v4 6/7] mtd: spi-nor: simplify write loop Brian Norris
@ 2015-11-20 18:59     ` Michal Suchanek
  0 siblings, 0 replies; 12+ messages in thread
From: Michal Suchanek @ 2015-11-20 18:59 UTC (permalink / raw)
  To: Brian Norris
  Cc: Hou Zhiqiang, Huang Shijie, David Woodhouse, Han Xu,
	Rafał Miłecki, Huang Shijie, Ben Hutchings,
	Marek Vasut, Gabor Juhos, Bean Huo 霍斌斌,,
	MTD Maling List, Linux Kernel Mailing List

On 20 November 2015 at 00:18, Brian Norris <computersforpeace@gmail.com> wrote:
> Hi Michal,
>
> Sorry this has sat so long...
>
> On Fri, Aug 14, 2015 at 09:23:08AM -0000, Michal Suchanek wrote:
>> The spi-nor write loop assumes that what is passed to the hardware
>> driver write() is what gets written.
>>
>> When write() writes less than page size at once data is dropped on the
>> floor. Check the amount of data writen.
>
> Have you seen write() return less than the page size? I know you've
> struggled with a SPI driver that can't do "very" (for some definition of
> very) long transfers, due to unknown bugs, but I thought that "very" was
> much larger than the page size.

Not in the case of a driver that can transfer 63 bytes at a time. It
because the DMA engine needed for normal operation was not merged yet,
though.

>
>> This also means that write can start mid-page any time so there is
>> no special case for first page.
>
> I think we'd have some problems if we start seeing hardware that can't
> write ~256 bytes at a time. If nothing else, this can be a problem
> because some SPI NOR flash are known to have inferior reliability if you
> regularly write in small chunks. I believe this is because they actually
> use some kind of internal ECC.
>
> So, if you're just guarding against a theoretical behavior, perhaps it's
> best if this is done with some kind of assertion, as I'd rather not
> encourage non-aligned writes if possible. I notice you used BUG_ON() in
> another patch, but I'd suggest maybe something less harsh, like WARN()
> or WARN_ONCE().

I agree that a warning is in order in the case when whole page cannot
be written in one go.

Thanks

Michal

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 7/7] mtd: spi-nor: add read loop
  2015-11-05  3:39       ` Hou Zhiqiang
@ 2015-11-20 19:18         ` Michal Suchanek
  2016-01-12  6:35           ` Zhiqiang Hou
  0 siblings, 1 reply; 12+ messages in thread
From: Michal Suchanek @ 2015-11-20 19:18 UTC (permalink / raw)
  To: Hou Zhiqiang
  Cc: Andrew Murray, Huang Shijie, David Woodhouse, Brian Norris,
	Xu Han, Rafa? Mi?ecki, Huang Shijie, Ben Hutchings, Marek Vasut,
	Gabor Juhos, Bean Huo 霍斌斌,,
	MTD Maling List, LKML

Hello,

On 5 November 2015 at 04:39, Hou Zhiqiang <B48286@freescale.com> wrote:
> Hi Michal,
>
> Does this have any updates?

I will try to update this patchset when it's agreed how the limit on
transfer size is handled. Writing less data than was requested is
acceptable for spi-nor but might disrupt other drivers so perhaps some
preemptive mechanism with the SPI master saying how much data it can
transfer and the drivers looking at that value would be preferred.

Thanks

Michal

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH v4 7/7] mtd: spi-nor: add read loop
  2015-11-20 19:18         ` Michal Suchanek
@ 2016-01-12  6:35           ` Zhiqiang Hou
  0 siblings, 0 replies; 12+ messages in thread
From: Zhiqiang Hou @ 2016-01-12  6:35 UTC (permalink / raw)
  To: Michal Suchanek
  Cc: Andrew Murray, Huang Shijie, David Woodhouse, Brian Norris,
	Han Xu, Rafa? Mi?ecki, Huang Shijie, Ben Hutchings, Marek Vasut,
	Gabor Juhos, Bean Huo 霍斌斌,,
	MTD Maling List, LKML

Hi Michal,

Thanks for your info, and I see you have updated the patchset on Dec.02. Is it the conclusion, right?

> -----Original Message-----
> From: Michal Suchanek [mailto:hramrach@gmail.com]
> Sent: 2015年11月21日 3:19
> To: Hou Zhiqiang-B48286
> Cc: Andrew Murray; Huang Shijie; David Woodhouse; Brian Norris; Xu Han-
> B45815; Rafa? Mi?ecki; Huang Shijie; Ben Hutchings; Marek Vasut; Gabor
> Juhos; Bean Huo 霍斌斌,; MTD Maling List; LKML
> Subject: Re: [PATCH v4 7/7] mtd: spi-nor: add read loop
> 
> Hello,
> 
> On 5 November 2015 at 04:39, Hou Zhiqiang <B48286@freescale.com> wrote:
> > Hi Michal,
> >
> > Does this have any updates?
> 
> I will try to update this patchset when it's agreed how the limit on
> transfer size is handled. Writing less data than was requested is
> acceptable for spi-nor but might disrupt other drivers so perhaps some
> preemptive mechanism with the SPI master saying how much data it can
> transfer and the drivers looking at that value would be preferred.
> 

Thanks,
Zhiqiang

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2016-01-12  7:07 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <cover.1439543572.git.hramrach@gmail.com>
     [not found] ` <1592668a061137b33c9a6392dfccc67c69fc1fe6.1439543572.git.hramrach@gmail.com>
2015-08-14 10:02   ` [PATCH v4 7/7] mtd: spi-nor: add read loop Andrew Murray
2015-08-14 10:08     ` Michal Suchanek
2015-11-05  3:39       ` Hou Zhiqiang
2015-11-20 19:18         ` Michal Suchanek
2016-01-12  6:35           ` Zhiqiang Hou
2015-11-19 23:39   ` Brian Norris
2015-11-20  6:26     ` Heiner Kallweit
2015-08-15  1:51 ` [PATCH v4 0/7] Add spi-nor SPI transfer error handling Bean Huo 霍斌斌 (beanhuo)
2015-08-16 10:20   ` Michal Suchanek
     [not found] ` <e1251eea4ec9cc8c72d54300f3396919a3afe4b1.1439543572.git.hramrach@gmail.com>
2015-11-19 23:18   ` [PATCH v4 6/7] mtd: spi-nor: simplify write loop Brian Norris
2015-11-20 18:59     ` Michal Suchanek
2015-11-19 23:43 ` [PATCH v4 0/7] Add spi-nor SPI transfer error handling Brian Norris

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).