From: Boris Brezillon <boris.brezillon@free-electrons.com>
To: Boris Brezillon <boris.brezillon@free-electrons.com>,
Richard Weinberger <richard@nod.at>,
linux-mtd@lists.infradead.org,
Chris Packham <Chris.Packham@alliedtelesis.co.nz>
Cc: David Woodhouse <dwmw2@infradead.org>,
Brian Norris <computersforpeace@gmail.com>,
Marek Vasut <marek.vasut@gmail.com>,
Cyrille Pitchen <cyrille.pitchen@atmel.com>,
Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Subject: Re: [PATCH] mtd: nand: Make sure drivers not supporting SET/GET_FEATURES return -ENOTSUPP
Date: Mon, 29 May 2017 11:23:55 +0200 [thread overview]
Message-ID: <20170529112355.73f55f89@bbrezillon> (raw)
In-Reply-To: <20170526151015.32673-1-boris.brezillon@free-electrons.com>
Hi Chris,
On Fri, 26 May 2017 17:10:15 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> A lot of drivers are providing their own ->cmdfunc(), and most of the
> time this implementation does not support all possible NAND operations.
> But since ->cmdfunc() cannot return an error code, the core has no way
> to know that the operation it requested is not supported.
>
> This is a problem we cannot address for all kind of operations with the
> current design, but we can prevent these silent failures for the
> GET/SET FEATURES operation by overloading the default
> ->onfi_{set,get}_features() methods with one returning -ENOTSUPP.
>
> Reported-by: Chris Packham <Chris.Packham@alliedtelesis.co.nz>
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Can you test this patch and add your Tested-by if it works?
Thanks,
Boris
> ---
>
> This bug has been discovered by Chris while testing linux-next on his
> marvell platform (which is using pxa3xx NAND controller driver).
> The bug was caused by commit b566d9c055de ("mtd: nand: add support for
> Micron on-die ECC") which is using ->set/get_features() to detect
> whether the NAND supports on-die ECC or not.
>
> This reveals how fragile the current ->cmdfunc() interface is. Not only
> ->cmdfunc() cannot return an error code, but even if it could, most of
> the time, learning that a specific operation is not supported when
> trying to execute it is already too late.
>
> Anyway, this is not something we can address immediately (I'm working
> on a new ->exec_op()/->supports_op() interface that will hopefully
> address the aforementioned problems), but I guess this fix can serve as
> a reference to show that overloading ->cmdfunc() is a really bad idea
> and that ->cmd_ctrl() should be implemented instead, unless it's proven
> to be impossible.
>
> Note that I didn't add a Fixes tag because I plan to rearrange my
> nand/next branch to put this commit before b566d9c055de to avoid
> breaking bisectability.
> ---
> drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c | 2 ++
> drivers/mtd/nand/cafe_nand.c | 2 ++
> drivers/mtd/nand/denali.c | 2 ++
> drivers/mtd/nand/docg4.c | 2 ++
> drivers/mtd/nand/fsl_elbc_nand.c | 2 ++
> drivers/mtd/nand/fsl_ifc_nand.c | 2 ++
> drivers/mtd/nand/hisi504_nand.c | 2 ++
> drivers/mtd/nand/mpc5121_nfc.c | 2 ++
> drivers/mtd/nand/nand_base.c | 19 +++++++++++++++++++
> drivers/mtd/nand/pxa3xx_nand.c | 2 ++
> drivers/mtd/nand/qcom_nandc.c | 2 ++
> drivers/mtd/nand/sh_flctl.c | 2 ++
> drivers/mtd/nand/vf610_nfc.c | 2 ++
> drivers/staging/mt29f_spinand/mt29f_spinand.c | 2 ++
> include/linux/mtd/nand.h | 5 +++++
> 15 files changed, 50 insertions(+)
>
> diff --git a/drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c b/drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c
> index f1da4ea88f2c..54bac5b73f0a 100644
> --- a/drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c
> +++ b/drivers/mtd/nand/bcm47xxnflash/ops_bcm4706.c
> @@ -392,6 +392,8 @@ int bcm47xxnflash_ops_bcm4706_init(struct bcm47xxnflash *b47n)
> b47n->nand_chip.read_byte = bcm47xxnflash_ops_bcm4706_read_byte;
> b47n->nand_chip.read_buf = bcm47xxnflash_ops_bcm4706_read_buf;
> b47n->nand_chip.write_buf = bcm47xxnflash_ops_bcm4706_write_buf;
> + b47n->nand_chip.onfi_set_features = nand_onfi_get_set_features_notsupp;
> + b47n->nand_chip.onfi_get_features = nand_onfi_get_set_features_notsupp;
>
> nand_chip->chip_delay = 50;
> b47n->nand_chip.bbt_options = NAND_BBT_USE_FLASH;
> diff --git a/drivers/mtd/nand/cafe_nand.c b/drivers/mtd/nand/cafe_nand.c
> index d40c32d311d8..2fd733eba0a3 100644
> --- a/drivers/mtd/nand/cafe_nand.c
> +++ b/drivers/mtd/nand/cafe_nand.c
> @@ -654,6 +654,8 @@ static int cafe_nand_probe(struct pci_dev *pdev,
> cafe->nand.read_buf = cafe_read_buf;
> cafe->nand.write_buf = cafe_write_buf;
> cafe->nand.select_chip = cafe_select_chip;
> + cafe->nand.onfi_set_features = nand_onfi_get_set_features_notsupp;
> + cafe->nand.onfi_get_features = nand_onfi_get_set_features_notsupp;
>
> cafe->nand.chip_delay = 0;
>
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index 16634df2e39a..b3c99d98fdee 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -1531,6 +1531,8 @@ int denali_init(struct denali_nand_info *denali)
> chip->cmdfunc = denali_cmdfunc;
> chip->read_byte = denali_read_byte;
> chip->waitfunc = denali_waitfunc;
> + chip->onfi_set_features = nand_onfi_get_set_features_notsupp;
> + chip->onfi_get_features = nand_onfi_get_set_features_notsupp;
>
> /*
> * scan for NAND devices attached to the controller
> diff --git a/drivers/mtd/nand/docg4.c b/drivers/mtd/nand/docg4.c
> index 7af2a3cd949e..a27a84fbfb84 100644
> --- a/drivers/mtd/nand/docg4.c
> +++ b/drivers/mtd/nand/docg4.c
> @@ -1260,6 +1260,8 @@ static void __init init_mtd_structs(struct mtd_info *mtd)
> nand->read_buf = docg4_read_buf;
> nand->write_buf = docg4_write_buf16;
> nand->erase = docg4_erase_block;
> + nand->onfi_set_features = nand_onfi_get_set_features_notsupp;
> + nand->onfi_get_features = nand_onfi_get_set_features_notsupp;
> nand->ecc.read_page = docg4_read_page;
> nand->ecc.write_page = docg4_write_page;
> nand->ecc.read_page_raw = docg4_read_page_raw;
> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c b/drivers/mtd/nand/fsl_elbc_nand.c
> index 113f76e59937..b9ac16f05057 100644
> --- a/drivers/mtd/nand/fsl_elbc_nand.c
> +++ b/drivers/mtd/nand/fsl_elbc_nand.c
> @@ -775,6 +775,8 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd *priv)
> chip->select_chip = fsl_elbc_select_chip;
> chip->cmdfunc = fsl_elbc_cmdfunc;
> chip->waitfunc = fsl_elbc_wait;
> + chip->onfi_set_features = nand_onfi_get_set_features_notsupp;
> + chip->onfi_get_features = nand_onfi_get_set_features_notsupp;
>
> chip->bbt_td = &bbt_main_descr;
> chip->bbt_md = &bbt_mirror_descr;
> diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
> index d1570f512f0b..89e14daeaba6 100644
> --- a/drivers/mtd/nand/fsl_ifc_nand.c
> +++ b/drivers/mtd/nand/fsl_ifc_nand.c
> @@ -831,6 +831,8 @@ static int fsl_ifc_chip_init(struct fsl_ifc_mtd *priv)
> chip->select_chip = fsl_ifc_select_chip;
> chip->cmdfunc = fsl_ifc_cmdfunc;
> chip->waitfunc = fsl_ifc_wait;
> + chip->onfi_set_features = nand_onfi_get_set_features_notsupp;
> + chip->onfi_get_features = nand_onfi_get_set_features_notsupp;
>
> chip->bbt_td = &bbt_main_descr;
> chip->bbt_md = &bbt_mirror_descr;
> diff --git a/drivers/mtd/nand/hisi504_nand.c b/drivers/mtd/nand/hisi504_nand.c
> index e40364eeb556..530caa80b1b6 100644
> --- a/drivers/mtd/nand/hisi504_nand.c
> +++ b/drivers/mtd/nand/hisi504_nand.c
> @@ -764,6 +764,8 @@ static int hisi_nfc_probe(struct platform_device *pdev)
> chip->write_buf = hisi_nfc_write_buf;
> chip->read_buf = hisi_nfc_read_buf;
> chip->chip_delay = HINFC504_CHIP_DELAY;
> + chip->onfi_set_features = nand_onfi_get_set_features_notsupp;
> + chip->onfi_get_features = nand_onfi_get_set_features_notsupp;
>
> hisi_nfc_host_init(host);
>
> diff --git a/drivers/mtd/nand/mpc5121_nfc.c b/drivers/mtd/nand/mpc5121_nfc.c
> index 6d6eaed2d20c..0e86fb6277c3 100644
> --- a/drivers/mtd/nand/mpc5121_nfc.c
> +++ b/drivers/mtd/nand/mpc5121_nfc.c
> @@ -708,6 +708,8 @@ static int mpc5121_nfc_probe(struct platform_device *op)
> chip->read_buf = mpc5121_nfc_read_buf;
> chip->write_buf = mpc5121_nfc_write_buf;
> chip->select_chip = mpc5121_nfc_select_chip;
> + chip->onfi_set_features = nand_onfi_get_set_features_notsupp;
> + chip->onfi_get_features = nand_onfi_get_set_features_notsupp;
> chip->bbt_options = NAND_BBT_USE_FLASH;
> chip->ecc.mode = NAND_ECC_SOFT;
> chip->ecc.algo = NAND_ECC_HAMMING;
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 54a1dfa327ff..d6fb4a139387 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3424,6 +3424,25 @@ static int nand_onfi_get_features(struct mtd_info *mtd, struct nand_chip *chip,
> }
>
> /**
> + * nand_onfi_get_set_features_notsupp - set/get features stub returning
> + * -ENOTSUPP
> + * @mtd: MTD device structure
> + * @chip: nand chip info structure
> + * @addr: feature address.
> + * @subfeature_param: the subfeature parameters, a four bytes array.
> + *
> + * Should be used by NAND controller drivers that do not support the SET/GET
> + * FEATURES operations.
> + */
> +int nand_onfi_get_set_features_notsupp(struct mtd_info *mtd,
> + struct nand_chip *chip,
> + int feature_addr, u8 *subfeature_para)
> +{
> + return -ENOTSUPP;
> +}
> +EXPORT_SYMBOL(nand_onfi_get_set_features_notsupp);
> +
> +/**
> * nand_suspend - [MTD Interface] Suspend the NAND flash
> * @mtd: MTD device structure
> */
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index 649ba8200832..74dae4bbdac8 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -1812,6 +1812,8 @@ static int alloc_nand_resource(struct platform_device *pdev)
> chip->write_buf = pxa3xx_nand_write_buf;
> chip->options |= NAND_NO_SUBPAGE_WRITE;
> chip->cmdfunc = nand_cmdfunc;
> + chip->onfi_set_features = nand_onfi_get_set_features_notsupp;
> + chip->onfi_get_features = nand_onfi_get_set_features_notsupp;
> }
>
> nand_hw_control_init(chip->controller);
> diff --git a/drivers/mtd/nand/qcom_nandc.c b/drivers/mtd/nand/qcom_nandc.c
> index 57d483ac5765..88af7145a51a 100644
> --- a/drivers/mtd/nand/qcom_nandc.c
> +++ b/drivers/mtd/nand/qcom_nandc.c
> @@ -2008,6 +2008,8 @@ static int qcom_nand_host_init(struct qcom_nand_controller *nandc,
> chip->read_byte = qcom_nandc_read_byte;
> chip->read_buf = qcom_nandc_read_buf;
> chip->write_buf = qcom_nandc_write_buf;
> + chip->onfi_set_features = nand_onfi_get_set_features_notsupp;
> + chip->onfi_get_features = nand_onfi_get_set_features_notsupp;
>
> /*
> * the bad block marker is readable only when we read the last codeword
> diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c
> index 442ce619b3b6..891ac7b99305 100644
> --- a/drivers/mtd/nand/sh_flctl.c
> +++ b/drivers/mtd/nand/sh_flctl.c
> @@ -1183,6 +1183,8 @@ static int flctl_probe(struct platform_device *pdev)
> nand->read_buf = flctl_read_buf;
> nand->select_chip = flctl_select_chip;
> nand->cmdfunc = flctl_cmdfunc;
> + nand->onfi_set_features = nand_onfi_get_set_features_notsupp;
> + nand->onfi_get_features = nand_onfi_get_set_features_notsupp;
>
> if (pdata->flcmncr_val & SEL_16BIT)
> nand->options |= NAND_BUSWIDTH_16;
> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
> index 3ea4bb19e12d..744ab10e8962 100644
> --- a/drivers/mtd/nand/vf610_nfc.c
> +++ b/drivers/mtd/nand/vf610_nfc.c
> @@ -703,6 +703,8 @@ static int vf610_nfc_probe(struct platform_device *pdev)
> chip->read_buf = vf610_nfc_read_buf;
> chip->write_buf = vf610_nfc_write_buf;
> chip->select_chip = vf610_nfc_select_chip;
> + chip->onfi_set_features = nand_onfi_get_set_features_notsupp;
> + chip->onfi_get_features = nand_onfi_get_set_features_notsupp;
>
> chip->options |= NAND_NO_SUBPAGE_WRITE;
>
> diff --git a/drivers/staging/mt29f_spinand/mt29f_spinand.c b/drivers/staging/mt29f_spinand/mt29f_spinand.c
> index e389009fca42..a4e3ae8f0c85 100644
> --- a/drivers/staging/mt29f_spinand/mt29f_spinand.c
> +++ b/drivers/staging/mt29f_spinand/mt29f_spinand.c
> @@ -915,6 +915,8 @@ static int spinand_probe(struct spi_device *spi_nand)
> chip->waitfunc = spinand_wait;
> chip->options |= NAND_CACHEPRG;
> chip->select_chip = spinand_select_chip;
> + chip->onfi_set_features = nand_onfi_get_set_features_notsupp;
> + chip->onfi_get_features = nand_onfi_get_set_features_notsupp;
>
> mtd = nand_to_mtd(chip);
>
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index f01991649118..5388c07836fd 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -1261,6 +1261,11 @@ int nand_read_oob_std(struct mtd_info *mtd, struct nand_chip *chip, int page);
> int nand_read_oob_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
> int page);
>
> +/* Stub used by drivers that do not support GET/SET FEATURES operations */
> +int nand_onfi_get_set_features_notsupp(struct mtd_info *mtd,
> + struct nand_chip *chip,
> + int feature_addr, u8 *subfeature_para);
> +
> /* Default read_page_raw implementation */
> int nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
> uint8_t *buf, int oob_required, int page);
next prev parent reply other threads:[~2017-05-29 9:24 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-26 15:10 [PATCH] mtd: nand: Make sure drivers not supporting SET/GET_FEATURES return -ENOTSUPP Boris Brezillon
2017-05-29 9:23 ` Boris Brezillon [this message]
2017-05-29 21:12 ` Chris Packham
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170529112355.73f55f89@bbrezillon \
--to=boris.brezillon@free-electrons.com \
--cc=Chris.Packham@alliedtelesis.co.nz \
--cc=computersforpeace@gmail.com \
--cc=cyrille.pitchen@atmel.com \
--cc=dwmw2@infradead.org \
--cc=linux-mtd@lists.infradead.org \
--cc=marek.vasut@gmail.com \
--cc=richard@nod.at \
--cc=thomas.petazzoni@free-electrons.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.