[PATCH v4] mtd: nand: Add support to use nand_base poi databuf as bounce buffer

[PATCH v4] mtd: nand: Add support to use nand_base poi databuf as bounce buffer

[Kamal Dasu]

nand_base can be passed a kmap()'d buffers from highmem by
filesystems like jffs2. This results in failure to map the
physical address of the DMA buffer on various contoller
driver on different platforms. This change adds a chip option
to use preallocated databuf as bounce buffers used in
nand_do_read_ops() and nand_do_write_ops().
This allows for specific nand controller driver to set this
option as needed.

Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
---

Changes since v3 :
* print the right highmem buffer address in pr_debug in
  nand_do_read_ops 

 drivers/mtd/nand/nand_base.c | 31 ++++++++++++++++++++++++-------
 include/linux/mtd/nand.h     |  5 +++++
 2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 9d01c4d..69ef5d9 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -37,6 +37,7 @@
 #include <linux/err.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
+#include <linux/mm.h>
 #include <linux/types.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/nand.h>
@@ -1501,6 +1502,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 		mtd->oobavail : mtd->oobsize;
 
 	uint8_t *bufpoi, *oob, *buf;
+	int use_bufpoi;
 	unsigned int max_bitflips = 0;
 	int retry_mode = 0;
 	bool ecc_fail = false;
@@ -1522,10 +1524,17 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 
 		bytes = min(mtd->writesize - col, readlen);
 		aligned = (bytes == mtd->writesize);
