[PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Helmut Schaa]

nand_write_subpage_hwecc causes a crash if the driver did not register
ecc->hwctl or ecc->calculate. Fix this by disabling subpage writes if
ecc->hwctl or ecc->calculate is not provided by the driver.

This behavior was introduced in commit 837a6ba4f3b6d23026674e6af6b6849a4634fff9
"mtd: nand: subpage write support for hardware based ECC schemes".

This fixes a crash with fsl_elbc_nand and maybe others:

Unable to handle kernel paging request for instruction fetch
Faulting instruction address: 0x00000000
Oops: Kernel access of bad area, sig: 11 [#1]
SMP NR_CPUS=2 P1020 RDB
Modules linked in: ath9k ath9k_common pppoe ppp_async option iptable_nat ath9k_hw ath usb_wwan pppox ppp_generic nf_nat_ipv4 nf_conntrack_ipv4 mac80211 ipt_MASQUERADE cfg80211 xt_time xt_tcpudp xt_state xt_quota xt_policy xt_pkttype xt_owner xt_nat xt_multiport xt_mh
CPU: 1 PID: 2161 Comm: ubiformat Not tainted 3.10.26 #6
task: efbc2700 ti: c7950000 task.ti: c7950000
NIP: 00000000 LR: c01a495c CTR: 00000000
REGS: c7951cb0 TRAP: 0400   Not tainted  (3.10.26)
MSR: 00029000 <CE,EE,ME>  CR: 24002028  XER: 00000000

GPR00: c01a4b6c c7951d60 efbc2700 ef84b000 00000001 00000000 000001ff c7800500
GPR08: 00000000 00000000 efae5e40 c01a4ae4 24002022 10023418 c7951e5c c7800500
GPR16: c017b6a8 00000000 0000003f c053404c 00000000 00000004 00000000 00000003
GPR24: 00000010 00000200 ef84b000 c7800d00 c7800000 c7800500 ef84b1c8 00000000
NIP [00000000]   (null)
LR [c01a495c] nand_write_subpage_hwecc+0x74/0x174
Call Trace:
[c7951d60] [c7951d64] 0xc7951d64 (unreliable)
[c7951da0] [c01a4b6c] nand_write_page+0x88/0x198
[c7951dd0] [c01a5f7c] nand_do_write_ops+0x2f4/0x39c
[c7951e40] [c01a61e0] nand_write+0x58/0x84
[c7951e80] [c019e29c] mtdchar_write+0x1dc/0x28c
[c7951ef0] [c00aba84] vfs_write+0xcc/0x1ac
[c7951f10] [c00ac04c] SyS_write+0x4c/0x90
[c7951f40] [c000cd84] ret_from_syscall+0x0/0x3c
 --- Exception: c01 at 0x48050ed8
     LR = 0x100071b8
 Instruction dump:
 XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
 XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
 ---[ end trace 161d3c65a2a15cb8 ]---

Kernel panic - not syncing: Fatal exception

Cc: Gupta, Pekon <pekon@ti.com>
Cc: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Cc: David Woodhouse <David.Woodhouse@intel.com>
Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---
I noticed this bug on openwrt with kernel 3.10 but it looks as if this can
still happen in more recent kernels.

 drivers/mtd/nand/nand_base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 9715a7b..2298289 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3768,7 +3768,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 			ecc->write_oob = nand_write_oob_std;
 		if (!ecc->read_subpage)
 			ecc->read_subpage = nand_read_subpage;
-		if (!ecc->write_subpage)
+		if (!ecc->write_subpage && ecc->hwctl && ecc->calculate)
 			ecc->write_subpage = nand_write_subpage_hwecc;
 
 	case NAND_ECC_HW_SYNDROME:
-- 
1.8.4.5

RE: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Gupta, Pekon]

>From: Helmut Schaa [mailto:helmut.schaa@googlemail.com]
>
>nand_write_subpage_hwecc causes a crash if the driver did not register
>ecc->hwctl or ecc->calculate. Fix this by disabling subpage writes if
>ecc->hwctl or ecc->calculate is not provided by the driver.
>
>This behavior was introduced in commit 837a6ba4f3b6d23026674e6af6b6849a4634fff9
>"mtd: nand: subpage write support for hardware based ECC schemes".
>
>This fixes a crash with fsl_elbc_nand and maybe others:
>
>Unable to handle kernel paging request for instruction fetch
>Faulting instruction address: 0x00000000
>Oops: Kernel access of bad area, sig: 11 [#1]
>SMP NR_CPUS=2 P1020 RDB
>Modules linked in: ath9k ath9k_common pppoe ppp_async option iptable_nat ath9k_hw ath usb_wwan pppox ppp_generic
>nf_nat_ipv4 nf_conntrack_ipv4 mac80211 ipt_MASQUERADE cfg80211 xt_time xt_tcpudp xt_state xt_quota xt_policy xt_pkttype
>xt_owner xt_nat xt_multiport xt_mh
>CPU: 1 PID: 2161 Comm: ubiformat Not tainted 3.10.26 #6
>task: efbc2700 ti: c7950000 task.ti: c7950000
>NIP: 00000000 LR: c01a495c CTR: 00000000
>REGS: c7951cb0 TRAP: 0400   Not tainted  (3.10.26)
>MSR: 00029000 <CE,EE,ME>  CR: 24002028  XER: 00000000
>
>GPR00: c01a4b6c c7951d60 efbc2700 ef84b000 00000001 00000000 000001ff c7800500
>GPR08: 00000000 00000000 efae5e40 c01a4ae4 24002022 10023418 c7951e5c c7800500
>GPR16: c017b6a8 00000000 0000003f c053404c 00000000 00000004 00000000 00000003
>GPR24: 00000010 00000200 ef84b000 c7800d00 c7800000 c7800500 ef84b1c8 00000000
>NIP [00000000]   (null)
>LR [c01a495c] nand_write_subpage_hwecc+0x74/0x174
>Call Trace:
>[c7951d60] [c7951d64] 0xc7951d64 (unreliable)
>[c7951da0] [c01a4b6c] nand_write_page+0x88/0x198
>[c7951dd0] [c01a5f7c] nand_do_write_ops+0x2f4/0x39c
>[c7951e40] [c01a61e0] nand_write+0x58/0x84
>[c7951e80] [c019e29c] mtdchar_write+0x1dc/0x28c
>[c7951ef0] [c00aba84] vfs_write+0xcc/0x1ac
>[c7951f10] [c00ac04c] SyS_write+0x4c/0x90
>[c7951f40] [c000cd84] ret_from_syscall+0x0/0x3c
> --- Exception: c01 at 0x48050ed8
>     LR = 0x100071b8
> Instruction dump:
> XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
> XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
> ---[ end trace 161d3c65a2a15cb8 ]---
>
>Kernel panic - not syncing: Fatal exception
>
>Cc: Gupta, Pekon <pekon@ti.com>
>Cc: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>Cc: David Woodhouse <David.Woodhouse@intel.com>
>Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
>---
>I noticed this bug on openwrt with kernel 3.10 but it looks as if this can
>still happen in more recent kernels.
>
> drivers/mtd/nand/nand_base.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>index 9715a7b..2298289 100644
>--- a/drivers/mtd/nand/nand_base.c
>+++ b/drivers/mtd/nand/nand_base.c
>@@ -3768,7 +3768,7 @@ int nand_scan_tail(struct mtd_info *mtd)
> 			ecc->write_oob = nand_write_oob_std;
> 		if (!ecc->read_subpage)
> 			ecc->read_subpage = nand_read_subpage;
>-		if (!ecc->write_subpage)
>+		if (!ecc->write_subpage && ecc->hwctl && ecc->calculate)

I don't think this is correct because nand_write_subpage_hwecc() is a
replaceable function (default generic implementation). So

(1) if chip->ecc.hwctl() and chip->ecc.calculate are not implemented but you
 still want to use subpage write feature, then you need to provide custom
 implementation for chip->ecc.write_subpage().
 that's same for other interfaces of nand_chip like chip->ecc.write_page().

(2) If you don't want to use subpage write feature then just disable it using
	chip->options |= NAND_NO_SUBPAGE_WRITE;

Can you please tell which NAND controller driver is causing this ?
We need to fix that..

with regards, pekon

Re: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Helmut Schaa]

