[01/13] mtd: nand: fsmc: fix NAND width handling
diff mbox

Message ID 1490090645-8576-2-git-send-email-thomas.petazzoni@free-electrons.com
State Accepted
Commit ee56874f23e5c11576540bd695177a5ebc4f4352
Delegated to: Boris Brezillon
Headers show

Commit Message

Thomas Petazzoni March 21, 2017, 10:03 a.m. UTC
In commit eea628199d5b ("mtd: Add device-tree support to fsmc_nand"),
Device Tree support was added to the fmsc_nand driver. However, this
code has a bug in how it handles the bank-width DT property to set the
bus width.

Indeed, in the function fsmc_nand_probe_config_dt() that parses the
Device Tree, it sets pdata->width to either 8 or 16 depending on the
value of the bank-width DT property.

Then, the ->probe() function will test if pdata->width is equal to
FSMC_NAND_BW16 (which is 2) to set NAND_BUSWIDTH_16 in
nand->options. Therefore, with the DT probing, this condition will never
match.

This commit fixes that by removing the "width" field from
fsmc_nand_platform_data and instead have the fsmc_nand_probe_config_dt()
function directly set the appropriate nand->options value.

It is worth mentioning that if this commit gets backported to older
kernels, prior to the drop of non-DT probing, then non-DT probing will
be broken because nand->options will no longer be set to
NAND_BUSWIDTH_16.

Fixes: eea628199d5b ("mtd: Add device-tree support to fsmc_nand")
Cc: <stable@vger.kernel.org>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/mtd/nand/fsmc_nand.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

Comments

Boris Brezillon March 22, 2017, 9:42 p.m. UTC | #1
+Greg for the backport to stable question.

On Tue, 21 Mar 2017 11:03:53 +0100
Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

> In commit eea628199d5b ("mtd: Add device-tree support to fsmc_nand"),
> Device Tree support was added to the fmsc_nand driver. However, this
> code has a bug in how it handles the bank-width DT property to set the
> bus width.
> 
> Indeed, in the function fsmc_nand_probe_config_dt() that parses the
> Device Tree, it sets pdata->width to either 8 or 16 depending on the
> value of the bank-width DT property.
> 
> Then, the ->probe() function will test if pdata->width is equal to
> FSMC_NAND_BW16 (which is 2) to set NAND_BUSWIDTH_16 in
> nand->options. Therefore, with the DT probing, this condition will never
> match.
> 
> This commit fixes that by removing the "width" field from
> fsmc_nand_platform_data and instead have the fsmc_nand_probe_config_dt()
> function directly set the appropriate nand->options value.
> 
> It is worth mentioning that if this commit gets backported to older
> kernels, prior to the drop of non-DT probing, then non-DT probing will
> be broken because nand->options will no longer be set to
> NAND_BUSWIDTH_16.

Then maybe we should the drop the Cc-stable tag, or put # vX.Y+ to
prevent this patch from being applied to versions where it could break
things.

Note that no-one complained about this bug so far, so I guess no-one
cares about this fix (probably because 16-bits NANDs are not widely
used) ;-).

