[PATCH v3 0/5] mtd: rawnand: brcmnand: driver and doc updates

[PATCH v3 0/5] mtd: rawnand: brcmnand: driver and doc updates

[William Zhang]

[-- Attachment #1: Type: text/plain, Size: 926 bytes --]

This patch series include the accumulative updates and fixes for the
brcmnand driver. The bcmbca SoC related updates are removed from v2 and
will send separate patch series for that. v3 series adds a new patch for
mtd oobsize fix.

Changes in v3:
- Fix kernel test robot sparse warning
- Add a new patch for mtd oobsize fix

Changes in v2:
- Use driver static data for ECC level shift
- Handle the remaining unaligned oob data after the oob data write loop

William Zhang (5):
  mtd: rawnand: brcmnand: Fix ECC level field setting for v7.2 controller
  mtd: rawnand: brcmnand: Fix potential false time out warning
  mtd: rawnand: brcmnand: Fix crash during the panic_write
  mtd: rawnand: brcmnand: Fix potential out-of-bounds access in oob write
  mtd: rawnand: brcmnand: Fix mtd oobsize

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 114 +++++++++++++++--------
 1 file changed, 76 insertions(+), 38 deletions(-)

-- 
2.37.3


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

[PATCH v3 1/5] mtd: rawnand: brcmnand: Fix ECC level field setting for v7.2 controller

[William Zhang]

[-- Attachment #1: Type: text/plain, Size: 5042 bytes --]

v7.2 controller has different ECC level field size and shift in the acc
control register than its predecessor and successor controller. It needs
to be set specifically.

Fixes: decba6d47869 ("mtd: brcmnand: Add v7.2 controller support")
Signed-off-by: William Zhang <william.zhang@broadcom.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>

---

Changes in v3: None
Changes in v2:
- Use driver static data for ECC level shift

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 74 +++++++++++++-----------
 1 file changed, 41 insertions(+), 33 deletions(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 2e9c2e2d9c9f..69709419516a 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -272,6 +272,7 @@ struct brcmnand_controller {
 	const unsigned int	*page_sizes;
 	unsigned int		page_size_shift;
 	unsigned int		max_oob;
+	u32			ecc_level_shift;
 	u32			features;
 
 	/* for low-power standby/resume only */
@@ -596,6 +597,34 @@ enum {
 	INTFC_CTLR_READY		= BIT(31),
 };
 
+/***********************************************************************
+ * NAND ACC CONTROL bitfield
+ *
+ * Some bits have remained constant throughout hardware revision, while
+ * others have shifted around.
+ ***********************************************************************/
+
+/* Constant for all versions (where supported) */
+enum {
+	/* See BRCMNAND_HAS_CACHE_MODE */
+	ACC_CONTROL_CACHE_MODE				= BIT(22),
+
+	/* See BRCMNAND_HAS_PREFETCH */
+	ACC_CONTROL_PREFETCH				= BIT(23),
+
+	ACC_CONTROL_PAGE_HIT				= BIT(24),
+	ACC_CONTROL_WR_PREEMPT				= BIT(25),
+	ACC_CONTROL_PARTIAL_PAGE			= BIT(26),
+	ACC_CONTROL_RD_ERASED				= BIT(27),
+	ACC_CONTROL_FAST_PGM_RDIN			= BIT(28),
+	ACC_CONTROL_WR_ECC				= BIT(30),
+	ACC_CONTROL_RD_ECC				= BIT(31),
+
+	ACC_CONTROL_ECC_SHIFT				= 16,
+	/* Only for v7.2 */
+	ACC_CONTROL_ECC_EXT_SHIFT			= 13,
+};
+
 static inline bool brcmnand_non_mmio_ops(struct brcmnand_controller *ctrl)
 {
 #if IS_ENABLED(CONFIG_MTD_NAND_BRCMNAND_BCMA)
@@ -737,6 +766,12 @@ static int brcmnand_revision_init(struct brcmnand_controller *ctrl)
 	else if (of_property_read_bool(ctrl->dev->of_node, "brcm,nand-has-wp"))
 		ctrl->features |= BRCMNAND_HAS_WP;
 
+	/* v7.2 has different ecc level shift in the acc register */
+	if (ctrl->nand_version == 0x0702)
+		ctrl->ecc_level_shift = ACC_CONTROL_ECC_EXT_SHIFT;
+	else
+		ctrl->ecc_level_shift = ACC_CONTROL_ECC_SHIFT;
+
 	return 0;
 }
 
@@ -931,30 +966,6 @@ static inline int brcmnand_cmd_shift(struct brcmnand_controller *ctrl)
 	return 0;
 }
 
-/***********************************************************************
- * NAND ACC CONTROL bitfield
- *
- * Some bits have remained constant throughout hardware revision, while
- * others have shifted around.
- ***********************************************************************/
-
-/* Constant for all versions (where supported) */
-enum {
-	/* See BRCMNAND_HAS_CACHE_MODE */
-	ACC_CONTROL_CACHE_MODE				= BIT(22),
-
-	/* See BRCMNAND_HAS_PREFETCH */
-	ACC_CONTROL_PREFETCH				= BIT(23),
-
-	ACC_CONTROL_PAGE_HIT				= BIT(24),
-	ACC_CONTROL_WR_PREEMPT				= BIT(25),
-	ACC_CONTROL_PARTIAL_PAGE			= BIT(26),
-	ACC_CONTROL_RD_ERASED				= BIT(27),
-	ACC_CONTROL_FAST_PGM_RDIN			= BIT(28),
-	ACC_CONTROL_WR_ECC				= BIT(30),
-	ACC_CONTROL_RD_ECC				= BIT(31),
-};
-
 static inline u32 brcmnand_spare_area_mask(struct brcmnand_controller *ctrl)
 {
 	if (ctrl->nand_version == 0x0702)
@@ -967,18 +978,15 @@ static inline u32 brcmnand_spare_area_mask(struct brcmnand_controller *ctrl)
 		return GENMASK(4, 0);
 }
 
-#define NAND_ACC_CONTROL_ECC_SHIFT	16
-#define NAND_ACC_CONTROL_ECC_EXT_SHIFT	13
-
 static inline u32 brcmnand_ecc_level_mask(struct brcmnand_controller *ctrl)
 {
 	u32 mask = (ctrl->nand_version >= 0x0600) ? 0x1f : 0x0f;
 
-	mask <<= NAND_ACC_CONTROL_ECC_SHIFT;
+	mask <<= ACC_CONTROL_ECC_SHIFT;
 
 	/* v7.2 includes additional ECC levels */
-	if (ctrl->nand_version >= 0x0702)
-		mask |= 0x7 << NAND_ACC_CONTROL_ECC_EXT_SHIFT;
+	if (ctrl->nand_version == 0x0702)
+		mask |= 0x7 << ACC_CONTROL_ECC_EXT_SHIFT;
 
 	return mask;
 }
@@ -992,8 +1000,8 @@ static void brcmnand_set_ecc_enabled(struct brcmnand_host *host, int en)
 
 	if (en) {
 		acc_control |= ecc_flags; /* enable RD/WR ECC */
-		acc_control |= host->hwcfg.ecc_level
-			       << NAND_ACC_CONTROL_ECC_SHIFT;
+		acc_control &= ~brcmnand_ecc_level_mask(ctrl);
+		acc_control |= host->hwcfg.ecc_level << ctrl->ecc_level_shift;
 	} else {
 		acc_control &= ~ecc_flags; /* disable RD/WR ECC */
 		acc_control &= ~brcmnand_ecc_level_mask(ctrl);
@@ -2561,7 +2569,7 @@ static int brcmnand_set_cfg(struct brcmnand_host *host,
 	tmp &= ~brcmnand_ecc_level_mask(ctrl);
 	tmp &= ~brcmnand_spare_area_mask(ctrl);
 	if (ctrl->nand_version >= 0x0302) {
-		tmp |= cfg->ecc_level << NAND_ACC_CONTROL_ECC_SHIFT;
+		tmp |= cfg->ecc_level << ctrl->ecc_level_shift;
 		tmp |= cfg->spare_area_size;
 	}
 	nand_writereg(ctrl, acc_control_offs, tmp);
-- 
2.37.3


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH v3 1/5] mtd: rawnand: brcmnand: Fix ECC level field setting for v7.2 controller

[Miquel Raynal]

Hi William,

william.zhang@broadcom.com wrote on Tue, 27 Jun 2023 12:37:34 -0700:

> v7.2 controller has different ECC level field size and shift in the acc
> control register than its predecessor and successor controller. It needs
> to be set specifically.
> 
> Fixes: decba6d47869 ("mtd: brcmnand: Add v7.2 controller support")
> Signed-off-by: William Zhang <william.zhang@broadcom.com>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> 
> ---
> 
> Changes in v3: None
> Changes in v2:
> - Use driver static data for ECC level shift
> 
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 74 +++++++++++++-----------
>  1 file changed, 41 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index 2e9c2e2d9c9f..69709419516a 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -272,6 +272,7 @@ struct brcmnand_controller {
>  	const unsigned int	*page_sizes;
>  	unsigned int		page_size_shift;
>  	unsigned int		max_oob;
> +	u32			ecc_level_shift;
>  	u32			features;
>  
>  	/* for low-power standby/resume only */
> @@ -596,6 +597,34 @@ enum {
>  	INTFC_CTLR_READY		= BIT(31),
>  };
>  
> +/***********************************************************************
> + * NAND ACC CONTROL bitfield
> + *
> + * Some bits have remained constant throughout hardware revision, while
> + * others have shifted around.
> + ***********************************************************************/
> +
> +/* Constant for all versions (where supported) */
> +enum {
> +	/* See BRCMNAND_HAS_CACHE_MODE */
> +	ACC_CONTROL_CACHE_MODE				= BIT(22),
> +
> +	/* See BRCMNAND_HAS_PREFETCH */
> +	ACC_CONTROL_PREFETCH				= BIT(23),
> +
> +	ACC_CONTROL_PAGE_HIT				= BIT(24),
> +	ACC_CONTROL_WR_PREEMPT				= BIT(25),
> +	ACC_CONTROL_PARTIAL_PAGE			= BIT(26),
> +	ACC_CONTROL_RD_ERASED				= BIT(27),
> +	ACC_CONTROL_FAST_PGM_RDIN			= BIT(28),
> +	ACC_CONTROL_WR_ECC				= BIT(30),
> +	ACC_CONTROL_RD_ECC				= BIT(31),
> +
> +	ACC_CONTROL_ECC_SHIFT				= 16,
> +	/* Only for v7.2 */
> +	ACC_CONTROL_ECC_EXT_SHIFT			= 13,
> +};

These do not look like they fit the purpose of enums. At least keep
these two last definitions outside like before (you can rename them, I
don't mind).

LGTM otherwise.

> +
>  static inline bool brcmnand_non_mmio_ops(struct brcmnand_controller *ctrl)
>  {
>  #if IS_ENABLED(CONFIG_MTD_NAND_BRCMNAND_BCMA)
> @@ -737,6 +766,12 @@ static int brcmnand_revision_init(struct brcmnand_controller *ctrl)
>  	else if (of_property_read_bool(ctrl->dev->of_node, "brcm,nand-has-wp"))
>  		ctrl->features |= BRCMNAND_HAS_WP;
>  
> +	/* v7.2 has different ecc level shift in the acc register */
> +	if (ctrl->nand_version == 0x0702)
> +		ctrl->ecc_level_shift = ACC_CONTROL_ECC_EXT_SHIFT;
> +	else
> +		ctrl->ecc_level_shift = ACC_CONTROL_ECC_SHIFT;
> +
>  	return 0;
>  }
>  
> @@ -931,30 +966,6 @@ static inline int brcmnand_cmd_shift(struct brcmnand_controller *ctrl)
>  	return 0;
>  }
>  
> -/***********************************************************************
> - * NAND ACC CONTROL bitfield
> - *
> - * Some bits have remained constant throughout hardware revision, while
> - * others have shifted around.
> - ***********************************************************************/
> -
> -/* Constant for all versions (where supported) */
> -enum {
> -	/* See BRCMNAND_HAS_CACHE_MODE */
> -	ACC_CONTROL_CACHE_MODE				= BIT(22),
> -
> -	/* See BRCMNAND_HAS_PREFETCH */
> -	ACC_CONTROL_PREFETCH				= BIT(23),
> -
> -	ACC_CONTROL_PAGE_HIT				= BIT(24),
> -	ACC_CONTROL_WR_PREEMPT				= BIT(25),
> -	ACC_CONTROL_PARTIAL_PAGE			= BIT(26),
> -	ACC_CONTROL_RD_ERASED				= BIT(27),
> -	ACC_CONTROL_FAST_PGM_RDIN			= BIT(28),
> -	ACC_CONTROL_WR_ECC				= BIT(30),
> -	ACC_CONTROL_RD_ECC				= BIT(31),
> -};
> -
>  static inline u32 brcmnand_spare_area_mask(struct brcmnand_controller *ctrl)
>  {
>  	if (ctrl->nand_version == 0x0702)
> @@ -967,18 +978,15 @@ static inline u32 brcmnand_spare_area_mask(struct brcmnand_controller *ctrl)
>  		return GENMASK(4, 0);
>  }
>  
> -#define NAND_ACC_CONTROL_ECC_SHIFT	16
> -#define NAND_ACC_CONTROL_ECC_EXT_SHIFT	13
> -
>  static inline u32 brcmnand_ecc_level_mask(struct brcmnand_controller *ctrl)
>  {
>  	u32 mask = (ctrl->nand_version >= 0x0600) ? 0x1f : 0x0f;
>  
> -	mask <<= NAND_ACC_CONTROL_ECC_SHIFT;
> +	mask <<= ACC_CONTROL_ECC_SHIFT;
>  
>  	/* v7.2 includes additional ECC levels */
> -	if (ctrl->nand_version >= 0x0702)
> -		mask |= 0x7 << NAND_ACC_CONTROL_ECC_EXT_SHIFT;
> +	if (ctrl->nand_version == 0x0702)
> +		mask |= 0x7 << ACC_CONTROL_ECC_EXT_SHIFT;
>  
>  	return mask;
>  }
> @@ -992,8 +1000,8 @@ static void brcmnand_set_ecc_enabled(struct brcmnand_host *host, int en)
>  
>  	if (en) {
>  		acc_control |= ecc_flags; /* enable RD/WR ECC */
> -		acc_control |= host->hwcfg.ecc_level
> -			       << NAND_ACC_CONTROL_ECC_SHIFT;
> +		acc_control &= ~brcmnand_ecc_level_mask(ctrl);
> +		acc_control |= host->hwcfg.ecc_level << ctrl->ecc_level_shift;
>  	} else {
>  		acc_control &= ~ecc_flags; /* disable RD/WR ECC */
>  		acc_control &= ~brcmnand_ecc_level_mask(ctrl);
> @@ -2561,7 +2569,7 @@ static int brcmnand_set_cfg(struct brcmnand_host *host,
>  	tmp &= ~brcmnand_ecc_level_mask(ctrl);
>  	tmp &= ~brcmnand_spare_area_mask(ctrl);
>  	if (ctrl->nand_version >= 0x0302) {
> -		tmp |= cfg->ecc_level << NAND_ACC_CONTROL_ECC_SHIFT;
> +		tmp |= cfg->ecc_level << ctrl->ecc_level_shift;
>  		tmp |= cfg->spare_area_size;
>  	}
>  	nand_writereg(ctrl, acc_control_offs, tmp);


Thanks,
Miquèl

[PATCH v3 2/5] mtd: rawnand: brcmnand: Fix potential false time out warning

[William Zhang]

[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]

If system is busy during the command status polling function, the driver
may not get the chance to poll the status register till the end of time
out and return the premature status.  Do a final check after time out
happens to ensure reading the correct status.

Fixes: 9d2ee0a60b8b ("mtd: nand: brcmnand: Check flash #WP pin status before nand erase/program")
Signed-off-by: William Zhang <william.zhang@broadcom.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---

Changes in v3: None
Changes in v2: None

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 69709419516a..37c2c7cfa00e 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -1080,6 +1080,14 @@ static int bcmnand_ctrl_poll_status(struct brcmnand_controller *ctrl,
 		cpu_relax();
 	} while (time_after(limit, jiffies));
 
+	/*
+	 * do a final check after time out in case CPU is busy and driver does
+	 * not get the enough time to poll to avoid false alarm
+	 */
+	val = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
+	if ((val & mask) == expected_val)
+		return 0;
+
 	dev_warn(ctrl->dev, "timeout on status poll (expected %x got %x)\n",
 		 expected_val, val & mask);
 
-- 
2.37.3


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH v3 2/5] mtd: rawnand: brcmnand: Fix potential false time out warning

[Miquel Raynal]

Hi William,

william.zhang@broadcom.com wrote on Tue, 27 Jun 2023 12:37:35 -0700:

> If system is busy during the command status polling function, the driver
> may not get the chance to poll the status register till the end of time
> out and return the premature status.  Do a final check after time out
> happens to ensure reading the correct status.
> 
> Fixes: 9d2ee0a60b8b ("mtd: nand: brcmnand: Check flash #WP pin status before nand erase/program")
> Signed-off-by: William Zhang <william.zhang@broadcom.com>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>

Cc: stable here and in patch 1 as well.

> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index 69709419516a..37c2c7cfa00e 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -1080,6 +1080,14 @@ static int bcmnand_ctrl_poll_status(struct brcmnand_controller *ctrl,
>  		cpu_relax();
>  	} while (time_after(limit, jiffies));
>  
> +	/*
> +	 * do a final check after time out in case CPU is busy and driver does
> +	 * not get the enough time to poll to avoid false alarm

Do a final... ...the CPU was busy and the driver did not get enough
time to perform the polling to avoid false alarms.

> +	 */
> +	val = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
> +	if ((val & mask) == expected_val)
> +		return 0;
> +
>  	dev_warn(ctrl->dev, "timeout on status poll (expected %x got %x)\n",
>  		 expected_val, val & mask);
>  

Otherwise looks good.

Thanks,
Miquèl

[PATCH v3 3/5] mtd: rawnand: brcmnand: Fix crash during the panic_write

[William Zhang]

[-- Attachment #1: Type: text/plain, Size: 1587 bytes --]

During the panic write path to execute another nand command, if
there is a pending command, we should wait for the command instead of
calling BUG_ON so we don't crash while crashing.

Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
Signed-off-by: William Zhang <william.zhang@broadcom.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Reviewed-by: Kursad Oney <kursad.oney@broadcom.com>
Reviewed-by: Kamal Dasu <kamal.dasu@broadcom.com>
---

Changes in v3: None
Changes in v2: None

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 37c2c7cfa00e..ea03104692bf 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -1608,7 +1608,17 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
 
 	dev_dbg(ctrl->dev, "send native cmd %d addr 0x%llx\n", cmd, cmd_addr);
 
-	BUG_ON(ctrl->cmd_pending != 0);
+	/*
+	 * If we came here through _panic_write and there is a pending
+	 * command, try to wait for it. If it times out, rather than
+	 * hitting BUG_ON, just return so we don't crash while crashing.
+	 */
+	if (oops_in_progress) {
+		if (ctrl->cmd_pending &&
+			bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0))
+			return;
+	} else
+		BUG_ON(ctrl->cmd_pending != 0);
 	ctrl->cmd_pending = cmd;
 
 	ret = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);
-- 
2.37.3


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH v3 3/5] mtd: rawnand: brcmnand: Fix crash during the panic_write

[Miquel Raynal]

Hi William,

william.zhang@broadcom.com wrote on Tue, 27 Jun 2023 12:37:36 -0700:

> During the panic write path to execute another nand command, if

"When executing a NAND command within the panic write path, wait for any
pending command instead of calling BUG_ON to avoid crashing while
(already) crashing."

> there is a pending command, we should wait for the command instead of
> calling BUG_ON so we don't crash while crashing.
> 
> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
> Signed-off-by: William Zhang <william.zhang@broadcom.com>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> Reviewed-by: Kursad Oney <kursad.oney@broadcom.com>
> Reviewed-by: Kamal Dasu <kamal.dasu@broadcom.com>
> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index 37c2c7cfa00e..ea03104692bf 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -1608,7 +1608,17 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
>  
>  	dev_dbg(ctrl->dev, "send native cmd %d addr 0x%llx\n", cmd, cmd_addr);
>  
> -	BUG_ON(ctrl->cmd_pending != 0);
> +	/*
> +	 * If we came here through _panic_write and there is a pending
> +	 * command, try to wait for it. If it times out, rather than
> +	 * hitting BUG_ON, just return so we don't crash while crashing.
> +	 */
> +	if (oops_in_progress) {
> +		if (ctrl->cmd_pending &&
> +			bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0))
> +			return;
> +	} else
> +		BUG_ON(ctrl->cmd_pending != 0);
>  	ctrl->cmd_pending = cmd;
>  
>  	ret = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);

LGTM otherwise.

Thanks,
Miquèl

Re: [PATCH v3 3/5] mtd: rawnand: brcmnand: Fix crash during the panic_write

[Miquel Raynal]

Hi William,

william.zhang@broadcom.com wrote on Tue, 27 Jun 2023 12:37:36 -0700:

> During the panic write path to execute another nand command, if
> there is a pending command, we should wait for the command instead of
> calling BUG_ON so we don't crash while crashing.
> 
> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")

The Fixes tag looks wrong.

> Signed-off-by: William Zhang <william.zhang@broadcom.com>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> Reviewed-by: Kursad Oney <kursad.oney@broadcom.com>
> Reviewed-by: Kamal Dasu <kamal.dasu@broadcom.com>
> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index 37c2c7cfa00e..ea03104692bf 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -1608,7 +1608,17 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
>  
>  	dev_dbg(ctrl->dev, "send native cmd %d addr 0x%llx\n", cmd, cmd_addr);
>  
> -	BUG_ON(ctrl->cmd_pending != 0);
> +	/*
> +	 * If we came here through _panic_write and there is a pending
> +	 * command, try to wait for it. If it times out, rather than
> +	 * hitting BUG_ON, just return so we don't crash while crashing.
> +	 */
> +	if (oops_in_progress) {
> +		if (ctrl->cmd_pending &&
> +			bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0))
> +			return;
> +	} else
> +		BUG_ON(ctrl->cmd_pending != 0);
>  	ctrl->cmd_pending = cmd;
>  
>  	ret = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);


