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

[Andrew Jeffery]

Hi Cédric

On Fri, 19 Apr 2019, at 15:32, Cédric Le Goater wrote:
> On 4/19/19 3:03 AM, Andrew Jeffery wrote:
> > 
> > 
> > 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.
> 
> It doesn't AFAICT looking at other drivers and also from my test.
> 
> >> In addition, the comment just before aspeed_smc_read_from_ahb
> >> tells why memcpy_fromio and memcpy_toio are broken on 32 bit
> 
> That might have been the case on Linux 4.7. It doesn't seem to be 
> the case anymore. See below.
> 
> >> 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.
> 
> I don't think you have enough convincing arguments for that.

That may be the case, but having seen the pain of the original corruption
problems that drove the ioreadX_rep() implementation above, Milton's
protest combined with my initial, briefly nagging concern was enough for
me to revert. Two things here:

1. We've run without this patch for quite some time. Despite oddities,
the existing implementation has been stable
2. With patch 4/4, you've resolved the 4B corruption problem. This was
the immediate concern, as it was impacting teams developing and
testing OpenBMC master. I appreciate the effort you put into hunting
that down, the root cause was certainly not obvious.

>From *my* testing we appear to be stable both with and without this
change, however my concern is that *my* testing is not complete enough
to be confident that we're not going to hit the subtle corruption problems
that drove the introduction of the existing code.

For some additional context, I'm now on leave until the 30th, and Joel to
the 29th. These patches have been through a process that has proceeded
much more hastily than I would have liked, and that's lead to where we
are now.

With that in mind, less change is better, and so I have decided to back
this patch out. It's a risk-based decision, not a reflection of the effort,
desires or technicalities involved.

So there are lessons in this for me too as I've felt squeezed by the
impact the 4B corruption bug has had on teams testing master, and
my impending leave. With that reasoning and 20/20 hindsight I should
probably have held off to begin with. Next time. Some reflection is in
order.

Anyway, I hope that helps clarify. Time to resolve the disagreement here.

Milton: Given your protest, can you please verify whether there continue
to be issues with memcpy_fromio() that prevent us from making use of it.
As Cédric points out we do make heavy use of it anyway, and using a
well-worn implementation rather than something bespoke for this particular
case would improve maintainability.

Cheers,

Andrew
 
> 
> > 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.
> 
> I think this needs more thinking and tests. Witherspoon and palmetto
> are fine. What's the problem.
> 
> > Reverting for dev-5.0, this is an isolated issue (arguably should have
> > been separated from the current series).
> 
> We have been using memcpy_fromio for reads for a very long time 
> since 4.13 in the Aspeed SMC driver on *all* platforms*. 
> 
> See aspeed_smc_read() for the "Command mode".
> 
> C.
> 
> 
> 
> 
> > 
> > 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
> >>>
> >>>
> >>
> >>
> 
>