Re: [PATCH dev-5.0 3/4] mtd: spi-nor: aspeed: use memcpy_fromio() to capture the optimization buffer

[Andrew Jeffery]

On Fri, 19 Apr 2019, at 06:53, Milton Miller II wrote:
> About 04/17/2019 09:20AM in some timezone, Cédric Le Goater wrote:
> 
> 
> >aspeed_smc_read_from_ahb() only reads the first word which is not
> >what
> >we want. We want to capture a CALIBRATE_BUF_SIZE size window of the
> >flash contents to optimize the read.
> >
> 
> NACK
> 
> This justifcation is false.  The routine reads the whole buffer
> because it calls the _rep routine and takes the size.
> 
> In addition, the comment just before aspeed_smc_read_from_ahb
> tells why memcpy_fromio and memcpy_toio are broken on 32 bit
> arm, and this is still the case judging from the recent bug
> reportfrom a Nuvaton user [1].
> 
> [1] https://github.com/openbmc/openbmc/issues/3521
> 
> Andrew, Please revert this patch.

Yeah, I had a briefly nagging thought about this when I applied it. I
should have gone with that gut instinct and inspected it in more
depth.

Thanks for calling it out.

Reverting for dev-5.0, this is an isolated issue (arguably should have
been separated from the current series).

Andrew

> 
> >Signed-off-by: Cédric Le Goater <clg at kaod.org>
> >---
> > drivers/mtd/spi-nor/aspeed-smc.c | 6 ++----
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/mtd/spi-nor/aspeed-smc.c
> >b/drivers/mtd/spi-nor/aspeed-smc.c
> >index 1437732fdea1..7e289ecb1c99 100644
> >--- a/drivers/mtd/spi-nor/aspeed-smc.c
> >+++ b/drivers/mtd/spi-nor/aspeed-smc.c
> >@@ -796,8 +796,7 @@ static bool aspeed_smc_check_reads(struct
> >aspeed_smc_chip *chip,
> > 	int i;
> > 
> > 	for (i = 0; i < 10; i++) {
> >-		aspeed_smc_read_from_ahb(test_buf, chip->ahb_base,
> >-					 CALIBRATE_BUF_SIZE);
> >+		memcpy_fromio(test_buf, chip->ahb_base, CALIBRATE_BUF_SIZE);
> > 		if (memcmp(test_buf, golden_buf, CALIBRATE_BUF_SIZE) != 0)
> > 			return false;
> > 	}
> >@@ -921,8 +920,7 @@ static int aspeed_smc_optimize_read(struct
> >aspeed_smc_chip *chip,
> > 
> > 	writel(chip->ctl_val[smc_read], chip->ctl);
> > 
> >-	aspeed_smc_read_from_ahb(golden_buf, chip->ahb_base,
> >-				 CALIBRATE_BUF_SIZE);
> >+	memcpy_fromio(golden_buf, chip->ahb_base, CALIBRATE_BUF_SIZE);
> > 
> > 	/* Establish our read mode with freq field set to 0 (HCLK/16) */
> > 	chip->ctl_val[smc_read] = save_read_val & 0xfffff0ff;
> >-- 
> >2.20.1
> >
> >
> 
>