On Wed, Apr 9, 2014 at 11:38 AM, Gupta, Pekon <pekon@ti.com> wrote:
>>diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>>index 9715a7b..2298289 100644
>>--- a/drivers/mtd/nand/nand_base.c
>>+++ b/drivers/mtd/nand/nand_base.c
>>@@ -3768,7 +3768,7 @@ int nand_scan_tail(struct mtd_info *mtd)
>>                       ecc->write_oob = nand_write_oob_std;
>>               if (!ecc->read_subpage)
>>                       ecc->read_subpage = nand_read_subpage;
>>-              if (!ecc->write_subpage)
>>+              if (!ecc->write_subpage && ecc->hwctl && ecc->calculate)
>
> I don't think this is correct because nand_write_subpage_hwecc() is a
> replaceable function (default generic implementation). So
>
> (1) if chip->ecc.hwctl() and chip->ecc.calculate are not implemented but you
>  still want to use subpage write feature, then you need to provide custom
>  implementation for chip->ecc.write_subpage().
>  that's same for other interfaces of nand_chip like chip->ecc.write_page().

But these don't cause panics :)

> (2) If you don't want to use subpage write feature then just disable it using
>         chip->options |= NAND_NO_SUBPAGE_WRITE;
>
> Can you please tell which NAND controller driver is causing this ?
> We need to fix that..

This happens with fsl_elbc_nand (while trying to run ubiformat on a
mtd dev) but the
crash was caused by the introduction of nand_write_subpage_hwecc. So, in this
case I think instead of trying to fix every possible driver we should
let the nand core
code handle this issue gracefully. Maybe we could add a WARN_ON_ONCE to
notice which drivers require adjustments.

Regards,
Helmut

RE: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Gupta, Pekon]

>From: Helmut Schaa [mailto:helmut.schaa@googlemail.com]
>>On Wed, Apr 9, 2014 at 11:38 AM, Gupta, Pekon <pekon@ti.com> wrote:
[...]
>>
>> (1) if chip->ecc.hwctl() and chip->ecc.calculate are not implemented but you
>>  still want to use subpage write feature, then you need to provide custom
>>  implementation for chip->ecc.write_subpage().
>>  that's same for other interfaces of nand_chip like chip->ecc.write_page().
>
>But these don't cause panics :)
>
because fsl_elbc_nand.c uses custom implementations of chip->ecc.write_page()
	@@ fsl_elbc_chip_init(...)
		chip->ecc.write_page = fsl_elbc_write_page;

Same needs to be done if subpage write is needed. However, as this is
a regression so please check if following patch solves your problem.[1]


>> (2) If you don't want to use subpage write feature then just disable it using
>>         chip->options |= NAND_NO_SUBPAGE_WRITE;
>>
>> Can you please tell which NAND controller driver is causing this ?
>> We need to fix that..
>
>This happens with fsl_elbc_nand (while trying to run ubiformat on a
>mtd dev) but the
>crash was caused by the introduction of nand_write_subpage_hwecc. So, in this
>case I think instead of trying to fix every possible driver we should
>let the nand core
>code handle this issue gracefully. Maybe we could add a WARN_ON_ONCE to
>notice which drivers require adjustments.
>
Yes agree. May be good to keep subpage write disabled by default,
as only handful drivers possibly use that.


[1] ## <not compile tested>
------------
>From bfd39102ed6aa99b7ac2b8394a2d12b879fbb4b7 Mon Sep 17 00:00:00 2001
From: Pekon Gupta <pekon@ti.com>
Date: Wed, 9 Apr 2014 15:51:25 +0530
Subject: [PATCH] mtd: eLBC NAND: disable subpage write support

As fsl_elbc_nand do not implement NAND ECC interfaces (like chip->ecc.hwctl(),
chip->ecc.calculate, and chip->ecc.correct) So it cannot use default
implementation of nand_write_subpage_hwecc() as in nand_base.c.
Hence disabling subpage write support till a custom implementation for
chip->ecc_write_subpage is added.

CC: <stable@vger.kernel.org> # 3.10.x+
Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 drivers/mtd/nand/fsl_elbc_nand.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c
index ec549cd..a21252c 100644
--- a/drivers/mtd/nand/fsl_elbc_nand.c
+++ b/drivers/mtd/nand/fsl_elbc_nand.c
@@ -755,6 +755,7 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)

        /* set up nand options */
        chip->bbt_options = NAND_BBT_USE_FLASH;
+       chip->options |= NAND_NO_SUBPAGE_WRITE;

        chip->controller = &elbc_fcm_ctrl->controller;
        chip->priv = priv;
--
1.8.5.1.163.gd7aced9

----------------
(looping possible maintainers of this driver Huang Shijie and Scott Wood)

with regards, pekon

Re: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Helmut Schaa]

On Wed, Apr 9, 2014 at 12:33 PM, Gupta, Pekon <pekon@ti.com> wrote:
>>From: Helmut Schaa [mailto:helmut.schaa@googlemail.com]
>>>On Wed, Apr 9, 2014 at 11:38 AM, Gupta, Pekon <pekon@ti.com> wrote:
> [...]
>>>
>>> (1) if chip->ecc.hwctl() and chip->ecc.calculate are not implemented but you
>>>  still want to use subpage write feature, then you need to provide custom
>>>  implementation for chip->ecc.write_subpage().
>>>  that's same for other interfaces of nand_chip like chip->ecc.write_page().
>>
>>But these don't cause panics :)
>>
> because fsl_elbc_nand.c uses custom implementations of chip->ecc.write_page()
>         @@ fsl_elbc_chip_init(...)
>                 chip->ecc.write_page = fsl_elbc_write_page;
>
> Same needs to be done if subpage write is needed. However, as this is
> a regression so please check if following patch solves your problem.[1]

That was my first approach too, seems to work ok for fsl_elbc_nand but some
other drivers might still experience crashes. A quick grep shows that
sh_flctl might
be affected as well.

So, I'd still argue that it would make more sense to keep the driver interface
behavior sane either by setting NAND_NO_SUBPAGE_WRITE as default or
by using some checks as proposed initially.

Thanks,
Helmut

Re: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Scott Wood]

On Wed, 2014-04-09 at 10:33 +0000, Gupta, Pekon wrote:
> >From: Helmut Schaa [mailto:helmut.schaa@googlemail.com]
> >>On Wed, Apr 9, 2014 at 11:38 AM, Gupta, Pekon <pekon@ti.com> wrote:
> [...]
> >>
> >> (1) if chip->ecc.hwctl() and chip->ecc.calculate are not implemented but you
> >>  still want to use subpage write feature, then you need to provide custom
> >>  implementation for chip->ecc.write_subpage().
> >>  that's same for other interfaces of nand_chip like chip->ecc.write_page().
> >
> >But these don't cause panics :)
> >
> because fsl_elbc_nand.c uses custom implementations of chip->ecc.write_page()
> 	@@ fsl_elbc_chip_init(...)
> 		chip->ecc.write_page = fsl_elbc_write_page;
> 
> Same needs to be done if subpage write is needed. However, as this is
> a regression so please check if following patch solves your problem.[1]
> 
> 
> >> (2) If you don't want to use subpage write feature then just disable it using
> >>         chip->options |= NAND_NO_SUBPAGE_WRITE;
> >>
> >> Can you please tell which NAND controller driver is causing this ?
> >> We need to fix that..
> >
> >This happens with fsl_elbc_nand (while trying to run ubiformat on a
> >mtd dev) but the
> >crash was caused by the introduction of nand_write_subpage_hwecc. So, in this
> >case I think instead of trying to fix every possible driver we should
> >let the nand core
> >code handle this issue gracefully. Maybe we could add a WARN_ON_ONCE to
> >notice which drivers require adjustments.
> >
> Yes agree. May be good to keep subpage write disabled by default,
> as only handful drivers possibly use that.
> 
> 
> [1] ## <not compile tested>
> ------------
> From bfd39102ed6aa99b7ac2b8394a2d12b879fbb4b7 Mon Sep 17 00:00:00 2001
> From: Pekon Gupta <pekon@ti.com>
> Date: Wed, 9 Apr 2014 15:51:25 +0530
> Subject: [PATCH] mtd: eLBC NAND: disable subpage write support
> 
> As fsl_elbc_nand do not implement NAND ECC interfaces (like chip->ecc.hwctl(),
> chip->ecc.calculate, and chip->ecc.correct) So it cannot use default
> implementation of nand_write_subpage_hwecc() as in nand_base.c.
> Hence disabling subpage write support till a custom implementation for
> chip->ecc_write_subpage is added.
> 
> CC: <stable@vger.kernel.org> # 3.10.x+
> Signed-off-by: Pekon Gupta <pekon@ti.com>
> ---
>  drivers/mtd/nand/fsl_elbc_nand.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c
> index ec549cd..a21252c 100644
> --- a/drivers/mtd/nand/fsl_elbc_nand.c
> +++ b/drivers/mtd/nand/fsl_elbc_nand.c
> @@ -755,6 +755,7 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
> 
>         /* set up nand options */
>         chip->bbt_options = NAND_BBT_USE_FLASH;
> +       chip->options |= NAND_NO_SUBPAGE_WRITE;

