Patchwork [v3,06/10] mtd: fix the wrong mtd->type for nand chip

login
register
mail settings
Submitter Huang Shijie
Date Aug. 26, 2013, 9:36 a.m.
Message ID <1377509808-29363-7-git-send-email-b32955@freescale.com>
Download mbox | patch
Permalink /patch/269846/
State New
Headers show

Comments

Huang Shijie - Aug. 26, 2013, 9:36 a.m.
Current code sets the mtd->type with MTD_NANDFLASH for both
SLC and MLC. So the jffs2 may supports the MLC nand, but in actually,
the jffs2 should not support the MLC.

This patch uses the nand_is_slc() to check the nand cell type,
and set the mtd->type with the right nand type.

After this patch, the jffs2 only can support the SLC nand.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/nand/nand_base.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Brian Norris - Aug. 28, 2013, 3:08 a.m.
Since we may see a v4 anyway, I'll make a comment here that I've been 
mulling over.

On 08/26/2013 02:36 AM, Huang Shijie wrote:
> Current code sets the mtd->type with MTD_NANDFLASH for both
> SLC and MLC. So the jffs2 may supports the MLC nand, but in actually,
> the jffs2 should not support the MLC.
>
> This patch uses the nand_is_slc() to check the nand cell type,
> and set the mtd->type with the right nand type.
>
> After this patch, the jffs2 only can support the SLC nand.
>
> Signed-off-by: Huang Shijie <b32955@freescale.com>

This patch doesn't note here that it is breaking the ABI: it is the 
first (AFAIK; I don't think onenand ever actually had MLC, right?) patch 
to cause an MTD to show up as MTD_MLCNANDFLASH (and "unknown" in sysfs 
-- but you change this later to "mlc-nand"). This should be made more 
clear, if we're going to do this. And you should document and explain it 
*before* this patch.

But more importantly, you don't really answer this question I have: why 
we want to expose this MTD_MLCNANDFLASH type to user-space? It seems you 
need this for internal drivers' usage and for JFFS2 (both valid 
reasons). But exporting it to user-space just makes us work to change 
mtd-utils (and any other scripts that might rely on these types). Note 
that not everybody upgrades mtd-utils along with their kernel.

One argument in favor of your change: so a user can programmatically 
determine whether to use JFFS2 or UBIFS on a particular NAND. (But then 
again, SLC are getting more and more MLC-like properties that make them 
unsuitable for JFFS2...)

So, I would recommend a few rearrangements of part of this series:
(1) fixup the sysfs show() function and ABI documentation (in the same 
patch) to cover "mlc-nand". *** must clearly explain the rationale for 
userspace change (currently missing) ***
(2) do any changes to JFFS2 based on the MLC type
(3) allow nand_base.c to export MTD_MLCNANDFLASH

> ---
>   drivers/mtd/nand/nand_base.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 8755a74..5957fe7 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3781,7 +3781,7 @@ int nand_scan_tail(struct mtd_info *mtd)
>   		chip->options |= NAND_SUBPAGE_READ;
>
>   	/* Fill in remaining MTD driver data */
> -	mtd->type = MTD_NANDFLASH;
> +	mtd->type = nand_is_slc(chip) ? MTD_NANDFLASH : MTD_MLCNANDFLASH;
>   	mtd->flags = (chip->options & NAND_ROM) ? MTD_CAP_ROM :
>   						MTD_CAP_NANDFLASH;
>   	mtd->_erase = nand_erase;
>

Brian
Huang Shijie - Aug. 28, 2013, 6:59 a.m.
于 2013年08月28日 11:08, Brian Norris 写道:
> Since we may see a v4 anyway, I'll make a comment here that I've been 
> mulling over.
>
> On 08/26/2013 02:36 AM, Huang Shijie wrote:
>> Current code sets the mtd->type with MTD_NANDFLASH for both
>> SLC and MLC. So the jffs2 may supports the MLC nand, but in actually,
>> the jffs2 should not support the MLC.
>>
>> This patch uses the nand_is_slc() to check the nand cell type,
>> and set the mtd->type with the right nand type.
>>
>> After this patch, the jffs2 only can support the SLC nand.
>>
>> Signed-off-by: Huang Shijie <b32955@freescale.com>
>
> This patch doesn't note here that it is breaking the ABI: it is the 
> first (AFAIK; I don't think onenand ever actually had MLC, right?) 
> patch to cause an MTD to show up as MTD_MLCNANDFLASH (and "unknown" in 
> sysfs -- but you change this later to "mlc-nand"). This should be made 
> more clear, if we're going to do this. And you should document and 
> explain it *before* this patch.
okay.
>
> But more importantly, you don't really answer this question I have: 
> why we want to expose this MTD_MLCNANDFLASH type to user-space? It 
> seems you need this for internal drivers' usage and for JFFS2 (both 
> valid reasons). But exporting it to user-space just makes us work to 
> change mtd-utils (and any other scripts that might rely on these 
> types). Note that not everybody upgrades mtd-utils along with their 
> kernel.
>
After we change the code with this patch, the SLC still expose the 
'nand' to the user space, it's ok;
but the MLC will expose "unknown" as you said above. Some mtd-utils, 
such as mtd_debug will not work well any more.

If we do expose the MTD_MLCNANDFLASH to use space, the mtd-utils may can 
not work with MLC nand, some code only checks the
MTD_NANDFLASH. Please see the :
http://lists.infradead.org/pipermail/linux-mtd/2013-August/048213.html


That's why we should expose the MTD_MLCNANDFLASH to the user-space

> One argument in favor of your change: so a user can programmatically 
> determine whether to use JFFS2 or UBIFS on a particular NAND. (But 
> then again, SLC are getting more and more MLC-like properties that 
> make them unsuitable for JFFS2...)
>
> So, I would recommend a few rearrangements of part of this series:
> (1) fixup the sysfs show() function and ABI documentation (in the same 
> patch) to cover "mlc-nand". *** must clearly explain the rationale for 
> userspace change (currently missing) ***
> (2) do any changes to JFFS2 based on the MLC type
> (3) allow nand_base.c to export MTD_MLCNANDFLASH
I will arrange the next version in this order.

thanks
Huang Shijie
Brian Norris - Aug. 28, 2013, 7:28 a.m.
On 08/27/2013 11:59 PM, Huang Shijie wrote:
> 于 2013年08月28日 11:08, Brian Norris 写道:
>> Since we may see a v4 anyway, I'll make a comment here that I've been
>> mulling over.
>>
>> On 08/26/2013 02:36 AM, Huang Shijie wrote:
>>> Current code sets the mtd->type with MTD_NANDFLASH for both
>>> SLC and MLC. So the jffs2 may supports the MLC nand, but in actually,
>>> the jffs2 should not support the MLC.
>>>
>>> This patch uses the nand_is_slc() to check the nand cell type,
>>> and set the mtd->type with the right nand type.
>>>
>>> After this patch, the jffs2 only can support the SLC nand.
>>>
>>> Signed-off-by: Huang Shijie <b32955@freescale.com>
>>
>> This patch doesn't note here that it is breaking the ABI: it is the
>> first (AFAIK; I don't think onenand ever actually had MLC, right?)
>> patch to cause an MTD to show up as MTD_MLCNANDFLASH (and "unknown" in
>> sysfs -- but you change this later to "mlc-nand"). This should be made
>> more clear, if we're going to do this. And you should document and
>> explain it *before* this patch.
> okay.
>>
>> But more importantly, you don't really answer this question I have:
>> why we want to expose this MTD_MLCNANDFLASH type to user-space? It
>> seems you need this for internal drivers' usage and for JFFS2 (both
>> valid reasons). But exporting it to user-space just makes us work to
>> change mtd-utils (and any other scripts that might rely on these
>> types). Note that not everybody upgrades mtd-utils along with their
>> kernel.
>>
> After we change the code with this patch, the SLC still expose the
> 'nand' to the user space, it's ok;
> but the MLC will expose "unknown" as you said above. Some mtd-utils,
> such as mtd_debug will not work well any more.

Right. That's part of the ABI breakage.

> If we do expose the MTD_MLCNANDFLASH to use space, the mtd-utils may can
> not work with MLC nand, some code only checks the
> MTD_NANDFLASH. Please see the :
> http://lists.infradead.org/pipermail/linux-mtd/2013-August/048213.html

Yes, I saw that.

> That's why we should expose the MTD_MLCNANDFLASH to the user-space

All you've shown is the breakage, not the reason for exposing 
MTD_MLCNANFLASH. There's a difference between MTD_MLCNANDFLASH being in 
the mtd-abi.h header and actually using it in a driver (and documenting 
it, and giving it a new sysfs string).

There is an alternative: that we don't pass MTD_MLCNANDFLASH through to 
user-space (we intercept it and change it to MTD_NANDFLASH in 
ioctl(MEMGETINFO)), and it just appears as "nand" in the sysfs 'type' 
entry. I'm not saying we should do that--I think it's useful to know SLC 
vs. MLC in user-space--but I am saying that we need a proper 
justification. So far I'm the only one who has explained why user-space 
needs this...

>> One argument in favor of your change: so a user can programmatically
>> determine whether to use JFFS2 or UBIFS on a particular NAND. (But
>> then again, SLC are getting more and more MLC-like properties that
>> make them unsuitable for JFFS2...)
>>
>> So, I would recommend a few rearrangements of part of this series:
>> (1) fixup the sysfs show() function and ABI documentation (in the same
>> patch) to cover "mlc-nand". *** must clearly explain the rationale for
>> userspace change (currently missing) ***
>> (2) do any changes to JFFS2 based on the MLC type
>> (3) allow nand_base.c to export MTD_MLCNANDFLASH
> I will arrange the next version in this order.

Brian
Huang Shijie - Aug. 28, 2013, 7:50 a.m.
于 2013年08月28日 15:28, Brian Norris 写道:
> All you've shown is the breakage, not the reason for exposing 
> MTD_MLCNANFLASH. There's a difference between MTD_MLCNANDFLASH being 
> in the mtd-abi.h header and actually using it in a driver (and 
> documenting it, and giving it a new sysfs string).
>


Just as you said belowing, the app may calls the MEMGETINFO get the nand 
type(SLC OR MLC).
So after this patch, the app will get the MTD_MLCNANDFLASH type for MLC 
which will is the breakage.

Are'nt the reason for exposing the MTD_MLCNANDFLASH? :(

> There is an alternative: that we don't pass MTD_MLCNANDFLASH through 
> to user-space (we intercept it and change it to MTD_NANDFLASH in 
> ioctl(MEMGETINFO)), and it just appears as "nand" in the sysfs 'type' 
> entry. I'm not saying we should do that--I think it's useful to know 
> SLC vs. MLC in user-space--but I am saying that we need a proper 
> justification. So far I'm the only one who has explained why 
> user-space needs this...
You concern is that exposing the MTD_MLCNANDFLASH may breaks mtd-utils 
or other app tools.

So you suggest to hack the IOCTL(MEMGETINFO) to avoid any changes of the 
apps?

yes, we can use this way to avoid changing the apps, but the side-effect 
is the mtd-utils can not get the
right SLC/MLC info.  If we does do the hack code to the ioctrl, I think 
some one will try to fix the hack code in the future.

thanks
Huang Shijie
Huang Shijie - Aug. 28, 2013, 9:26 a.m.
于 2013年08月28日 15:28, Brian Norris 写道:
> There is an alternative: that we don't pass MTD_MLCNANDFLASH through 
> to user-space (we intercept it and change it to MTD_NANDFLASH in 
> ioctl(MEMGETINFO)), and it just appears as "nand" in the sysfs 'type' 
> entry. I'm not saying we should do that--I think it's useful to know 
> SLC vs. MLC in user-space--but I am saying that we need a proper 
> justification. So far I'm the only one who has explained why 
> user-space needs this... 
There is another issue if we hack the ioctl(MEMGETINFO):
      The "flash_eraseall --jffs2" can writes the marker even it is a 
MLC nand, since the flash_eraseall
regards the MLC as the SLC.

thanks
Huang Shijie
Huang Shijie - Aug. 29, 2013, 1:24 a.m.
On Wed, Aug 28, 2013 at 12:28:51AM -0700, Brian Norris wrote:
> On 08/27/2013 11:59 PM, Huang Shijie wrote:
> 
> Yes, I saw that.
> 
> >That's why we should expose the MTD_MLCNANDFLASH to the user-space
> 
> All you've shown is the breakage, not the reason for exposing
> MTD_MLCNANFLASH. There's a difference between MTD_MLCNANDFLASH being
Do you mean this patch exposes the MTD_MLCNANDFLASH to user space?
or the patch 8 exposes the MTD_MLCNANDFLASH to user space?


thanks
Huang Shijie
Brian Norris - Aug. 29, 2013, 5:05 a.m.
On 08/28/2013 06:24 PM, Huang Shijie wrote:
> On Wed, Aug 28, 2013 at 12:28:51AM -0700, Brian Norris wrote:
>> On 08/27/2013 11:59 PM, Huang Shijie wrote:
>>
>> Yes, I saw that.
>>
>>> That's why we should expose the MTD_MLCNANDFLASH to the user-space
>>
>> All you've shown is the breakage, not the reason for exposing
>> MTD_MLCNANFLASH. There's a difference between MTD_MLCNANDFLASH being
> Do you mean this patch exposes the MTD_MLCNANDFLASH to user space?
> or the patch 8 exposes the MTD_MLCNANDFLASH to user space?

This patch (patch 6) changes the mtd->type for MLC NAND. And mtd->type 
is exposed to user-space via ioctl(MEMGETINFO), and (as of this patch), 
it is exposed as "unknown" in sysfs.

Patch 8 only changes the "unknown" to "mlc-nand"---an improvement, but 
patch 6 is still the first one that makes a visible change to the ABI.

Brian
Huang Shijie - Aug. 29, 2013, 5:34 a.m.
于 2013年08月29日 13:05, Brian Norris 写道:
> This patch (patch 6) changes the mtd->type for MLC NAND. And mtd->type 
> is exposed to user-space via ioctl(MEMGETINFO), and (as of this 
> patch), it is exposed as "unknown" in sysfs.
thanks.

this patch changes the mtd->type for the jffs2, and the jffs2 can uses 
the mtd->type to do the sanity check.

If without this patch, how the jffs2 can distinguish a SLC or a MLC?

Huang Shijie
Huang Shijie - Aug. 29, 2013, 5:50 a.m.
于 2013年08月29日 13:34, Huang Shijie 写道:
> 于 2013年08月29日 13:05, Brian Norris 写道:
>> This patch (patch 6) changes the mtd->type for MLC NAND. And 
>> mtd->type is exposed to user-space via ioctl(MEMGETINFO), and (as of 
>> this patch), it is exposed as "unknown" in sysfs.
> thanks.
>
> this patch changes the mtd->type for the jffs2, and the jffs2 can uses 
> the mtd->type to do the sanity check.
>
> If without this patch, how the jffs2 can distinguish a SLC or a MLC?
hi Brian:

I find a method to distinguish the SLC/MLC for jffs2, we can add the 
code for jffs2:
   -------------------------------------------------------
      if (mtd->flags == MTD_CAP_NANDFLASH) {

              struch nand_chip *chip = c->mtd->priv;
              if (!nand_is_slc)
                 return -EINVAL;
      }


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

If you think this code is okay, we can abandon serveral patches.

