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

[Brian Norris]

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