[EXT] Re: [RESEND PATCH V2 1/2] mtd: core: add erase preparation hook function pointer

[EXT] Re: [RESEND PATCH V2 1/2] mtd: core: add erase preparation hook function pointer

[Piotr Wojtaszczyk]

> On some legacy planar 2D Micron NAND devices when a
> block erase command is issued, occasionally even
> though a block erase operation successfully completes
> and returns a pass status, the flash block may not be
> completely erased. Subsequent operations to this block
> on very rare cases can result in subtle failures or
> corruption. These extremely rare cases should nevertheless
> be considered.
>
> These rare occurrences have been observed on partially
> written blocks. Partially written blocks are not uncommon
> with UBI/UBIFS.
>
> To avoid this rare occurrence, we make sure that at least
> 15 pages have been programmed to a block before it is erased.
> In case we find that less than 15 pages have been programmed,
> additional pages are programmed in the block. The observation
> is that additional pages rarely need to be written and most of
> the time UBI/UBIFS erases blocks that contain more programmed
> pages.
>
> Signed-off-by: Bean Huo <beanhuo at micron.com>
> Reviewed-by: ZOLTAN SZUBBOCSEV <zszubbocsev at micron.com>
> ---
>  drivers/mtd/nand/raw/nand_micron.c | 119 
> +++++++++++++++++++++++++++++++++++++
>  1 file changed, 119 insertions(+)
>
> diff --git a/drivers/mtd/nand/raw/nand_micron.c 
> b/drivers/mtd/nand/raw/nand_micron.c
> index b85e1c1..f52e072 100644
> --- a/drivers/mtd/nand/raw/nand_micron.c
> +++ b/drivers/mtd/nand/raw/nand_micron.c

Is there a plan to merge this patch soon?


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [EXT] Re: [RESEND PATCH V2 1/2] mtd: core: add erase preparation hook function pointer

[Bean Huo (beanhuo)]

Hi, Piotr

>> To avoid this rare occurrence, we make sure that at least
>> 15 pages have been programmed to a block before it is erased.
>> In case we find that less than 15 pages have been programmed,
>> additional pages are programmed in the block. The observation is that
>> additional pages rarely need to be written and most of the time
>> UBI/UBIFS erases blocks that contain more programmed pages.
>>
>> Signed-off-by: Bean Huo <beanhuo at micron.com>
>> Reviewed-by: ZOLTAN SZUBBOCSEV <zszubbocsev at micron.com>
>> ---
>>  drivers/mtd/nand/raw/nand_micron.c | 119
>> +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 119 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/raw/nand_micron.c
>> b/drivers/mtd/nand/raw/nand_micron.c
>> index b85e1c1..f52e072 100644
>> --- a/drivers/mtd/nand/raw/nand_micron.c
>> +++ b/drivers/mtd/nand/raw/nand_micron.c
>
>Is there a plan to merge this patch soon?
>
>
I think, it is quickly merged in.
I need to update a new version since there are some places need to change.

>______________________________________________________
>Linux MTD discussion mailing list
>http://lists.infradead.org/mailman/listinfo/linux-mtd/
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [EXT] Re: [RESEND PATCH V2 1/2] mtd: core: add erase preparation hook function pointer

[Boris Brezillon]

On Thu, 24 Jan 2019 15:52:03 +0000
"Bean Huo (beanhuo)" <beanhuo@micron.com> wrote:

> Hi, Boris
> 
> >> >>  struct nand_manufacturer_ops {
> >> >>  	void (*detect)(struct nand_chip *chip); @@ -49,6 +50,7 @@ struct
> >> >> nand_manufacturer_ops {
> >> >>  	void (*cleanup)(struct nand_chip *chip);
> >> >>  	void (*fixup_onfi_param_page)(struct nand_chip *chip,
> >> >>  				      struct nand_onfi_params *p);
> >> >> +	int (*erase_pre)(struct nand_chip *chip, int page);  
> >> >
> >> >Let's move this hook to nand_chip and name it ->pre_erase() or  
> >> >->erase_preparation().  
> >> >  
> >>
> >> Can you tell us more reasons why moves this hook to nand_chip?  
> >
> >Because it's supposed to be a per-chip thing. I mean, not all of your chips have
> >this bug (at least I hope), so we want to only have a  
> >->pre_erase() implementation when it's really needed. The manufacturer  
> >specific ->init() hook will decide when it's appropriate to populate this -  
> >>pre_erase pointer based on the NAND chip id (or the NAND chip model).  
> >  
> 
> >> In my opinion, it is better to add this hook in nand_manufacturer_ops,
> >> since nand_manufacturer_ops Already exists in nand_chip, also this function is  
> >related to specific NAND manufacturer.  
> I still think, keep this hook in nand_manufacturer_ops is better, otherwise, just add one
> Function hook pointer in nand_chip, it is very weird.
> 

And I tell you it's not. What's the point of adding a hook at the
nand_manufacturer_ops level (which is the same for all NAND chips
produced by a manufacturer) if you then have to check whether a
specific chip has anything to do inside the hook itself. When we added
nand_manufacturer and the associated ops it was created to address
chip detection and initialization, nothing more. Anything that is
needed at runtime and is manufacturer specific should have a hook/flag
in nand_chip.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [EXT] Re: [RESEND PATCH V2 1/2] mtd: core: add erase preparation hook function pointer

[Bean Huo (beanhuo)]

Hi, Boris

>> >>  struct nand_manufacturer_ops {
>> >>  	void (*detect)(struct nand_chip *chip); @@ -49,6 +50,7 @@ struct
>> >> nand_manufacturer_ops {
>> >>  	void (*cleanup)(struct nand_chip *chip);
>> >>  	void (*fixup_onfi_param_page)(struct nand_chip *chip,
>> >>  				      struct nand_onfi_params *p);
>> >> +	int (*erase_pre)(struct nand_chip *chip, int page);
>> >
>> >Let's move this hook to nand_chip and name it ->pre_erase() or
>> >->erase_preparation().
>> >
>>
>> Can you tell us more reasons why moves this hook to nand_chip?
>
>Because it's supposed to be a per-chip thing. I mean, not all of your chips have
>this bug (at least I hope), so we want to only have a
>->pre_erase() implementation when it's really needed. The manufacturer
>specific ->init() hook will decide when it's appropriate to populate this -
>>pre_erase pointer based on the NAND chip id (or the NAND chip model).
>

>> In my opinion, it is better to add this hook in nand_manufacturer_ops,
>> since nand_manufacturer_ops Already exists in nand_chip, also this function is
>related to specific NAND manufacturer.
I still think, keep this hook in nand_manufacturer_ops is better, otherwise, just add one
Function hook pointer in nand_chip, it is very weird.

//Bean

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [EXT] Re: [RESEND PATCH V2 1/2] mtd: core: add erase preparation hook function pointer

[Bean Huo (beanhuo)]

Thanks for reviewing, Miquel.

>> > diff --git a/drivers/mtd/nand/raw/internals.h
>> > b/drivers/mtd/nand/raw/internals.h
>> > index fbf6ca0..8382948 100644
>> > --- a/drivers/mtd/nand/raw/internals.h
>> > +++ b/drivers/mtd/nand/raw/internals.h
>> > @@ -42,6 +42,7 @@
>> >   *	     is here to let vendor specific code release those resources.
>> >   * @fixup_onfi_param_page: apply vendor specific fixups to the ONFI
>parameter
>> >   *			   page. This is called after the checksum is verified.
>> > + * @erase_pre: preparation before actually erase a physical block.
>
>What about "prepare a physical erase block before erasure" ?

I prefer your suggestion, apply it to next version patch.

//Bean
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [EXT] Re: [RESEND PATCH V2 1/2] mtd: core: add erase preparation hook function pointer

[Boris Brezillon]

On Mon, 21 Jan 2019 09:25:26 +0000
"Bean Huo (beanhuo)" <beanhuo@micron.com> wrote:

> Hi, Boris
> Thanks for reviewing this patch.
> 
> >>   *	     is here to let vendor specific code release those resources.
> >>   * @fixup_onfi_param_page: apply vendor specific fixups to the ONFI  
> >parameter  
> >>   *			   page. This is called after the checksum is verified.
> >> + * @erase_pre: preparation before actually erase a physical block.
> >>   */
> >>  struct nand_manufacturer_ops {
> >>  	void (*detect)(struct nand_chip *chip); @@ -49,6 +50,7 @@ struct
> >> nand_manufacturer_ops {
> >>  	void (*cleanup)(struct nand_chip *chip);
> >>  	void (*fixup_onfi_param_page)(struct nand_chip *chip,
> >>  				      struct nand_onfi_params *p);
> >> +	int (*erase_pre)(struct nand_chip *chip, int page);  
> >
> >Let's move this hook to nand_chip and name it ->pre_erase() or  
> >->erase_preparation().  
> >  
> 
> Can you tell us more reasons why moves this hook to nand_chip?

Because it's supposed to be a per-chip thing. I mean, not all of your
chips have this bug (at least I hope), so we want to only have a
->pre_erase() implementation when it's really needed. The manufacturer
specific ->init() hook will decide when it's appropriate to populate
this ->pre_erase pointer based on the NAND chip id (or the NAND chip
model).

> In my opinion, it is better to add this hook in nand_manufacturer_ops, since nand_manufacturer_ops
> Already exists in nand_chip, also this function is related to specific NAND manufacturer.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [EXT] Re: [RESEND PATCH V2 1/2] mtd: core: add erase preparation hook function pointer

[Bean Huo (beanhuo)]

Hi, Boris
Thanks for reviewing this patch.

>>   *	     is here to let vendor specific code release those resources.
>>   * @fixup_onfi_param_page: apply vendor specific fixups to the ONFI
>parameter
>>   *			   page. This is called after the checksum is verified.
>> + * @erase_pre: preparation before actually erase a physical block.
>>   */
>>  struct nand_manufacturer_ops {
>>  	void (*detect)(struct nand_chip *chip); @@ -49,6 +50,7 @@ struct
>> nand_manufacturer_ops {
>>  	void (*cleanup)(struct nand_chip *chip);
>>  	void (*fixup_onfi_param_page)(struct nand_chip *chip,
>>  				      struct nand_onfi_params *p);
>> +	int (*erase_pre)(struct nand_chip *chip, int page);
>
>Let's move this hook to nand_chip and name it ->pre_erase() or
>->erase_preparation().
>

Can you tell us more reasons why moves this hook to nand_chip?
In my opinion, it is better to add this hook in nand_manufacturer_ops, since nand_manufacturer_ops
Already exists in nand_chip, also this function is related to specific NAND manufacturer.

//Bean Huo

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/