[RFC PATCH v5 3/6] mtd: rawnand: meson: only expose unprotected user OOB bytes
Arseniy Krasnov
avkrasnov at sberdevices.ru
Mon Jun 5 21:42:58 PDT 2023
On 05.06.2023 12:48, Miquel Raynal wrote:
> Hi Arseniy,
>
> avkrasnov at sberdevices.ru wrote on Fri, 2 Jun 2023 11:53:47 +0300:
>
>> Hello Miquel, thanks for review!
>>
>> On 01.06.2023 11:31, Miquel Raynal wrote:
>>> Hi Arseniy,
>>>
>>> AVKrasnov at sberdevices.ru wrote on Thu, 1 Jun 2023 09:18:46 +0300:
>>>
>>>> This moves free bytes of OOB to non-protected ECC area. It is needed to
>>>
>>> As we discussed, I expect this commit to just change the OOB layout to
>>> expose unprotected OOB bytes to the user, that is the only change this
>>> commit should carry. If that does not work, you should add a
>>> preparation patch.
>>
>> Ok, but I thought, if i change only OOB layout, e.g. update 'free' callback of
>> mtd_ooblayout_ops, I also need to implement code which performs read/write
>> according new layout (it must be done in a single patch)?
>
> No, this is orthogonal.
>
> The driver must read the the whole OOB area (and perhaps reorder the
> data), but you should not make any decision regarding what bytes you
> want or not want to expose.
>
> Then, the user (no matter what "user" is here) will decide how to deal
> with the data.
Hello Miquel!
Ok, so in case of:
1) read I just need to read OOB data using 'nand_change_read_column_op()' and place it to 'oob_buf'.
2) write I need to write OOB data using 'nand_change_write_column_op()' to controller internal RAM and then call PAGE_PROG.
Even in ECC mode, data which occupies places of ECC codes will be removed by hw ( as You mentinoed below).
That's all?:)
>
>> Main thing is:
>>
>> I guess that general confuse with this patch is that You consider
>> that we change only OOB layout by moving user bytes out of ECC area, but at the same
>> time I also increased size of OOB from 4 bytes (e.g. 2 x 2 bytes clean markers)
>> to 32 bytes (e.g. tail of page after data and ECC codes), so if this
>> assumption is correct, in the next version I won't change size of user area in
>> OOB, thus this patch will be reduced as some comments from this review.
>
> Exposing only 4 bytes was a mistake in the first place, please fix this
> in a dedicated patch.
So current (not merged) version exposes bytes 0,1,16,17 of OOB, You mean this is wrong?
Correct way is to expose 32,33,48,49 (e.g. shifted by 32)?
Thanks, Arseniy
>
>>>> make JFFS2 works correctly with this NAND controller. Problem fires when
>>>> JFFS2 driver writes cleanmarker to some page and later it tries to write
>>>> to this page - write will be done successfully, but after that such page
>>>> becomes unreadable due to invalid ECC codes. This happens because second
>>>> write needs to update ECC codes, but it is impossible to do it correctly
>>>> without block erase. So idea of this patch is to use the unprotected OOB
>>>> area to store the cleanmarkers, so that they can be written by the
>>>> filesystem without caring much about the page being empty or not: the
>>>> ECC codes will not be written anyway.
>>>>
>>>> JFFS2 is only useful on tiny NAND devices, where UBI does not fit, which
>>>> are usually true SLC flashes, with the capability of writing a page with
>>>> empty (0xFF) data, and still be able to write actual data to it later in
>>>> a second write.
>>>>
>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov at sberdevices.ru>
>>>> ---
>>>> Changelog v4->v5:
>>>> * Drop cosmetic changes from this patch.
>>>> * Do not ignore ECC protected user bytes provided by hw. Even these
>>>> bytes are out of user area of OOB, its values are still read from
>>>> the provided OOB buffer and written by hardware. Same behaviour is
>>>> preserved for read access - such bytes are read from DMA buffer and
>>>> placed to OOB buffer.
>>>> * OOB read and write become more lightweight because I removed heavy
>>>> READ0 and PAGEPROG command from it (both commands are still sent
>>>> when OOB access is performed using OOB callbacks). In case of page
>>>> read/write OOB data is handled in the internal SRAM of the controller.
>>>> * Commit message updated.
>>>> * Temporary buffer for OOB read/write is removed. Seems everything
>>>> works correctly without it.
>>>>
>>>> drivers/mtd/nand/raw/meson_nand.c | 134 ++++++++++++++++++++++++++----
>>>> 1 file changed, 117 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
>>>> index 82a629025adc..e42c28be02f3 100644
>>>> --- a/drivers/mtd/nand/raw/meson_nand.c
>>>> +++ b/drivers/mtd/nand/raw/meson_nand.c
>>>> @@ -358,8 +358,11 @@ static u8 *meson_nfc_data_ptr(struct nand_chip *nand, int i)
>>>> static void meson_nfc_get_data_oob(struct nand_chip *nand,
>>>> u8 *buf, u8 *oobbuf)
>>>> {
>>>> + struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
>>>> + struct mtd_info *mtd = nand_to_mtd(nand);
>>>> int i, oob_len = 0;
>>>> u8 *dsrc, *osrc;
>>>> + u8 *oobtail;
>>>>
>>>> oob_len = nand->ecc.bytes + 2;
>>>> for (i = 0; i < nand->ecc.steps; i++) {
>>>> @@ -368,17 +371,27 @@ static void meson_nfc_get_data_oob(struct nand_chip *nand,
>>>> memcpy(buf, dsrc, nand->ecc.size);
>>>> buf += nand->ecc.size;
>>>> }
>>>> +
>>>> osrc = meson_nfc_oob_ptr(nand, i);
>>>> memcpy(oobbuf, osrc, oob_len);
>>>> oobbuf += oob_len;
>>>> }
>>>> +
>>>> + oobtail = meson_chip->data_buf + nand->ecc.steps *
>>>> + (nand->ecc.size + oob_len);
>>>> +
>>>> + /* 'oobbuf' points to the start of user area. */
>>>> + memcpy(oobbuf, oobtail, mtd->oobsize - nand->ecc.steps * oob_len);
>>>> }
>>>>
>>>> static void meson_nfc_set_data_oob(struct nand_chip *nand,
>>>> const u8 *buf, u8 *oobbuf)
>>>> {
>>>> + struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand);
>>>> + struct mtd_info *mtd = nand_to_mtd(nand);
>>>> int i, oob_len = 0;
>>>> u8 *dsrc, *osrc;
>>>> + u8 *oobtail;
>>>>
>>>> oob_len = nand->ecc.bytes + 2;
>>>> for (i = 0; i < nand->ecc.steps; i++) {
>>>> @@ -391,6 +404,12 @@ static void meson_nfc_set_data_oob(struct nand_chip *nand,
>>>> memcpy(osrc, oobbuf, oob_len);
>>>> oobbuf += oob_len;
>>>> }
>>>> +
>>>> + oobtail = meson_chip->data_buf + nand->ecc.steps *
>>>> + (nand->ecc.size + oob_len);
>>>
>>> This is always targeting the same area, so it looks strange to me.
>>>
>>>> +
>>>> + /* 'oobbuf' points to the start of user area. */
>>>> + memcpy(oobtail, oobbuf, mtd->oobsize - nand->ecc.steps * oob_len);
>>>
>>> TBH I don't get what you do here.
>>
>> This code works in raw mode and places OOB data from provided OOB buffer to DMA buffer.
>> This is because number of user bytes is increased in this patch.
>>
>>>
>>>> }
>>>>
>>>> static int meson_nfc_queue_rb(struct nand_chip *nand, int timeout_ms)
>>>> @@ -433,7 +452,7 @@ static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
>>>> __le64 *info;
>>>> int i, count;
>>>>
>>>> - for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
>>>> + for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
>>>> info = &meson_chip->info_buf[i];
>>>> *info |= oob_buf[count];
>>>> *info |= oob_buf[count + 1] << 8;
>>>> @@ -446,7 +465,7 @@ static void meson_nfc_get_user_byte(struct nand_chip *nand, u8 *oob_buf)
>>>> __le64 *info;
>>>> int i, count;
>>>>
>>>> - for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
>>>> + for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (nand->ecc.bytes + 2)) {
>>>> info = &meson_chip->info_buf[i];
>>>> oob_buf[count] = *info;
>>>> oob_buf[count + 1] = *info >> 8;
>>>> @@ -638,6 +657,84 @@ static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
>>>> return 0;
>>>> }
>>>>
>>>> +static u32 meson_nfc_oob_free_bytes(struct nand_chip *nand)
>>>> +{
>>>> + struct mtd_info *mtd = nand_to_mtd(nand);
>>>> +
>>>> + return mtd->oobsize - nand->ecc.steps * (nand->ecc.bytes + 2);
>>>
>>> This looks like a static value, just save it somewhere instead of
>>> recomputing it?
>>
>> Ack
>>
>>>
>>>> +}
>>>> +
>>>> +static int meson_nfc_write_oob(struct nand_chip *nand, int page)
>>>> +{
>>>> + struct mtd_info *mtd = nand_to_mtd(nand);
>>>> + u32 page_size = mtd->writesize + mtd->oobsize;
>>>> + u32 oob_bytes = meson_nfc_oob_free_bytes(nand);
>>>> + u8 *oob_buf;
>>>> + int ret;
>>>> +
>>>> + if (!oob_bytes)
>>>> + return 0;
>>>
>>> Can this happen?
>>
>> Ack, seems forget to remove it
>>
>>>
>>>> +
>>>> + /* Called as OOB write helper, will send NAND_CMD_PAGEPROG. */
>>>> + if (page != -1) {
>>>> + ret = nand_prog_page_begin_op(nand, page, 0, NULL, 0);
>>>> + if (ret)
>>>> + return ret;
>>>> + }
>>>> +
>>>> + oob_buf = nand->oob_poi;
>>>> +
>>>> + ret = nand_change_write_column_op(nand, page_size - oob_bytes,
>>>> + oob_buf + (mtd->oobsize - oob_bytes),
>>>> + oob_bytes, false);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + return (page != -1) ? nand_prog_page_end_op(nand) : 0;
>>>> +}
>>>> +
>>>> +static int meson_nfc_read_oob(struct nand_chip *nand, int page)
>>>> +{
>>>> + struct mtd_info *mtd = nand_to_mtd(nand);
>>>> + u8 *oob_buf = nand->oob_poi;
>>>> + u32 oob_bytes;
>>>> + u32 page_size;
>>>> + int ret;
>>>> + int i;
>>>> +
>>>> + /* Called as OOB read helper, send NAND_CMD_READ0. */
>>>> + if (page != -1) {
>>>
>>> I don't like this logic with the "-1", it really hides what the
>>> controller needs to do, if you need a helper to send a command, then
>>> create that helper and give it a decent name.
>>
>> I see, so I think I need to implement this in the following way:
>> 1) For OOB callback it always sends NAND_CMD_READ0 (e.g. without any 'if')
>> 2) For read OOB with data page we don't need to send NAND_CMD_READ0. (also without any 'if')
>>
>>>
>>>> + ret = nand_read_page_op(nand, page, 0, NULL, 0);
>>>> + if (ret)
>>>> + return ret;
>>>> + }
>>>> +
>>>> + /* Read ECC codes and user bytes. */
>>>> + for (i = 0; i < nand->ecc.steps; i++) {
>>>> + u32 ecc_offs = nand->ecc.size * (i + 1) +
>>>> + (nand->ecc.bytes + 2) * i;
>>>> +
>>>> + ret = nand_change_read_column_op(nand, ecc_offs,
>>>> + oob_buf + i * (nand->ecc.bytes + 2),
>>>> + (nand->ecc.bytes + 2), false);
>>>> + if (ret)
>>>> + return ret;
>>>> + }
>>>> +
>>>> + oob_bytes = meson_nfc_oob_free_bytes(nand);
>>>> +
>>>> + if (!oob_bytes)
>>>> + return 0;
>>>> +
>>>> + page_size = mtd->writesize + mtd->oobsize;
>>>> +
>>>> + ret = nand_change_read_column_op(nand, page_size - oob_bytes,
>>>> + oob_buf + (mtd->oobsize - oob_bytes),
>>>> + oob_bytes, false);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> static int meson_nfc_write_page_sub(struct nand_chip *nand,
>>>> int page, int raw)
>>>> {
>>>> @@ -674,6 +771,12 @@ static int meson_nfc_write_page_sub(struct nand_chip *nand,
>>>> NFC_CMD_SCRAMBLER_DISABLE);
>>>> }
>>>>
>>>> + if (!raw) {
>>>
>>> Why this check?
>>>
>>> You should instead propagate the oob_required field and check that
>>> value I believe.
>>
>>
>> This check is for ECC mode, because in this mode we write user bytes of OOB.
>> ECC bytes of OOB are written by hardware.
>
> Just provide the buffer. The ECC engine will smash data if there was
> any there. Otherwise it will fill the holes. It's expected. Don't try
> to be smarter than you should :)
>
>> I think I made a mistake, because
>> I need new callback to write OOB in raw mode - it will write both ECC and user
>> parts,
>
> There is no such thing as user and ECC part at the driver level. You
> get a buffer, you need to write it to the flash.
>
> The user expects:
>
> | data | OOB |
>
> The controller expects something like:
>
> | data1 | OOB1 | data2 | OOB2 |
>
> So just perform the reordering between data and OOB in the DMA buffer,
> that is _all_.
>
>> in current version I write only user part in raw mode.
>>
>>>
>>>> + ret = meson_nfc_write_oob(nand, -1);
>>>> + if (ret)
>>>> + return ret;
>>>> + }
>>>> +
>>>> cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_PAGEPROG;
>>>> writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>> meson_nfc_queue_rb(nand, PSEC_TO_MSEC(sdr->tPROG_max));
>>>> @@ -834,17 +937,10 @@ static int meson_nfc_read_page_hwecc(struct nand_chip *nand, u8 *buf,
>>>> memcpy(buf, meson_chip->data_buf, mtd->writesize);
>>>> }
>>>>
>>>> - return bitflips;
>>>> -}
>>>> -
>>>> -static int meson_nfc_read_oob_raw(struct nand_chip *nand, int page)
>>>> -{
>>>> - return meson_nfc_read_page_raw(nand, NULL, 1, page);
>>>> -}
>>>> + if (oob_required && ret)
>>>
>>> Unclear why you check ret here?
>
> In general, if (ret) means there is an error.
>
> Please consider using:
>
> if (ret)
> goto error path;
>
> do something else;
>
>>>
>>
>> If read was successful, we read OOB. If not - there is no sense in it.
>>
>>>> + meson_nfc_read_oob(nand, -1);
>>>>
>>>> -static int meson_nfc_read_oob(struct nand_chip *nand, int page)
>>>> -{
>>>> - return meson_nfc_read_page_hwecc(nand, NULL, 1, page);
>>>> + return bitflips;
>>>> }
>>>>
>>>> static bool meson_nfc_is_buffer_dma_safe(const void *buffer)
>>>> @@ -987,12 +1083,16 @@ static int meson_ooblayout_free(struct mtd_info *mtd, int section,
>>>> struct mtd_oob_region *oobregion)
>>>> {
>>>> struct nand_chip *nand = mtd_to_nand(mtd);
>>>> + u32 oob_bytes = meson_nfc_oob_free_bytes(nand);
>>>>
>>>> if (section >= nand->ecc.steps)
>>>> return -ERANGE;
>>>>
>>>> - oobregion->offset = section * (2 + nand->ecc.bytes);
>>>
>>> The first two bytes of OOB are reserved for the bad block markers. This
>>> is not related to your controller.
>>
>> I think first two bytes (in fact there are 4 bytes at positions 0, 1, 16 and 17)
>> is considered by hardware as user bytes covered by ECC.
>
> The two first bytes should not be available. They are not "ECC" bytes,
> they are not "free" bytes. None of these two callbacks should give
> access to these two bytes reserved for bad block markers.
>
> Just to be clear: "ECC bytes" as in "meson_ooblayout_ecc" do *not* mean
> "these are the protected bytes". They mean "these are the bytes in OOB
> the hardware ECC engine will use to place its own data to make the
> recovery process work".
>
> Thanks,
> Miquèl
More information about the linux-amlogic
mailing list