Thanks,
Miquèl

Re: [PATCH v3 3/5] mtd: rawnand: brcmnand: Fix crash during the panic_write

[William Zhang]

[-- Attachment #1: Type: text/plain, Size: 2072 bytes --]

Hi Miquel,

On 07/04/2023 08:26 AM, Miquel Raynal wrote:
> Hi William,
> 
> william.zhang@broadcom.com wrote on Tue, 27 Jun 2023 12:37:36 -0700:
> 
>> During the panic write path to execute another nand command, if
>> there is a pending command, we should wait for the command instead of
>> calling BUG_ON so we don't crash while crashing.
>>
>> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
> 
> The Fixes tag looks wrong.
> 
The brcmnand_send_cmd function and BUG_ON line were added by this commit 
and the function didn't changed much since then. Not sure why you think 
it is wrong?

>> Signed-off-by: William Zhang <william.zhang@broadcom.com>
>> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
>> Reviewed-by: Kursad Oney <kursad.oney@broadcom.com>
>> Reviewed-by: Kamal Dasu <kamal.dasu@broadcom.com>
>> ---
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>>   drivers/mtd/nand/raw/brcmnand/brcmnand.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> index 37c2c7cfa00e..ea03104692bf 100644
>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> @@ -1608,7 +1608,17 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
>>   
>>   	dev_dbg(ctrl->dev, "send native cmd %d addr 0x%llx\n", cmd, cmd_addr);
>>   
>> -	BUG_ON(ctrl->cmd_pending != 0);
>> +	/*
>> +	 * If we came here through _panic_write and there is a pending
>> +	 * command, try to wait for it. If it times out, rather than
>> +	 * hitting BUG_ON, just return so we don't crash while crashing.
>> +	 */
>> +	if (oops_in_progress) {
>> +		if (ctrl->cmd_pending &&
>> +			bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0))
>> +			return;
>> +	} else
>> +		BUG_ON(ctrl->cmd_pending != 0);
>>   	ctrl->cmd_pending = cmd;
>>   
>>   	ret = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);
> 
> 
> Thanks,
> Miquèl
> 

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH v3 3/5] mtd: rawnand: brcmnand: Fix crash during the panic_write

