Date:   Wed, 29 Jan 2020 16:00:45 +0100
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Boris Brezillon <boris.brezillon@...labora.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Masahiro Yamada <masahiroy@...nel.org>,
        linux-mtd <linux-mtd@...ts.infradead.org>,
        Boris Brezillon <bbrezillon@...nel.org>
Subject: Re: How to handle write-protect pin of NAND device ?

Hi Boris,

Boris Brezillon <boris.brezillon@...labora.com> wrote on Wed, 29 Jan
2020 15:49:26 +0100:

> On Wed, 29 Jan 2020 14:59:50 +0100
> Miquel Raynal <miquel.raynal@...tlin.com> wrote:
> 
> > Hi Boris,
> > 
> > Boris Brezillon <boris.brezillon@...labora.com> wrote on Wed, 29 Jan
> > 2020 14:53:36 +0100:
> >   
> > > On Wed, 29 Jan 2020 14:36:39 +0100
> > > Miquel Raynal <miquel.raynal@...tlin.com> wrote:
> > >     
> > > > Hello,
> > > > 
> > > > Masahiro Yamada <masahiroy@...nel.org> wrote on Wed, 29 Jan 2020
> > > > 19:06:46 +0900:
> > > >       
> > > > > On Tue, Jan 28, 2020 at 3:58 PM Boris Brezillon
> > > > > <boris.brezillon@...labora.com> wrote:        
> > > > > >
> > > > > > On Mon, 27 Jan 2020 16:47:55 +0100
> > > > > > Miquel Raynal <miquel.raynal@...tlin.com> wrote:
> > > > > >          
> > > > > > > Hi Hello,
> > > > > > >
> > > > > > > Boris Brezillon <boris.brezillon@...labora.com> wrote on Mon, 27 Jan
> > > > > > > 2020 16:45:54 +0100:
> > > > > > >          
> > > > > > > > On Mon, 27 Jan 2020 15:35:59 +0100
> > > > > > > > Miquel Raynal <miquel.raynal@...tlin.com> wrote:
> > > > > > > >          
> > > > > > > > > Hi Masahiro,
> > > > > > > > >
> > > > > > > > > Masahiro Yamada <masahiroy@...nel.org> 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.  
> 
> I don't think we really care about ancient (AKA non-exec_op()) drivers.
> They seem to work fine as they are now, so let's focus on the modern
> ones.

Not my point: the ->write_page[_raw]() helpers are implemented by
everyone, no ->exec_op() is involved and they are destructive as well.

> 
> > 
> > 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.  
> 
> Or we could patch the ->exec_op() path to pass this information (and
> maybe provide helpers for the GPIO case). Should be as simple as:

This approach is fine.

Without the delay penalty in mind, I would say it is useless and the
driver can simply deassert WP at the start of ->exec_op() but as there
is a small penalty, why not.

> 
> --->8---  
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index f64e3b6605c6..4f0fdbd5b760 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -1343,6 +1343,7 @@ static int nand_exec_prog_page_op(struct nand_chip *chip, unsigned int page,
>                 op.ninstrs--;
>         }
>  
> +       op.flags = NAND_OPERATION_DEASSERT_WP;
>         ret = nand_exec_op(chip, &op);
>         if (!prog || ret)
>                 return ret;
> @@ -1416,6 +1417,7 @@ int nand_prog_page_end_op(struct nand_chip *chip)
>                 };
>                 struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs);
>  
> +               op.flags = NAND_OPERATION_DEASSERT_WP;
>                 ret = nand_exec_op(chip, &op);
>                 if (ret)
>                         return ret;
> @@ -1692,6 +1694,7 @@ int nand_erase_op(struct nand_chip *chip, unsigned int eraseblock)
>                 if (chip->options & NAND_ROW_ADDR_3)
>                         instrs[1].ctx.addr.naddrs++;
>  
> +               op.flags = NAND_OPERATION_DEASSERT_WP;
>                 ret = nand_exec_op(chip, &op);
>                 if (ret)
>                         return ret;
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index 4ab9bccfcde0..1b08ddf67a12 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -849,9 +849,12 @@ struct nand_op_parser {
>                              sizeof(struct nand_op_parser_pattern),                             \
>         }
>  
> +#define NAND_OPERATION_DEASSERT_WP     BIT(0)
> +
>  /**
>   * struct nand_operation - NAND operation descriptor
>   * @cs: the CS line to select for this NAND operation
> + * @flags: operation flags
>   * @instrs: array of instructions to execute
>   * @ninstrs: length of the @instrs array
>   *
> @@ -859,6 +862,7 @@ struct nand_op_parser {
>   */
>  struct nand_operation {
>         unsigned int cs;
> +       u32 flags;
>         const struct nand_op_instr *instrs;
>         unsigned int ninstrs;
>  };

Thanks,
Miquèl

Powered by blists - more mailing lists