mtd: nand: localize ECC failures per page

drivers/mtd/nand/nand_base.c static uint8_t *nand_transfer_oob(struct nand_chip *chip, uint8_t *oob, · line 1425

Quoted 13 lines · Brian Norris

+ * nand_set_read_retry - [INTERN] Set the READ RETRY mode
+ * @mtd: MTD device structure
+ * @retry_mode: the retry mode to use
+ *
+ * Some vendors supply a special command to shift the Vt threshold, to be used
+ * when there are too many bitflips in a page (i.e., ECC error). After setting
+ * a new threshold, the host should retry reading the page.
+ */

drivers/mtd/nand/nand_base.c +5 -0

+static int nand_set_read_retry(struct mtd_info *mtd, int retry_mode)
+{
+	struct nand_chip *chip = mtd->priv;
+	uint8_t feature[ONFI_SUBFEATURE_PARAM_LEN] = {retry_mode};
+

This can cause a DMA warning.

drivers/mtd/nand/nand_base.c static uint8_t *nand_transfer_oob(struct nand_chip *chip, uint8_t *oob, · line 1431

Quoted 6 lines

drivers/mtd/nand/nand_base.c +6 -0

+	if (retry_mode >= chip->read_retries)
+		return -EINVAL;
+
+	if (chip->onfi_params.jedec_id == NAND_MFR_MICRON)
+		return chip->onfi_set_features(mtd, chip,
+				ONFI_FEATURE_ADDR_READ_RETRY, feature);

I suggest to add a hook such as for nand_chip{}:

	chip->read_retry(..)

Different nand chips fill different hook.

For Micron, fill it with micron_read_retry();
for Toshiba, fill it with a toshiab_read_retry();
For Hynix, fill it with hynix_read_retry().

I am wondar if we should add a file for the read-retry in the drivers/mtd/nand folder.

Quoted 15 lines · Huang Shijie

drivers/mtd/nand/nand_base.c static uint8_t *nand_transfer_oob(struct nand_chip *chip, uint8_t *oob, · line 1425

Quoted 13 lines · Brian Norris

+ * nand_set_read_retry - [INTERN] Set the READ RETRY mode
+ * @mtd: MTD device structure
+ * @retry_mode: the retry mode to use
+ *
+ * Some vendors supply a special command to shift the Vt threshold, to be used
+ * when there are too many bitflips in a page (i.e., ECC error). After setting
+ * a new threshold, the host should retry reading the page.
+ */

drivers/mtd/nand/nand_base.c +5 -0

+static int nand_set_read_retry(struct mtd_info *mtd, int retry_mode)
+{
+	struct nand_chip *chip = mtd->priv;
+	uint8_t feature[ONFI_SUBFEATURE_PARAM_LEN] = {retry_mode};
+

This can cause a DMA warning.

...on GPMI NAND, but not on most (any?) other drivers. Why does GPMI try to use DMA on *every* operation? That doesn't even make sense for a 4 or 5 byte transfer. Plus, we don't give a guarantee that buffers will be DMA-able in MTD (UBI uses vmalloc() buffers, for instance), so I'm sure you'll hit problems other places. I can fix this one (use chip->buffers->databuf instead?) but I think GPMI is a bad citizen in this regard, given that you have no guarantee. You need to fix MTD systematically if you expect this guarantee.

(For one, nand_default_block_markbad() uses stack-allocated buffers; but I see that GPMI overrides this callback.)

Isn't completely bloated to setup a DMA operation just to transfer a bunch of bytes? Wouldn't that seriously hurt performance?

Or maybe it doesn't matter because those commands are scarcely invoked...

Quoted 15 lines · Huang Shijie

drivers/mtd/nand/nand_base.c static uint8_t *nand_transfer_oob(struct nand_chip *chip, uint8_t *oob, · line 1425

Quoted 13 lines · Brian Norris

+ * nand_set_read_retry - [INTERN] Set the READ RETRY mode
+ * @mtd: MTD device structure
+ * @retry_mode: the retry mode to use
+ *
+ * Some vendors supply a special command to shift the Vt threshold, to be used
+ * when there are too many bitflips in a page (i.e., ECC error). After setting
+ * a new threshold, the host should retry reading the page.
+ */

drivers/mtd/nand/nand_base.c +5 -0

+static int nand_set_read_retry(struct mtd_info *mtd, int retry_mode)
+{
+	struct nand_chip *chip = mtd->priv;
+	uint8_t feature[ONFI_SUBFEATURE_PARAM_LEN] = {retry_mode};
+

This can cause a DMA warning.

...on GPMI NAND, but not on most (any?) other drivers. Why does GPMI try to use DMA on *every* operation? That doesn't even make sense for a 4 or 5 byte transfer. Plus, we don't give a guarantee that buffers will be DMA-able in MTD (UBI uses vmalloc() buffers, for instance), so I'm sure you'll hit problems other places. I can fix this one (use chip->buffers->databuf instead?) but I think GPMI is a bad citizen in this regard, given that you have no guarantee. You need to fix MTD systematically if you expect this guarantee.

(For one, nand_default_block_markbad() uses stack-allocated buffers; but I see that GPMI overrides this callback.)

Quoted 10 lines

drivers/mtd/nand/nand_base.c static uint8_t *nand_transfer_oob(struct nand_chip *chip, uint8_t *oob, · line 1431

Quoted 6 lines

drivers/mtd/nand/nand_base.c +6 -0

+	if (retry_mode >= chip->read_retries)
+		return -EINVAL;
+
+	if (chip->onfi_params.jedec_id == NAND_MFR_MICRON)
+		return chip->onfi_set_features(mtd, chip,
+				ONFI_FEATURE_ADDR_READ_RETRY, feature);

I suggest to add a hook such as for nand_chip{}:

	chip->read_retry(..)

Different nand chips fill different hook.

OK, that can make sense.

Quoted 3 lines

For Micron, fill it with micron_read_retry();
for Toshiba, fill it with a toshiab_read_retry();
For Hynix, fill it with hynix_read_retry().

Do you have info on Toshiba/Hynix read retry? I'd be interested to know what their "features" look like. Do they have a similar set of supported retry modes, where we have to cycle through each mode and retry? I'm interested in whether the nand_chip.read_retries field is useful for the others.

Also, what would a 'read_retry()' callback look like? What are its parameters? Right now, it's just the struct mtd_info + an incrementing integer, from 0 to chip->read_retries-1.

Quoted 3 lines

I am wondar if we should add a file for the read-retry in the drivers/mtd/nand folder.

TBH, I'm not really very happy to be opening the read-retry can of worms. From what I hear, read-retry is just the first step down a path

Quoted 6 lines · Brian Norris

Quoted 4 lines · Huang Shijie

Review

Quoted 2 lines · Brian Norris

(quoted diff) +2 -0

+	uint8_t feature[ONFI_SUBFEATURE_PARAM_LEN] = {retry_mode};
+

This can cause a DMA warning.

Quoted 8 lines

...on GPMI NAND, but not on most (any?) other drivers. Why does GPMI try to use DMA on *every* operation? That doesn't even make sense for a 4 or 5 byte transfer. Plus, we don't give a guarantee that buffers will be DMA-able in MTD (UBI uses vmalloc() buffers, for instance), so I'm sure you'll hit problems other places. I can fix this one (use chip->buffers->databuf instead?) but I think GPMI is a bad citizen in this regard, given that you have no guarantee. You need to fix MTD systematically if you expect this guarantee.

You do not need to change this code, if you like it.

The DMA warning only occurs when we enable the CONFIG_DMA_API_DEBUG.

Different nand chips use different read-retry methods. A headache to us.

The common idea between the read-retry methods is:
"we have to cycle through each mode and retry"

So the chip.read_retries is used by others too.

After the nand chip use <19nm, afaik, all the NAND chips needs the read-retry. We have to implement the read-retry for all of them. The nightmare is coming.

Quoted 2 lines

The DMA warning only occurs when we enable the CONFIG_DMA_API_DEBUG.

But the warning has a significant purpose: it is *not* legal to perform DMA on stack memory, and no driver should do this. Either nand_base or gpmi-nand need to be fixed here.

But I think it's the GPMI driver is the one that needs fixing, not this patch. Are there any scenarios where it makes sense for gpmi-nand to use DMA for its read_buf/write_buf? Shouldn't the only DMA-necessary operations come through the ecc.write_page and ecc.read_page callbacks? If I'm correct, then gpmi-nand should not be using DMA at all in read_buf/write_buf.

Quoted 2 lines

Different nand chips use different read-retry methods. A headache to us.

Any chance we can push these vendors to implement things through a standard, like JEDEC? I believe there's a JEDEC parameter page standard

now, though I haven't looked at it closely. Micron's support is nice and simple because it properly uses the ONFI vendor block. Other vendors should take note...

Quoted 7 lines · Brian Norris

Quoted 2 lines

The DMA warning only occurs when we enable the CONFIG_DMA_API_DEBUG.

But the warning has a significant purpose: it is *not* legal to perform DMA on stack memory, and no driver should do this. Either nand_base or gpmi-nand need to be fixed here.

Quoted 6 lines

But I think it's the GPMI driver is the one that needs fixing, not this patch. Are there any scenarios where it makes sense for gpmi-nand to use DMA for its read_buf/write_buf? Shouldn't the only DMA-necessary operations come through the ecc.write_page and ecc.read_page callbacks? If I'm correct, then gpmi-nand should not be using DMA at all in read_buf/write_buf.

Currently, all the operations, including the read_buf/write_buf, use the DMA.

Is there any ABI tells us we should not use the DMA for read_buf/write_buf? :)

Quoted 4 lines

Quoted 1 line

Different nand chips use different read-retry methods. A headache to us.

Any chance we can push these vendors to implement things through a standard, like JEDEC? I believe there's a JEDEC parameter page standard

we do not have such influence to push these vendors. they will not listen to us. :(

Quoted 3 lines · Huang Shijie

Quoted 2 lines · Brian Norris

Quoted 2 lines

The DMA warning only occurs when we enable the CONFIG_DMA_API_DEBUG.

Particularly, I think it is quite reasonable to restrict DMA to mtd_read()/mtd_write()-like operations -- so this trickles down to read_page/write_page, but not to device configuration operations like ONFI set_features/get_features/read parameter tables, etc.

Do you disagree? Do you see some need to perform DMA on all operations?

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Different nand chips use different read-retry methods. A headache to us.

Any chance we can push these vendors to implement things through a standard, like JEDEC? I believe there's a JEDEC parameter page standard

we do not have such influence to push these vendors. they will not listen to us. :(

We can try! Strength in numbers! BTW, last time I brought up ONFI w/ a Toshiba representative, I think I offended them :) But at least I had their ear for a moment.

Quoted 9 lines · Brian Norris

Quoted 3 lines · Huang Shijie

Quoted 2 lines · Brian Norris

Quoted 2 lines

The DMA warning only occurs when we enable the CONFIG_DMA_API_DEBUG.

Particularly, I think it is quite reasonable to restrict DMA to mtd_read()/mtd_write()-like operations -- so this trickles down to read_page/write_page, but not to device configuration operations like ONFI set_features/get_features/read parameter tables, etc.

Quoted 1 line

Do you disagree? Do you see some need to perform DMA on all operations?

The gpmi is much coupled with the MXS-DMA engine. the gpmi can uses the mxs-dma to set the gpmi registers.

Use the DMA makes the GPMI code very simple, it really makes the code very _ugly_ if we do not use the DMA for read_buf/write_buf.

If you insist that it is okay to use the local array in the nand_base.c, I can use kmalloc/memcpy in the gpmi's read_buf/write_buf hooks. In such a way, we can make the gpmi looks more normal.

Quoted 10 lines · Huang Shijie

Quoted 9 lines · Brian Norris

Quoted 3 lines · Huang Shijie

Quoted 2 lines · Brian Norris

Quoted 2 lines

The DMA warning only occurs when we enable the CONFIG_DMA_API_DEBUG.

Particularly, I think it is quite reasonable to restrict DMA to mtd_read()/mtd_write()-like operations -- so this trickles down to read_page/write_page, but not to device configuration operations like ONFI set_features/get_features/read parameter tables, etc.

Quoted 7 lines

Quoted 1 line

Do you disagree? Do you see some need to perform DMA on all operations?

The gpmi is much coupled with the MXS-DMA engine. the gpmi can uses the mxs-dma to set the gpmi registers.

Use the DMA makes the GPMI code very simple, it really makes the code very _ugly_ if we do not use the DMA for read_buf/write_buf.

It's not just about ugliness, it's about maintainability. I can clean up a few cases here and there that don't conform to your driver's needs now (notice I pointed out another instance of this in Uwe's patch), but unless we establish better guidelines for NAND, this is fragile.

As a diversion, let's look at atmel_nand for an example. It uses DMA as well, but it dodges some of your problems using a heuristic, so that it only uses DMA for larger-than-OOB-size transactions (see atmel_read_buf). Additionally, it has a hack-y check for highmem, to try to confirm that the region it's trying to map is, in fact, DMA-able.

So, atmel_nand does not have an ideal solution, but it at least attempts to solve parts of this problem. GPMI, on the other hand, ignores it entirely.

Quoted 4 lines

If you insist that it is okay to use the local array in the nand_base.c, I can use kmalloc/memcpy in the gpmi's read_buf/write_buf hooks. In such a way, we can make the gpmi looks more normal.

I think a bounce buffer is necessary, if you really want to use DMA all the time; some transactions are just not worth initializing DMA-able buffers in the high-level code. But even better, can GPMI easily utilize some sort of PIO mode for small/slow-path transactions like this, or is

it inextricably tied to the MXS-DMA engine?