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

[sashiko-bot@kernel.org]

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