Re: About protecting and unprotecting in spi-nor

[<Tudor.Ambarus@xxxxxxxxxxxxx>]

Hi,

On 02/21/2019 10:35 AM, Uwe Kleine-König wrote:
> Hello,
> 
> I already mentioned the topic of this mail in the irc channel #mtd, but
> I think it is easier to discuss it here.
> 
> In the past some developers at Pengutronix noticed several problems with
> the code in the spi-nor driver that handles protecting blocks. The
> result is that we effectively disabled all the logic implemented in
> stm_lock() (making it a noop) and stm_unlock() (making it unprotect the
> whole flash).
> 
> The problems I'm aware of are:
> 
>  - There are parts with three BP bits and others with four. The code
>    only handles those with three.

4bit block protection it's on its way:
https://patchwork.ozlabs.org/project/linux-mtd/list/?series=81359

> 
>  - The position of the optional TB bit (that selects locking from the
>    bottom up instead of from the top down) differs between parts. The
>    code only assumes a single position.

we can extend the support

> 
>  - For some parts the smallest protection area is 1/16 of the device,
>    others have 1/64 or 1/256. The code only handles 1/64.
> 
>  - The API to call the locking code is more flexible than the hardware
>    capabilities. So with the API you can request an arbitrarily
>    continuous range of blocks to protect (or unprotect) while most
>    hardware can only work on some powers of two starting from the bottom
>    or going up to the top. For both locking and unlocking the behaviour
>    to handle non-matching requests is to not affect blocks out of the
>    requested range and in doubt handle only a part of the requested
>    area. The problem arising from that is that assuming a part that can
>    lock 1/64 and a 1/32 from the top, the following can happen starting
>    from an unlocked flash:
> 
>    	lock(last 1/64) -> success, last 1/64 locked
> 	lock(the last but one 1/64) -> success, last 1/32 locked
> 	unlock(last 1/64) -> -EINVAL
> 
>    So unless you have some domain specific knowledge the only chance to
>    reliably unlock a certain area is to unlock the complete chip. Ditto
>    for locking.

I'll study the rest and get back to you.

> 
>    IMHO the algorithm must be changed to have locking as is and
>    implement unlocking such that after the call the requested range is
>    unlocked (maybe unlocking more than requested). This might result in
>    other surprises though if you rely on certain blocks being locked.
>    But then you are in trouble already today.
> 
> I'd like to prove the first three items with some data sheets, but the
> information which parts are affected didn't make it into the respective
> commit in our trees, so I cannot provide it without some research. Maybe
> you believe me without this prove or even noticed the same problems?
> 
> In my eyes it's questionable if repairing all that is sensible and I
> wonder how the problems should be addressed. I tend to suggest using the
> approach we chose: Make stm_lock() a noop and stm_unlock() unprotect the
> whole chip.
> 

My first feeling is that we should focus on adding support for the unavailable
features and fix the bugs if present. When all will be solved the "make
stm_lock() a noop and stm_unlock() unprotect the whole chip" approach will be
removed anyway, so why adding code that will be removed?

Manufacturers usually ship the flashes unlocked by default, you can unset the
SPI_NOR_HAS_LOCK flag if the actual code doesn't meet the flash's expectations.

We can take down the problems one by one and if something can not be solved in a
generic way, we can add a per-manufacturer fixup hook (check
https://patchwork.ozlabs.org/project/linux-mtd/list/?series=80353 to make an
idea of the big picture).

Will get back to this,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/