[Miquel Raynal]

Hi William,

william.zhang@broadcom.com wrote on Tue, 4 Jul 2023 17:40:03 -0700:

> Hi Miquel,
> 
> On 07/04/2023 08:26 AM, Miquel Raynal wrote:
> > Hi William,
> > 
> > william.zhang@broadcom.com wrote on Tue, 27 Jun 2023 12:37:36 -0700:
> >   
> >> During the panic write path to execute another nand command, if
> >> there is a pending command, we should wait for the command instead of
> >> calling BUG_ON so we don't crash while crashing.
> >>
> >> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")  
> > 
> > The Fixes tag looks wrong.
> >   
> The brcmnand_send_cmd function and BUG_ON line were added by this commit and the function didn't changed much since then. Not sure why you think it is wrong?

Ok, the title of that commit let me think it was moving code rather
than adding it. Alright.

> >> Signed-off-by: William Zhang <william.zhang@broadcom.com>
> >> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> >> Reviewed-by: Kursad Oney <kursad.oney@broadcom.com>
> >> Reviewed-by: Kamal Dasu <kamal.dasu@broadcom.com>
> >> ---
> >>
> >> Changes in v3: None
> >> Changes in v2: None
> >>
> >>   drivers/mtd/nand/raw/brcmnand/brcmnand.c | 12 +++++++++++-
> >>   1 file changed, 11 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> index 37c2c7cfa00e..ea03104692bf 100644
> >> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> @@ -1608,7 +1608,17 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)  
> >>   >>   	dev_dbg(ctrl->dev, "send native cmd %d addr 0x%llx\n", cmd, cmd_addr);
> >>   >> -	BUG_ON(ctrl->cmd_pending != 0);  
> >> +	/*
> >> +	 * If we came here through _panic_write and there is a pending
> >> +	 * command, try to wait for it. If it times out, rather than
> >> +	 * hitting BUG_ON, just return so we don't crash while crashing.
> >> +	 */
> >> +	if (oops_in_progress) {
> >> +		if (ctrl->cmd_pending &&
> >> +			bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0))
> >> +			return;
> >> +	} else
> >> +		BUG_ON(ctrl->cmd_pending != 0);
> >>   	ctrl->cmd_pending = cmd;  
> >>   >>   	ret = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);  
> > 
> > 
> > Thanks,
> > Miquèl
> >   


