| Message ID | 1522845745-6624-5-git-send-email-absahu@codeaurora.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Update for QCOM NAND driver | expand |
Hi Abhishek, On Wed, 4 Apr 2018 18:12:20 +0530, Abhishek Sahu <absahu@codeaurora.org> wrote: > parse_read_errors can be called with only oob buf also in which > case data_buf will be NULL. If data_buf is NULL, then don’t > treat this page as completely erased in case of ECC uncorrectable > error. > > Signed-off-by: Abhishek Sahu <absahu@codeaurora.org> > --- > drivers/mtd/nand/qcom_nandc.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/nand/qcom_nandc.c b/drivers/mtd/nand/qcom_nandc.c > index 57c16a6..0ebcc55 100644 > --- a/drivers/mtd/nand/qcom_nandc.c > +++ b/drivers/mtd/nand/qcom_nandc.c > @@ -1607,9 +1607,11 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf, > if (host->bch_enabled) { > erased = (erased_cw & ERASED_CW) == ERASED_CW ? > true : false; Why the parse_read_errors() function could not be called without data_buf when using BCH? Are you sure the situation can only happen without it? Would the following apply here too, with a: if (!data_buf) { erased = false; } else { if (host->bch_enabled) ... else ... } > - } else { > + } else if (data_buf) { > erased = erased_chunk_check_and_fixup(data_buf, > data_len); > + } else { > + erased = false; > } > > if (erased) { > @@ -1652,7 +1654,8 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf, > max_bitflips = max(max_bitflips, stat); > } > > - data_buf += data_len; > + if (data_buf) > + data_buf += data_len; > if (oob_buf) > oob_buf += oob_len + ecc->bytes; > } Thanks, Miquèl
On 2018-04-10 14:42, Miquel Raynal wrote: > Hi Abhishek, > > On Wed, 4 Apr 2018 18:12:20 +0530, Abhishek Sahu > <absahu@codeaurora.org> wrote: > >> parse_read_errors can be called with only oob buf also in which >> case data_buf will be NULL. If data_buf is NULL, then don’t >> treat this page as completely erased in case of ECC uncorrectable >> error. >> >> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org> >> --- >> drivers/mtd/nand/qcom_nandc.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mtd/nand/qcom_nandc.c >> b/drivers/mtd/nand/qcom_nandc.c >> index 57c16a6..0ebcc55 100644 >> --- a/drivers/mtd/nand/qcom_nandc.c >> +++ b/drivers/mtd/nand/qcom_nandc.c >> @@ -1607,9 +1607,11 @@ static int parse_read_errors(struct >> qcom_nand_host *host, u8 *data_buf, >> if (host->bch_enabled) { >> erased = (erased_cw & ERASED_CW) == ERASED_CW ? >> true : false; > > Why the parse_read_errors() function could not be called without > data_buf when using BCH? Are you sure the situation can only happen > without it? > host->bch_enabled case is different where controller itself tells regarding erased page in status register. > Would the following apply here too, with a: > erased_chunk_check_and_fixup will be used only for 4 bit RS ECC code in which there is no support from HW for erased page detection and we need to check few data bytes value. Thanks, Abhishek > if (!data_buf) { > erased = false; > } else { > if (host->bch_enabled) > ... > else > ... > } > >> - } else { >> + } else if (data_buf) { >> erased = erased_chunk_check_and_fixup(data_buf, >> data_len); >> + } else { >> + erased = false; >> } >> >> if (erased) { >> @@ -1652,7 +1654,8 @@ static int parse_read_errors(struct >> qcom_nand_host *host, u8 *data_buf, >> max_bitflips = max(max_bitflips, stat); >> } >> >> - data_buf += data_len; >> + if (data_buf) >> + data_buf += data_len; >> if (oob_buf) >> oob_buf += oob_len + ecc->bytes; >> } > > Thanks, > Miquèl
Hi Abhishek, On Thu, 12 Apr 2018 12:24:16 +0530, Abhishek Sahu <absahu@codeaurora.org> wrote: > On 2018-04-10 14:42, Miquel Raynal wrote: > > Hi Abhishek, > > > On Wed, 4 Apr 2018 18:12:20 +0530, Abhishek Sahu > > <absahu@codeaurora.org> wrote: > > >> parse_read_errors can be called with only oob buf also in which > >> case data_buf will be NULL. If data_buf is NULL, then don’t > >> treat this page as completely erased in case of ECC uncorrectable > >> error. > >> >> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org> > >> --- > >> drivers/mtd/nand/qcom_nandc.c | 7 +++++-- > >> 1 file changed, 5 insertions(+), 2 deletions(-) > >> >> diff --git a/drivers/mtd/nand/qcom_nandc.c >> b/drivers/mtd/nand/qcom_nandc.c > >> index 57c16a6..0ebcc55 100644 > >> --- a/drivers/mtd/nand/qcom_nandc.c > >> +++ b/drivers/mtd/nand/qcom_nandc.c > >> @@ -1607,9 +1607,11 @@ static int parse_read_errors(struct >> qcom_nand_host *host, u8 *data_buf, > >> if (host->bch_enabled) { > >> erased = (erased_cw & ERASED_CW) == ERASED_CW ? > >> true : false; > > > Why the parse_read_errors() function could not be called without > > data_buf when using BCH? Are you sure the situation can only happen > > without it? > > > host->bch_enabled case is different where controller itself tells > regarding erased page in status register. > > > Would the following apply here too, with a: > > > erased_chunk_check_and_fixup will be used only for 4 bit RS ECC > code in which there is no support from HW for erased page detection > and we need to check few data bytes value. So please explain this with a comment. Thanks, Miquèl
On 2018-04-22 21:55, Miquel Raynal wrote: > Hi Abhishek, > > On Thu, 12 Apr 2018 12:24:16 +0530, Abhishek Sahu > <absahu@codeaurora.org> wrote: > >> On 2018-04-10 14:42, Miquel Raynal wrote: >> > Hi Abhishek, >> > > On Wed, 4 Apr 2018 18:12:20 +0530, Abhishek Sahu >> > <absahu@codeaurora.org> wrote: >> > >> parse_read_errors can be called with only oob buf also in which >> >> case data_buf will be NULL. If data_buf is NULL, then don’t >> >> treat this page as completely erased in case of ECC uncorrectable >> >> error. >> >> >> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org> >> >> --- >> >> drivers/mtd/nand/qcom_nandc.c | 7 +++++-- >> >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> >> diff --git a/drivers/mtd/nand/qcom_nandc.c >> b/drivers/mtd/nand/qcom_nandc.c >> >> index 57c16a6..0ebcc55 100644 >> >> --- a/drivers/mtd/nand/qcom_nandc.c >> >> +++ b/drivers/mtd/nand/qcom_nandc.c >> >> @@ -1607,9 +1607,11 @@ static int parse_read_errors(struct >> qcom_nand_host *host, u8 *data_buf, >> >> if (host->bch_enabled) { >> >> erased = (erased_cw & ERASED_CW) == ERASED_CW ? >> >> true : false; >> > > Why the parse_read_errors() function could not be called without >> > data_buf when using BCH? Are you sure the situation can only happen >> > without it? >> > >> host->bch_enabled case is different where controller itself tells >> regarding erased page in status register. >> >> > Would the following apply here too, with a: >> > >> erased_chunk_check_and_fixup will be used only for 4 bit RS ECC >> code in which there is no support from HW for erased page detection >> and we need to check few data bytes value. > > So please explain this with a comment. > > Thanks, > Miquèl Sure Miquel. I will do the same and update the patch with more comments. Thanks, Abhishek
diff --git a/drivers/mtd/nand/qcom_nandc.c b/drivers/mtd/nand/qcom_nandc.c index 57c16a6..0ebcc55 100644 --- a/drivers/mtd/nand/qcom_nandc.c +++ b/drivers/mtd/nand/qcom_nandc.c @@ -1607,9 +1607,11 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf, if (host->bch_enabled) { erased = (erased_cw & ERASED_CW) == ERASED_CW ? true : false; - } else { + } else if (data_buf) { erased = erased_chunk_check_and_fixup(data_buf, data_len); + } else { + erased = false; } if (erased) { @@ -1652,7 +1654,8 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf, max_bitflips = max(max_bitflips, stat); } - data_buf += data_len; + if (data_buf) + data_buf += data_len; if (oob_buf) oob_buf += oob_len + ecc->bytes; }
parse_read_errors can be called with only oob buf also in which case data_buf will be NULL. If data_buf is NULL, then don’t treat this page as completely erased in case of ECC uncorrectable error. Signed-off-by: Abhishek Sahu <absahu@codeaurora.org> --- drivers/mtd/nand/qcom_nandc.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)