Re: How to handle write-protect pin of NAND device ?

[Boris Brezillon]

On Mon, 27 Jan 2020 16:47:55 +0100
Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:

> Hi Hello,
> 
> Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote on Mon, 27 Jan
> 2020 16:45:54 +0100:
> 
> > On Mon, 27 Jan 2020 15:35:59 +0100
> > Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
> >   
> > > Hi Masahiro,
> > > 
> > > Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote on Mon, 27 Jan 2020
> > > 21:55:25 +0900:
> > >     
> > > > Hi.
> > > > 
> > > > I have a question about the
> > > > WP_n pin of a NAND chip.
> > > > 
> > > > 
> > > > As far as I see, the NAND framework does not
> > > > handle it.      
> > > 
> > > There is a nand_check_wp() which reads the status of the pin before
> > > erasing/writing.
> > >     
> > > > 
> > > > Instead, it is handled in a driver level.
> > > > I see some DT-bindings that handle the WP_n pin.
> > > > 
> > > > $ git grep wp -- Documentation/devicetree/bindings/mtd/
> > > > Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt:-
> > > > brcm,nand-has-wp          : Some versions of this IP include a
> > > > write-protect      
> > > 
> > > Just checked: brcmnand de-assert WP when writing/erasing and asserts it
> > > otherwise. IMHO this switching is useless.
> > >     
> > > > Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt:-
> > > > wp-gpios: GPIO specifier for the write protect pin.
> > > > Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt:
> > > >          wp-gpios = <&gpf 22 GPIO_ACTIVE_LOW>;
> > > > Documentation/devicetree/bindings/mtd/nvidia-tegra20-nand.txt:-
> > > > wp-gpios: GPIO specifier for the write protect pin.
> > > > Documentation/devicetree/bindings/mtd/nvidia-tegra20-nand.txt:
> > > >          wp-gpios = <&gpio TEGRA_GPIO(S, 0) GPIO_ACTIVE_LOW>;      
> > > 
> > > In both cases, the WP GPIO is unused in the code, just de-asserted at
> > > boot time like what you do in the patch below.
> > >     
> > > > 
> > > > 
> > > > 
> > > > I wrote a patch to avoid read-only issue in some cases:
> > > > http://patchwork.ozlabs.org/patch/1229749/
> > > > 
> > > > Generally speaking, we expect NAND devices
> > > > are writable in Linux. So, I think my patch is OK.      
> > > 
> > > I think the patch is fine.
> > >     
> > > > 
> > > > 
> > > > However, I asked this myself:
> > > > Is there a useful case to assert the write protect
> > > > pin in order to make the NAND chip really read-only?
> > > > For example, the system recovery image is stored in
> > > > a read-only device, and the write-protect pin is
> > > > kept asserted to assure nobody accidentally corrupts it.      
> > > 
> > > It is very likely that the same device is used for RO and RW storage so
> > > in most cases this is not possible. We already have squashfs which is
> > > actually read-only at filesystem level, I'm not sure it is needed to
> > > enforce this at a lower level... Anyway if there is actually a pin for
> > > that, one might want to handle the pin directly as a GPIO, what do you
> > > think?    
> > 
> > FWIW, I've always considered the WP pin as a way to protect against
> > spurious destructive command emission, which is most likely to happen
> > during transition phases (bootloader -> linux, linux -> kexeced-linux,
> > platform reset, ..., or any other transition where the pin state might
> > be undefined at some point). This being said, if you're worried about
> > other sources of spurious cmds (say your bus is shared between
> > different kind of memory devices, and the CS pin is unreliable), you
> > might want to leave the NAND in a write-protected state de-asserting WP
> > only when explicitly issuing a destructive command (program page, erase
> > block).  
> 
> Ok so with this in mind, only the brcmnand driver does a useful use of
> the WP output.

Well, I'd just say that brcmnand is more paranoid, which is a good
thing I guess, but that doesn't make other solutions useless, just less
safe. We could probably flag operations as 'destructive' at the
nand_operation level, so drivers can assert/de-assert the pin on a
per-operation basis.