[PATCH 1/2] mtd: rawnand: Constrain even more when continuous reads are enabled

[PATCH 1/2] mtd: rawnand: Constrain even more when continuous reads are enabled

[Miquel Raynal]

As a matter of fact, continuous reads require additional handling at the
operation level in order for them to work properly. The core helpers do
have this additional logic now, but any time a controller implements its
own page helper, this extra logic is "lost". This means we need another
level of per-controller driver checks to ensure they can leverage
continuous reads. This is for now unsupported, so in order to ensure
continuous reads are enabled only when fully using the core page
helpers, we need to add more initial checks.

Also, as performance is not relevant during raw accesses, we also
prevent these from enabling the feature.

This should solve the issue seen with controllers such as the STM32 FMC2
when in sequencer mode. In this case, the continuous read feature would
be enabled but not leveraged, and most importantly not disabled, leading
to further operations to fail.

Reported-by: Christophe Kerello <christophe.kerello@foss.st.com>
Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads")
Cc: stable@vger.kernel.org
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/nand_base.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 4d5a663e4e05..2479fa98f991 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -3594,7 +3594,8 @@ static int nand_do_read_ops(struct nand_chip *chip, loff_t from,
 	oob = ops->oobbuf;
 	oob_required = oob ? 1 : 0;
 
-	rawnand_enable_cont_reads(chip, page, readlen, col);
+	if (likely(ops->mode != MTD_OPS_RAW))
+		rawnand_enable_cont_reads(chip, page, readlen, col);
 
 	while (1) {
 		struct mtd_ecc_stats ecc_stats = mtd->ecc_stats;
@@ -5212,6 +5213,15 @@ static void rawnand_late_check_supported_ops(struct nand_chip *chip)
 	if (!nand_has_exec_op(chip))
 		return;
 
+	/*
+	 * For now, continuous reads can only be used with the core page helpers.
+	 * This can be extended later.
+	 */
+	if (!(chip->ecc.read_page == nand_read_page_hwecc ||
+	      chip->ecc.read_page == nand_read_page_syndrome ||
+	      chip->ecc.read_page == nand_read_page_swecc))
+		return;
+
 	rawnand_check_cont_read_support(chip);
 }
 
-- 
2.40.1

[PATCH 1/2] mtd: rawnand: Constrain even more when continuous reads are enabled

[Miquel Raynal]

As a matter of fact, continuous reads require additional handling at the
operation level in order for them to work properly. The core helpers do
have this additional logic now, but any time a controller implements its
own page helper, this extra logic is "lost". This means we need another
level of per-controller driver checks to ensure they can leverage
continuous reads. This is for now unsupported, so in order to ensure
continuous reads are enabled only when fully using the core page
helpers, we need to add more initial checks.

Also, as performance is not relevant during raw accesses, we also
prevent these from enabling the feature.

This should solve the issue seen with controllers such as the STM32 FMC2
when in sequencer mode. In this case, the continuous read feature would
be enabled but not leveraged, and most importantly not disabled, leading
to further operations to fail.

Reported-by: Christophe Kerello <christophe.kerello@foss.st.com>
Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads")
Cc: stable@vger.kernel.org
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/nand_base.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 4d5a663e4e05..2479fa98f991 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -3594,7 +3594,8 @@ static int nand_do_read_ops(struct nand_chip *chip, loff_t from,
 	oob = ops->oobbuf;
 	oob_required = oob ? 1 : 0;
 
-	rawnand_enable_cont_reads(chip, page, readlen, col);
+	if (likely(ops->mode != MTD_OPS_RAW))
+		rawnand_enable_cont_reads(chip, page, readlen, col);
 
 	while (1) {
 		struct mtd_ecc_stats ecc_stats = mtd->ecc_stats;
@@ -5212,6 +5213,15 @@ static void rawnand_late_check_supported_ops(struct nand_chip *chip)
 	if (!nand_has_exec_op(chip))
 		return;
 
+	/*
+	 * For now, continuous reads can only be used with the core page helpers.
+	 * This can be extended later.
+	 */
+	if (!(chip->ecc.read_page == nand_read_page_hwecc ||
+	      chip->ecc.read_page == nand_read_page_syndrome ||
+	      chip->ecc.read_page == nand_read_page_swecc))
+		return;
+
 	rawnand_check_cont_read_support(chip);
 }
 
-- 
2.40.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/2] mtd: rawnand: Ensure continuous reads are well disabled

[Miquel Raynal]

The cont_read.ongoing flag should only be enabled at the beginning of a
read operation, and also disabled at its end, so we never end up
triggering nasty side effects outside of this scope. The mtd core being
highly serialized, we should not be bothered by parallel accesses
anyway.

In case we reach the end of a read operation and the boolean was not
properly disabled, it's a bug, but it's totally manageable. So warn, and
then fix the boolean state.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/nand_base.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 2479fa98f991..d7dbbd469b89 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -3728,6 +3728,9 @@ static int nand_do_read_ops(struct nand_chip *chip, loff_t from,
 	}
 	nand_deselect_target(chip);
 
+	if (WARN_ON_ONCE(chip->cont_read.ongoing))
+		chip->cont_read.ongoing = false;
+
 	ops->retlen = ops->len - (size_t) readlen;
 	if (oob)
 		ops->oobretlen = ops->ooblen - oobreadlen;
-- 
2.40.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: rawnand: Constrain even more when continuous reads are enabled

[Christophe Kerello]

Hi Miquel,

On 3/7/24 12:53, Miquel Raynal wrote:
> As a matter of fact, continuous reads require additional handling at the
> operation level in order for them to work properly. The core helpers do
> have this additional logic now, but any time a controller implements its
> own page helper, this extra logic is "lost". This means we need another
> level of per-controller driver checks to ensure they can leverage
> continuous reads. This is for now unsupported, so in order to ensure
> continuous reads are enabled only when fully using the core page
> helpers, we need to add more initial checks.
> 
> Also, as performance is not relevant during raw accesses, we also
> prevent these from enabling the feature.
> 
> This should solve the issue seen with controllers such as the STM32 FMC2
> when in sequencer mode. In this case, the continuous read feature would
> be enabled but not leveraged, and most importantly not disabled, leading
> to further operations to fail.
> 
> Reported-by: Christophe Kerello <christophe.kerello@foss.st.com>
> Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads")
> Cc: stable@vger.kernel.org
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Tested-by: Christophe Kerello <christophe.kerello@foss.st.com>

Regards,
Christophe Kerello.

> ---
>   drivers/mtd/nand/raw/nand_base.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 4d5a663e4e05..2479fa98f991 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -3594,7 +3594,8 @@ static int nand_do_read_ops(struct nand_chip *chip, loff_t from,
>   	oob = ops->oobbuf;
>   	oob_required = oob ? 1 : 0;
>   
> -	rawnand_enable_cont_reads(chip, page, readlen, col);
> +	if (likely(ops->mode != MTD_OPS_RAW))
> +		rawnand_enable_cont_reads(chip, page, readlen, col);
>   
>   	while (1) {
>   		struct mtd_ecc_stats ecc_stats = mtd->ecc_stats;
> @@ -5212,6 +5213,15 @@ static void rawnand_late_check_supported_ops(struct nand_chip *chip)
>   	if (!nand_has_exec_op(chip))
>   		return;
>   
> +	/*
> +	 * For now, continuous reads can only be used with the core page helpers.
> +	 * This can be extended later.
> +	 */
> +	if (!(chip->ecc.read_page == nand_read_page_hwecc ||
> +	      chip->ecc.read_page == nand_read_page_syndrome ||
> +	      chip->ecc.read_page == nand_read_page_swecc))
> +		return;
> +
>   	rawnand_check_cont_read_support(chip);
>   }
>   

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: rawnand: Constrain even more when continuous reads are enabled

[Christophe Kerello]

Hi Miquel,

On 3/7/24 12:53, Miquel Raynal wrote:
> As a matter of fact, continuous reads require additional handling at the
> operation level in order for them to work properly. The core helpers do
> have this additional logic now, but any time a controller implements its
> own page helper, this extra logic is "lost". This means we need another
> level of per-controller driver checks to ensure they can leverage
> continuous reads. This is for now unsupported, so in order to ensure
> continuous reads are enabled only when fully using the core page
> helpers, we need to add more initial checks.
> 
> Also, as performance is not relevant during raw accesses, we also
> prevent these from enabling the feature.
> 
> This should solve the issue seen with controllers such as the STM32 FMC2
> when in sequencer mode. In this case, the continuous read feature would
> be enabled but not leveraged, and most importantly not disabled, leading
> to further operations to fail.
> 
> Reported-by: Christophe Kerello <christophe.kerello@foss.st.com>
> Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads")
> Cc: stable@vger.kernel.org
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Tested-by: Christophe Kerello <christophe.kerello@foss.st.com>

Regards,
Christophe Kerello.

> ---
>   drivers/mtd/nand/raw/nand_base.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 4d5a663e4e05..2479fa98f991 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -3594,7 +3594,8 @@ static int nand_do_read_ops(struct nand_chip *chip, loff_t from,
>   	oob = ops->oobbuf;
>   	oob_required = oob ? 1 : 0;
>   
> -	rawnand_enable_cont_reads(chip, page, readlen, col);
> +	if (likely(ops->mode != MTD_OPS_RAW))
> +		rawnand_enable_cont_reads(chip, page, readlen, col);
>   
>   	while (1) {
>   		struct mtd_ecc_stats ecc_stats = mtd->ecc_stats;
> @@ -5212,6 +5213,15 @@ static void rawnand_late_check_supported_ops(struct nand_chip *chip)
>   	if (!nand_has_exec_op(chip))
>   		return;
>   
> +	/*
> +	 * For now, continuous reads can only be used with the core page helpers.
> +	 * This can be extended later.
> +	 */
> +	if (!(chip->ecc.read_page == nand_read_page_hwecc ||
> +	      chip->ecc.read_page == nand_read_page_syndrome ||
> +	      chip->ecc.read_page == nand_read_page_swecc))
> +		return;
> +
>   	rawnand_check_cont_read_support(chip);
>   }
>   

Re: [PATCH 2/2] mtd: rawnand: Ensure continuous reads are well disabled

[Miquel Raynal]

On Thu, 2024-03-07 at 11:53:15 UTC, Miquel Raynal wrote:
> The cont_read.ongoing flag should only be enabled at the beginning of a
> read operation, and also disabled at its end, so we never end up
> triggering nasty side effects outside of this scope. The mtd core being
> highly serialized, we should not be bothered by parallel accesses
> anyway.
> 
> In case we reach the end of a read operation and the boolean was not
> properly disabled, it's a bug, but it's totally manageable. So warn, and
> then fix the boolean state.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/2] mtd: rawnand: Constrain even more when continuous reads are enabled

[Miquel Raynal]

On Thu, 2024-03-07 at 11:53:14 UTC, Miquel Raynal wrote:
> As a matter of fact, continuous reads require additional handling at the
> operation level in order for them to work properly. The core helpers do
> have this additional logic now, but any time a controller implements its
> own page helper, this extra logic is "lost". This means we need another
> level of per-controller driver checks to ensure they can leverage
> continuous reads. This is for now unsupported, so in order to ensure
> continuous reads are enabled only when fully using the core page
> helpers, we need to add more initial checks.
> 
> Also, as performance is not relevant during raw accesses, we also
> prevent these from enabling the feature.
> 
> This should solve the issue seen with controllers such as the STM32 FMC2
> when in sequencer mode. In this case, the continuous read feature would
> be enabled but not leveraged, and most importantly not disabled, leading
> to further operations to fail.
> 
> Reported-by: Christophe Kerello <christophe.kerello@foss.st.com>
> Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads")
> Cc: stable@vger.kernel.org
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Tested-by: Christophe Kerello <christophe.kerello@foss.st.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next.

Miquel

Re: [PATCH 1/2] mtd: rawnand: Constrain even more when continuous reads are enabled

[Miquel Raynal]

On Thu, 2024-03-07 at 11:53:14 UTC, Miquel Raynal wrote:
> As a matter of fact, continuous reads require additional handling at the
> operation level in order for them to work properly. The core helpers do
> have this additional logic now, but any time a controller implements its
> own page helper, this extra logic is "lost". This means we need another
> level of per-controller driver checks to ensure they can leverage
> continuous reads. This is for now unsupported, so in order to ensure
> continuous reads are enabled only when fully using the core page
> helpers, we need to add more initial checks.
> 
> Also, as performance is not relevant during raw accesses, we also
> prevent these from enabling the feature.
> 
> This should solve the issue seen with controllers such as the STM32 FMC2
> when in sequencer mode. In this case, the continuous read feature would
> be enabled but not leveraged, and most importantly not disabled, leading
> to further operations to fail.
> 
> Reported-by: Christophe Kerello <christophe.kerello@foss.st.com>
> Fixes: 003fe4b9545b ("mtd: rawnand: Support for sequential cache reads")
> Cc: stable@vger.kernel.org
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Tested-by: Christophe Kerello <christophe.kerello@foss.st.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next.

Miquel

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/