Won't this break compatibility with existing UBI volumes?  That's why I
didn't set this flag on eLBC when I set it on IFC (on the latter UBI is
simply broken without that flag, but eLBC gets away with it because of
the ECC algorithm used).

-Scott

RE: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Gupta, Pekon]

>From: Scott Wood [mailto:scottwood@freescale.com]
>>On Wed, 2014-04-09 at 10:33 +0000, Gupta, Pekon wrote:
>> >From: Helmut Schaa [mailto:helmut.schaa@googlemail.com]
>> >>On Wed, Apr 9, 2014 at 11:38 AM, Gupta, Pekon <pekon@ti.com> wrote:
>> [...]
>> >>
>> >> (1) if chip->ecc.hwctl() and chip->ecc.calculate are not implemented but you
>> >>  still want to use subpage write feature, then you need to provide custom
>> >>  implementation for chip->ecc.write_subpage().
>> >>  that's same for other interfaces of nand_chip like chip->ecc.write_page().
>> >
>> >But these don't cause panics :)
>> >
>> because fsl_elbc_nand.c uses custom implementations of chip->ecc.write_page()
>> 	@@ fsl_elbc_chip_init(...)
>> 		chip->ecc.write_page = fsl_elbc_write_page;
>>
>> Same needs to be done if subpage write is needed. However, as this is
>> a regression so please check if following patch solves your problem.[1]
>>
>>
>> >> (2) If you don't want to use subpage write feature then just disable it using
>> >>         chip->options |= NAND_NO_SUBPAGE_WRITE;
>> >>
>> >> Can you please tell which NAND controller driver is causing this ?
>> >> We need to fix that..
>> >
>> >This happens with fsl_elbc_nand (while trying to run ubiformat on a
>> >mtd dev) but the
>> >crash was caused by the introduction of nand_write_subpage_hwecc. So, in this
>> >case I think instead of trying to fix every possible driver we should
>> >let the nand core
>> >code handle this issue gracefully. Maybe we could add a WARN_ON_ONCE to
>> >notice which drivers require adjustments.
>> >
>> Yes agree. May be good to keep subpage write disabled by default,
>> as only handful drivers possibly use that.
>>
>>
>> [1] ## <not compile tested>
>> ------------
>> From bfd39102ed6aa99b7ac2b8394a2d12b879fbb4b7 Mon Sep 17 00:00:00 2001
>> From: Pekon Gupta <pekon@ti.com>
>> Date: Wed, 9 Apr 2014 15:51:25 +0530
>> Subject: [PATCH] mtd: eLBC NAND: disable subpage write support
>>
>> As fsl_elbc_nand do not implement NAND ECC interfaces (like chip->ecc.hwctl(),
>> chip->ecc.calculate, and chip->ecc.correct) So it cannot use default
>> implementation of nand_write_subpage_hwecc() as in nand_base.c.
>> Hence disabling subpage write support till a custom implementation for
>> chip->ecc_write_subpage is added.
>>
>> CC: <stable@vger.kernel.org> # 3.10.x+
>> Signed-off-by: Pekon Gupta <pekon@ti.com>
>> ---
>>  drivers/mtd/nand/fsl_elbc_nand.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c
>> index ec549cd..a21252c 100644
>> --- a/drivers/mtd/nand/fsl_elbc_nand.c
>> +++ b/drivers/mtd/nand/fsl_elbc_nand.c
>> @@ -755,6 +755,7 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
>>
>>         /* set up nand options */
>>         chip->bbt_options = NAND_BBT_USE_FLASH;
>> +       chip->options |= NAND_NO_SUBPAGE_WRITE;
>
>Won't this break compatibility with existing UBI volumes?
> That's why I didn't set this flag on eLBC when I set it on IFC (on the latter UBI is
>simply broken without that flag, but eLBC gets away with it because of
>the ECC algorithm used).

Are you sure subpage write is supported on this driver ? I couldn't find any
custom implementation for chip->ecc.write_subpage() in fsl_elbc_nand.c.

And when fsl_elbc_nand.c uses default implementation of
chip->ecc.write_subpage() = nand_write_subpage_hwecc then it crashes
because it does not implement chip->ecc.hwctl().  (refer [1])

However above is just a fix for this regression, as it needs to be back ported
till kernel 3.10.x+. I'll send another patch to  'disable' subpage write support
by default for drivers, and only the ones which need it should explicitly enable it.


[1] http://lists.infradead.org/pipermail/linux-mtd/2014-April/053182.html

with regards, pekon

Re: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Helmut Schaa]

Scott Wood <scottwood@freescale.com> schrieb:
>On Wed, 2014-04-09 at 10:33 +0000, Gupta, Pekon wrote:
>> >From: Helmut Schaa [mailto:helmut.schaa@googlemail.com]
>> >>On Wed, Apr 9, 2014 at 11:38 AM, Gupta, Pekon <pekon@ti.com> wrote:
>> [...]
>> >>
>> >> (1) if chip->ecc.hwctl() and chip->ecc.calculate are not
>implemented but you
>> >>  still want to use subpage write feature, then you need to provide
>custom
>> >>  implementation for chip->ecc.write_subpage().
>> >>  that's same for other interfaces of nand_chip like
>chip->ecc.write_page().
>> >
>> >But these don't cause panics :)
>> >
>> because fsl_elbc_nand.c uses custom implementations of
>chip->ecc.write_page()
>> 	@@ fsl_elbc_chip_init(...)
>> 		chip->ecc.write_page = fsl_elbc_write_page;
>> 
>> Same needs to be done if subpage write is needed. However, as this is
>> a regression so please check if following patch solves your
>problem.[1]
>> 
>> 
>> >> (2) If you don't want to use subpage write feature then just
>disable it using
>> >>         chip->options |= NAND_NO_SUBPAGE_WRITE;
>> >>
>> >> Can you please tell which NAND controller driver is causing this ?
>> >> We need to fix that..
>> >
>> >This happens with fsl_elbc_nand (while trying to run ubiformat on a
>> >mtd dev) but the
>> >crash was caused by the introduction of nand_write_subpage_hwecc.
>So, in this
>> >case I think instead of trying to fix every possible driver we
>should
>> >let the nand core
>> >code handle this issue gracefully. Maybe we could add a WARN_ON_ONCE
>to
>> >notice which drivers require adjustments.
>> >
>> Yes agree. May be good to keep subpage write disabled by default,
>> as only handful drivers possibly use that.
>> 
>> 
>> [1] ## <not compile tested>
>> ------------
>> From bfd39102ed6aa99b7ac2b8394a2d12b879fbb4b7 Mon Sep 17 00:00:00
>2001
>> From: Pekon Gupta <pekon@ti.com>
>> Date: Wed, 9 Apr 2014 15:51:25 +0530
>> Subject: [PATCH] mtd: eLBC NAND: disable subpage write support
>> 
>> As fsl_elbc_nand do not implement NAND ECC interfaces (like
>chip->ecc.hwctl(),
>> chip->ecc.calculate, and chip->ecc.correct) So it cannot use default
>> implementation of nand_write_subpage_hwecc() as in nand_base.c.
>> Hence disabling subpage write support till a custom implementation
>for
>> chip->ecc_write_subpage is added.
>> 
>> CC: <stable@vger.kernel.org> # 3.10.x+
>> Signed-off-by: Pekon Gupta <pekon@ti.com>
>> ---
>>  drivers/mtd/nand/fsl_elbc_nand.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c
>b/drivers/mtd/nand/fsl_elbc_nand.c
>> index ec549cd..a21252c 100644
>> --- a/drivers/mtd/nand/fsl_elbc_nand.c
>> +++ b/drivers/mtd/nand/fsl_elbc_nand.c
>> @@ -755,6 +755,7 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd
>*priv)
>> 
>>         /* set up nand options */
>>         chip->bbt_options = NAND_BBT_USE_FLASH;
>> +       chip->options |= NAND_NO_SUBPAGE_WRITE;
>
>Won't this break compatibility with existing UBI volumes?  That's why I
>didn't set this flag on eLBC when I set it on IFC (on the latter UBI is
>simply broken without that flag, but eLBC gets away with it because of
>the ECC algorithm used).

Could be. I had to override the VID header offset accordingly to be able to attach to the ubi volume after applying this patch ...

Helmut

RE: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Gupta, Pekon]

Hi Helmut Schaa,

