Patchwork mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver

login
register
mail settings
Submitter Brian Norris
Date July 13, 2012, 4:28 p.m.
Message ID <1342196904-13850-1-git-send-email-computersforpeace@gmail.com>
Download mbox | patch
Permalink /patch/170929/
State Accepted
Commit bf7a01bf7987b63b121d572b240c132ec44129c4
Headers show

Comments

Brian Norris - July 13, 2012, 4:28 p.m.
The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It
silently masks off at least one flag that might be set by the driver
(NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly
others.

Really, as long as driver writers exercise a small amount of care with
NAND_* options, this mask is not necessary at all; it was only here to
prevent certain options from accidentally being set by the driver. But the
original thought turns out to be a bad idea occasionally. Thus, kill it.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Cc: Huang Shijie <shijie8@gmail.com>
Cc: Marek Vasut <marex@denx.de>
---
Huang, please test this with your driver.

Artem,

If this is acceptable, should it go to -stable, since it fixes some major
gpmi-nand breakage? It probably won't apply directly, since I've touched some
code around it recently.

 drivers/mtd/nand/nand_base.c | 7 ++-----
 include/linux/mtd/nand.h     | 3 ---
 2 files changed, 2 insertions(+), 8 deletions(-)
Marek Vasut - July 13, 2012, 5:12 p.m.
Dear Brian Norris,

> The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It
> silently masks off at least one flag that might be set by the driver
> (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly
> others.
> 
> Really, as long as driver writers exercise a small amount of care with
> NAND_* options, this mask is not necessary at all; it was only here to
> prevent certain options from accidentally being set by the driver. But the
> original thought turns out to be a bad idea occasionally. Thus, kill it.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Cc: Huang Shijie <shijie8@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
[...]

Maybe we should simply split it into two sets of flags to make it clear -- chip 
ones and controller ones ?

Best regards,
Marek Vasut
Scott Wood - July 13, 2012, 5:19 p.m.
On 07/13/2012 12:12 PM, Marek Vasut wrote:
> Dear Brian Norris,
> 
>> The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It
>> silently masks off at least one flag that might be set by the driver
>> (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly
>> others.
>>
>> Really, as long as driver writers exercise a small amount of care with
>> NAND_* options, this mask is not necessary at all; it was only here to
>> prevent certain options from accidentally being set by the driver. But the
>> original thought turns out to be a bad idea occasionally. Thus, kill it.
>>
>> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
>> Cc: Huang Shijie <shijie8@gmail.com>
>> Cc: Marek Vasut <marex@denx.de>
> [...]
> 
> Maybe we should simply split it into two sets of flags to make it clear -- chip 
> ones and controller ones ?

Isn't that what we're trying to get rid of?

-Scott
Brian Norris - July 13, 2012, 5:21 p.m.
On Fri, Jul 13, 2012 at 10:19 AM, Scott Wood <scottwood@freescale.com> wrote:
> On 07/13/2012 12:12 PM, Marek Vasut wrote:
>>> The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It
>>> silently masks off at least one flag that might be set by the driver
>>> (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly
>>> others.
>>>
>>> Really, as long as driver writers exercise a small amount of care with
>>> NAND_* options, this mask is not necessary at all; it was only here to
>>> prevent certain options from accidentally being set by the driver. But the
>>> original thought turns out to be a bad idea occasionally. Thus, kill it.
>>>
>>> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
>>> Cc: Huang Shijie <shijie8@gmail.com>
>>> Cc: Marek Vasut <marex@denx.de>
>> [...]
>>
>> Maybe we should simply split it into two sets of flags to make it clear -- chip
>> ones and controller ones ?
>
> Isn't that what we're trying to get rid of?

I thought so.

Brian
William F. - July 13, 2012, 5:34 p.m.
Em 13-07-2012 14:19, Scott Wood escreveu:
> On 07/13/2012 12:12 PM, Marek Vasut wrote:
>> Dear Brian Norris,
>>
>>> The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It
>>> silently masks off at least one flag that might be set by the driver
>>> (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly
>>> others.
>>>
>>> Really, as long as driver writers exercise a small amount of care with
>>> NAND_* options, this mask is not necessary at all; it was only here to
>>> prevent certain options from accidentally being set by the driver. But the
>>> original thought turns out to be a bad idea occasionally. Thus, kill it.
>>>
>>> Signed-off-by: Brian Norris<computersforpeace@gmail.com>
>>> Cc: Huang Shijie<shijie8@gmail.com>
>>> Cc: Marek Vasut<marex@denx.de>
>> [...]
>>
>> Maybe we should simply split it into two sets of flags to make it clear -- chip
>> ones and controller ones ?
> Isn't that what we're trying to get rid of?
>
> -Scott
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
William F. - July 13, 2012, 5:35 p.m.
Em 13-07-2012 14:12, Marek Vasut escreveu:
> Dear Brian Norris,
>
>> The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It
>> silently masks off at least one flag that might be set by the driver
>> (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly
>> others.
>>
>> Really, as long as driver writers exercise a small amount of care with
>> NAND_* options, this mask is not necessary at all; it was only here to
>> prevent certain options from accidentally being set by the driver. But the
>> original thought turns out to be a bad idea occasionally. Thus, kill it.
>>
>> Signed-off-by: Brian Norris<computersforpeace@gmail.com>
>> Cc: Huang Shijie<shijie8@gmail.com>
>> Cc: Marek Vasut<marex@denx.de>
> [...]
>
> Maybe we should simply split it into two sets of flags to make it clear -- chip
> ones and controller ones ?
>
> Best regards,
> Marek Vasut
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> .
>
Marek Vasut - July 13, 2012, 5:35 p.m.
Dear Brian Norris,

> On Fri, Jul 13, 2012 at 10:19 AM, Scott Wood <scottwood@freescale.com> wrote:
> > On 07/13/2012 12:12 PM, Marek Vasut wrote:
> >>> The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs.
> >>> It silently masks off at least one flag that might be set by the
> >>> driver (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and
> >>> possibly others.
> >>> 
> >>> Really, as long as driver writers exercise a small amount of care with
> >>> NAND_* options, this mask is not necessary at all; it was only here to
> >>> prevent certain options from accidentally being set by the driver. But
> >>> the original thought turns out to be a bad idea occasionally. Thus,
> >>> kill it.
> >>> 
> >>> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> >>> Cc: Huang Shijie <shijie8@gmail.com>
> >>> Cc: Marek Vasut <marex@denx.de>
> >> 
> >> [...]
> >> 
> >> Maybe we should simply split it into two sets of flags to make it clear
> >> -- chip ones and controller ones ?
> > 
> > Isn't that what we're trying to get rid of?

Sure, but aren't there controller specific properties and chip-specific 
properties? And than even chip-specific state?

> I thought so.
> 
> Brian

Best regards,
Marek Vasut
Scott Wood - July 13, 2012, 5:42 p.m.
On 07/13/2012 12:35 PM, Marek Vasut wrote:
> Dear Brian Norris,
> 
>> On Fri, Jul 13, 2012 at 10:19 AM, Scott Wood <scottwood@freescale.com> wrote:
>>> On 07/13/2012 12:12 PM, Marek Vasut wrote:
>>>>> The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs.
>>>>> It silently masks off at least one flag that might be set by the
>>>>> driver (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and
>>>>> possibly others.
>>>>>
>>>>> Really, as long as driver writers exercise a small amount of care with
>>>>> NAND_* options, this mask is not necessary at all; it was only here to
>>>>> prevent certain options from accidentally being set by the driver. But
>>>>> the original thought turns out to be a bad idea occasionally. Thus,
>>>>> kill it.
>>>>>
>>>>> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
>>>>> Cc: Huang Shijie <shijie8@gmail.com>
>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>
>>>> [...]
>>>>
>>>> Maybe we should simply split it into two sets of flags to make it clear
>>>> -- chip ones and controller ones ?
>>>
>>> Isn't that what we're trying to get rid of?
> 
> Sure, but aren't there controller specific properties and chip-specific 
> properties?

Yes, but there are some properties than can be specified by either one.
 If you're proposing three sets of options (chip, controller, or both),
or duplicating certain options between the sets and then checking both
everywhere, I don't see how that's an improvement.

> And than even chip-specific state?

Not sure what you mean here.

-Scott
Marek Vasut - July 13, 2012, 5:56 p.m.
Dear Scott Wood,

> On 07/13/2012 12:35 PM, Marek Vasut wrote:
> > Dear Brian Norris,
> > 
> >> On Fri, Jul 13, 2012 at 10:19 AM, Scott Wood <scottwood@freescale.com> 
wrote:
> >>> On 07/13/2012 12:12 PM, Marek Vasut wrote:
> >>>>> The NAND_CHIPOPTIONS_MSK has limited utility and is causing real
> >>>>> bugs. It silently masks off at least one flag that might be set by
> >>>>> the driver (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver
> >>>>> and possibly others.
> >>>>> 
> >>>>> Really, as long as driver writers exercise a small amount of care
> >>>>> with NAND_* options, this mask is not necessary at all; it was only
> >>>>> here to prevent certain options from accidentally being set by the
> >>>>> driver. But the original thought turns out to be a bad idea
> >>>>> occasionally. Thus, kill it.
> >>>>> 
> >>>>> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> >>>>> Cc: Huang Shijie <shijie8@gmail.com>
> >>>>> Cc: Marek Vasut <marex@denx.de>
> >>>> 
> >>>> [...]
> >>>> 
> >>>> Maybe we should simply split it into two sets of flags to make it
> >>>> clear -- chip ones and controller ones ?
> >>> 
> >>> Isn't that what we're trying to get rid of?
> > 
> > Sure, but aren't there controller specific properties and chip-specific
> > properties?
> 
> Yes, but there are some properties than can be specified by either one.
>  If you're proposing three sets of options (chip, controller, or both),
> or duplicating certain options between the sets and then checking both
> everywhere, I don't see how that's an improvement.

Ok, you're right here, screw that and let's do a single set ;-)

> > And than even chip-specific state?
> 
> Not sure what you mean here.
> 
> -Scott

Best regards,
Marek Vasut
Huang Shijie - July 14, 2012, 2:02 a.m.
On Fri, Jul 13, 2012 at 12:28 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It
> silently masks off at least one flag that might be set by the driver
> (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly
> others.
>
> Really, as long as driver writers exercise a small amount of care with
> NAND_* options, this mask is not necessary at all; it was only here to
> prevent certain options from accidentally being set by the driver. But the
> original thought turns out to be a bad idea occasionally. Thus, kill it.
>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Cc: Huang Shijie <shijie8@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> ---
> Huang, please test this with your driver.
>
I tested this patch on imx6q-arm2 board with Micron MT29F32G08QAA.
it works fine, thanks.

Tested-by: Huang Shijie <shijie8@gmail.com>


> Artem,
>
> If this is acceptable, should it go to -stable, since it fixes some major
> gpmi-nand breakage? It probably won't apply directly, since I've touched some
> code around it recently.
>
>  drivers/mtd/nand/nand_base.c | 7 ++-----
>  include/linux/mtd/nand.h     | 3 ---
>  2 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 98ba46e..6652d1f 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2909,8 +2909,6 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
>         if (le16_to_cpu(p->features) & 1)
>                 *busw = NAND_BUSWIDTH_16;
>
> -       chip->options &= ~NAND_CHIPOPTIONS_MSK;
> -
>         pr_info("ONFI flash detected\n");
>         return 1;
>  }
> @@ -3074,9 +3072,8 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
>                         mtd->erasesize <<= ((id_data[3] & 0x03) << 1);
>                 }
>         }
> -       /* Get chip options, preserve non chip based options */
> -       chip->options &= ~NAND_CHIPOPTIONS_MSK;
> -       chip->options |= type->options & NAND_CHIPOPTIONS_MSK;
> +       /* Get chip options */
> +       chip->options |= type->options;
>
>         /*
>          * Check if chip is not a Samsung device. Do not clear the
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 6dce5a7..eeb7015 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -206,9 +206,6 @@ typedef enum {
>  #define NAND_SUBPAGE_READ(chip) ((chip->ecc.mode == NAND_ECC_SOFT) \
>                                         && (chip->page_shift > 9))
>
> -/* Mask to zero out the chip options, which come from the id table */
> -#define NAND_CHIPOPTIONS_MSK   0x0000ffff
> -
>  /* Non chip related options */
>  /* This option skips the bbt scan during initialization. */
>  #define NAND_SKIP_BBTSCAN      0x00010000
> --
> 1.7.11.1
>
Brian Norris - July 17, 2012, 6:20 a.m.
Bump.

On Fri, Jul 13, 2012 at 7:02 PM, Huang Shijie <shijie8@gmail.com> wrote:
> On Fri, Jul 13, 2012 at 12:28 PM, Brian Norris
> <computersforpeace@gmail.com> wrote:
>> The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It
>> silently masks off at least one flag that might be set by the driver
>> (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly
>> others.
>>
>> Really, as long as driver writers exercise a small amount of care with
>> NAND_* options, this mask is not necessary at all; it was only here to
>> prevent certain options from accidentally being set by the driver. But the
>> original thought turns out to be a bad idea occasionally. Thus, kill it.
>>
>> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
>> Cc: Huang Shijie <shijie8@gmail.com>
>> Cc: Marek Vasut <marex@denx.de>
>> ---
>> Huang, please test this with your driver.
>>
> I tested this patch on imx6q-arm2 board with Micron MT29F32G08QAA.
> it works fine, thanks.
>
> Tested-by: Huang Shijie <shijie8@gmail.com>

Any other Acks? Or acceptance from Artem/David?

Brian
Artem Bityutskiy - Aug. 17, 2012, 12:32 p.m.
On Fri, 2012-07-13 at 09:28 -0700, Brian Norris wrote:
> The NAND_CHIPOPTIONS_MSK has limited utility and is causing real bugs. It
> silently masks off at least one flag that might be set by the driver
> (NAND_NO_SUBPAGE_WRITE). This breaks the GPMI NAND driver and possibly
> others.
> 
> Really, as long as driver writers exercise a small amount of care with
> NAND_* options, this mask is not necessary at all; it was only here to
> prevent certain options from accidentally being set by the driver. But the
> original thought turns out to be a bad idea occasionally. Thus, kill it.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Pushed to l2-mtd.git, thanks.

> If this is acceptable, should it go to -stable, since it fixes some major
> gpmi-nand breakage? It probably won't apply directly, since I've touched some
> code around it recently.

Done.

Patch

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 98ba46e..6652d1f 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2909,8 +2909,6 @@  static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
 	if (le16_to_cpu(p->features) & 1)
 		*busw = NAND_BUSWIDTH_16;
 
-	chip->options &= ~NAND_CHIPOPTIONS_MSK;
-
 	pr_info("ONFI flash detected\n");
 	return 1;
 }
@@ -3074,9 +3072,8 @@  static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 			mtd->erasesize <<= ((id_data[3] & 0x03) << 1);
 		}
 	}
-	/* Get chip options, preserve non chip based options */
-	chip->options &= ~NAND_CHIPOPTIONS_MSK;
-	chip->options |= type->options & NAND_CHIPOPTIONS_MSK;
+	/* Get chip options */
+	chip->options |= type->options;
 
 	/*
 	 * Check if chip is not a Samsung device. Do not clear the
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 6dce5a7..eeb7015 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -206,9 +206,6 @@  typedef enum {
 #define NAND_SUBPAGE_READ(chip) ((chip->ecc.mode == NAND_ECC_SOFT) \
 					&& (chip->page_shift > 9))
 
-/* Mask to zero out the chip options, which come from the id table */
-#define NAND_CHIPOPTIONS_MSK	0x0000ffff
-
 /* Non chip related options */
 /* This option skips the bbt scan during initialization. */
 #define NAND_SKIP_BBTSCAN	0x00010000