How to handle write-protect pin of NAND device ?

How to handle write-protect pin of NAND device ?

[Masahiro Yamada]

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.

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



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.


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.

But, I am not sure if it should be handled in the
framework level with a more generic DT-binding.


Comments are appreciated.

--
Best Regards
Masahiro Yamada

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

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

[Miquel Raynal]

Hi Masahiro,

Masahiro Yamada <masahiroy@kernel.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?

> But, I am not sure if it should be handled in the
> framework level with a more generic DT-binding.
> 
> 
> Comments are appreciated.
> 
> --
> Best Regards
> Masahiro Yamada

Thanks,
Miquèl

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

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

[Boris Brezillon]

On Mon, 27 Jan 2020 15:35:59 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Masahiro,
> 
> Masahiro Yamada <masahiroy@kernel.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).

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

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

[Miquel Raynal]

Hi Hello,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 27 Jan
2020 16:45:54 +0100:

> On Mon, 27 Jan 2020 15:35:59 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hi Masahiro,
> > 
> > Masahiro Yamada <masahiroy@kernel.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.

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

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@bootlin.com> wrote:

> Hi Hello,
> 
> Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 27 Jan
> 2020 16:45:54 +0100:
> 
> > On Mon, 27 Jan 2020 15:35:59 +0100
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > Hi Masahiro,
> > > 
> > > Masahiro Yamada <masahiroy@kernel.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.

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

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

[Masahiro Yamada]

On Tue, Jan 28, 2020 at 3:58 PM Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> On Mon, 27 Jan 2020 16:47:55 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> > Hi Hello,
> >
> > Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 27 Jan
> > 2020 16:45:54 +0100:
> >
> > > On Mon, 27 Jan 2020 15:35:59 +0100
> > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >
> > > > Hi Masahiro,
> > > >
> > > > Masahiro Yamada <masahiroy@kernel.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.

Thank you.


-- 
Best Regards
Masahiro Yamada

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

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

[Miquel Raynal]

Hello,

Masahiro Yamada <masahiroy@kernel.org> wrote on Wed, 29 Jan 2020
19:06:46 +0900:

> On Tue, Jan 28, 2020 at 3:58 PM Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> >
> > On Mon, 27 Jan 2020 16:47:55 +0100
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >  
> > > Hi Hello,
> > >
> > > Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 27 Jan
> > > 2020 16:45:54 +0100:
> > >  
> > > > On Mon, 27 Jan 2020 15:35:59 +0100
> > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > >  
> > > > > Hi Masahiro,
> > > > >
> > > > > Masahiro Yamada <masahiroy@kernel.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?

Thanks,
Miquèl

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

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

[Boris Brezillon]

On Wed, 29 Jan 2020 14:36:39 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hello,
> 
> Masahiro Yamada <masahiroy@kernel.org> wrote on Wed, 29 Jan 2020
> 19:06:46 +0900:
> 
> > On Tue, Jan 28, 2020 at 3:58 PM Boris Brezillon
> > <boris.brezillon@collabora.com> wrote:  
> > >
> > > On Mon, 27 Jan 2020 16:47:55 +0100
> > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >    
> > > > Hi Hello,
> > > >
> > > > Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 27 Jan
> > > > 2020 16:45:54 +0100:
> > > >    
> > > > > On Mon, 27 Jan 2020 15:35:59 +0100
> > > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > >    
> > > > > > Hi Masahiro,
> > > > > >
> > > > > > Masahiro Yamada <masahiroy@kernel.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.

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

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

[Miquel Raynal]

Hi Boris,

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

> On Wed, 29 Jan 2020 14:36:39 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hello,
> > 
> > Masahiro Yamada <masahiroy@kernel.org> wrote on Wed, 29 Jan 2020
> > 19:06:46 +0900:
> >   
> > > On Tue, Jan 28, 2020 at 3:58 PM Boris Brezillon
> > > <boris.brezillon@collabora.com> wrote:    
> > > >
> > > > On Mon, 27 Jan 2020 16:47:55 +0100
> > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > >      
> > > > > Hi Hello,
> > > > >
> > > > > Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 27 Jan
> > > > > 2020 16:45:54 +0100:
> > > > >      
> > > > > > On Mon, 27 Jan 2020 15:35:59 +0100
> > > > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > > >      
> > > > > > > Hi Masahiro,
> > > > > > >
> > > > > > > Masahiro Yamada <masahiroy@kernel.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.

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

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

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

[Boris Brezillon]

On Wed, 29 Jan 2020 14:59:50 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> Boris Brezillon <boris.brezillon@collabora.com> wrote on Wed, 29 Jan
> 2020 14:53:36 +0100:
> 
> > On Wed, 29 Jan 2020 14:36:39 +0100
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > Hello,
> > > 
> > > Masahiro Yamada <masahiroy@kernel.org> wrote on Wed, 29 Jan 2020
> > > 19:06:46 +0900:
> > >     
> > > > On Tue, Jan 28, 2020 at 3:58 PM Boris Brezillon
> > > > <boris.brezillon@collabora.com> wrote:      
> > > > >
> > > > > On Mon, 27 Jan 2020 16:47:55 +0100
> > > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > >        
> > > > > > Hi Hello,
> > > > > >
> > > > > > Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 27 Jan
> > > > > > 2020 16:45:54 +0100:
> > > > > >        
> > > > > > > On Mon, 27 Jan 2020 15:35:59 +0100
> > > > > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > > > >        
> > > > > > > > Hi Masahiro,
> > > > > > > >
> > > > > > > > Masahiro Yamada <masahiroy@kernel.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.

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

