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 17:39, Cédric Le Goater wrote:
> On 4/19/19 9:23 AM, Andrew Jeffery wrote:
> > 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.
> 
> QEMU would have caught this regression if SFDP was modeled. It does today
> if 4B opcodes are forced on the mx25l25635e. Given the time the team spent
> on this, I would say 1 or 2PM overall, QEMU is a good investment. 
>                                                   ^
>                                                   |
> Managers are you reading this ? ------------------+ 
> 
> > 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.
> 
> Back to where we were before, it's fine as it works. 
> 
> The optimize reads are just reading the first 4 bytes :
> 
> [   14.130480] aspeed-smc 1e630000.spi: mx25l25635e (32768 Kbytes)
> [   14.136664] aspeed-smc 1e630000.spi: write control register: 00122302
> [   14.143326] aspeed-smc 1e630000.spi: read control register: 203c2341
> [   14.149750] aspeed-smc 1e630000.spi: AHB frequency: 192 MHz
> [   14.181561] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54  PARTPARTPARTPART
> [   14.188894] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54  PARTPARTPARTPART
> [   14.196230] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54  PARTPARTPARTPART
> [   14.203558] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54  PARTPARTPARTPART
> [   14.210751] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54  PARTPARTPARTPART
> [   14.218067] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54  PARTPARTPARTPART
> [   14.225397] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54  PARTPARTPARTPART
> [   14.232722] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54  PARTPARTPARTPART
> [   14.239912] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54  PARTPARTPARTPART
> [   14.247232] 50 41 52 54 50 41 52 54 50 41 52 54 50 41 52 54  PARTPARTPARTPART
> 
> with memcpy_fromio :
> 
> [   13.779087] aspeed-smc 1e630000.spi: mx25l25635e (32768 Kbytes)
> [   13.785255] aspeed-smc 1e630000.spi: write control register: 00122302
> [   13.791762] aspeed-smc 1e630000.spi: read control register: 203c2341
> [   13.798326] aspeed-smc 1e630000.spi: AHB frequency: 192 MHz
> [   13.815420] 50 41 52 54 00 00 00 01 00 00 00 01 00 00 00 80  PART............
> [   13.822759] 00 00 00 1b 00 00 10 00 00 00 20 00 00 00 00 00  .......... .....
> [   13.829946] 00 00 00 00 00 00 00 00 00 00 00 00 50 41 62 cf  ............PAb.
> [   13.837266] 70 61 72 74 00 00 00 00 00 00 00 00 00 00 00 00  part............
> [   13.844597] 00 00 00 00 00 00 00 01 ff ff ff ff 00 00 00 01  ................
> [   13.851788] 00 00 00 03 00 00 00 01 00 00 10 00 00 00 00 00  ................
> [   13.859105] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> [   13.866433] 00 40 00 00 00 00 00 00 00 00 00 00 00 00 00 00  . at ..............
> [   13.873759] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> [   13.880951] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> [   13.888267] 00 00 00 00 00 00 00 00 00 00 00 00 8f de 9d 89  ................
> [   13.895591] 48 42 49 00 00 00 00 00 00 00 00 00 00 00 00 00  HBI.............
> [   13.902917] 00 00 00 10 00 00 05 a0 ff ff ff ff 00 00 00 02  ................
> 
> 
> I should have added these tests in the commit log. Sorry about that.
> We will see later on. There are no hurries for this fix. Optimization
> is still being done.

Milton: Given you NACK'ed the patch I'd appreciate a follow-up in light of
this data.

Cheers,

Andrew