[PATCH v2] nand: sunxi: fix write to USER_DATA reg

[PATCH v2] nand: sunxi: fix write to USER_DATA reg

[Boris Brezillon]

The USER_DATA register cannot be accessed using byte accessors on A13
SoCs, thus triggering a bug when using memcpy_toio on this register.
Declare an NFC_USER_DATA() macro to convert the OOB buffer content into
a suitable u32 value which can then be programmed with writel().

Also drop the oob_required contidion, since ->oob_pio already contains
FFs when oob_required is false.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>

---
Changes since v1:
- drop the !oob_required conditional path
- replace endianness conversions by a macro relying on byte shifting
  operations
---
 drivers/mtd/nand/sunxi_nand.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
index 6f93b29..0c29f6c 100644
--- a/drivers/mtd/nand/sunxi_nand.c
+++ b/drivers/mtd/nand/sunxi_nand.c
@@ -138,6 +138,10 @@
 #define NFC_ECC_MODE		GENMASK(15, 12)
 #define NFC_RANDOM_SEED		GENMASK(30, 16)
 
+/* NFC_USER_DATA helper macro */
+#define NFC_USER_DATA(buf)	((buf)[0] | ((buf)[1] << 8) | \
+				 ((buf)[2] << 16) | ((buf)[3] << 24))
+
 #define NFC_DEFAULT_TIMEOUT_MS	1000
 
 #define NFC_SRAM_SIZE		1024
@@ -632,15 +636,9 @@ static int sunxi_nfc_hw_ecc_write_page(struct mtd_info *mtd,
 		offset = layout->eccpos[i * ecc->bytes] - 4 + mtd->writesize;
 
 		/* Fill OOB data in */
-		if (oob_required) {
-			tmp = 0xffffffff;
-			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE, &tmp,
-				    4);
-		} else {
-			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE,
-				    chip->oob_poi + offset - mtd->writesize,
-				    4);
-		}
+		writel(NFC_USER_DATA(chip->oob_poi +
+				     layout->oobfree[i].offset),
+		       nfc->regs + NFC_REG_USER_DATA_BASE);
 
 		chip->cmdfunc(mtd, NAND_CMD_RNDIN, offset, -1);
 
@@ -770,14 +768,7 @@ static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd,
 		offset += ecc->size;
 
 		/* Fill OOB data in */
-		if (oob_required) {
-			tmp = 0xffffffff;
-			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE, &tmp,
-				    4);
-		} else {
-			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE, oob,
-				    4);
-		}
+		writel(NFC_USER_DATA(oob), nfc->regs + NFC_REG_USER_DATA_BASE);
 
 		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | NFC_ACCESS_DIR |
 		      (1 << 30);
-- 
1.9.1

[PATCH v2] nand: sunxi: fix write to USER_DATA reg

[Boris Brezillon]

Hi Brian,

On Wed,  2 Sep 2015 09:57:01 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> The USER_DATA register cannot be accessed using byte accessors on A13
> SoCs, thus triggering a bug when using memcpy_toio on this register.
> Declare an NFC_USER_DATA() macro to convert the OOB buffer content into
> a suitable u32 value which can then be programmed with writel().
> 
> Also drop the oob_required contidion, since ->oob_pio already contains
> FFs when oob_required is false.

Please ignore this version. I just figured I would need a macro to
convert a USER_DATA value into an u8 buffer.
I'll send a v3 adding two macros (NFC_BUF_TO_USER_DATA() and
NFC_USER_DATA_TO_BUF()) instead of the NFC_USER_DATA() one.

Best Regards,

Boris

> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> 
> ---
> Changes since v1:
> - drop the !oob_required conditional path
> - replace endianness conversions by a macro relying on byte shifting
>   operations
> ---
>  drivers/mtd/nand/sunxi_nand.c | 25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
> index 6f93b29..0c29f6c 100644
> --- a/drivers/mtd/nand/sunxi_nand.c
> +++ b/drivers/mtd/nand/sunxi_nand.c
> @@ -138,6 +138,10 @@
>  #define NFC_ECC_MODE		GENMASK(15, 12)
>  #define NFC_RANDOM_SEED		GENMASK(30, 16)
>  
> +/* NFC_USER_DATA helper macro */
> +#define NFC_USER_DATA(buf)	((buf)[0] | ((buf)[1] << 8) | \
> +				 ((buf)[2] << 16) | ((buf)[3] << 24))
> +
>  #define NFC_DEFAULT_TIMEOUT_MS	1000
>  
>  #define NFC_SRAM_SIZE		1024
> @@ -632,15 +636,9 @@ static int sunxi_nfc_hw_ecc_write_page(struct mtd_info *mtd,
>  		offset = layout->eccpos[i * ecc->bytes] - 4 + mtd->writesize;
>  
>  		/* Fill OOB data in */
> -		if (oob_required) {
> -			tmp = 0xffffffff;
> -			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE, &tmp,
> -				    4);
> -		} else {
> -			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE,
> -				    chip->oob_poi + offset - mtd->writesize,
> -				    4);
> -		}
> +		writel(NFC_USER_DATA(chip->oob_poi +
> +				     layout->oobfree[i].offset),
> +		       nfc->regs + NFC_REG_USER_DATA_BASE);
>  
>  		chip->cmdfunc(mtd, NAND_CMD_RNDIN, offset, -1);
>  
> @@ -770,14 +768,7 @@ static int sunxi_nfc_hw_syndrome_ecc_write_page(struct mtd_info *mtd,
>  		offset += ecc->size;
>  
>  		/* Fill OOB data in */
> -		if (oob_required) {
> -			tmp = 0xffffffff;
> -			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE, &tmp,
> -				    4);
> -		} else {
> -			memcpy_toio(nfc->regs + NFC_REG_USER_DATA_BASE, oob,
> -				    4);
> -		}
> +		writel(NFC_USER_DATA(oob), nfc->regs + NFC_REG_USER_DATA_BASE);
>  
>  		tmp = NFC_DATA_TRANS | NFC_DATA_SWAP_METHOD | NFC_ACCESS_DIR |
>  		      (1 << 30);



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v2] nand: sunxi: fix write to USER_DATA reg

[Brian Norris]

On Wed, Sep 02, 2015 at 09:57:01AM +0200, Boris Brezillon wrote:
> Also drop the oob_required contidion, since ->oob_pio already contains
> FFs when oob_required is false.

I was pointing out this issues not just because you didn't note it in
the commit message, but because the previous behavior actually
potentially has user-visible effects. Particularly, because you had the
conditions backwards, you were programming 0xff to the spare area, no
matter what the user asked for. Normally, this isn't a big issue, since
most users aren't storing data in the OOB region. But there may be cases
where your driver was subtly broken (marking bad blocks, perhaps?), and
this patch is subtly fixing them. Especially since this will get
backported to -stable, it'd be nice to have this noted completely.

Did this driver pass mtd_oobtest previously?

Brian