Patchwork [2/2] mtd: nand: nand_do_{read, write}_ops - pass OOB buffer through

login
register
mail settings
Submitter Brian Norris
Date April 16, 2012, 10:35 p.m.
Message ID <1334615755-15418-3-git-send-email-computersforpeace@gmail.com>
Download mbox | patch
Permalink /patch/153004/
State New
Headers show

Comments

Brian Norris - April 16, 2012, 10:35 p.m.
Now that we have a function parameter for the OOB buffer, we can pass the OOB
buffer as an argument to the nand_ecc_ctrl functions. This allows drivers to
know when OOB data must be returned to the upper layers and when it is simply
needed for internal calculations, potentially saving time for NAND HW/SW that
can simply avoid reading the OOB data.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/nand/nand_base.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)
Shmulik Ladkani - April 18, 2012, 11:52 a.m.
Hi Brian,

On Mon, 16 Apr 2012 15:35:55 -0700 Brian Norris <computersforpeace@gmail.com> wrote:
> Now that we have a function parameter for the OOB buffer, we can pass the OOB
> buffer as an argument to the nand_ecc_ctrl functions. This allows drivers to
> know when OOB data must be returned to the upper layers and when it is simply
> needed for internal calculations, potentially saving time for NAND HW/SW that
> can simply avoid reading the OOB data.

I think for consistency sake, existing chip->ecc.{read,write}_page_xxx
methods do need to be ported to support the new 'oob' parameter.