+		use_bufpoi = (chip->options & NAND_USE_BOUNCE_BUFFER) ?
+				!virt_addr_valid(buf) : 0;
 
 		/* Is the current page in the buffer? */
 		if (realpage != chip->pagebuf || oob) {
-			bufpoi = aligned ? buf : chip->buffers->databuf;
+			bufpoi = (aligned && !use_bufpoi) ? buf :
+				chip->buffers->databuf;
+
+			if (use_bufpoi && aligned)
+				pr_debug("%s: using read bounce buffer for buf@%p\n",
+						 __func__, buf);
 
 read_retry:
 			chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
@@ -1547,7 +1556,7 @@ read_retry:
 				ret = chip->ecc.read_page(mtd, chip, bufpoi,
 							  oob_required, page);
 			if (ret < 0) {
-				if (!aligned)
+				if (!aligned || use_bufpoi)
 					/* Invalidate page cache */
 					chip->pagebuf = -1;
 				break;
@@ -1556,7 +1565,7 @@ read_retry:
 			max_bitflips = max_t(unsigned int, max_bitflips, ret);
 
 			/* Transfer not aligned data */
-			if (!aligned) {
+			if (!aligned || use_bufpoi) {
 				if (!NAND_HAS_SUBPAGE_READ(chip) && !oob &&
 				    !(mtd->ecc_stats.failed - ecc_failures) &&
 				    (ops->mode != MTD_OPS_RAW)) {
@@ -2376,11 +2385,19 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to,
 		int bytes = mtd->writesize;
 		int cached = writelen > bytes && page != blockmask;
 		uint8_t *wbuf = buf;
-
-		/* Partial page write? */
-		if (unlikely(column || writelen < (mtd->writesize - 1))) {
+		int use_bufpoi = (chip->options & NAND_USE_BOUNCE_BUFFER ?
+				  !virt_addr_valid(buf) : 0);
+		int part_pagewr = unlikely(column ||
+					   writelen < (mtd->writesize - 1));
+
+		/* Partial page write?, or need to use bounce buffer */
+		if (part_pagewr || use_bufpoi) {
+			pr_debug("%s: using write bounce buffer for buf@%p\n",
+					 __func__, buf);
 			cached = 0;
-			bytes = min_t(int, bytes - column, (int) writelen);
+			if (part_pagewr)
+				bytes = min_t(int,
+					      bytes - column, (int) writelen);
 			chip->pagebuf = -1;
 			memset(chip->buffers->databuf, 0xff, mtd->writesize);
 			memcpy(&chip->buffers->databuf[column], buf, bytes);
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 450d61e..bb3e064 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -176,6 +176,11 @@ typedef enum {
 /* Chip may not exist, so silence any errors in scan */
 #define NAND_SCAN_SILENT_NODEV	0x00040000
 /*
+ * This option is defined to protect against kmapped buffers
+ * being passed from highmem when using DMA
+ */
+#define NAND_USE_BOUNCE_BUFFER	0x00080000
+/*
  * Autodetect nand buswidth with readid/onfi.
  * This suppose the driver will configure the hardware in 8 bits mode
  * when calling nand_scan_ident, and update its configuration
-- 
1.9.2

Re: [PATCH v4] mtd: nand: Add support to use nand_base poi databuf as bounce buffer

[Brian Norris]

On Mon, Apr 28, 2014 at 04:27:51PM -0400, Kamal Dasu wrote:
> nand_base can be passed a kmap()'d buffers from highmem by
> filesystems like jffs2. This results in failure to map the
> physical address of the DMA buffer on various contoller
> driver on different platforms. This change adds a chip option
> to use preallocated databuf as bounce buffers used in
> nand_do_read_ops() and nand_do_write_ops().
> This allows for specific nand controller driver to set this
> option as needed.
> 
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> ---
> 
> Changes since v3 :
> * print the right highmem buffer address in pr_debug in
>   nand_do_read_ops 
> 
>  drivers/mtd/nand/nand_base.c | 31 ++++++++++++++++++++++++-------
>  include/linux/mtd/nand.h     |  5 +++++
>  2 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 9d01c4d..69ef5d9 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -37,6 +37,7 @@
>  #include <linux/err.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
> +#include <linux/mm.h>
>  #include <linux/types.h>
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/nand.h>
> @@ -1501,6 +1502,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
>  		mtd->oobavail : mtd->oobsize;
>  
>  	uint8_t *bufpoi, *oob, *buf;
> +	int use_bufpoi;
>  	unsigned int max_bitflips = 0;
>  	int retry_mode = 0;
>  	bool ecc_fail = false;
> @@ -1522,10 +1524,17 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
>  
>  		bytes = min(mtd->writesize - col, readlen);
>  		aligned = (bytes == mtd->writesize);
> +		use_bufpoi = (chip->options & NAND_USE_BOUNCE_BUFFER) ?
> +				!virt_addr_valid(buf) : 0;
>  
>  		/* Is the current page in the buffer? */
>  		if (realpage != chip->pagebuf || oob) {
> -			bufpoi = aligned ? buf : chip->buffers->databuf;
> +			bufpoi = (aligned && !use_bufpoi) ? buf :
> +				chip->buffers->databuf;
> +
> +			if (use_bufpoi && aligned)
> +				pr_debug("%s: using read bounce buffer for buf@%p\n",
> +						 __func__, buf);
>  
>  read_retry:
>  			chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
> @@ -1547,7 +1556,7 @@ read_retry:
>  				ret = chip->ecc.read_page(mtd, chip, bufpoi,
>  							  oob_required, page);
>  			if (ret < 0) {
> -				if (!aligned)
> +				if (!aligned || use_bufpoi)

It could help readability a lot if this was just a single condition:
	if (use_bufpoi)

We can compute 'use_bufpoi' like this earlier:

	if (!aligned)
		use_bufpoi = 1;
	else if (chip->options & NAND_USE_BOUNCE_BUFFER)
		use_bufpoi = !virt_addr_valid(buf);
	else
		use_bufpoi = 0;


>  					/* Invalidate page cache */
>  					chip->pagebuf = -1;
>  				break;
> @@ -1556,7 +1565,7 @@ read_retry:
>  			max_bitflips = max_t(unsigned int, max_bitflips, ret);
>  
>  			/* Transfer not aligned data */
> -			if (!aligned) {
> +			if (!aligned || use_bufpoi) {
>  				if (!NAND_HAS_SUBPAGE_READ(chip) && !oob &&
>  				    !(mtd->ecc_stats.failed - ecc_failures) &&
>  				    (ops->mode != MTD_OPS_RAW)) {
> @@ -2376,11 +2385,19 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to,
>  		int bytes = mtd->writesize;
>  		int cached = writelen > bytes && page != blockmask;
>  		uint8_t *wbuf = buf;
> -
> -		/* Partial page write? */
> -		if (unlikely(column || writelen < (mtd->writesize - 1))) {
> +		int use_bufpoi = (chip->options & NAND_USE_BOUNCE_BUFFER ?
> +				  !virt_addr_valid(buf) : 0);
> +		int part_pagewr = unlikely(column ||
> +					   writelen < (mtd->writesize - 1));

I'm pretty sure the 'unlikely' has no effect here, since you're not
doing the branch here. If you really wanted to "optimize" like that, you
need to put it in the 'if' below. But really, it's totally unnecessary
IMO.

And again, I think it makes sense to modify 'use_bufpoi' like this:

	if (partial)
		use_bufpoi = 1;
	else if (chip->options & NAND_USE_BOUNCE_BUFFER)
		use_bufpoi = !virt_addr_valid(buf);
	else
		use_bufpoi = 0;

I think the clarity is a lot better than the ?: operators, plus 2
different variables.

> +
> +		/* Partial page write?, or need to use bounce buffer */
> +		if (part_pagewr || use_bufpoi) {
> +			pr_debug("%s: using write bounce buffer for buf@%p\n",
> +					 __func__, buf);
>  			cached = 0;
> -			bytes = min_t(int, bytes - column, (int) writelen);
> +			if (part_pagewr)
> +				bytes = min_t(int,
> +					      bytes - column, (int) writelen);

The (int) cast for writelen is unnecessary in the first place, and you
can save some indentation by dropping it.

>  			chip->pagebuf = -1;
>  			memset(chip->buffers->databuf, 0xff, mtd->writesize);
>  			memcpy(&chip->buffers->databuf[column], buf, bytes);
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 450d61e..bb3e064 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -176,6 +176,11 @@ typedef enum {
>  /* Chip may not exist, so silence any errors in scan */
>  #define NAND_SCAN_SILENT_NODEV	0x00040000
>  /*
> + * This option is defined to protect against kmapped buffers
> + * being passed from highmem when using DMA
> + */
> +#define NAND_USE_BOUNCE_BUFFER	0x00080000
> +/*
>   * Autodetect nand buswidth with readid/onfi.
>   * This suppose the driver will configure the hardware in 8 bits mode
>   * when calling nand_scan_ident, and update its configuration

Brian

Re: [PATCH v4] mtd: nand: Add support to use nand_base poi databuf as bounce buffer

[Huang Shijie]

On Mon, Apr 28, 2014 at 04:27:51PM -0400, Kamal Dasu wrote:
> nand_base can be passed a kmap()'d buffers from highmem by
> filesystems like jffs2. This results in failure to map the
> physical address of the DMA buffer on various contoller
> driver on different platforms. This change adds a chip option
> to use preallocated databuf as bounce buffers used in
> nand_do_read_ops() and nand_do_write_ops().
> This allows for specific nand controller driver to set this
> option as needed.
> 
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> ---
> 
> Changes since v3 :
> * print the right highmem buffer address in pr_debug in
>   nand_do_read_ops 
> 
>  drivers/mtd/nand/nand_base.c | 31 ++++++++++++++++++++++++-------
>  include/linux/mtd/nand.h     |  5 +++++
>  2 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 9d01c4d..69ef5d9 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -37,6 +37,7 @@
>  #include <linux/err.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
> +#include <linux/mm.h>
>  #include <linux/types.h>
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/nand.h>
> @@ -1501,6 +1502,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
>  		mtd->oobavail : mtd->oobsize;
>  
>  	uint8_t *bufpoi, *oob, *buf;
> +	int use_bufpoi;
>  	unsigned int max_bitflips = 0;
>  	int retry_mode = 0;
>  	bool ecc_fail = false;
> @@ -1522,10 +1524,17 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
>  
>  		bytes = min(mtd->writesize - col, readlen);
>  		aligned = (bytes == mtd->writesize);
> +		use_bufpoi = (chip->options & NAND_USE_BOUNCE_BUFFER) ?
> +				!virt_addr_valid(buf) : 0;
>  
>  		/* Is the current page in the buffer? */
>  		if (realpage != chip->pagebuf || oob) {
> -			bufpoi = aligned ? buf : chip->buffers->databuf;
> +			bufpoi = (aligned && !use_bufpoi) ? buf :
> +				chip->buffers->databuf;
> +
> +			if (use_bufpoi && aligned)
> +				pr_debug("%s: using read bounce buffer for buf@%p\n",
> +						 __func__, buf);
>  
>  read_retry:
>  			chip->cmdfunc(mtd, NAND_CMD_READ0, 0x00, page);
> @@ -1547,7 +1556,7 @@ read_retry:
>  				ret = chip->ecc.read_page(mtd, chip, bufpoi,
>  							  oob_required, page);
>  			if (ret < 0) {
> -				if (!aligned)
> +				if (!aligned || use_bufpoi)
>  					/* Invalidate page cache */
>  					chip->pagebuf = -1;
>  				break;
> @@ -1556,7 +1565,7 @@ read_retry:
>  			max_bitflips = max_t(unsigned int, max_bitflips, ret);
>  
>  			/* Transfer not aligned data */
> -			if (!aligned) {
> +			if (!aligned || use_bufpoi) {
>  				if (!NAND_HAS_SUBPAGE_READ(chip) && !oob &&
>  				    !(mtd->ecc_stats.failed - ecc_failures) &&
>  				    (ops->mode != MTD_OPS_RAW)) {
> @@ -2376,11 +2385,19 @@ static int nand_do_write_ops(struct mtd_info *mtd, loff_t to,
>  		int bytes = mtd->writesize;
>  		int cached = writelen > bytes && page != blockmask;
>  		uint8_t *wbuf = buf;
> -
> -		/* Partial page write? */
> -		if (unlikely(column || writelen < (mtd->writesize - 1))) {
> +		int use_bufpoi = (chip->options & NAND_USE_BOUNCE_BUFFER ?
> +				  !virt_addr_valid(buf) : 0);
> +		int part_pagewr = unlikely(column ||
> +					   writelen < (mtd->writesize - 1));
> +
> +		/* Partial page write?, or need to use bounce buffer */
> +		if (part_pagewr || use_bufpoi) {
> +			pr_debug("%s: using write bounce buffer for buf@%p\n",
> +					 __func__, buf);
>  			cached = 0;
> -			bytes = min_t(int, bytes - column, (int) writelen);
> +			if (part_pagewr)
> +				bytes = min_t(int,
> +					      bytes - column, (int) writelen);
>  			chip->pagebuf = -1;
>  			memset(chip->buffers->databuf, 0xff, mtd->writesize);
>  			memcpy(&chip->buffers->databuf[column], buf, bytes);
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 450d61e..bb3e064 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -176,6 +176,11 @@ typedef enum {
>  /* Chip may not exist, so silence any errors in scan */
>  #define NAND_SCAN_SILENT_NODEV	0x00040000
>  /*
> + * This option is defined to protect against kmapped buffers

Could we change this to a more accurate comment?

Not only the kmapped buffers, also the vmalloc buffers are the highmem we
will meet.

So I suggest write the comment like this:
  
  "This option is defined to protect against the highmem buffers when using DMA,
   such as the kmapped buffers or the buffers allocated by vmalloc()."


You can change the above more readable :)

thanks
Huang Shijie


> + * being passed from highmem when using DMA
> + */
> +#define NAND_USE_BOUNCE_BUFFER	0x00080000