About protecting and unprotecting in spi-nor

About protecting and unprotecting in spi-nor

[Uwe Kleine-König]

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.

 - 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.

 - 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.

   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.

What do you think?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: About protecting and unprotecting in spi-nor

[Tudor.Ambarus]

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/