>From: Helmut Schaa [mailto:helmut.schaa@googlemail.com]
>>Scott Wood <scottwood@freescale.com> schrieb:
[...]
>>> From: Pekon Gupta <pekon@ti.com>
>>> Date: Wed, 9 Apr 2014 15:51:25 +0530
>>> Subject: [PATCH] mtd: eLBC NAND: disable subpage write support
>>>
>>> As fsl_elbc_nand do not implement NAND ECC interfaces (like
>>chip->ecc.hwctl(),
>>> chip->ecc.calculate, and chip->ecc.correct) So it cannot use default
>>> implementation of nand_write_subpage_hwecc() as in nand_base.c.
>>> Hence disabling subpage write support till a custom implementation
>>for
>>> chip->ecc_write_subpage is added.
>>>
>>> CC: <stable@vger.kernel.org> # 3.10.x+
>>> Signed-off-by: Pekon Gupta <pekon@ti.com>
>>> ---
>>>  drivers/mtd/nand/fsl_elbc_nand.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c
>>b/drivers/mtd/nand/fsl_elbc_nand.c
>>> index ec549cd..a21252c 100644
>>> --- a/drivers/mtd/nand/fsl_elbc_nand.c
>>> +++ b/drivers/mtd/nand/fsl_elbc_nand.c
>>> @@ -755,6 +755,7 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd
>>*priv)
>>>
>>>         /* set up nand options */
>>>         chip->bbt_options = NAND_BBT_USE_FLASH;
>>> +       chip->options |= NAND_NO_SUBPAGE_WRITE;
>>
>>Won't this break compatibility with existing UBI volumes?  That's why I
>>didn't set this flag on eLBC when I set it on IFC (on the latter UBI is
>>simply broken without that flag, but eLBC gets away with it because of
>>the ECC algorithm used).
>
>Could be. I had to override the VID header offset accordingly to be able to attach to the ubi volume after applying this patch ...
>
There could be some misconfiguration in the way you generate your
UBIFS image. Just check following:
(1) min-io-size (-m) passed to mkfs.ubifs and ubinize commands?
(2) vid-hdr-offset (-O) passed to ubinize and ubiattach commands?
(3) subpage-size (-s) passed to ubinize commands? [optional]
All of the above should be set to $PAGE_SIZE of your NAND device.

Because if your image itself was generated such a way that vid-hdr was
offset at page-boundaries then you shouldn't have needed this change.
---------------------------
nand_base.c @@ nand_write_page(..)
	if (!(chip->options & NAND_NO_SUBPAGE_WRITE) &&
		chip->ecc.write_subpage)
		subpage = offset || (data_len < mtd->writesize);   <<-----
	else
		subpage = 0;
	[...]
	else if (subpage)  <<--------
		status = chip->ecc.write_subpage(mtd, chip, offset, data_len,
							 buf, oob_required);
---------------------------
So, even if the subpage-write is enabled, but all the write accesses are
page-aligned, (that is offset==0 && data_len % mtd->writesize ==0)
then also chip->ecc.write_subpage() does _not_ come in path.


with regards, pekon

Re: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Helmut Schaa]

On Thu, Apr 10, 2014 at 9:26 AM, Gupta, Pekon <pekon@ti.com> wrote:
> Hi Helmut Schaa,
>
>>From: Helmut Schaa [mailto:helmut.schaa@googlemail.com]
>>>Scott Wood <scottwood@freescale.com> schrieb:
> [...]
>>>> From: Pekon Gupta <pekon@ti.com>
>>>> Date: Wed, 9 Apr 2014 15:51:25 +0530
>>>> Subject: [PATCH] mtd: eLBC NAND: disable subpage write support
>>>>
>>>> As fsl_elbc_nand do not implement NAND ECC interfaces (like
>>>chip->ecc.hwctl(),
>>>> chip->ecc.calculate, and chip->ecc.correct) So it cannot use default
>>>> implementation of nand_write_subpage_hwecc() as in nand_base.c.
>>>> Hence disabling subpage write support till a custom implementation
>>>for
>>>> chip->ecc_write_subpage is added.
>>>>
>>>> CC: <stable@vger.kernel.org> # 3.10.x+
>>>> Signed-off-by: Pekon Gupta <pekon@ti.com>
>>>> ---
>>>>  drivers/mtd/nand/fsl_elbc_nand.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c
>>>b/drivers/mtd/nand/fsl_elbc_nand.c
>>>> index ec549cd..a21252c 100644
>>>> --- a/drivers/mtd/nand/fsl_elbc_nand.c
>>>> +++ b/drivers/mtd/nand/fsl_elbc_nand.c
>>>> @@ -755,6 +755,7 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd
>>>*priv)
>>>>
>>>>         /* set up nand options */
>>>>         chip->bbt_options = NAND_BBT_USE_FLASH;
>>>> +       chip->options |= NAND_NO_SUBPAGE_WRITE;
>>>
>>>Won't this break compatibility with existing UBI volumes?  That's why I
>>>didn't set this flag on eLBC when I set it on IFC (on the latter UBI is
>>>simply broken without that flag, but eLBC gets away with it because of
>>>the ECC algorithm used).
>>
>>Could be. I had to override the VID header offset accordingly to be able to attach to the ubi volume after applying this patch ...
>>
> There could be some misconfiguration in the way you generate your
> UBIFS image. Just check following:
> (1) min-io-size (-m) passed to mkfs.ubifs and ubinize commands?
> (2) vid-hdr-offset (-O) passed to ubinize and ubiattach commands?
> (3) subpage-size (-s) passed to ubinize commands? [optional]
> All of the above should be set to $PAGE_SIZE of your NAND device.
>
> Because if your image itself was generated such a way that vid-hdr was
> offset at page-boundaries then you shouldn't have needed this change.
> ---------------------------
> nand_base.c @@ nand_write_page(..)
>         if (!(chip->options & NAND_NO_SUBPAGE_WRITE) &&
>                 chip->ecc.write_subpage)
>                 subpage = offset || (data_len < mtd->writesize);   <<-----
>         else
>                 subpage = 0;
>         [...]
>         else if (subpage)  <<--------
>                 status = chip->ecc.write_subpage(mtd, chip, offset, data_len,
>                                                          buf, oob_required);
> ---------------------------
> So, even if the subpage-write is enabled, but all the write accesses are
> page-aligned, (that is offset==0 && data_len % mtd->writesize ==0)
> then also chip->ecc.write_subpage() does _not_ come in path.

Reverting "mtd: nand: subpage write support for hardware based ECC
schemes" and then
using ubiformat (without additional options) to create a volume
results in a VID header offset
of 512. And it was possible to attach to this volume successfully.
Setting NAND_NO_SUBPAGE_WRITE prevents this ubi volume to be attached
without explicitly
setting the VID header offset. This indicates that with this patch
some setups might break.

I don't necessarily want to get my patch in but IMO this is not a regression in
fsl_elbc_nand but in nand_base (also possibly affecting other drivers).

Helmut

Re: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Scott Wood]

On Thu, 2014-04-10 at 04:19 +0000, Gupta, Pekon wrote:
> >From: Scott Wood [mailto:scottwood@freescale.com]
> >>On Wed, 2014-04-09 at 10:33 +0000, Gupta, Pekon wrote:
> >> >From: Helmut Schaa [mailto:helmut.schaa@googlemail.com]
> >> >>On Wed, Apr 9, 2014 at 11:38 AM, Gupta, Pekon <pekon@ti.com> wrote:
> >> [...]
> >> >>
> >> >> (1) if chip->ecc.hwctl() and chip->ecc.calculate are not implemented but you
> >> >>  still want to use subpage write feature, then you need to provide custom
> >> >>  implementation for chip->ecc.write_subpage().
> >> >>  that's same for other interfaces of nand_chip like chip->ecc.write_page().
> >> >
> >> >But these don't cause panics :)
> >> >
> >> because fsl_elbc_nand.c uses custom implementations of chip->ecc.write_page()
> >> 	@@ fsl_elbc_chip_init(...)
> >> 		chip->ecc.write_page = fsl_elbc_write_page;
> >>
> >> Same needs to be done if subpage write is needed. However, as this is
> >> a regression so please check if following patch solves your problem.[1]
> >>
> >>
> >> >> (2) If you don't want to use subpage write feature then just disable it using
> >> >>         chip->options |= NAND_NO_SUBPAGE_WRITE;
> >> >>
> >> >> Can you please tell which NAND controller driver is causing this ?
> >> >> We need to fix that..
> >> >
> >> >This happens with fsl_elbc_nand (while trying to run ubiformat on a
> >> >mtd dev) but the
> >> >crash was caused by the introduction of nand_write_subpage_hwecc. So, in this
> >> >case I think instead of trying to fix every possible driver we should
> >> >let the nand core
> >> >code handle this issue gracefully. Maybe we could add a WARN_ON_ONCE to
> >> >notice which drivers require adjustments.
> >> >
> >> Yes agree. May be good to keep subpage write disabled by default,
> >> as only handful drivers possibly use that.
> >>
> >>
> >> [1] ## <not compile tested>
> >> ------------
> >> From bfd39102ed6aa99b7ac2b8394a2d12b879fbb4b7 Mon Sep 17 00:00:00 2001
> >> From: Pekon Gupta <pekon@ti.com>
> >> Date: Wed, 9 Apr 2014 15:51:25 +0530
> >> Subject: [PATCH] mtd: eLBC NAND: disable subpage write support
> >>
> >> As fsl_elbc_nand do not implement NAND ECC interfaces (like chip->ecc.hwctl(),
> >> chip->ecc.calculate, and chip->ecc.correct) So it cannot use default
> >> implementation of nand_write_subpage_hwecc() as in nand_base.c.
> >> Hence disabling subpage write support till a custom implementation for
> >> chip->ecc_write_subpage is added.
> >>
> >> CC: <stable@vger.kernel.org> # 3.10.x+
> >> Signed-off-by: Pekon Gupta <pekon@ti.com>
> >> ---
> >>  drivers/mtd/nand/fsl_elbc_nand.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c
> >> index ec549cd..a21252c 100644
> >> --- a/drivers/mtd/nand/fsl_elbc_nand.c
> >> +++ b/drivers/mtd/nand/fsl_elbc_nand.c
> >> @@ -755,6 +755,7 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
> >>
> >>         /* set up nand options */
> >>         chip->bbt_options = NAND_BBT_USE_FLASH;
> >> +       chip->options |= NAND_NO_SUBPAGE_WRITE;
> >
> >Won't this break compatibility with existing UBI volumes?
> > That's why I didn't set this flag on eLBC when I set it on IFC (on the latter UBI is
> >simply broken without that flag, but eLBC gets away with it because of
> >the ECC algorithm used).
> 
> Are you sure subpage write is supported on this driver ? I couldn't find any
> custom implementation for chip->ecc.write_subpage() in fsl_elbc_nand.c.

