Re: [PATCH 4/9] mtd: nand: qcom: fix null pointer access for erased buffer detection
On 2018-04-22 21:55, Miquel Raynal wrote: Hi Abhishek, On Thu, 12 Apr 2018 12:24:16 +0530, Abhishek Sahu wrote: On 2018-04-10 14:42, Miquel Raynal wrote: > Hi Abhishek, > > On Wed, 4 Apr 2018 18:12:20 +0530, Abhishek Sahu > 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 >> --- >> 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
Re: [PATCH 4/9] mtd: nand: qcom: fix null pointer access for erased buffer detection
Hi Abhishek, On Thu, 12 Apr 2018 12:24:16 +0530, Abhishek Sahu wrote: > On 2018-04-10 14:42, Miquel Raynal wrote: > > Hi Abhishek, > > > On Wed, 4 Apr 2018 18:12:20 +0530, Abhishek Sahu > > 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 > >> --- > >> 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
Re: [PATCH 4/9] mtd: nand: qcom: fix null pointer access for erased buffer detection
On 2018-04-10 14:42, Miquel Raynal wrote: Hi Abhishek, On Wed, 4 Apr 2018 18:12:20 +0530, Abhishek Sahu 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 --- 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
Re: [PATCH 4/9] mtd: nand: qcom: fix null pointer access for erased buffer detection
Hi Abhishek, On Wed, 4 Apr 2018 18:12:20 +0530, Abhishek Sahu 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 > --- > 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 -- Miquel Raynal, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
[PATCH 4/9] mtd: nand: qcom: fix null pointer access for erased buffer detection
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 --- 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; - } 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; } -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation