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

>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(+)

--
1.8.5.1.163.gd7aced9

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

with regards, pekon

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

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

>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

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

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

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

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

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

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