I'm referring to UBI volumes created before chip->ecc.write_subpage()
existed.

-Scott

RE: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Gupta, Pekon]

Hi Schaa, Scott,

>From: Helmut Schaa [mailto:helmut.schaa@googlemail.com]
>>On Thu, Apr 10, 2014 at 9:26 AM, Gupta, Pekon <pekon@ti.com> wrote:
>>>From: Helmut Schaa [mailto:helmut.schaa@googlemail.com]
>>>>Scott Wood <scottwood@freescale.com> schrieb:
>> [...]
>>>>> From: Pekon Gupta <pekon@ti.com>
>>>>> Date: Wed, 9 Apr 2014 15:51:25 +0530
>>>>> Subject: [PATCH] mtd: eLBC NAND: disable subpage write support
>>>>>
>>>>> As fsl_elbc_nand do not implement NAND ECC interfaces (like
>>>>chip->ecc.hwctl(),
>>>>> chip->ecc.calculate, and chip->ecc.correct) So it cannot use default
>>>>> implementation of nand_write_subpage_hwecc() as in nand_base.c.
>>>>> Hence disabling subpage write support till a custom implementation
>>>>for
>>>>> chip->ecc_write_subpage is added.
>>>>>
>>>>> CC: <stable@vger.kernel.org> # 3.10.x+
>>>>> Signed-off-by: Pekon Gupta <pekon@ti.com>
>>>>> ---
>>>>>  drivers/mtd/nand/fsl_elbc_nand.c | 1 +
>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c
>>>>b/drivers/mtd/nand/fsl_elbc_nand.c
>>>>> index ec549cd..a21252c 100644
>>>>> --- a/drivers/mtd/nand/fsl_elbc_nand.c
>>>>> +++ b/drivers/mtd/nand/fsl_elbc_nand.c
>>>>> @@ -755,6 +755,7 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd
>>>>*priv)
>>>>>
>>>>>         /* set up nand options */
>>>>>         chip->bbt_options = NAND_BBT_USE_FLASH;
>>>>> +       chip->options |= NAND_NO_SUBPAGE_WRITE;
>>>>
>>>>Won't this break compatibility with existing UBI volumes?  That's why I
>>>>didn't set this flag on eLBC when I set it on IFC (on the latter UBI is
>>>>simply broken without that flag, but eLBC gets away with it because of
>>>>the ECC algorithm used).
>>>
>>>Could be. I had to override the VID header offset accordingly to be able to attach to the ubi volume after applying this patch ...
>>>
>> There could be some misconfiguration in the way you generate your
>> UBIFS image. Just check following:
>> (1) min-io-size (-m) passed to mkfs.ubifs and ubinize commands?
>> (2) vid-hdr-offset (-O) passed to ubinize and ubiattach commands?
>> (3) subpage-size (-s) passed to ubinize commands? [optional]
>> All of the above should be set to $PAGE_SIZE of your NAND device.
>>
>> Because if your image itself was generated such a way that vid-hdr was
>> offset at page-boundaries then you shouldn't have needed this change.
>> ---------------------------
>> nand_base.c @@ nand_write_page(..)
>>         if (!(chip->options & NAND_NO_SUBPAGE_WRITE) &&
>>                 chip->ecc.write_subpage)
>>                 subpage = offset || (data_len < mtd->writesize);   <<-----
>>         else
>>                 subpage = 0;
>>         [...]
>>         else if (subpage)  <<--------
>>                 status = chip->ecc.write_subpage(mtd, chip, offset, data_len,
>>                                                          buf, oob_required);
>> ---------------------------
>> So, even if the subpage-write is enabled, but all the write accesses are
>> page-aligned, (that is offset==0 && data_len % mtd->writesize ==0)
>> then also chip->ecc.write_subpage() does _not_ come in path.
>
>Reverting "mtd: nand: subpage write support for hardware based ECC
>schemes" and then
>using ubiformat (without additional options) to create a volume
>results in a VID header offset
>of 512. And it was possible to attach to this volume successfully.
>Setting NAND_NO_SUBPAGE_WRITE prevents this ubi volume to be attached
>without explicitly
>setting the VID header offset. This indicates that with this patch
>some setups might break.
>
As per [1] and [2], If the subpage is _not_ supported then VID-header offset
should be aligned to  PAGE_SIZE boundary. And it has implications on LEB size
calculation too. I don't know how it was working earlier but if the earlier
UBI images were using 512 as VID Header offset even without subpage
Support then it's incorrect configuration.
Now whether we still want backward compatibility for something which was
incorrect, is something I leave it for Maintainers {Artem, Brian} to decide.

>I don't necessarily want to get my patch in but IMO this is not a regression in
>fsl_elbc_nand but in nand_base (also possibly affecting other drivers).
>
>Helmut
Yes, it's not about the patch, but to fix the root issue for all the drivers.
Hopefully you are observations are on latest mtd-utils, as older versions
of the tool might have some bug-fixes.

[1] http://www.linux-mtd.infradead.org/doc/ubi.html#L_min_io_unit
[2] http://www.linux-mtd.infradead.org/doc/ubi.html#L_subpage


with regards, pekon

Re: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Scott Wood]

On Fri, 2014-04-11 at 06:35 +0000, Gupta, Pekon wrote:
> As per [1] and [2], If the subpage is _not_ supported then VID-header offset
> should be aligned to  PAGE_SIZE boundary. And it has implications on LEB size
> calculation too. I don't know how it was working earlier but if the earlier
> UBI images were using 512 as VID Header offset even without subpage
> Support then it's incorrect configuration.

I believe what was happening before was full page writes with only
subpage content (the rest left at 0xff).

> Now whether we still want backward compatibility for something which was
> incorrect, is something I leave it for Maintainers {Artem, Brian} to decide.

Why is it incorrect?  Subpage writes were happening -- if they weren't,
then we wouldn't have seen problems on IFC without that flag.

-Scott

RE: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Gupta, Pekon]

>From: Scott Wood [mailto:scottwood@freescale.com]
>>On Fri, 2014-04-11 at 06:35 +0000, Gupta, Pekon wrote:
>> As per [1] and [2], If the subpage is _not_ supported then VID-header offset
>> should be aligned to  PAGE_SIZE boundary. And it has implications on LEB size
>> calculation too. I don't know how it was working earlier but if the earlier
>> UBI images were using 512 as VID Header offset even without subpage
>> Support then it's incorrect configuration.
>
>I believe what was happening before was full page writes with only
>subpage content (the rest left at 0xff).
>
Yes, that is possible, sorry I missed that nand_do_write_ops() does 0xff padding
for non-page aligned writes. In that case replicating fsl_elbc_write_page() for
chip->ecc.write_subpage should also work and maintain backward compatibility.

