Re: [PATCH v2 7/8] mtd: spi-nor: add TB (Top/Bottom) protect support

[Brian Norris]

On Mon, Feb 29, 2016 at 05:35:02PM -0300, Ezequiel Garcia wrote:
> On 29 January 2016 at 16:25, Brian Norris <computersforpeace@xxxxxxxxx> wrote:
> > Some flash support a bit in the status register that inverts protection
> > so that it applies to the bottom of the flash, not the top. This yields
> > additions to the protection range table, as noted in the comments.
> >
> > Because this feature is not universal to all flash that support
> > lock/unlock, control it via a new flag.
> >
> > Signed-off-by: Brian Norris <computersforpeace@xxxxxxxxx>
> > ---
> > v2:
> >  * Rewrite the bounds checking for top/bottom support, since there were some
> >    bad corner cases. Now lock/unlock are more symmetric.
> >
> >  drivers/mtd/spi-nor/spi-nor.c | 70 ++++++++++++++++++++++++++++++++++++++-----
> >  include/linux/mtd/spi-nor.h   |  2 ++
> >  2 files changed, 65 insertions(+), 7 deletions(-)
> >
> [..]
> > @@ -476,12 +484,14 @@ static int stm_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
> >
> >  /*
> >   * Lock a region of the flash. Compatible with ST Micro and similar flash.
> > - * Supports only the block protection bits BP{0,1,2} in the status register
> > + * Supports the block protection bits BP{0,1,2} in the status register
> >   * (SR). Does not support these features found in newer SR bitfields:
> > - *   - TB: top/bottom protect - only handle TB=0 (top protect)
> >   *   - SEC: sector/block protect - only handle SEC=0 (block protect)
> 
> While reviewing and testing this patchset, I realised that *no* Micron device
> define BIT(6) as SEC (sector/block) bit. Instead, it's used as BP3, to extend
> the region defined by BP0-BP2.

Hmm, OK. Maybe it's worth a note, if it's not going to get fixed
immediately.

> I've checked the following:
> 
>   N25Q256A
>   N25Q128A
>   N25Q064A
>   N25Q032A
>   N25Q016A
>   M25Pxx
> 
> So I believe we need to separate stm_{lock,unlock), from
> winbond_{lock,unlock}.

I'm not yet confident that we need separate functions. We would just
make SEC and BP3 support mutually exclusive, and then we can see whether
separate functions or a dual-purpose (single) implementation makes more
sense. I'd think the latter, actually, since adding an extra bit to the
'mask' should be pretty simple.

> We might want to explicitly mark devices that
> currently support locking with the new _HAS_LOCK flag.

Yeah, I think there are enough problems that we at least need a
_HAS_LOCK flag to opt in, rather than assuming every device by a certain
vendor works. It's really not clear which devices we claimed ever used
to work with lock/unlock, and some will change over time -- possibly
even in incompatible ways. You never know how wrong vendors can make
things.

> Also, I wonder if we can really separate based on vendor, or if we'll need
> more flags to distinguish the lock implementation per device.

For now, I'd like it if we can transition to using SPI_NOR_HAS_LOCK for
every flash that supports it, instead of auto-opting in all
Micron/STMicro flash. I think a new flag for SPI_NOR_HAS_BP3 would also
be in order.

> Of course, all the devices that define a BP3 are broken with respect to flash
> locking. I can try to cook some patches for this, once we are decided on how
> to do it.

Brian