thanks
Huang Shijie
Brian Norris - Sept. 4, 2013, 6:51 p.m.
On Wed, Aug 28, 2013 at 10:50 PM, Huang Shijie <b32955@freescale.com> wrote:
> 于 2013年08月29日 13:34, Huang Shijie 写道:
>> 于 2013年08月29日 13:05, Brian Norris 写道:
>>>
>>> This patch (patch 6) changes the mtd->type for MLC NAND. And mtd->type is
>>> exposed to user-space via ioctl(MEMGETINFO), and (as of this patch), it is
>>> exposed as "unknown" in sysfs.
>>
>> thanks.
>>
>> this patch changes the mtd->type for the jffs2, and the jffs2 can uses the
>> mtd->type to do the sanity check.

But the key point is that not only does this change mtd->type for
JFFS2, but it also exports this new mtd->type to user-space.

>> If without this patch, how the jffs2 can distinguish a SLC or a MLC?

You could precede this patch with one that prevents exporting
MTD_MLCNANDFLASH in mtd_type_show() and in ioctl(MEMGETINFO). I'm not
saying you *must* do this, but that this side effect must be
acknowledged and explained better, or else it seems that you don't
understand the ramifications of your proposed changes.

> hi Brian:
>
> I find a method to distinguish the SLC/MLC for jffs2, we can add the code
> for jffs2:
>   -------------------------------------------------------
>      if (mtd->flags == MTD_CAP_NANDFLASH) {
>
>              struch nand_chip *chip = c->mtd->priv;
>              if (!nand_is_slc)
>                 return -EINVAL;
>      }
>
>
>   ------------------------------------------------------
>
> If you think this code is okay, we can abandon serveral patches.

No, we don't want to leak the mtd_info and nand_chip abstractions that
far. I think the MTD_MLCNANDFLASH type is the right way to do this for
the in-kernel MTD API, but it is a bigger step to automatically leak
that to user-space.

Brian
Brian Norris - Sept. 17, 2013, 12:13 a.m.
Hi Huang,

Sorry, I overlooked responding to this one.

On Thu, Sep 05, 2013 at 12:03:04PM +0800, Huang Shijie wrote:
> 于 2013年09月05日 02:51, Brian Norris 写道:
> 
>     You could precede this patch with one that prevents exporting
>     MTD_MLCNANDFLASH in mtd_type_show() and in ioctl(MEMGETINFO). I'm not
>     saying you *must* do this, but that this side effect must be
>     acknowledged and explained better, or else it seems that you don't
>     understand the ramifications of your proposed changes.
> 
> [1] If you think we should not _change_ any code of the application, such as
> the mtd-utils,

That's the true meaning of "we don't break ABI compatibility". mtd-utils
that work today on a given board with a given set of hardware (e.g., MLC
NAND flash) should not need modified to handle the same features on a
newer kernel.

However, I think it is safe to say that the MLC vs. SLC distinction
isn't made clear in mtd-abi.h (i.e., we already have MTD_MLCNANDFLASH),
and that this change is justified.

>     I can create a patch to prevents exporting the MTD_MLCNANDFLASH in
> mtd_type_show() and
>     in ioctrl(MEMGETINFO).

I don't think this is actually necessary. It is desirable that users can
see the difference between SLC and MLC, but this should be explained.

> [2] else, I prefer to explain better for this patch.

Yes, please explain it better (and reorder some patches, as I explained
previously) in order to

(1) show that you understand completely what your patches are doing and

(2) more clearly notify other developers of the modified ABI

Thanks,
Brian

Patch

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 8755a74..5957fe7 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3781,7 +3781,7 @@  int nand_scan_tail(struct mtd_info *mtd)
 		chip->options |= NAND_SUBPAGE_READ;
 
 	/* Fill in remaining MTD driver data */
-	mtd->type = MTD_NANDFLASH;
+	mtd->type = nand_is_slc(chip) ? MTD_NANDFLASH : MTD_MLCNANDFLASH;
 	mtd->flags = (chip->options & NAND_ROM) ? MTD_CAP_ROM :
 						MTD_CAP_NANDFLASH;
 	mtd->_erase = nand_erase;