[PATCH] mtd: spinand: Do not re-enable continuous reads vetoed during variant selection

[PATCH] mtd: spinand: Do not re-enable continuous reads vetoed during variant selection

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

spinand_match_and_init() clears cont_read_possible when the fastest
continuous read variant is slower than the fastest read from cache
variant, or when no SSDR/ODTR continuous read variant is supported by
the controller. However, at that point cont_read_possible has not been
set yet, and spinand_cont_read_init(), which runs later during
spinand_init(), unconditionally sets it to true for any chip
implementing ->set_cont_read().

As a result, continuous reads end up being used while
op_templates->cont_read_cache is NULL. spinand_create_dirmap() then does
not provide a secondary template and spinand_read_from_cache_op() falls
back to the regular read from cache operation while the chip is in
continuous read mode. On Winbond chips, which do not expect the column
address in this mode, this misaligns the data returned.

Track the decision in a dedicated cont_read_unsupported flag and check it
in spinand_cont_read_init() before enabling continuous reads.

Fixes: 6eb7c193e751 ("mtd: spinand: Use secondary ops for continuous reads")
Assisted-by: LLM
Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
I was using an LLM to check sync patches for U-Boot and the LLM reported
a bug in the upstream kernel code. To me it looks like this is indeed a
bug and the fix also looks correct to me, but I would like to mention, that
this is purely LLM-generated and theoretical. I didn't verify the bug and
the fix on actual hardware.

@Miquel: Do you have a hardware setup that would be affected by this? Do
you think there is a better fix?
---
 drivers/mtd/nand/spi/core.c | 9 +++++----
 include/linux/mtd/spinand.h | 5 +++++
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 95353777d7f7..e83341290ec3 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -995,7 +995,8 @@ static void spinand_cont_read_init(struct spinand_device *spinand)
 
 		if ((engine_type == NAND_ECC_ENGINE_TYPE_ON_DIE ||
 		     engine_type == NAND_ECC_ENGINE_TYPE_NONE) &&
-		    !spi_mem_controller_is_capable(ctlr, no_cs_assertion))
+		    !spi_mem_controller_is_capable(ctlr, no_cs_assertion) &&
+		    !spinand->cont_read_unsupported)
 			spinand->cont_read_possible = true;
 	}
 }
@@ -1700,11 +1701,11 @@ int spinand_match_and_init(struct spinand_device *spinand,
 				    (read_op->addr.dtr && !op->addr.dtr) ||
 				    read_op->data.buswidth > op->data.buswidth ||
 				    (read_op->data.dtr && !op->data.dtr))
-					spinand->cont_read_possible = false;
+					spinand->cont_read_unsupported = true;
 				else
 					spinand->ssdr_op_templates.cont_read_cache = op;
 			} else {
-				spinand->cont_read_possible = false;
+				spinand->cont_read_unsupported = true;
 			}
 		}
 
