Error handling in spi-nor and dealing with short reads/writes

[Brian Norris]

Hi Heiner,

I see you've sent followup patches for this already. Haven't had a
chance to look at them fully yet, but I can answer a bit.

On Wed, Oct 07, 2015 at 09:41:37PM +0200, Heiner Kallweit wrote:
> I stumbled across some missing error handling in spi-nor and when having a look at the
> list archive I found some proposed patches and related discussions.

Looking at this?

http://lists.infradead.org/pipermail/linux-mtd/2015-August/061058.html

(Unfortunately, this series got dropped by patchwork:
http://patchwork.ozlabs.org/project/linux-mtd/list/)

> However it seems like there was no result, at least I didn't find accepted patches
> dealing with this issue.

Yes, they haven't been accepted yet. IIRC, they partly had to do with
buggy SPI drivers, and so it was hard to get things straight when the
submitter just wanted to paper over driver bugs. I haven't had the
incentive or time to properly revisit the later versions of those
patches. But I may review/merge them soon, and it looks like that may
conflict with your patches.

It'd be great if you could review those patches.

> To summarize few issues:
> 
> 1. I didn't find any clear definition how the read / write callbacks in spi_nor and mtd_info
>    are supposed to handle the retlen argument (returning the actual length of the current
>    transfer or adding it to the current value of *retlen).

Yeah...they're not extremely well specified. mtd_info, at least, has a
fairly well established de facto specification, but spi-nor might be a
bit more inconsistent.

>    Maybe due to this missing specification there are different implementations of the read
>    callback in spi_nor:
>    m25p80_read: sets *retlen to the actual length of the current transfer
>    nxp_spifi_read: adds it to *retlen
>    fsl_qspi_read: adds it to *retlen
>    Luckily this has no impact so far as mtd_read initializes *retlen to 0 before calling
>    the _read callback (once).

Right, all MTD users assume that the core MTD code initializes *retlen.
That was done because otherwise, the initialization was done
inconsistently across all drivers.

> 2. The write callback in spi_nor is defined with a void return type and doesn't allow to
>    handle the case of a failing transfer.

That one has been odd to me.

> 3. It's unclear at which layer incomplete reads/writes are acceptable (and therefore which
>    layer can / should loop to assemble chunks).

Yep.

> My 2ct:
> 
> 1. They should return the actual length of the current transfer in *retlen.

Seems reasonable.

>    In case of a partial transfer they should return an error (e.g. -EIO or -EMSGSIZE).
>    This allows the caller to identify a partial transfer and the amount of
>    bytes successfully transferred. Then the caller can decide whether he wants
>    to retry for the failed part of the transfer and later assemble the chunks.

I don't think we want to handle both errors and *retlen; those are
mutually exclusive. If we have to negotiate a max message size for some
reason, that's a different story.

> 2. Make it return int (like _write in mtd_info).

I assume 'it' is spi_nor->_write() callback. Then yes.

> 3. I prefer to handle read chunks in spi_nor_read (submitted a related patch already).

Sounds OK.

>    Not sure whether it would make sense to add chunk handling also for writes.
>    I'm not aware of any controller which is not able to transfer at least a single page
>    at once.

At most, we should just make sure the error reporting and *retlen
handling is correct, and if drivers run into problems, then we can talk.
For one, you sometimes get less wear reliability from NOR flash when
writing in smaller-than-page-size chunks, so we shouldn't encourage it.

> This is meant as a RfC. Also I may have missed important parts of the discussion.
> Appreciate any hint or comment.

Brian