| Message ID | 1377509808-29363-7-git-send-email-b32955@freescale.com |
|---|---|
| State | New, archived |
| Headers | show |
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
于 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
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
于 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
于 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
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
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
于 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
于 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
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
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
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;
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(-)