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

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.

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

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

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

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.

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

My 2ct:

1. They should return the actual length of the current transfer in *retlen.

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.

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

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

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.

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

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.

Quoted 2 lines · Heiner Kallweit

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/)

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 7 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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

Yep.

Quoted 3 lines

My 2ct:

1. They should return the actual length of the current transfer in *retlen.

Seems reasonable.

Quoted 4 lines

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.

Quoted 2 lines

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

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

Quoted 1 line

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

Sounds OK.

Quoted 3 lines

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.

Quoted 2 lines

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

Brian