Thanks,
Miquèl

[PATCH v3 4/5] mtd: rawnand: brcmnand: Fix potential out-of-bounds access in oob write

[William Zhang]

[-- Attachment #1: Type: text/plain, Size: 2007 bytes --]

When the oob buffer length is not in multiple of words, the oob write
function does out-of-bounds read on the oob source buffer at the last
iteration. Fix that by always checking length limit on the oob buffer
read and fill with 0xff when reaching the end of the buffer to the oob
registers.

Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
Signed-off-by: William Zhang <william.zhang@broadcom.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>

---

Changes in v3:
- Fix kernel test robot sparse warning:
   drivers/mtd/nand/raw/brcmnand/brcmnand.c:1500:54: sparse: expected unsigned int [usertype] data
   drivers/mtd/nand/raw/brcmnand/brcmnand.c:1500:54: sparse: got restricted __be32 [usertype]

Changes in v2:
- Handle the remaining unaligned oob data after the oob data write loop

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index ea03104692bf..407bf79cbaf4 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -1477,19 +1477,28 @@ static int write_oob_to_regs(struct brcmnand_controller *ctrl, int i,
 			     const u8 *oob, int sas, int sector_1k)
 {
 	int tbytes = sas << sector_1k;
-	int j;
+	int j, k = 0;
+	u32 last = 0xffffffff;
+	u8 *plast = (u8 *)&last;
 
 	/* Adjust OOB values for 1K sector size */
 	if (sector_1k && (i & 0x01))
 		tbytes = max(0, tbytes - (int)ctrl->max_oob);
 	tbytes = min_t(int, tbytes, ctrl->max_oob);
 
-	for (j = 0; j < tbytes; j += 4)
+	for (j = 0; (j + 3) < tbytes; j += 4)
 		oob_reg_write(ctrl, j,
 				(oob[j + 0] << 24) |
 				(oob[j + 1] << 16) |
 				(oob[j + 2] <<  8) |
 				(oob[j + 3] <<  0));
+
+	while (j < tbytes)
+		plast[k++] = oob[j++];
+
+	if (tbytes & 0x3)
+		oob_reg_write(ctrl, (tbytes & ~0x3), (__force u32)cpu_to_be32(last));
+
 	return tbytes;
 }
 
-- 
2.37.3


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH v3 4/5] mtd: rawnand: brcmnand: Fix potential out-of-bounds access in oob write