(not compile tested)
------------------------------------
diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c
index a21252c..cc79ce4 100644
--- a/drivers/mtd/nand/fsl_elbc_nand.c
+++ b/drivers/mtd/nand/fsl_elbc_nand.c
@@ -723,6 +723,19 @@ static int fsl_elbc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
        return 0;
 }

+/* ECC will be calculated automatically, and errors will be detected in
+ * waitfunc.
+ */
+static int fsl_elbc_write_subpage(struct mtd_info *mtd, struct nand_chip *chip,
+                               uint32_t offset, uint32_t data_len,
+                               const uint8_t *buf, int oob_required)
+{
+       fsl_elbc_write_buf(mtd, buf, mtd->writesize);
+       fsl_elbc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
+
+       return 0;
+}
+
 static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
 {
        struct fsl_lbc_ctrl *ctrl = priv->ctrl;
@@ -762,6 +775,7 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)

        chip->ecc.read_page = fsl_elbc_read_page;
        chip->ecc.write_page = fsl_elbc_write_page;
+       chip->ecc.write_subpage = fsl_elbc_write_subpage;

        /* If CS Base Register selects full hardware ECC then use it */
        if ((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==

------------------------------------

with regards, pekon

RE: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Helmut Schaa]

On 14. April 2014 07:55:30 MESZ, "Gupta, Pekon" <pekon@ti.com> wrote:
>>From: Scott Wood [mailto:scottwood@freescale.com]
>>>On Fri, 2014-04-11 at 06:35 +0000, Gupta, Pekon wrote:
>>> As per [1] and [2], If the subpage is _not_ supported then
>VID-header offset
>>> should be aligned to  PAGE_SIZE boundary. And it has implications on
>LEB size
>>> calculation too. I don't know how it was working earlier but if the
>earlier
>>> UBI images were using 512 as VID Header offset even without subpage
>>> Support then it's incorrect configuration.
>>
>>I believe what was happening before was full page writes with only
>>subpage content (the rest left at 0xff).
>>
>Yes, that is possible, sorry I missed that nand_do_write_ops() does
>0xff padding
>for non-page aligned writes. In that case replicating
>fsl_elbc_write_page() for
>chip->ecc.write_subpage should also work and maintain backward
>compatibility.
>
>(not compile tested)

JFI, I'm in vacation till next week and don't have access to the affected system. Will do a test once I'm back.
Helmut

>------------------------------------
>diff --git a/drivers/mtd/nand/fsl_elbc_nand.c
>b/drivers/mtd/nand/fsl_elbc_nand.c
>index a21252c..cc79ce4 100644
>--- a/drivers/mtd/nand/fsl_elbc_nand.c
>+++ b/drivers/mtd/nand/fsl_elbc_nand.c
>@@ -723,6 +723,19 @@ static int fsl_elbc_write_page(struct mtd_info
>*mtd, struct nand_chip *chip,
>        return 0;
> }
>
>+/* ECC will be calculated automatically, and errors will be detected
>in
>+ * waitfunc.
>+ */
>+static int fsl_elbc_write_subpage(struct mtd_info *mtd, struct
>nand_chip *chip,
>+                               uint32_t offset, uint32_t data_len,
>+                               const uint8_t *buf, int oob_required)
>+{
>+       fsl_elbc_write_buf(mtd, buf, mtd->writesize);
>+       fsl_elbc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
>+
>+       return 0;
>+}
>+
> static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
> {
>        struct fsl_lbc_ctrl *ctrl = priv->ctrl;
>@@ -762,6 +775,7 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd
>*priv)
>
>        chip->ecc.read_page = fsl_elbc_read_page;
>        chip->ecc.write_page = fsl_elbc_write_page;
>+       chip->ecc.write_subpage = fsl_elbc_write_subpage;
>
>        /* If CS Base Register selects full hardware ECC then use it */
>        if ((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
>
>------------------------------------
>
>with regards, pekon

-- 
Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.

RE: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Gupta, Pekon]

Hi,

>From: Helmut Schaa [mailto:helmut.schaa@googlemail.com]
>>On 14. April 2014 07:55:30 MESZ, "Gupta, Pekon" <pekon@ti.com>
>>>From: Scott Wood [mailto:scottwood@freescale.com]
>>>>On Fri, 2014-04-11 at 06:35 +0000, Gupta, Pekon wrote:

>>>> As per [1] and [2], If the subpage is _not_ supported then
>>VID-header offset
>>>> should be aligned to  PAGE_SIZE boundary. And it has
>implications on
>>LEB size
>>>> calculation too. I don't know how it was working earlier but if the
>>earlier
>>>> UBI images were using 512 as VID Header offset even without
>subpage
>>>> Support then it's incorrect configuration.
>>>
>>>I believe what was happening before was full page writes with only
>>>subpage content (the rest left at 0xff).
>>>
>>Yes, that is possible, sorry I missed that nand_do_write_ops() does
>>0xff padding
>>for non-page aligned writes. In that case replicating
>>fsl_elbc_write_page() for
>>chip->ecc.write_subpage should also work and maintain backward
>>compatibility.
>>
>>(not compile tested)
>
>JFI, I'm in vacation till next week and don't have access to the affected
>system. Will do a test once I'm back.
>Helmut
>

Any update on below patch? Does this maintain compatibility to
existing UBIFS images ?

with regards, Pekon



>>------------------------------------
>>diff --git a/drivers/mtd/nand/fsl_elbc_nand.c
>>b/drivers/mtd/nand/fsl_elbc_nand.c
>>index a21252c..cc79ce4 100644
>>--- a/drivers/mtd/nand/fsl_elbc_nand.c
>>+++ b/drivers/mtd/nand/fsl_elbc_nand.c
>>@@ -723,6 +723,19 @@ static int fsl_elbc_write_page(struct
>mtd_info
>>*mtd, struct nand_chip *chip,
>>        return 0;
>> }
>>
>>+/* ECC will be calculated automatically, and errors will be detected
>>in
>>+ * waitfunc.
>>+ */
>>+static int fsl_elbc_write_subpage(struct mtd_info *mtd, struct
>>nand_chip *chip,
>>+                               uint32_t offset, uint32_t data_len,
>>+                               const uint8_t *buf, int oob_required)
>>+{
>>+       fsl_elbc_write_buf(mtd, buf, mtd->writesize);
>>+       fsl_elbc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
>>+
>>+       return 0;
>>+}
>>+
>> static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
>> {
>>        struct fsl_lbc_ctrl *ctrl = priv->ctrl;
>>@@ -762,6 +775,7 @@ static int fsl_elbc_chip_init(struct
>fsl_elbc_mtd
>>*priv)
>>
>>        chip->ecc.read_page = fsl_elbc_read_page;
>>        chip->ecc.write_page = fsl_elbc_write_page;
>>+       chip->ecc.write_subpage = fsl_elbc_write_subpage;
>>
>>        /* If CS Base Register selects full hardware ECC then use it */
>>        if ((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
>>
>>------------------------------------

Re: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Helmut Schaa]

Hi Pekon,

On Mon, May 5, 2014 at 10:46 AM, Gupta, Pekon <pekon@ti.com> wrote:
> Any update on below patch? Does this maintain compatibility to
> existing UBIFS images ?

Patch works fine with a VID header offset of 512 and without the crash :)

Feel free to add my

Reported-by: Helmut Schaa <helmut.schaa@googlemail>
Tested-by: Helmut Schaa <helmut.schaa@googlemail>

when sending the patch.

Thanks,
Helmut

> with regards, Pekon
>
>
>
>>>------------------------------------
>>>diff --git a/drivers/mtd/nand/fsl_elbc_nand.c
>>>b/drivers/mtd/nand/fsl_elbc_nand.c
>>>index a21252c..cc79ce4 100644
>>>--- a/drivers/mtd/nand/fsl_elbc_nand.c
>>>+++ b/drivers/mtd/nand/fsl_elbc_nand.c
>>>@@ -723,6 +723,19 @@ static int fsl_elbc_write_page(struct
>>mtd_info
>>>*mtd, struct nand_chip *chip,
>>>        return 0;
>>> }
>>>
>>>+/* ECC will be calculated automatically, and errors will be detected
>>>in
>>>+ * waitfunc.
>>>+ */
>>>+static int fsl_elbc_write_subpage(struct mtd_info *mtd, struct
>>>nand_chip *chip,
>>>+                               uint32_t offset, uint32_t data_len,
>>>+                               const uint8_t *buf, int oob_required)
>>>+{
>>>+       fsl_elbc_write_buf(mtd, buf, mtd->writesize);
>>>+       fsl_elbc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
>>>+
>>>+       return 0;
>>>+}
>>>+
>>> static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
>>> {
>>>        struct fsl_lbc_ctrl *ctrl = priv->ctrl;
>>>@@ -762,6 +775,7 @@ static int fsl_elbc_chip_init(struct
>>fsl_elbc_mtd
>>>*priv)
>>>
>>>        chip->ecc.read_page = fsl_elbc_read_page;
>>>        chip->ecc.write_page = fsl_elbc_write_page;
>>>+       chip->ecc.write_subpage = fsl_elbc_write_subpage;
>>>
>>>        /* If CS Base Register selects full hardware ECC then use it */
>>>        if ((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
>>>
>>>------------------------------------

Re: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Brian Norris]

Hi,

I have my archaeology hat on, in the deep corners of my mailbox...

On Wed, Apr 09, 2014 at 11:13:24AM +0200, Helmut Schaa wrote:
> nand_write_subpage_hwecc causes a crash if the driver did not register
> ecc->hwctl or ecc->calculate. Fix this by disabling subpage writes if
> ecc->hwctl or ecc->calculate is not provided by the driver.
> 
> This behavior was introduced in commit 837a6ba4f3b6d23026674e6af6b6849a4634fff9
> "mtd: nand: subpage write support for hardware based ECC schemes".
> 
> This fixes a crash with fsl_elbc_nand and maybe others:
> 
[...]
> 
> Cc: Gupta, Pekon <pekon@ti.com>
> Cc: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> Cc: David Woodhouse <David.Woodhouse@intel.com>
> Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
> ---
> I noticed this bug on openwrt with kernel 3.10 but it looks as if this can
> still happen in more recent kernels.
> 
>  drivers/mtd/nand/nand_base.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 9715a7b..2298289 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3768,7 +3768,7 @@ int nand_scan_tail(struct mtd_info *mtd)
>  			ecc->write_oob = nand_write_oob_std;
>  		if (!ecc->read_subpage)
>  			ecc->read_subpage = nand_read_subpage;
> -		if (!ecc->write_subpage)
> +		if (!ecc->write_subpage && ecc->hwctl && ecc->calculate)
>  			ecc->write_subpage = nand_write_subpage_hwecc;
>  
>  	case NAND_ECC_HW_SYNDROME:

I realize we've merged a fix for fsl_elbc_nand long ago (commit
f034d87def51 "mtd: eLBC NAND: fix subpage write support"), but this
change still looks sane, applies, and could possibly fix some other
drivers that are lurking somewhere. Any reason I shouldn't apply it?

Regards,
Brian

Re: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Helmut Schaa]

On Thu, Dec 10, 2015 at 12:58 AM, Brian Norris
<computersforpeace@gmail.com> wrote:
> Hi,
>
> I have my archaeology hat on, in the deep corners of my mailbox...
>
> On Wed, Apr 09, 2014 at 11:13:24AM +0200, Helmut Schaa wrote:
>> nand_write_subpage_hwecc causes a crash if the driver did not register
>> ecc->hwctl or ecc->calculate. Fix this by disabling subpage writes if
>> ecc->hwctl or ecc->calculate is not provided by the driver.
>>
>> This behavior was introduced in commit 837a6ba4f3b6d23026674e6af6b6849a4634fff9
>> "mtd: nand: subpage write support for hardware based ECC schemes".
>>
>> This fixes a crash with fsl_elbc_nand and maybe others:
>>
> [...]
>>
>> Cc: Gupta, Pekon <pekon@ti.com>
>> Cc: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>> Cc: David Woodhouse <David.Woodhouse@intel.com>
>> Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
>> ---
>> I noticed this bug on openwrt with kernel 3.10 but it looks as if this can
>> still happen in more recent kernels.
>>
>>  drivers/mtd/nand/nand_base.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> index 9715a7b..2298289 100644
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -3768,7 +3768,7 @@ int nand_scan_tail(struct mtd_info *mtd)
>>                       ecc->write_oob = nand_write_oob_std;
>>               if (!ecc->read_subpage)
>>                       ecc->read_subpage = nand_read_subpage;
>> -             if (!ecc->write_subpage)
>> +             if (!ecc->write_subpage && ecc->hwctl && ecc->calculate)
>>                       ecc->write_subpage = nand_write_subpage_hwecc;
>>
>>       case NAND_ECC_HW_SYNDROME:
>
> I realize we've merged a fix for fsl_elbc_nand long ago (commit
> f034d87def51 "mtd: eLBC NAND: fix subpage write support"), but this
> change still looks sane, applies, and could possibly fix some other
> drivers that are lurking somewhere. Any reason I shouldn't apply it?

I'm not carrying this patch in my tree anymore and did not see any more
crashes with the flash configuration I'm using. So, I think the original patch
is kind of superfluous.

Regards,
Helmut

Re: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Brian Norris]

On Thu, Dec 10, 2015 at 10:31:32AM +0100, Helmut Schaa wrote:
> On Thu, Dec 10, 2015 at 12:58 AM, Brian Norris
> <computersforpeace@gmail.com> wrote:
> > On Wed, Apr 09, 2014 at 11:13:24AM +0200, Helmut Schaa wrote:
> >> nand_write_subpage_hwecc causes a crash if the driver did not register
> >> ecc->hwctl or ecc->calculate. Fix this by disabling subpage writes if
> >> ecc->hwctl or ecc->calculate is not provided by the driver.
> >>
> >> This behavior was introduced in commit 837a6ba4f3b6d23026674e6af6b6849a4634fff9
> >> "mtd: nand: subpage write support for hardware based ECC schemes".
> >>
> >> This fixes a crash with fsl_elbc_nand and maybe others:
> >>
> > [...]
> >>
> >> --- a/drivers/mtd/nand/nand_base.c
> >> +++ b/drivers/mtd/nand/nand_base.c
> >> @@ -3768,7 +3768,7 @@ int nand_scan_tail(struct mtd_info *mtd)
> >>                       ecc->write_oob = nand_write_oob_std;
> >>               if (!ecc->read_subpage)
> >>                       ecc->read_subpage = nand_read_subpage;
> >> -             if (!ecc->write_subpage)
> >> +             if (!ecc->write_subpage && ecc->hwctl && ecc->calculate)
> >>                       ecc->write_subpage = nand_write_subpage_hwecc;
> >>
> >>       case NAND_ECC_HW_SYNDROME:
> >
> > I realize we've merged a fix for fsl_elbc_nand long ago (commit
> > f034d87def51 "mtd: eLBC NAND: fix subpage write support"), but this
> > change still looks sane, applies, and could possibly fix some other
> > drivers that are lurking somewhere. Any reason I shouldn't apply it?
> 
> I'm not carrying this patch in my tree anymore and did not see any more
> crashes with the flash configuration I'm using. So, I think the original patch
> is kind of superfluous.

