Re: [PATCH v2] mtd: spi-nor: keep lock bits if they are non-volatile

[Tudor.Ambarus]

Hi, Michael, Vignesh,

On Sunday, January 12, 2020 12:50:57 AM EET Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi Tudor,
> 
> thanks for looking into this.
> 
> Am 2020-01-11 14:46, schrieb Tudor.Ambarus@xxxxxxxxxxxxx:
> > Hi, Michael,
> > 
> > On Saturday, January 4, 2020 12:12:29 AM EET Michael Walle wrote:
> >> Traditionally, linux unlocks the whole flash because there are legacy
> >> devices which has the write protections bits set by default at
> >> startup.
> >> If you actually want to use the flash protection bits, eg. because
> >> there
> >> is a read-only part for a bootloader, this automatic unlocking is
> >> harmful. If there is no hardware write protection in place (usually
> >> called WP#), a startup of the kernel just discards this protection.
> >> 
> >> I've gone through the datasheets of all the flashes (except the Intel
> >> ones where I could not find any datasheet nor reference) which
> >> supports
> >> the unlocking feature and looked how the sector protection was
> >> implemented. The currently supported flashes can be divided into the
> >> 
> >> following two categories:
> >>  (1) block protection bits are non-volatile. Thus they keep their
> >> 
> >> values
> >> 
> >>      at reset and power-cycle
> >>  
> >>  (2) flashes where these bits are volatile. After reset or
> >> 
> >> power-cycle,
> >> 
> >>      the whole memory array is protected.
> >>      (a) some devices needs a special "Global Unprotect" command, eg.
> >>      
> >>          the Atmel AT25DF041A.
> >>      
> >>      (b) some devices require to clear the BPn bits in the status
> >>      
> >>          register.
> >> 
> >> Due to the reasons above, we do not want to clear the bits for flashes
> >> which belong to category (1). Fortunately for us, the flashes in (2a)
> >> and (2b) are compatible with each other in a sense that the "Global
> >> Unprotect" command will clear the block protection bits in all the
> >> (2b)
> >> flashes.
> >> 
> >> This patch adds a new flag to indicate the case (2). Only if we have
> >> such a flash we perform a "Global Unprotect". Hopefully, this will
> >> clean
> >> up "unlock the entire flash for legacy devices" once and for all.
> > 
> > Thanks for the detailed explanation. Unlocking the flash at probe time
> > was
> > badly designed from the beginning, we should disable the write
> > protection only
> > on request, to avoid destructive commands during power-up.
> > 
> > Breaking the backward compatibility is a no-go, and looks like you
> > break it,
> > by not treating case (1).
> 
> Yes but that was the whole idea of this patch. So if I get you correct
> it is
> not possible to change that even if:
> 
> (1) it was never intended that way. Eg. the original patch(es) were
> about
> removing the volatile write protection (which makes perfectly sense,
> even
> during probe time) to be able to write to the flash. But it was never
> intended
> to disable the non-volatile write protection.
> 
> (2) it might be even harmful. It is still an open question wether the
> write
> to the non-volatile bits (even if it is the same value) might wear them
> out.
> Unfortunately our FAE didn't answered yet..
> 
> (3) it makes the write protection utterly useless, because if you lock
> the
> flash it will be automatically unlocked after the next reboot. Even
> worse, the
> user likely won't notice it.

Breaking backward compatibility and keeping the locking state of the spi-nor 
flashes at probe is a no-go, because there might be user space apps that 
expect that all the spi-nor flashes are by default unlocked. The unlocking of 
the flash at probe time was introduced 12 years ago, we definitely can't 
change this now.

> 
> > We can indeed continue your idea and treat both (1)
> > and (2), thus disabling the write protection at power-up for all the
> > flashes
> > that we support as of now (in order to not break backward compat), and
> > to not
> > disable the block protection for the new flashes that will come. This
> > means to
> > have some point in time before which some less fortunate flashes don't
> > benefit
> > of write protection at power-up, and after which the others benefit. I
> > wouldn't got this way, I prefer a generic method that handles all the
> > flashes
> > in the same way.
> 
> I'd also prefer a solution for all existing flashes. But it seems that
> the rule
> above makes that almost impossible. Esp. this behaviour will then be
> there until
> eternity.
> 
> > I see three choices:
> > 1/ dt prop which gives a per flash granularity. The prop is related to
> > hw
> > protection and there might be some chances to get this accepted, maybe
> > it is
> > worth to involve Rob. But I tend to share Vignesh's opinion, this would
> > configure the flash and not describe it.
> 
> Still my preferred way. but also see below. But I wouldn't say it

Try to convince Rob.

> configures the
> flash but describe that the user want to use the write protection.
> 
> > 2/ kconfig option, the behavior would be enforced on all the flashes.
> > It would
> > be similar to what we have with CONFIG_MTD_SPI_NOR_USE_4K_SECTORS. I
> > did a
> > patch to address this some time ago:
> > https://patchwork.ozlabs.org/patch/
> > 1133278/
> 
> Mhh. If we would combine this with this patch that would be at least a
> step into
> the right direction. At least a distro could enable that kernel option
> without
> breaking old boards/flashes. Because as outlined about you need that for
> flashes
> in category (2). Or you'd have to do a flash_unlock every time you want
> to write
> to it. But that would be really a backwards incompatible change.. ;)
> 
> > 3/ module param, the behavior would be enforced on all the flashes.
> > 
> > Preferences or suggestions?
> 
I would go with 2/ or 3/. Vignesh, what do you prefer and why?

Cheers,
ta