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

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote on Wed, 29 Jan
2020 14:53:36 +0100:

> On Wed, 29 Jan 2020 14:36:39 +0100
> Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
> 
> > Hello,
> > 
> > Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote on Wed, 29 Jan 2020
> > 19:06:46 +0900:
> >   
> > > On Tue, Jan 28, 2020 at 3:58 PM Boris Brezillon
> > > <boris.brezillon@xxxxxxxxxxxxx> wrote:    
> > > >
> > > > 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.      
> > > 
> > > Sounds a good idea.
> > > 
> > > If it is supported in the NAND framework,
> > > I will be happy to implement in the Denali NAND driver.
> > >     
> > 
> > There is currently no such thing at NAND level but I doubt there is
> > more than erase and write operation during which it would be needed
> > to assert/deassert WP. I don't see why having this flag would help
> > the controller drivers?  
> 
> Because ->exec_op() was designed to avoid leaving such decisions to the
> NAND controller drivers :P. If you now ask drivers to look at the
> opcode and guess when they should de-assert the WP pin, you're just
> going back to the ->cmdfunc() mess.

I was actually thinking to the ->write_page(_raw)() helpers, but
yeah, in the case of ->exec_op() it's different. However, for these
helpers as don't use ->exec_op(), we need another way to flag the
operation as destructive.

But actually we could let the driver toggle the pin for any operation.
If we want to be protected against spurious access, not directly ordered
by the controller driver itself, then we don't care if the operation is
actually destructive or not as long as the pin is deasserted during our
operations and asserted otherwise.

MiquÃl