--->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;
 };

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

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

[Boris Brezillon]

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

> On Wed, 29 Jan 2020 14:59:50 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hi Boris,
> > 
> > Boris Brezillon <boris.brezillon@collabora.com> wrote on Wed, 29 Jan
> > 2020 14:53:36 +0100:
> >   
> > > On Wed, 29 Jan 2020 14:36:39 +0100
> > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >     
> > > > Hello,
> > > > 
> > > > Masahiro Yamada <masahiroy@kernel.org> wrote on Wed, 29 Jan 2020
> > > > 19:06:46 +0900:
> > > >       
> > > > > On Tue, Jan 28, 2020 at 3:58 PM Boris Brezillon
> > > > > <boris.brezillon@collabora.com> wrote:        
> > > > > >
> > > > > > On Mon, 27 Jan 2020 16:47:55 +0100
> > > > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > > >          
> > > > > > > Hi Hello,
> > > > > > >
> > > > > > > Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 27 Jan
> > > > > > > 2020 16:45:54 +0100:
> > > > > > >          
> > > > > > > > On Mon, 27 Jan 2020 15:35:59 +0100
> > > > > > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > > > > >          
> > > > > > > > > Hi Masahiro,
> > > > > > > > >
> > > > > > > > > Masahiro Yamada <masahiroy@kernel.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.
> 
> > 
> > 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:

Just noticed that WP has to be de-asserted 100 ns (tWW) before issuing
the command cycle, so it might have a minor impact on the perfs (let's
be honest, 100ns is nothing compared to the page transfer/erase time
so I don't think it's a good reason for not re-asserting the pin after
each write program operation).

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

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

[Miquel Raynal]

Hi Boris,

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

> On Wed, 29 Jan 2020 14:59:50 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Hi Boris,
> > 
> > Boris Brezillon <boris.brezillon@collabora.com> wrote on Wed, 29 Jan
> > 2020 14:53:36 +0100:
> >   
> > > On Wed, 29 Jan 2020 14:36:39 +0100
> > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > >     
> > > > Hello,
> > > > 
> > > > Masahiro Yamada <masahiroy@kernel.org> wrote on Wed, 29 Jan 2020
> > > > 19:06:46 +0900:
> > > >       
> > > > > On Tue, Jan 28, 2020 at 3:58 PM Boris Brezillon
> > > > > <boris.brezillon@collabora.com> wrote:        
> > > > > >
> > > > > > On Mon, 27 Jan 2020 16:47:55 +0100
> > > > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > > >          
> > > > > > > Hi Hello,
> > > > > > >
> > > > > > > Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 27 Jan
> > > > > > > 2020 16:45:54 +0100:
> > > > > > >          
> > > > > > > > On Mon, 27 Jan 2020 15:35:59 +0100
> > > > > > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > > > > >          
> > > > > > > > > Hi Masahiro,
> > > > > > > > >
> > > > > > > > > Masahiro Yamada <masahiroy@kernel.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

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

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

[Boris Brezillon]

On Wed, 29 Jan 2020 16:00:45 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> Boris Brezillon <boris.brezillon@collabora.com> wrote on Wed, 29 Jan
> 2020 15:49:26 +0100:
> 
> > On Wed, 29 Jan 2020 14:59:50 +0100
> > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >   
> > > Hi Boris,
> > > 
> > > Boris Brezillon <boris.brezillon@collabora.com> wrote on Wed, 29 Jan
> > > 2020 14:53:36 +0100:
> > >     
> > > > On Wed, 29 Jan 2020 14:36:39 +0100
> > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > >       
> > > > > Hello,
> > > > > 
> > > > > Masahiro Yamada <masahiroy@kernel.org> wrote on Wed, 29 Jan 2020
> > > > > 19:06:46 +0900:
> > > > >         
> > > > > > On Tue, Jan 28, 2020 at 3:58 PM Boris Brezillon
> > > > > > <boris.brezillon@collabora.com> wrote:          
> > > > > > >
> > > > > > > On Mon, 27 Jan 2020 16:47:55 +0100
> > > > > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > > > >            
> > > > > > > > Hi Hello,
> > > > > > > >
> > > > > > > > Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 27 Jan
> > > > > > > > 2020 16:45:54 +0100:
> > > > > > > >            
> > > > > > > > > On Mon, 27 Jan 2020 15:35:59 +0100
> > > > > > > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > > > > > >            
> > > > > > > > > > Hi Masahiro,
> > > > > > > > > >
> > > > > > > > > > Masahiro Yamada <masahiroy@kernel.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.

Well, yes. If the driver has custom ->write_page[_raw](), they should
be patched to handle WP de-assertion/assertion.

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

Right. 100ns on an operation that takes more than 100us is negligible,
but if you start de-asserting/asserting WP on shorter operations, like
READ_STATUS, that might have an impact.

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