Re: [PATCH 2/3] MTD: spi-nor: check for short writes in spi_nor_write.

Re: [PATCH 2/3] MTD: spi-nor: check for short writes in spi_nor_write.

[Brian Norris]

+ linux-spi, Mark

On Thu, Apr 30, 2015 at 03:38:50PM +0200, Michal Suchanek wrote:
> My SPI controller driver does not support DMA so writes are truncated to
> FIFO size.

Which SPI master driver?

> Check the amount of data actually written by the driver.

I'm not sure if we should just reactively use the retlen, or if we
should be communicating such a limitation via the SPI API. Thoughts?

Brian

> Signed-off-by: Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 42 +++++++++++++++++-------------------------
>  1 file changed, 17 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 14a5d23..75904b5 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -807,7 +807,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;
> +	u32 page_offset, page_remainder, page_size, i;
>  	int ret;
>  
>  	dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len);
> @@ -816,36 +816,28 @@ 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);
> -
> -	/* do all the bytes fit onto one page? */
> -	if (page_offset + len <= nor->page_size) {
> -		nor->write(nor, to, len, retlen, buf);
> -	} else {
> -		/* the size of data remaining on the first page */
> -		page_size = nor->page_size - page_offset;
> -		nor->write(nor, to, page_size, retlen, buf);
> +	/* write everything in nor->page_size chunks */
> +	/* check the size of data actually written */
> +	for (i = 0; i < len; i += *retlen) {
> +		page_size = len - i;
> +		page_offset = (to + i) & (nor->page_size - 1);
> +		page_remainder = nor->page_size - page_offset;
> +		if (page_size > nor->page_size)
> +			page_size = nor->page_size;
> +		if (page_remainder && (page_size > page_remainder))
> +			page_size = page_remainder;
>  
> -		/* write everything in nor->page_size chunks */
> -		for (i = page_size; i < len; i += page_size) {
> -			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;
> +		write_enable(nor);
>  
> -			write_enable(nor);
> +		nor->write(nor, to + i, page_size, retlen, buf + i);
>  
> -			nor->write(nor, to + i, page_size, retlen, buf + i);
> -		}
> +		ret = spi_nor_wait_till_ready(nor);
> +		if (ret)
> +			goto write_err;
>  	}
>  
> -	ret = spi_nor_wait_till_ready(nor);
>  write_err:
> +	*retlen = i;
>  	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_WRITE);
>  	return ret;
>  }

Re: [PATCH 2/3] MTD: spi-nor: check for short writes in spi_nor_write.

[Michal Suchanek]

On 21 May 2015 at 01:38, Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> + linux-spi, Mark
>
> On Thu, Apr 30, 2015 at 03:38:50PM +0200, Michal Suchanek wrote:
>> My SPI controller driver does not support DMA so writes are truncated to
>> FIFO size.
>
> Which SPI master driver?

I am using sunxi SPI driver. The dmaengine support for sunxi is not
yet in mainline kernel so the SPI master functionality is limited.

>
>> Check the amount of data actually written by the driver.
>
> I'm not sure if we should just reactively use the retlen, or if we
> should be communicating such a limitation via the SPI API. Thoughts?
>

Is there any driver that would break if the SPI master truncated
writes when the message is too long rather than returning an error an
refusing the transfer?

m25p80 as is would because it assumes all data is always written.

Some display driver I tried earlier has an option to limit transfer
size actively in the driver.

Thanks

Michal

Re: [PATCH 2/3] MTD: spi-nor: check for short writes in spi_nor_write.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

On Thu, May 21, 2015 at 10:39:13AM +0200, Michal Suchanek wrote:
> On 21 May 2015 at 01:38, Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

Why is this thread about SPI error handling CCed to quite so many
people?

> >> Check the amount of data actually written by the driver.

> > I'm not sure if we should just reactively use the retlen, or if we
> > should be communicating such a limitation via the SPI API. Thoughts?

> Is there any driver that would break if the SPI master truncated
> writes when the message is too long rather than returning an error an
> refusing the transfer?

