Message-Id: <D37UT8JYBDX1.2OURSPGD336B1@kernel.org>
Date: Mon, 05 Aug 2024 11:01:09 +0200
From: "Michael Walle" <mwalle@...nel.org>
To: "Brian Norris" <computersforpeace@...il.com>
Cc: "Tudor Ambarus" <tudor.ambarus@...aro.org>,
 <linux-mtd@...ts.infradead.org>, "Miquel Raynal"
 <miquel.raynal@...tlin.com>, "Pratyush Yadav" <pratyush@...nel.org>,
 "Richard Weinberger" <richard@....at>, "Vignesh Raghavendra"
 <vigneshr@...com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mtd: spi-nor: micron-st: Add n25q064a WP support

Hi,

> > We really need some kind of dump the relevant registers here. I have
> > some very old patch, which dumps the status register, but is has
> > it's own quirks. But IMHO we should (maybe additional to the
> > functional tests) look at the locking bits in the corresponding
> > registers. I.e.
> >  flash_lock foobar
> >  <verify the status register>
> >  flash_unlock foobar
> >  <verify the status register>
> >  flash_lock barfoo
> >  <verify the status register>
> >  etc.
>
> I don't actually think that would be a very good test. It would be
> testing the implementation more than the functionality. What do you
> "verify" in the status register? Would the test just re-implement the
> swp.c protection-range logic? And notably, this omits *all* checks
> that the protection register actually protects from anything (write,
> erase).
>
> Or maybe I'm misinterpreting what you mean.

No that's what I've meant. Maybe I'm having another perspective and
I'm biased because of that, but I'm really not trusting the software
layer, esp. not when it comes to flash locking. Because most of the
time this involves the "very last resort recovery" on our products.
So we really have to ensure the flash is locked and therefore, one
*must* look at the corresponding bits in the hardware (using the
simplest method possible). The beauty of the lock bits is that if
you know they are set, there is nothing software can do to write or
erase these sectors. Once you know all the bits are set correctly,
you can just skip the write/erase/read test.

> > Just inferring the correctness from behavior (exercised by writing
> > to the flash and verifying it) will lead to errors as it is hard to
> > catch all the corner cases.
>
> Why would that lead to errors?

Ever tried to lock two different ranges after another? Or unlock a
smaller range than the one that is currently locked? IIRC, that
should work. But I've never tried it myself. Or just locking (parts)
of a smaller partition (i.e. an mtd device which doesn't cover the
whole spi flash).

> It should be relatively easy to:
>
> 1. enumerate the supported protection ranges (MEMLOCK / MEMUNLOCK
> ioctls on known-likely ranges, looking for EINVAL return codes)
> 2. iterate through all such ranges; for a given range:
> 2(a). erase the whole flash
> 2(b). write the whole flash with a known pattern
> 2(c). read the whole flash
> 2(d). ensure that the expected-protected range remains 0xff
> 2(e). ensure that the expected-unprotected range contains the known pattern

Not sure 2(a) will work or if some flashes will reject the chip
erase command if some sectors are locked. To sidestep this I guess
you should use the smallest possible erase (i.e. ususally the 4k
erase opcode).

But yeah, once this is all in place you can probably do that with a
tool, otherwise it's really tedious work and doing it by hand is
error prone.

> I suppose step #1 can be tough, because the full slate of possible
> protection ranges is technically ... enormous. But "likely" ranges are
> much fewer, with a few power-of-2 patterns, top/bottom, and maybe some
> "both top and bottom" ranges on some flashes? Anyway, like I said in
> my other reply, this should take on the order of 60 minutes on some
> flashes, which is expensive but not prohibitive.

Well we support 4 block protection bits and one TB bit. So there are
2^5 different configurations?

-michael

> > From what I remember, flashrom has it's own drivers in userspace,
> > no?
>
> Yes, and that's all rather ugly. But it also has a linux_mtd backend
> since a few years back:
>
> https://review.coreboot.org/plugins/gitiles/flashrom/+/HEAD/linux_mtd.c
>
> Brian


Download attachment "signature.asc" of type "application/pgp-signature" (298 bytes)

Powered by blists - more mailing lists