Re: mtd nand erase and bad block

[Angus CLARK <angus.clark@st.com>]

Hi Shmulik,

On 06/01/2012 03:54 PM, Shmulik Ladkani wrote:
> Hi Angus,
> 
> On Fri, 01 Jun 2012 15:03:19 +0100 Angus CLARK <angus.clark@st.com> wrote:
>> Hi Shmulik,
>>
>>>>>
>>>>> Typically, debugfs is only enabled in development environments, and even then it
>>>>> requires explicit user action, so this method of enabling erasing bad blocks is
>>>>> safe enough for our needs.
>>>>
>>>> Sounds ok to me, especially if you send the patch together with a piece
>>>> of doc for the mtd web-site. I just think it is important to document
>>>> this feature. Is this doable?
>>>
>>> I think we should prefer a local "allow erase bad blocks" policy than a
>>> global one.
>>>
>>> This is because when the global debugfs flag is on, *every* mtd erase
>>> operation might lead to erasure of bad blocks - not necessarily those
>>> triggered by the user which set the flag prior issuing his 'flash_erase'
>>> command.
>>>
>>> Meaning, other MTD users (ubi, various ffs) which currently work on
>>> other mtd partitions, are suddenly relaxed and allowed to erase bad
>>> blocks - which is probably not what user intented.
>>>
>>> I suggest to be more restrictive and have the "allow erase bad blocks"
>>> propery be local policy, that is - per an erase request.
>>>
>>> And since we'll probably need this thing only for userspace erase
>>> calls (e.g. flash_erase) - I suggest placing it into the MEMERASE ioctl.
>>>
>>> Comments?
>>>
>>
>> I would agree to some extent.  Enabling the "allow erase bad blocks" option per
>> erase request would certainly be a safer solution.  However, I suspect extending
>> the existing MEMERASE/MEMERASE64 IOCTLs is not really an option.  That leaves
>> inventing another IOCTL, or perhaps adding another file mode, which would
>> achieve per-file-descriptor scope, if not per-erase-request.
>>
>> My approach was largely motivated by the desire not to change the existing ABI,
>> and/or mtd-utils.  One could argue that such an option should only ever be
>> enabled by someone who knows what they are doing, and that might include things
>> like un-mounting any filesystems beforehand.
>>
>> To be honest, I am in two minds.  The solution I have at present is very simple
>> (or perhaps naive!), requires minimial changes to the kernel, no changes to
>> mtd-utils, and can be disabled completely by not including debugfs (which is
>> standard practice on production systems).  On the other hand, the ability to
>> enable per-erase-request is a safer and more elegant solution.  However, it
>> would require updates to mtd-utils, and agreement from the MTD community
>> regarding changes to the ABI...
>>
>> What do you think?
> 
> To be honest, I'm in two minds either ;)
> 
> I completely understand the reasons and motivation for a global
> debugfs option. And it seems as a reasonable compromise.
> 
> OTOH, adding a new ioctl makes sense as we're offering a functionality
> that didn't exist before.
> 
> Anyways, I guess its up to David or Artem.
> 
> My personal preference would be:
> 1. A new ioctl (MEMSCRUB?)
> 2. debugfs flag, PER MTD PART (slightly safer than your global flag)
> 3. global debugfs flag
> 
> BTW what do you think about option (2)? Would you consider it, or do you
> think it's an overdesign, if we already accept the debugfs way?
> 

Yes, option 2 could be a good compromise.  It would require a few extra hooks in
mtdpart, to dynamically create/remove debugfs entries when partitions are
added/deleted, and some updates to mtd_erase, to allow the flags to be passed to
nand_base:nand_erase_nand(), but it shouldn't be too bad.  Perhaps moving it to
sysfs might be cleaner?

In any case, I will wait for advice from David and Artem before commencing.  (I
am away next week, so it will have to wait until I get back anyway.)

Cheers,

Angus