Re: [PATCH v2 2/3] mtd: rawnand: Add the nand_chip->write_oob hook

[Boris Brezillon <boris.brezillon@collabora.com>]

On Sun,  3 May 2020 13:40:28 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> With the same approach as for the ->erase hook, in order to solve an
> issue with Micron NANDs, we must be able to overload the write
> operation. With this in mind, we create a ->write_oob hook in the
> nand_chip structure which points by default to the
> currently in use nand_write_oob() helper, renamed
> nand_write_oob_nand() for the parallel with the nand_erase_nand()
> one.

First of all, I must say I hate the hook name. Having mtd->_write_oob()
that writes both OOB and data is confusing, and you're pulling this
confusing name to the raw NAND layer.

> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/nand/raw/internals.h | 2 ++
>  drivers/mtd/nand/raw/nand_base.c | 8 ++++++++
>  include/linux/mtd/rawnand.h      | 3 +++
>  3 files changed, 13 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
> index 9d0caadf940e..caf534a6586a 100644
> --- a/drivers/mtd/nand/raw/internals.h
> +++ b/drivers/mtd/nand/raw/internals.h
> @@ -81,6 +81,8 @@ int nand_bbm_get_next_page(struct nand_chip *chip, int page);
>  int nand_markbad_bbm(struct nand_chip *chip, loff_t ofs);
>  int nand_erase_nand(struct nand_chip *chip, struct erase_info *instr,
>  		    int allowbbt);
> +int nand_write_oob_nand(struct nand_chip *chip, loff_t to,
> +			struct mtd_oob_ops *ops);
>  int onfi_fill_data_interface(struct nand_chip *chip,
>  			     enum nand_data_interface_type type,
>  			     int timing_mode);
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 7c7ac722d88b..f9cf30949f49 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -4121,6 +4121,13 @@ static int nand_write_oob(struct mtd_info *mtd, loff_t to,
>  			  struct mtd_oob_ops *ops)
>  {
>  	struct nand_chip *chip = mtd_to_nand(mtd);
> +
> +	return chip->write_oob(chip, to, ops);
> +}
> +
> +int nand_write_oob_nand(struct nand_chip *chip, loff_t to,
> +			struct mtd_oob_ops *ops)

Hm, what happens next time we have a similar name, do we suffix it with
_nand_nand? :P

> +{
>  	int ret;
>  
>  	ops->retlen = 0;
> @@ -4423,6 +4430,7 @@ static void nand_set_defaults(struct nand_chip *chip)
>  		chip->buf_align = 1;
>  
>  	chip->erase = nand_erase_nand;
> +	chip->write_oob = nand_write_oob_nand;
>  }
>  
>  /* Sanitize ONFI strings so we can safely print them */
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index 505c13f7a2ba..7fbbd5d7088f 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -1021,6 +1021,7 @@ struct nand_legacy {
>   * @setup_read_retry:	[FLASHSPECIFIC] flash (vendor) specific function for
>   *			setting the read-retry mode. Mostly needed for MLC NAND.
>   * @erase:		Raw NAND erase operation.
> + * @write_oob:		Raw NAND write operation.
>   * @ecc:		[BOARDSPECIFIC] ECC control structure
>   * @buf_align:		minimum buffer alignment required by a platform
>   * @oob_poi:		"poison value buffer," used for laying out OOB data
> @@ -1092,6 +1093,8 @@ struct nand_chip {
>  	int (*setup_read_retry)(struct nand_chip *chip, int retry_mode);
>  	int (*erase)(struct nand_chip *chip, struct erase_info *instr,
>  		     int allowbbt);
> +	int (*write_oob)(struct nand_chip *chip, loff_t to,
> +			 struct mtd_oob_ops *ops);
>  

Okay, so I'm not sure duplicating the nand_write_oob() logic is the
best option here. I'd rather go for a post write_page() hook.

Note that we probably want a post read_page() hook so we can flag
pages as written by analyzing what's returned to the caller. That would
saves us unneeded writes when the page has been read.

>  	unsigned int options;
>  	unsigned int bbt_options;


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