There could easily be other drivers that don't have hwctl() or
calculate() callbacks yet use NAND_ECC_HW. By inspection, I see that
maybe the new hisi504_nand.c is susceptible, as well as sh_flctl.c and
maybe even sunxi_nand.c (?). Some of these might not be hit if they
don't use NAND that support subpage writes.

Anyway, I think this patch is helpful, because it prevents drivers from
having to fill in a 'subpage' write callback that looks just like their
write_page() callback.

Brian

Re: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Boris Brezillon]

Hi Brian,

On Fri, 11 Dec 2015 16:40:25 -0800
Brian Norris <computersforpeace@gmail.com> wrote:

> On Thu, Dec 10, 2015 at 10:31:32AM +0100, Helmut Schaa wrote:
> > On Thu, Dec 10, 2015 at 12:58 AM, Brian Norris
> > <computersforpeace@gmail.com> wrote:
> > > On Wed, Apr 09, 2014 at 11:13:24AM +0200, Helmut Schaa wrote:
> > >> nand_write_subpage_hwecc causes a crash if the driver did not register
> > >> ecc->hwctl or ecc->calculate. Fix this by disabling subpage writes if
> > >> ecc->hwctl or ecc->calculate is not provided by the driver.
> > >>
> > >> This behavior was introduced in commit 837a6ba4f3b6d23026674e6af6b6849a4634fff9
> > >> "mtd: nand: subpage write support for hardware based ECC schemes".
> > >>
> > >> This fixes a crash with fsl_elbc_nand and maybe others:
> > >>
> > > [...]
> > >>
> > >> --- a/drivers/mtd/nand/nand_base.c
> > >> +++ b/drivers/mtd/nand/nand_base.c
> > >> @@ -3768,7 +3768,7 @@ int nand_scan_tail(struct mtd_info *mtd)
> > >>                       ecc->write_oob = nand_write_oob_std;
> > >>               if (!ecc->read_subpage)
> > >>                       ecc->read_subpage = nand_read_subpage;

Shouldn't we have the same kind of test for ->read_subpage.

		if (!ecc->read_subpage && ecc->correct &&
		    ecc->calculate)
			ecc->read_subpage = nand_read_subpage;