@@ -1736,7 +1737,7 @@ int spinand_match_and_init(struct spinand_device *spinand,
 			if (op)
 				spinand->odtr_op_templates.cont_read_cache = op;
 			else
-				spinand->cont_read_possible = false;
+				spinand->cont_read_unsupported = true;
 		}
 
 		return 0;
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 5f4c00ae72a7..2c90426c4000 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -772,6 +772,10 @@ struct spinand_mem_ops {
  *		suitable to use or not in general with this chip/configuration.
  *		A per-transfer check must of course be done to ensure it is
  *		actually relevant to enable this feature.
+ * @cont_read_unsupported: Set by the core during I/O variant selection when
+ *			   continuous reads cannot be used with the selected
+ *			   variants. Prevents @cont_read_possible from being
+ *			   set later on.
  * @set_cont_read: Enable/disable the continuous read feature
  * @priv: manufacturer private data
  * @fact_otp: SPI NAND factory OTP info.
@@ -810,6 +814,7 @@ struct spinand_device {
 	int (*configure_chip)(struct spinand_device *spinand,
 			      enum spinand_bus_interface iface);
 	bool cont_read_possible;
+	bool cont_read_unsupported;
 	int (*set_cont_read)(struct spinand_device *spinand,
 			     bool enable);
 	int (*set_randomizer)(struct spinand_device *spinand,

---
base-commit: a90ee4305c4a5df72c11b31dacfdc76e00fcf78a
change-id: 20261007-spinand-cont-read-supported-cdaf25f0bf45

Best regards,
--  
Frieder Schrempf <frieder.schrempf@kontron.de>

Re: [PATCH] mtd: spinand: Do not re-enable continuous reads vetoed during variant selection

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mtd: spinand: cont_read_unsupported flag cross-contaminates SSDR and ODTR states
--

commit b383d1cfd2600477f020bc396d311d923c48ff86
Author: Frieder Schrempf <frieder.schrempf@kontron.de>

mtd: spinand: Do not re-enable continuous reads vetoed during variant selection

spinand_match_and_init() clears cont_read_possible when the fastest
continuous read variant is slower than the fastest read from cache
variant, or when no SSDR/ODTR continuous read variant is supported by
the controller. However, at that point cont_read_possible has not been
set yet, and spinand_cont_read_init(), which runs later during
spinand_init(), unconditionally sets it to true for any chip
implementing ->set_cont_read().

> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 95353777d7f74..e83341290ec32 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -995,7 +995,8 @@ static void spinand_cont_read_init(struct spinand_device *spinand)
>  
>  		if ((engine_type == NAND_ECC_ENGINE_TYPE_ON_DIE ||
>  		     engine_type == NAND_ECC_ENGINE_TYPE_NONE) &&
> -		    !spi_mem_controller_is_capable(ctlr, no_cs_assertion))
> +		    !spi_mem_controller_is_capable(ctlr, no_cs_assertion) &&
> +		    !spinand->cont_read_unsupported)

[Severity: Medium]
Will this erroneously disable continuous reads for valid bus interface
configurations?

Prior to this patch, spinand_cont_read_init() unconditionally set
cont_read_possible to true, masking the fact that the old flag was being
shared across SSDR and ODTR evaluations. By checking the shared flag here,
it appears continuous read is disabled if either interface failed the check,
even if the driver successfully configures and uses an interface that
supports it.

>  			spinand->cont_read_possible = true;
>  	}
>  }

[ ... ]

> @@ -1700,11 +1701,11 @@ int spinand_match_and_init(struct spinand_device *spinand,
>  				    (read_op->addr.dtr && !op->addr.dtr) ||
>  				    read_op->data.buswidth > op->data.buswidth ||
>  				    (read_op->data.dtr && !op->data.dtr))
> -					spinand->cont_read_possible = false;
> +					spinand->cont_read_unsupported = true;

[Severity: Medium]
If the SSDR continuous read is unsupported or slower, this sets the global
spinand->cont_read_unsupported to true.

Later, if ODTR supports continuous read, it populates the ODTR template but
does not clear this flag. Could this incorrectly prevent ODTR from using
continuous read?

>  				else
>  					spinand->ssdr_op_templates.cont_read_cache = op;
>  			} else {
> -				spinand->cont_read_possible = false;
> +				spinand->cont_read_unsupported = true;
>  			}
>  		}
>  
> @@ -1736,7 +1737,7 @@ int spinand_match_and_init(struct spinand_device *spinand,
>  			if (op)
>  				spinand->odtr_op_templates.cont_read_cache = op;
>  			else
> -				spinand->cont_read_possible = false;
> +				spinand->cont_read_unsupported = true;

[Severity: Medium]
If SSDR supports continuous read but ODTR does not, this unconditionally sets
the global flag to true, overwriting any successful SSDR state.

Does this cross-contaminate the state and incorrectly disable continuous reads
for a flash chip that supports continuous reads on SSDR?

>  		}
>  
>  		return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007-spinand-cont-read-supported-v1-1-411facc04d99@kontron.de?part=1