[PATCH] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver

[PATCH] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver

[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>
---
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(-)

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

Re: [PATCH] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver

[Marek Vasut]

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

Re: [PATCH] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver

[Scott Wood]

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

Re: [PATCH] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver

[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?

I thought so.

Brian

Re: [PATCH] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver

[Marek Vasut]

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

Re: [PATCH] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver

[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.

> And than even chip-specific state?

Not sure what you mean here.

-Scott

Re: [PATCH] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver

[Marek Vasut]

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

Re: [PATCH] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver (REPORT SPAM)

[William F.]

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/
>

Re: [PATCH] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver (REPORT SPAM)

[William F.]

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/
> .
>

Re: [PATCH] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver

[Huang Shijie]

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
>

Re: [PATCH] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver

[Brian Norris]

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

Re: [PATCH] mtd: nand: allow NAND_NO_SUBPAGE_WRITE to be set from driver

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 931 bytes --]

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.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]