[v3,07/10] mtd: IFC NAND: utilize oob_required parameter
diff mbox

Message ID 1335576594-25267-8-git-send-email-computersforpeace@gmail.com
State New, archived
Headers show

Commit Message

Brian Norris April 28, 2012, 1:29 a.m. UTC
Don't read/write OOB if the caller doesn't requre it.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/nand/fsl_ifc_nand.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

Comments

Scott Wood April 30, 2012, 4:43 p.m. UTC | #1
On 04/27/2012 08:29 PM, Brian Norris wrote:
> @@ -717,7 +718,8 @@ static void fsl_ifc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
>  			       const uint8_t *buf, int oob_required)
>  {
>  	fsl_ifc_write_buf(mtd, buf, mtd->writesize);
> -	fsl_ifc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
> +	if (oob_required)
> +		fsl_ifc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
>  }

This will result in writing junk to the non-ECC OOB bytes as opposed to
leaving it alone.

-Scott
Brian Norris April 30, 2012, 7:08 p.m. UTC | #2
On Mon, Apr 30, 2012 at 9:43 AM, Scott Wood <scottwood@freescale.com> wrote:
> On 04/27/2012 08:29 PM, Brian Norris wrote:
>> @@ -717,7 +718,8 @@ static void fsl_ifc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
>>                              const uint8_t *buf, int oob_required)
>>  {
>>       fsl_ifc_write_buf(mtd, buf, mtd->writesize);
>> -     fsl_ifc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
>> +     if (oob_required)
>> +             fsl_ifc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
>>  }
>
> This will result in writing junk to the non-ECC OOB bytes as opposed to
> leaving it alone.

Then I'll drop the write_page change from this patch.

Is the read_page change sane?

Did you review the (misspelled) eLBC patch? (patch 06/10)

Thanks.

Brian
Scott Wood April 30, 2012, 7:13 p.m. UTC | #3
On 04/30/2012 02:08 PM, Brian Norris wrote:
> On Mon, Apr 30, 2012 at 9:43 AM, Scott Wood <scottwood@freescale.com> wrote:
>> On 04/27/2012 08:29 PM, Brian Norris wrote:
>>> @@ -717,7 +718,8 @@ static void fsl_ifc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
>>>                              const uint8_t *buf, int oob_required)
>>>  {
>>>       fsl_ifc_write_buf(mtd, buf, mtd->writesize);
>>> -     fsl_ifc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
>>> +     if (oob_required)
>>> +             fsl_ifc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
>>>  }
>>
>> This will result in writing junk to the non-ECC OOB bytes as opposed to
>> leaving it alone.
> 
> Then I'll drop the write_page change from this patch.
> 
> Is the read_page change sane?

It should be harmless.

> Did you review the (misspelled) eLBC patch? (patch 06/10)

No, that one wasn't CCed to me (I should probably get around to
subscribing to linux-mtd...).  It looks like the same situation as IFC.

-Scott
Brian Norris April 30, 2012, 7:23 p.m. UTC | #4
On Mon, Apr 30, 2012 at 12:13 PM, Scott Wood <scottwood@freescale.com> wrote:
> On 04/30/2012 02:08 PM, Brian Norris wrote:
>> Is the read_page change sane?
>
> It should be harmless.

Right, but I am now handling Shmulik's comments about auto-incremented
NAND. It seems like there are few/no users of auto-incrementing page
reads, but if any driver relied on reading page,oob,page,oob,etc.
without a READ command in between, then we might cause problems by
skipping the OOB buffer read. Of course, with various NAND
controllers/drivers, this issue is hard for me to sort through.

>> Did you review the (misspelled) eLBC patch? (patch 06/10)
>
> No, that one wasn't CCed to me (I should probably get around to
> subscribing to linux-mtd...).

scripts/get_maintainers.pl only goes so far. It's hard to track down
the *real* experts here...

> It looks like the same situation as IFC.

OK, I'll fix it too then.

Thanks.

Brian
Scott Wood April 30, 2012, 7:32 p.m. UTC | #5
On 04/30/2012 02:23 PM, Brian Norris wrote:
> On Mon, Apr 30, 2012 at 12:13 PM, Scott Wood <scottwood@freescale.com> wrote:
>> On 04/30/2012 02:08 PM, Brian Norris wrote:
>>> Is the read_page change sane?
>>
>> It should be harmless.
> 
> Right, but I am now handling Shmulik's comments about auto-incremented
> NAND. It seems like there are few/no users of auto-incrementing page
> reads, but if any driver relied on reading page,oob,page,oob,etc.
> without a READ command in between, then we might cause problems by
> skipping the OOB buffer read. Of course, with various NAND
> controllers/drivers, this issue is hard for me to sort through.

This isn't an issue with eLBC/IFC -- besides not currently supporting
autoincrement, the OOB is still read.  We just wouldn't pull it out of
the controller buffer.  Not sure if the time saved is significant.

-Scott

Patch
diff mbox

diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
index a8d1e87..2bbb9d5 100644
--- a/drivers/mtd/nand/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/fsl_ifc_nand.c
@@ -699,7 +699,8 @@  static int fsl_ifc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 	struct fsl_ifc_ctrl *ctrl = priv->ctrl;
 
 	fsl_ifc_read_buf(mtd, buf, mtd->writesize);
-	fsl_ifc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
+	if (oob_required)
+		fsl_ifc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
 
 	if (ctrl->nand_stat & IFC_NAND_EVTER_STAT_ECCER)
 		dev_err(priv->dev, "NAND Flash ECC Uncorrectable Error\n");
@@ -717,7 +718,8 @@  static void fsl_ifc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
 			       const uint8_t *buf, int oob_required)
 {
 	fsl_ifc_write_buf(mtd, buf, mtd->writesize);
-	fsl_ifc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
+	if (oob_required)
+		fsl_ifc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
 }
 
 static int fsl_ifc_chip_init_tail(struct mtd_info *mtd)