> 
> Fixes: eea628199d5b ("mtd: Add device-tree support to fsmc_nand")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/mtd/nand/fsmc_nand.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
> index bda1e46..66aece9 100644
> --- a/drivers/mtd/nand/fsmc_nand.c
> +++ b/drivers/mtd/nand/fsmc_nand.c
> @@ -150,7 +150,6 @@ struct fsmc_nand_platform_data {
>  	struct mtd_partition	*partitions;
>  	unsigned int		nr_partitions;
>  	unsigned int		options;
> -	unsigned int		width;
>  	unsigned int		bank;
>  
>  	enum access_mode	mode;
> @@ -844,18 +843,19 @@ static int fsmc_nand_probe_config_dt(struct platform_device *pdev,
>  	u32 val;
>  	int ret;
>  
> -	/* Set default NAND width to 8 bits */
> -	pdata->width = 8;
> +	pdata->options = 0;
> +
>  	if (!of_property_read_u32(np, "bank-width", &val)) {
>  		if (val == 2) {
> -			pdata->width = 16;
> +			pdata->options |= NAND_BUSWIDTH_16;
>  		} else if (val != 1) {
>  			dev_err(&pdev->dev, "invalid bank-width %u\n", val);
>  			return -EINVAL;
>  		}
>  	}
> +
>  	if (of_get_property(np, "nand-skip-bbtscan", NULL))
> -		pdata->options = NAND_SKIP_BBTSCAN;
> +		pdata->options |= NAND_SKIP_BBTSCAN;
>  
>  	pdata->nand_timings = devm_kzalloc(&pdev->dev,
>  				sizeof(*pdata->nand_timings), GFP_KERNEL);
> @@ -992,9 +992,6 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
>  	nand->badblockbits = 7;
>  	nand_set_flash_node(nand, np);
>  
> -	if (pdata->width == FSMC_NAND_BW16)
> -		nand->options |= NAND_BUSWIDTH_16;
> -
>  	switch (host->mode) {
>  	case USE_DMA_ACCESS:
>  		dma_cap_zero(mask);
Thomas Petazzoni March 22, 2017, 10:06 p.m. UTC | #2
Hello,

On Wed, 22 Mar 2017 22:42:37 +0100, Boris Brezillon wrote:

> > It is worth mentioning that if this commit gets backported to older
> > kernels, prior to the drop of non-DT probing, then non-DT probing will
> > be broken because nand->options will no longer be set to
> > NAND_BUSWIDTH_16.  
> 
> Then maybe we should the drop the Cc-stable tag, or put # vX.Y+ to
> prevent this patch from being applied to versions where it could break
> things.
> 
> Note that no-one complained about this bug so far, so I guess no-one
> cares about this fix (probably because 16-bits NANDs are not widely
> used) ;-).

It's really up to you. I didn't encounter the bug myself, I just found
out the mistake by reading the code and cooking this cleanup series.

Feel free to drop the Cc: to stable when applying, I don't particularly
care about this specific feature, and since no-one noticed so far, I
doubt anyone cares :)

Thanks,

Thomas
Linus Walleij March 23, 2017, 9:53 a.m. UTC | #3
On Tue, Mar 21, 2017 at 11:03 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:

> In commit eea628199d5b ("mtd: Add device-tree support to fsmc_nand"),
> Device Tree support was added to the fmsc_nand driver. However, this
> code has a bug in how it handles the bank-width DT property to set the
> bus width.
>
> Indeed, in the function fsmc_nand_probe_config_dt() that parses the
> Device Tree, it sets pdata->width to either 8 or 16 depending on the
> value of the bank-width DT property.
>
> Then, the ->probe() function will test if pdata->width is equal to
> FSMC_NAND_BW16 (which is 2) to set NAND_BUSWIDTH_16 in
> nand->options. Therefore, with the DT probing, this condition will never
> match.
>
> This commit fixes that by removing the "width" field from
> fsmc_nand_platform_data and instead have the fsmc_nand_probe_config_dt()
> function directly set the appropriate nand->options value.
>
> It is worth mentioning that if this commit gets backported to older
> kernels, prior to the drop of non-DT probing, then non-DT probing will
> be broken because nand->options will no longer be set to
> NAND_BUSWIDTH_16.
>
> Fixes: eea628199d5b ("mtd: Add device-tree support to fsmc_nand")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Good catch.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Patch
diff mbox

diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
index bda1e46..66aece9 100644
--- a/drivers/mtd/nand/fsmc_nand.c
+++ b/drivers/mtd/nand/fsmc_nand.c
@@ -150,7 +150,6 @@  struct fsmc_nand_platform_data {
 	struct mtd_partition	*partitions;
 	unsigned int		nr_partitions;
 	unsigned int		options;
-	unsigned int		width;
 	unsigned int		bank;
 
 	enum access_mode	mode;
@@ -844,18 +843,19 @@  static int fsmc_nand_probe_config_dt(struct platform_device *pdev,
 	u32 val;
 	int ret;
 
-	/* Set default NAND width to 8 bits */
-	pdata->width = 8;
+	pdata->options = 0;
+
 	if (!of_property_read_u32(np, "bank-width", &val)) {
 		if (val == 2) {
-			pdata->width = 16;
+			pdata->options |= NAND_BUSWIDTH_16;
 		} else if (val != 1) {
 			dev_err(&pdev->dev, "invalid bank-width %u\n", val);
 			return -EINVAL;
 		}
 	}
+
 	if (of_get_property(np, "nand-skip-bbtscan", NULL))
-		pdata->options = NAND_SKIP_BBTSCAN;
+		pdata->options |= NAND_SKIP_BBTSCAN;
 
 	pdata->nand_timings = devm_kzalloc(&pdev->dev,
 				sizeof(*pdata->nand_timings), GFP_KERNEL);
@@ -992,9 +992,6 @@  static int __init fsmc_nand_probe(struct platform_device *pdev)
 	nand->badblockbits = 7;
 	nand_set_flash_node(nand, np);
 
-	if (pdata->width == FSMC_NAND_BW16)
-		nand->options |= NAND_BUSWIDTH_16;
-
 	switch (host->mode) {
 	case USE_DMA_ACCESS:
 		dma_cap_zero(mask);