[Miquel Raynal]

Hi William,

william.zhang@broadcom.com wrote on Tue, 27 Jun 2023 12:37:37 -0700:

> When the oob buffer length is not in multiple of words, the oob write
> function does out-of-bounds read on the oob source buffer at the last
> iteration. Fix that by always checking length limit on the oob buffer
> read and fill with 0xff when reaching the end of the buffer to the oob
> registers.
> 
> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")

Wrong Fixes.

Missing Cc stable.

> Signed-off-by: William Zhang <william.zhang@broadcom.com>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> 
> ---
> 
> Changes in v3:
> - Fix kernel test robot sparse warning:
>    drivers/mtd/nand/raw/brcmnand/brcmnand.c:1500:54: sparse: expected unsigned int [usertype] data
>    drivers/mtd/nand/raw/brcmnand/brcmnand.c:1500:54: sparse: got restricted __be32 [usertype]
> 
> Changes in v2:
> - Handle the remaining unaligned oob data after the oob data write loop
> 
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index ea03104692bf..407bf79cbaf4 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -1477,19 +1477,28 @@ static int write_oob_to_regs(struct brcmnand_controller *ctrl, int i,
>  			     const u8 *oob, int sas, int sector_1k)
>  {
>  	int tbytes = sas << sector_1k;
> -	int j;
> +	int j, k = 0;
> +	u32 last = 0xffffffff;
> +	u8 *plast = (u8 *)&last;
>  
>  	/* Adjust OOB values for 1K sector size */
>  	if (sector_1k && (i & 0x01))
>  		tbytes = max(0, tbytes - (int)ctrl->max_oob);
>  	tbytes = min_t(int, tbytes, ctrl->max_oob);
>  
> -	for (j = 0; j < tbytes; j += 4)
> +	for (j = 0; (j + 3) < tbytes; j += 4)

Maybe a comment here as well to mention that you stop at the last
iteration? Otherwise, just reading the line does not make you choice
obvious.

>  		oob_reg_write(ctrl, j,
>  				(oob[j + 0] << 24) |
>  				(oob[j + 1] << 16) |
>  				(oob[j + 2] <<  8) |
>  				(oob[j + 3] <<  0));
> +
> +	while (j < tbytes)
> +		plast[k++] = oob[j++];
> +
> +	if (tbytes & 0x3)
> +		oob_reg_write(ctrl, (tbytes & ~0x3), (__force u32)cpu_to_be32(last));
> +
>  	return tbytes;
>  }
>  