Any such driver is buggy, the actual length transferred has always been
a part of the SPI API.  We should probably expose limitations so clients
can query in advance (we don't at the minute) but it'd be a while before
clients could rely on that information being useful and it's still
possible things can be truncated by error.

With modern drivers using transfer_one() where we have control of the
chip select we do also have the ability to add support to the core for
chopping large transfers up into smaller ones that the hardware can
handle transparently.  That won't for everything though since it depends
on us being able to manually control the chip select which not all
hardware can do.

Re: [PATCH 2/3] MTD: spi-nor: check for short writes in spi_nor_write.

[Brian Norris]

On Thu, May 21, 2015 at 11:28:02AM +0100, Mark Brown wrote:
> On Thu, May 21, 2015 at 10:39:13AM +0200, Michal Suchanek wrote:
> > On 21 May 2015 at 01:38, Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> Why is this thread about SPI error handling CCed to quite so many
> people?

I can't really speak for the original patch author, who created the CC
list. I added you for comment on the SPI API (thanks BTW).

This is part of a series that included an ill-conceived DT binding for
recording a "max transfer length" property in the flash node. That's one
step that easily blows up the CC list for the series, as there are 5 DT
maintainers and a mailing list. Others are contributors to the m25p80 /
spi-nor drivers. (All in all, you can probably trace this back to
scripts/get_maintainer.pl.)

> > >> Check the amount of data actually written by the driver.
> 
> > > I'm not sure if we should just reactively use the retlen, or if we
> > > should be communicating such a limitation via the SPI API. Thoughts?
> 
> > Is there any driver that would break if the SPI master truncated
> > writes when the message is too long rather than returning an error an
> > refusing the transfer?
> 
> Any such driver is buggy, the actual length transferred has always been
> a part of the SPI API.

OK, so m25p80.c currently checks the retlen
(spi_message::actual_length), but it doesn't actually handle it well if
the SPI master can't write a full page-size chunk in one go. It looks
like we'd leave holes in the programmed data right now.

So that qualifies as buggy, and that's part of what Michal is trying to
fix, I think. Admittedly, as he's using an out-of-tree driver, I'm not
sure I know exactly what failure modes he is hitting yet.

> We should probably expose limitations so clients
> can query in advance (we don't at the minute) but it'd be a while before
> clients could rely on that information being useful and it's still
> possible things can be truncated by error.

That might help in the long run. In this case, I think we might be able
to get by (for a properly-functioning SPI driver with a limited transfer
size) with the current API, and handling the 'return length < message
length' case better.

BTW, one extra note for Michal regarding your SPI controller/driver
transfer length limitation: you would do well to try as much as possible
to allow full NOR pages to be programmed in one SPINOR_OP_PP sequence. I
know of some SPI NOR that, though they are byte addressable, actually
have opaque internal ECC which is encoded on page boundaries, so you
get better flash lifetime if you program on page boundaries, rather than
on whatever smaller chunk size your SPI driver supports. So that might
require a little more work on your SPI driver.

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] MTD: spi-nor: check for short writes in spi_nor_write.

[Brian Norris]

(trimming CC a little this time, though it's still a bit large)

On Fri, May 22, 2015 at 12:17:27AM -0700, Brian Norris wrote:
> Admittedly, as he's using an out-of-tree driver, I'm not
> sure I know exactly what failure modes he is hitting yet.

Sorry, I realized I misread here. He's using spi-sunxi. Given that...

... is this code even valid?

static int sun6i_spi_transfer_one(struct spi_master *master,
                                  struct spi_device *spi,
                                  struct spi_transfer *tfr)
{
...
	/* We don't support transfer larger than the FIFO */
	if (tfr->len > SUN6I_FIFO_DEPTH)
		return -EINVAL;

Seems like it should be looping over the transfer in multiple chunks
instead.

Brian

Re: [PATCH 2/3] MTD: spi-nor: check for short writes in spi_nor_write.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1052 bytes --]

On Fri, May 22, 2015 at 12:25:05AM -0700, Brian Norris wrote:
> On Fri, May 22, 2015 at 12:17:27AM -0700, Brian Norris wrote:

> > Admittedly, as he's using an out-of-tree driver, I'm not
> > sure I know exactly what failure modes he is hitting yet.

> Sorry, I realized I misread here. He's using spi-sunxi. Given that...

> ... is this code even valid?

> static int sun6i_spi_transfer_one(struct spi_master *master,
>                                   struct spi_device *spi,
>                                   struct spi_transfer *tfr)
> {
> ...
> 	/* We don't support transfer larger than the FIFO */
> 	if (tfr->len > SUN6I_FIFO_DEPTH)
> 		return -EINVAL;

> Seems like it should be looping over the transfer in multiple chunks
> instead.

Well, it's not ideal.  Like I say I think that logic probably belongs in
the core rather than individual drivers then we minimise the problems,
if I remember correctly there was the suggestion that the DMA code was
going to follow soon and be used for larger transfers when the original
driver was merged.

Re: [PATCH 0/3] Using SPI NOR flah on sunxi.

[Brian Norris]

On Thu, Apr 30, 2015 at 08:34:36PM +0200, Marek Vasut wrote:
> On Thursday, April 30, 2015 at 06:56:18 PM, Michal Suchanek wrote:
> > On 30 April 2015 at 18:30,  <Thomas.Betker@rohde-schwarz.com> wrote:
> > > Hello Michal:
> > >> I tried to connect a SPI NOR flash to my sunxi board and due to the
> > > 
> > > current
> > > 
> > >> sunxi SPI driver limitations it does not work.
> > >> 
> > >> The SPI driver returns an error when more than 64 bytes are
> > >> transferred at once
> > >> due to lack of DMA support.
> > > 
> > > Wouldn't it be easier to fix the SPI driver to handle transfers larger
> > > than 64 bytes, filling and draining the FIFO multiple times if
> > > neccessary? (As far as I can tell, most SPI drivers do this.)
> > 
> > Yes, the intent is to fix this by adding dma support to the driver,
> > eventually.
> > 
> > The patch might be still useful for other hardware with developing SPI
> > support.
> 
> Please just fix the controller driver to correctly handle arbitrary transfer
> lengths.

Just noticed this. Yes, that would definitely be a better option!

Brian