> > >> -             if (!ecc->write_subpage)
> > >> +             if (!ecc->write_subpage && ecc->hwctl && ecc->calculate)
> > >>                       ecc->write_subpage = nand_write_subpage_hwecc;
> > >>
> > >>       case NAND_ECC_HW_SYNDROME:
> > >
> > > I realize we've merged a fix for fsl_elbc_nand long ago (commit
> > > f034d87def51 "mtd: eLBC NAND: fix subpage write support"), but this
> > > change still looks sane, applies, and could possibly fix some other
> > > drivers that are lurking somewhere. Any reason I shouldn't apply it?
> > 
> > I'm not carrying this patch in my tree anymore and did not see any more
> > crashes with the flash configuration I'm using. So, I think the original patch
> > is kind of superfluous.
> 
> There could easily be other drivers that don't have hwctl() or
> calculate() callbacks yet use NAND_ECC_HW. By inspection, I see that
> maybe the new hisi504_nand.c is susceptible, as well as sh_flctl.c and
> maybe even sunxi_nand.c (?). Some of these might not be hit if they
> don't use NAND that support subpage writes.

Yep, sunxi platforms could be hit by this bug, but it seems nobody
decided to design a board with an SLC NAND (and MLCs are not supporting
subpage writes).

> 
> Anyway, I think this patch is helpful, because it prevents drivers from
> having to fill in a 'subpage' write callback that looks just like their
> write_page() callback.

I agree.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Helmut Schaa]

On Sat, Dec 12, 2015 at 8:59 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Hi Brian,
>
> On Fri, 11 Dec 2015 16:40:25 -0800
> Brian Norris <computersforpeace@gmail.com> wrote:
>
>> On Thu, Dec 10, 2015 at 10:31:32AM +0100, Helmut Schaa wrote:
>> > On Thu, Dec 10, 2015 at 12:58 AM, Brian Norris
>> > <computersforpeace@gmail.com> wrote:
>> > > On Wed, Apr 09, 2014 at 11:13:24AM +0200, Helmut Schaa wrote:
>> > >> nand_write_subpage_hwecc causes a crash if the driver did not register
>> > >> ecc->hwctl or ecc->calculate. Fix this by disabling subpage writes if
>> > >> ecc->hwctl or ecc->calculate is not provided by the driver.
>> > >>
>> > >> This behavior was introduced in commit 837a6ba4f3b6d23026674e6af6b6849a4634fff9
>> > >> "mtd: nand: subpage write support for hardware based ECC schemes".
>> > >>
>> > >> This fixes a crash with fsl_elbc_nand and maybe others:
>> > >>
>> > > [...]
>> > >>
>> > >> --- a/drivers/mtd/nand/nand_base.c
>> > >> +++ b/drivers/mtd/nand/nand_base.c
>> > >> @@ -3768,7 +3768,7 @@ int nand_scan_tail(struct mtd_info *mtd)
>> > >>                       ecc->write_oob = nand_write_oob_std;
>> > >>               if (!ecc->read_subpage)
>> > >>                       ecc->read_subpage = nand_read_subpage;
>
> Shouldn't we have the same kind of test for ->read_subpage.
>
>                 if (!ecc->read_subpage && ecc->correct &&
>                     ecc->calculate)
>                         ecc->read_subpage = nand_read_subpage;
>
>
>> > >> -             if (!ecc->write_subpage)
>> > >> +             if (!ecc->write_subpage && ecc->hwctl && ecc->calculate)
>> > >>                       ecc->write_subpage = nand_write_subpage_hwecc;
>> > >>
>> > >>       case NAND_ECC_HW_SYNDROME:
>> > >
>> > > I realize we've merged a fix for fsl_elbc_nand long ago (commit
>> > > f034d87def51 "mtd: eLBC NAND: fix subpage write support"), but this
>> > > change still looks sane, applies, and could possibly fix some other
>> > > drivers that are lurking somewhere. Any reason I shouldn't apply it?
>> >
>> > I'm not carrying this patch in my tree anymore and did not see any more
>> > crashes with the flash configuration I'm using. So, I think the original patch
>> > is kind of superfluous.
>>
>> There could easily be other drivers that don't have hwctl() or
>> calculate() callbacks yet use NAND_ECC_HW. By inspection, I see that
>> maybe the new hisi504_nand.c is susceptible, as well as sh_flctl.c and
>> maybe even sunxi_nand.c (?). Some of these might not be hit if they
>> don't use NAND that support subpage writes.
>
> Yep, sunxi platforms could be hit by this bug, but it seems nobody
> decided to design a board with an SLC NAND (and MLCs are not supporting
> subpage writes).
>
>>
>> Anyway, I think this patch is helpful, because it prevents drivers from
>> having to fill in a 'subpage' write callback that looks just like their
>> write_page() callback.
>
> I agree.

Fine with me then.
Helmut

Re: [PATCH] mtd: nand: Disable subpage writes for drivers without ecc->hwctl

[Brian Norris]

On Sat, Dec 12, 2015 at 08:59:00AM +0100, Boris Brezillon wrote:
> On Fri, 11 Dec 2015 16:40:25 -0800
> Brian Norris <computersforpeace@gmail.com> wrote:
> 
> > On Thu, Dec 10, 2015 at 10:31:32AM +0100, Helmut Schaa wrote:
> > > On Thu, Dec 10, 2015 at 12:58 AM, Brian Norris
> > > <computersforpeace@gmail.com> wrote:
> > > > On Wed, Apr 09, 2014 at 11:13:24AM +0200, Helmut Schaa wrote:
> > > >> nand_write_subpage_hwecc causes a crash if the driver did not register
> > > >> ecc->hwctl or ecc->calculate. Fix this by disabling subpage writes if
> > > >> ecc->hwctl or ecc->calculate is not provided by the driver.
> > > >>
> > > >> This behavior was introduced in commit 837a6ba4f3b6d23026674e6af6b6849a4634fff9
> > > >> "mtd: nand: subpage write support for hardware based ECC schemes".
> > > >>
> > > >> This fixes a crash with fsl_elbc_nand and maybe others:
> > > >>
> > > > [...]
> > > >>
> > > >> --- a/drivers/mtd/nand/nand_base.c
> > > >> +++ b/drivers/mtd/nand/nand_base.c
> > > >> @@ -3768,7 +3768,7 @@ int nand_scan_tail(struct mtd_info *mtd)
> > > >>                       ecc->write_oob = nand_write_oob_std;
> > > >>               if (!ecc->read_subpage)
> > > >>                       ecc->read_subpage = nand_read_subpage;
> 
> Shouldn't we have the same kind of test for ->read_subpage.
> 
> 		if (!ecc->read_subpage && ecc->correct &&
> 		    ecc->calculate)
> 			ecc->read_subpage = nand_read_subpage;

We could do that, though right now we have a little less danger for
read_subpage() than we do with write_subpage(), because drivers have to
opt in with the NAND_SUBPAGE_READ flag before this function will
actually be used. Feel free to send a patch if you'd like.

> > > >> -             if (!ecc->write_subpage)
> > > >> +             if (!ecc->write_subpage && ecc->hwctl && ecc->calculate)
> > > >>                       ecc->write_subpage = nand_write_subpage_hwecc;
> > > >>
> > > >>       case NAND_ECC_HW_SYNDROME:
> > > >
> > > > I realize we've merged a fix for fsl_elbc_nand long ago (commit
> > > > f034d87def51 "mtd: eLBC NAND: fix subpage write support"), but this
> > > > change still looks sane, applies, and could possibly fix some other
> > > > drivers that are lurking somewhere. Any reason I shouldn't apply it?
> > > 
> > > I'm not carrying this patch in my tree anymore and did not see any more
> > > crashes with the flash configuration I'm using. So, I think the original patch
> > > is kind of superfluous.
> > 
> > There could easily be other drivers that don't have hwctl() or
> > calculate() callbacks yet use NAND_ECC_HW. By inspection, I see that
> > maybe the new hisi504_nand.c is susceptible, as well as sh_flctl.c and
> > maybe even sunxi_nand.c (?). Some of these might not be hit if they
> > don't use NAND that support subpage writes.
> 
> Yep, sunxi platforms could be hit by this bug, but it seems nobody
> decided to design a board with an SLC NAND (and MLCs are not supporting
> subpage writes).
> 
> > 
> > Anyway, I think this patch is helpful, because it prevents drivers from
> > having to fill in a 'subpage' write callback that looks just like their
> > write_page() callback.
> 
> I agree.

OK. Applied to l2-mtd.git, with additional commentary about how the
reported issue is already fixed, but there could be other drivers who
run across this problem.

Brian