Thanks,
Miquèl

Re: [PATCH v3 4/5] mtd: rawnand: brcmnand: Fix potential out-of-bounds access in oob write

[William Zhang]

[-- Attachment #1: Type: text/plain, Size: 2687 bytes --]

Hi Miquel,

On 07/04/2023 08:28 AM, Miquel Raynal wrote:
> Hi William,
> 
> william.zhang@broadcom.com wrote on Tue, 27 Jun 2023 12:37:37 -0700:
> 
>> When the oob buffer length is not in multiple of words, the oob write
>> function does out-of-bounds read on the oob source buffer at the last
>> iteration. Fix that by always checking length limit on the oob buffer
>> read and fill with 0xff when reaching the end of the buffer to the oob
>> registers.
>>
>> Fixes: 27c5b17cd1b1 ("mtd: nand: add NAND driver "library" for Broadcom STB NAND controller")
> 
> Wrong Fixes.
Same here.  The function write_oob_to_regs was added by this commit and 
no change since then.


> 
> Missing Cc stable.
Will add
> 
>> Signed-off-by: William Zhang <william.zhang@broadcom.com>
>> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
>>
>> ---
>>
>> Changes in v3:
>> - Fix kernel test robot sparse warning:
>>     drivers/mtd/nand/raw/brcmnand/brcmnand.c:1500:54: sparse: expected unsigned int [usertype] data
>>     drivers/mtd/nand/raw/brcmnand/brcmnand.c:1500:54: sparse: got restricted __be32 [usertype]
>>
>> Changes in v2:
>> - Handle the remaining unaligned oob data after the oob data write loop
>>
>>   drivers/mtd/nand/raw/brcmnand/brcmnand.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> index ea03104692bf..407bf79cbaf4 100644
>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> @@ -1477,19 +1477,28 @@ static int write_oob_to_regs(struct brcmnand_controller *ctrl, int i,
>>   			     const u8 *oob, int sas, int sector_1k)
>>   {
>>   	int tbytes = sas << sector_1k;
>> -	int j;
>> +	int j, k = 0;
>> +	u32 last = 0xffffffff;
>> +	u8 *plast = (u8 *)&last;
>>   
>>   	/* Adjust OOB values for 1K sector size */
>>   	if (sector_1k && (i & 0x01))
>>   		tbytes = max(0, tbytes - (int)ctrl->max_oob);
>>   	tbytes = min_t(int, tbytes, ctrl->max_oob);
>>   
>> -	for (j = 0; j < tbytes; j += 4)
>> +	for (j = 0; (j + 3) < tbytes; j += 4)
> 
> Maybe a comment here as well to mention that you stop at the last
> iteration? Otherwise, just reading the line does not make you choice
> obvious.
> 
Will add comment.

>>   		oob_reg_write(ctrl, j,
>>   				(oob[j + 0] << 24) |
>>   				(oob[j + 1] << 16) |
>>   				(oob[j + 2] <<  8) |
>>   				(oob[j + 3] <<  0));
>> +
>> +	while (j < tbytes)
>> +		plast[k++] = oob[j++];
>> +
>> +	if (tbytes & 0x3)
>> +		oob_reg_write(ctrl, (tbytes & ~0x3), (__force u32)cpu_to_be32(last));
>> +
>>   	return tbytes;
>>   }
>>   
> 
> 
> Thanks,
> Miquèl
> 

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

[PATCH v3 5/5] mtd: rawnand: brcmnand: Fix mtd oobsize

[William Zhang]

[-- Attachment #1: Type: text/plain, Size: 2250 bytes --]

brcmnand controller can only access the flash spare area up to certain
bytes based on the ECC level. It can be less than the actual flash spare
area size. For example, for many NAND chip supporting ECC BCH-8, it has
226 bytes spare area. But controller can only uses 218 bytes. So brcmand
driver overrides the mtd oobsize with the controller's accessible spare
area size. When the nand base driver utilizes the nand_device object, it
resets the oobsize back to the actual flash spare aprea size from
nand_memory_organization structure and controller may not able to access
all the oob area as mtd advises.

This change fixes the issue by overriding the oobsize in the
nand_memory_organization structure to the controller's accessible spare
area size.

Fixes: a7ab085d7c16 ("mtd: rawnand: Initialize the nand_device object")
Signed-off-by: William Zhang <william.zhang@broadcom.com>

---

Changes in v3: None
Changes in v2: None

 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 407bf79cbaf4..39c7f547db1f 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -2647,6 +2647,8 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
 	struct nand_chip *chip = &host->chip;
 	const struct nand_ecc_props *requirements =
 		nanddev_get_ecc_requirements(&chip->base);
+	struct nand_memory_organization *memorg =
+		nanddev_get_memorg(&chip->base);
 	struct brcmnand_controller *ctrl = host->ctrl;
 	struct brcmnand_cfg *cfg = &host->hwcfg;
 	char msg[128];
@@ -2668,10 +2670,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
 	if (cfg->spare_area_size > ctrl->max_oob)
 		cfg->spare_area_size = ctrl->max_oob;
 	/*
-	 * Set oobsize to be consistent with controller's spare_area_size, as
-	 * the rest is inaccessible.
+	 * Set mtd and memorg oobsize to be consistent with controller's
+	 * spare_area_size, as the rest is inaccessible.
 	 */
 	mtd->oobsize = cfg->spare_area_size * (mtd->writesize >> FC_SHIFT);
+	memorg->oobsize = mtd->oobsize;
 
 	cfg->device_size = mtd->size;
 	cfg->block_size = mtd->erasesize;
-- 
2.37.3


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH v3 5/5] mtd: rawnand: brcmnand: Fix mtd oobsize

[Miquel Raynal]

Hi William,

william.zhang@broadcom.com wrote on Tue, 27 Jun 2023 12:37:38 -0700:

> brcmnand controller can only access the flash spare area up to certain
> bytes based on the ECC level. It can be less than the actual flash spare
> area size. For example, for many NAND chip supporting ECC BCH-8, it has
> 226 bytes spare area. But controller can only uses 218 bytes. So brcmand
> driver overrides the mtd oobsize with the controller's accessible spare
> area size. When the nand base driver utilizes the nand_device object, it
> resets the oobsize back to the actual flash spare aprea size from
> nand_memory_organization structure and controller may not able to access
> all the oob area as mtd advises.
> 
> This change fixes the issue by overriding the oobsize in the
> nand_memory_organization structure to the controller's accessible spare
> area size.

I am clearly not a big fan of this solution. memorg should be and
remain a read only object. Can you please find another solution?

> 
> Fixes: a7ab085d7c16 ("mtd: rawnand: Initialize the nand_device object")
> Signed-off-by: William Zhang <william.zhang@broadcom.com>
> 
> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> index 407bf79cbaf4..39c7f547db1f 100644
> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> @@ -2647,6 +2647,8 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
>  	struct nand_chip *chip = &host->chip;
>  	const struct nand_ecc_props *requirements =
>  		nanddev_get_ecc_requirements(&chip->base);
> +	struct nand_memory_organization *memorg =
> +		nanddev_get_memorg(&chip->base);
>  	struct brcmnand_controller *ctrl = host->ctrl;
>  	struct brcmnand_cfg *cfg = &host->hwcfg;
>  	char msg[128];
> @@ -2668,10 +2670,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
>  	if (cfg->spare_area_size > ctrl->max_oob)
>  		cfg->spare_area_size = ctrl->max_oob;
>  	/*
> -	 * Set oobsize to be consistent with controller's spare_area_size, as
> -	 * the rest is inaccessible.
> +	 * Set mtd and memorg oobsize to be consistent with controller's
> +	 * spare_area_size, as the rest is inaccessible.
>  	 */
>  	mtd->oobsize = cfg->spare_area_size * (mtd->writesize >> FC_SHIFT);
> +	memorg->oobsize = mtd->oobsize;
>  
>  	cfg->device_size = mtd->size;
>  	cfg->block_size = mtd->erasesize;


Thanks,
Miquèl

Re: [PATCH v3 5/5] mtd: rawnand: brcmnand: Fix mtd oobsize

[William Zhang]

[-- Attachment #1: Type: text/plain, Size: 3130 bytes --]

Hi Miquel,

On 07/04/2023 08:30 AM, Miquel Raynal wrote:
> Hi William,
> 
> william.zhang@broadcom.com wrote on Tue, 27 Jun 2023 12:37:38 -0700:
> 
>> brcmnand controller can only access the flash spare area up to certain
>> bytes based on the ECC level. It can be less than the actual flash spare
>> area size. For example, for many NAND chip supporting ECC BCH-8, it has
>> 226 bytes spare area. But controller can only uses 218 bytes. So brcmand
>> driver overrides the mtd oobsize with the controller's accessible spare
>> area size. When the nand base driver utilizes the nand_device object, it
>> resets the oobsize back to the actual flash spare aprea size from
>> nand_memory_organization structure and controller may not able to access
>> all the oob area as mtd advises.
>>
>> This change fixes the issue by overriding the oobsize in the
>> nand_memory_organization structure to the controller's accessible spare
>> area size.
> 
> I am clearly not a big fan of this solution. memorg should be and
> remain a read only object. Can you please find another solution?
> 
I was debating on this too but I don't see option because there is no 
other hooks after nanddev_init to set the mtd->oobsize as far as I can 
see and I see there were similar fixes for other controller drivers 
after the nand device object init was committed, for example:
Fixes: 629a442cad5f ("mtd: rawnand: Fill memorg during detection")

I will think through this again but I am open to any suggestion.

>>
>> Fixes: a7ab085d7c16 ("mtd: rawnand: Initialize the nand_device object")
>> Signed-off-by: William Zhang <william.zhang@broadcom.com>
>>
>> ---
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>>   drivers/mtd/nand/raw/brcmnand/brcmnand.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> index 407bf79cbaf4..39c7f547db1f 100644
>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> @@ -2647,6 +2647,8 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
>>   	struct nand_chip *chip = &host->chip;
>>   	const struct nand_ecc_props *requirements =
>>   		nanddev_get_ecc_requirements(&chip->base);
>> +	struct nand_memory_organization *memorg =
>> +		nanddev_get_memorg(&chip->base);
>>   	struct brcmnand_controller *ctrl = host->ctrl;
>>   	struct brcmnand_cfg *cfg = &host->hwcfg;
>>   	char msg[128];
>> @@ -2668,10 +2670,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
>>   	if (cfg->spare_area_size > ctrl->max_oob)
>>   		cfg->spare_area_size = ctrl->max_oob;
>>   	/*
>> -	 * Set oobsize to be consistent with controller's spare_area_size, as
>> -	 * the rest is inaccessible.
>> +	 * Set mtd and memorg oobsize to be consistent with controller's
>> +	 * spare_area_size, as the rest is inaccessible.
>>   	 */
>>   	mtd->oobsize = cfg->spare_area_size * (mtd->writesize >> FC_SHIFT);
>> +	memorg->oobsize = mtd->oobsize;
>>   
>>   	cfg->device_size = mtd->size;
>>   	cfg->block_size = mtd->erasesize;
> 
> 
> Thanks,
> Miquèl
> 

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

Re: [PATCH v3 5/5] mtd: rawnand: brcmnand: Fix mtd oobsize

[Miquel Raynal]

Hi William,

william.zhang@broadcom.com wrote on Tue, 4 Jul 2023 17:50:28 -0700:

> Hi Miquel,
> 
> On 07/04/2023 08:30 AM, Miquel Raynal wrote:
> > Hi William,
> > 
> > william.zhang@broadcom.com wrote on Tue, 27 Jun 2023 12:37:38 -0700:
> >   
> >> brcmnand controller can only access the flash spare area up to certain
> >> bytes based on the ECC level. It can be less than the actual flash spare
> >> area size. For example, for many NAND chip supporting ECC BCH-8, it has
> >> 226 bytes spare area. But controller can only uses 218 bytes. So brcmand
> >> driver overrides the mtd oobsize with the controller's accessible spare
> >> area size. When the nand base driver utilizes the nand_device object, it
> >> resets the oobsize back to the actual flash spare aprea size from
> >> nand_memory_organization structure and controller may not able to access
> >> all the oob area as mtd advises.
> >>
> >> This change fixes the issue by overriding the oobsize in the
> >> nand_memory_organization structure to the controller's accessible spare
> >> area size.  
> > 
> > I am clearly not a big fan of this solution. memorg should be and
> > remain a read only object. Can you please find another solution?
> >   
> I was debating on this too but I don't see option because there is no other hooks after nanddev_init to set the mtd->oobsize as far as I can see and I see there were similar fixes for other controller drivers after the nand device object init was committed, for example:
> Fixes: 629a442cad5f ("mtd: rawnand: Fill memorg during detection")

You are right, that was indeed the first approach, I'll remain on this
path then. Thanks for the pointer.

> 
> I will think through this again but I am open to any suggestion.
> 
> >>
> >> Fixes: a7ab085d7c16 ("mtd: rawnand: Initialize the nand_device object")
> >> Signed-off-by: William Zhang <william.zhang@broadcom.com>
> >>
> >> ---
> >>
> >> Changes in v3: None
> >> Changes in v2: None
> >>
> >>   drivers/mtd/nand/raw/brcmnand/brcmnand.c | 7 +++++--
> >>   1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> index 407bf79cbaf4..39c7f547db1f 100644
> >> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
> >> @@ -2647,6 +2647,8 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
> >>   	struct nand_chip *chip = &host->chip;
> >>   	const struct nand_ecc_props *requirements =
> >>   		nanddev_get_ecc_requirements(&chip->base);
> >> +	struct nand_memory_organization *memorg =
> >> +		nanddev_get_memorg(&chip->base);
> >>   	struct brcmnand_controller *ctrl = host->ctrl;
> >>   	struct brcmnand_cfg *cfg = &host->hwcfg;
> >>   	char msg[128];
> >> @@ -2668,10 +2670,11 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
> >>   	if (cfg->spare_area_size > ctrl->max_oob)
> >>   		cfg->spare_area_size = ctrl->max_oob;
> >>   	/*
> >> -	 * Set oobsize to be consistent with controller's spare_area_size, as
> >> -	 * the rest is inaccessible.
> >> +	 * Set mtd and memorg oobsize to be consistent with controller's
> >> +	 * spare_area_size, as the rest is inaccessible.
> >>   	 */
> >>   	mtd->oobsize = cfg->spare_area_size * (mtd->writesize >> FC_SHIFT);
> >> +	memorg->oobsize = mtd->oobsize;  
> >>   >>   	cfg->device_size = mtd->size;  
> >>   	cfg->block_size = mtd->erasesize;  
> > 
> > 
> > Thanks,
> > Miquèl
> >   


Thanks,
Miquèl