> @@ -2272,12 +2272,14 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to,
>  			size_t len = min(oobwritelen, oobmaxlen);
>  			oob = nand_fill_oob(mtd, oob, len, ops);
>  			oobwritelen -= len;
> +			oobpoi = chip->oob_poi;
>  		} else {
> +			oobpoi = NULL;
>  			/* We still need to erase leftover OOB data */
>  			memset(chip->oob_poi, 0xff, mtd->oobsize);
>  		}
>  
> -		ret = chip->write_page(mtd, chip, wbuf, NULL, page, cached,
> +		ret = chip->write_page(mtd, chip, wbuf, oobpoi, page, cached,
>  				       (ops->mode == MTD_OPS_RAW));
>  		if (ret)
>  			break;

The 'write_page' interface is problematic, as the meaning of 'oob'
parameter is a bit inconsistent:
- A NULL 'oob' actually states "no OOB buffer to write"
- Your driver instructs HW to write the page (ECC taken care of by HW)
- However default chip->ecc.write_page_xxx methods do need a temp buffer
  for OOB ECC calculation (hence will probably use the internal
  chip->oob_poi buffer)
- But when non-null 'oob' is passed to the default methods, they should
  probably use the given 'oob' buffer (and not a temp buffer)

(This is same for the read interface.)

So the 'oob' parameter is more of a boolean than an actual buffer to be
used by the various ecc.{read,write}_page implementors.

Any reason not to pass a boolean instead?

Regards,
Shmulik
Brian Norris - April 18, 2012, 4:13 p.m.
On 4/18/2012 4:52 AM, Shmulik Ladkani wrote:
> On Mon, 16 Apr 2012 15:35:55 -0700 Brian Norris<computersforpeace@gmail.com>  wrote:
>> Now that we have a function parameter for the OOB buffer, we can pass the OOB
>> buffer as an argument to the nand_ecc_ctrl functions. This allows drivers to
>> know when OOB data must be returned to the upper layers and when it is simply
>> needed for internal calculations, potentially saving time for NAND HW/SW that
>> can simply avoid reading the OOB data.
>
> I think for consistency sake, existing chip->ecc.{read,write}_page_xxx
> methods do need to be ported to support the new 'oob' parameter.

OK, but it's difficult to tell sometimes what is and isn't needed; some 
drivers might expect OOB data in chip->oob_poi unconditionally so they 
can perform correction, whereas others might fill up buffers that won't 
be used in the end.

>> @@ -2272,12 +2272,14 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to,
>>   			size_t len = min(oobwritelen, oobmaxlen);
>>   			oob = nand_fill_oob(mtd, oob, len, ops);
>>   			oobwritelen -= len;
>> +			oobpoi = chip->oob_poi;
>>   		} else {
>> +			oobpoi = NULL;
>>   			/* We still need to erase leftover OOB data */
>>   			memset(chip->oob_poi, 0xff, mtd->oobsize);
>>   		}
>>
>> -		ret = chip->write_page(mtd, chip, wbuf, NULL, page, cached,
>> +		ret = chip->write_page(mtd, chip, wbuf, oobpoi, page, cached,
>>   				       (ops->mode == MTD_OPS_RAW));
>>   		if (ret)
>>   			break;
>
> The 'write_page' interface is problematic, as the meaning of 'oob'
> parameter is a bit inconsistent:
> - A NULL 'oob' actually states "no OOB buffer to write"
> - Your driver instructs HW to write the page (ECC taken care of by HW)
> - However default chip->ecc.write_page_xxx methods do need a temp buffer
>    for OOB ECC calculation (hence will probably use the internal
>    chip->oob_poi buffer)

Right, this is a trouble spot for 'porting them to support the new oob 
parameter', since many driver-users still need a buffer even when OOB is 
not needed for the higher levels.

> - But when non-null 'oob' is passed to the default methods, they should
>    probably use the given 'oob' buffer (and not a temp buffer)

Yes. This gets strange and potentially ugly, with code snippets like below.

> (This is same for the read interface.)
>
> So the 'oob' parameter is more of a boolean than an actual buffer to be
> used by the various ecc.{read,write}_page implementors.

Yes, I suppose so. I naturally used 'oob' as a buffer, since that's very 
straightforward and logical from a 'layers' perspective and because my 
driver doesn't need any buffer when oob is not required. But I see that 
it essentially would become a boolean flag for many of the other 
interfaces, and so a boolean can work just as well.

> Any reason not to pass a boolean instead?

Only reason I'm thinking of: a cleaner interface.

To me, the interface is rather non-obvious and ugly when data is 
constantly shuttled back and forth behind the scenes (i.e., not via 
function arguments or ret values) by using chip->oob_poi.

However, this sense of "ugliness" competes with the ugliness of needing 
a buffer even when the interface might otherwise say "no OOB." Many 
{read,write}_page functions would need something like:

     uint8_t *oobbuf = oob ? oob : chip->oob_poi;

which is not pretty.

I'm open to either way, I guess, but I'm now leaning a little toward 
'oob' as a boolean.

Brian
Shmulik Ladkani - April 18, 2012, 7:43 p.m.
Hi Brian,

On Wed, 18 Apr 2012 09:13:38 -0700 Brian Norris <computersforpeace@gmail.com> wrote:
> > So the 'oob' parameter is more of a boolean than an actual buffer to be
> > used by the various ecc.{read,write}_page implementors.
> 
> Yes, I suppose so. I naturally used 'oob' as a buffer, since that's very 
> straightforward and logical from a 'layers' perspective and because my 
> driver doesn't need any buffer when oob is not required. But I see that 
> it essentially would become a boolean flag for many of the other 
> interfaces, and so a boolean can work just as well.
> 
> > Any reason not to pass a boolean instead?
> 
> Only reason I'm thinking of: a cleaner interface.
> 
> To me, the interface is rather non-obvious and ugly when data is 
> constantly shuttled back and forth behind the scenes (i.e., not via 
> function arguments or ret values) by using chip->oob_poi.
> 
> However, this sense of "ugliness" competes with the ugliness of needing 
> a buffer even when the interface might otherwise say "no OOB." Many 
> {read,write}_page functions would need something like:
> 
>      uint8_t *oobbuf = oob ? oob : chip->oob_poi;
> 
> which is not pretty.
> 
> I'm open to either way, I guess, but I'm now leaning a little toward 
> 'oob' as a boolean.

Yes, both approaches aren't perfect.

However a 'int has_user_oob' (or 'has_oob') has a precise, consistent
meaning: the mtd user explicitly provided an OOB buffer (or in the read
case, the mtd user expects the OOB to be returned).
(need help with the boolean's name, though)

Also, I think it will result in less changes (makes the whole
s/oob/oobbuf/ collateral damage of 1st patch irrelevant).

So IMO a boolean is better.

Regards,
Shmulik

Patch

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 95ba987..a206f43 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1475,7 +1475,7 @@  static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 	uint32_t max_oobsize = ops->mode == MTD_OPS_AUTO_OOB ?
 		mtd->oobavail : mtd->oobsize;
 
-	uint8_t *bufpoi, *oob, *buf;
+	uint8_t *bufpoi, *oobpoi, *oob, *buf;
 
 	stats = mtd->ecc_stats;
 
@@ -1489,6 +1489,7 @@  static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 
 	buf = ops->datbuf;
 	oob = ops->oobbuf;
+	oobpoi = oob ? chip->oob_poi : NULL;
 
 	while (1) {
 		bytes = min(mtd->writesize - col, readlen);
@@ -1506,13 +1507,13 @@  static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 			/* Now read the page into the buffer */
 			if (unlikely(ops->mode == MTD_OPS_RAW))
 				ret = chip->ecc.read_page_raw(mtd, chip, bufpoi,
-							      NULL, page);
+							      oobpoi, page);
 			else if (!aligned && NAND_SUBPAGE_READ(chip) && !oob)
 				ret = chip->ecc.read_subpage(mtd, chip,
 							col, bytes, bufpoi);
 			else
 				ret = chip->ecc.read_page(mtd, chip, bufpoi,
-							  NULL, page);
+							  oobpoi, page);
 			if (ret < 0) {
 				if (!aligned)
 					/* Invalidate page cache */
@@ -1535,7 +1536,6 @@  static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 			buf += bytes;
 
 			if (unlikely(oob)) {
-
 				int toread = min(oobreadlen, max_oobsize);
 
 				if (toread) {
@@ -2256,7 +2256,7 @@  static int nand_do_write_ops(struct mtd_info *mtd, loff_t to,
 	while (1) {
 		int bytes = mtd->writesize;
 		int cached = writelen > bytes && page != blockmask;
-		uint8_t *wbuf = buf;
+		uint8_t *wbuf = buf, *oobpoi;
 
 		/* Partial page write? */
 		if (unlikely(column || writelen < (mtd->writesize - 1))) {
@@ -2272,12 +2272,14 @@  static int nand_do_write_ops(struct mtd_info *mtd, loff_t to,
 			size_t len = min(oobwritelen, oobmaxlen);
 			oob = nand_fill_oob(mtd, oob, len, ops);
 			oobwritelen -= len;
+			oobpoi = chip->oob_poi;
 		} else {
+			oobpoi = NULL;
 			/* We still need to erase leftover OOB data */
 			memset(chip->oob_poi, 0xff, mtd->oobsize);
 		}
 
-		ret = chip->write_page(mtd, chip, wbuf, NULL, page, cached,
+		ret = chip->write_page(mtd, chip, wbuf, oobpoi, page, cached,
 				       (ops->mode == MTD_OPS_RAW));
 		if (ret)
 			break;