| Message ID | 1382703654.8522.114.camel@shinybook.infradead.org |
|---|---|
| State | Accepted |
| Commit | 031e2777e03401d629e62602c8ce42b017732d4d |
| Headers | show |
On Friday 25 of October 2013 13:20:54 David Woodhouse wrote: > On Fri, 2013-10-25 at 13:03 +0100, David Woodhouse wrote: > > So... what if someone has already shipped the new chips that require > > stronger ECC, without realising that legacy_set_geometry() is > > insufficient? (And is legacy_set_geometry *actually* doing precisely the > > same as 3.10/3.11?) > > Answering my own question: If the required ECC strength is known and the > legacy ECC layout is insufficient, that's caused a failure since commit > 92d0e09abeebd ("mtd: gpmi: add sanity check for the ECC") in 3.9, so I'm > not worried about supporting that. > > And legacy_set_geometry() *is* doing what 3.11 did, verbatim. > > So the question is whether we want this "if legacy is sufficient then > use it else use the new method" that you offer in v2 of the patch, or if > a device-tree property is the better way to do it. > > I'm actually slightly in favour of the device-tree property. But since > 3.12 is imminent I think the *best* option is just to do this to > preserve the 3.11 behaviour, and worry about getting it right for 3.13: > > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c index 59ab069..a9830ff 100644 > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > @@ -349,7 +349,7 @@ static int legacy_set_geometry(struct gpmi_nand_data > *this) > > int common_nfc_set_geometry(struct gpmi_nand_data *this) > { > - return set_geometry_by_ecc_info(this) ? 0 : legacy_set_geometry(this); > + return legacy_set_geometry(this); > } > > struct dma_chan *get_dma_chan(struct gpmi_nand_data *this) I can confirm this patch fixes the regression, UBIFS can now again be mounted on 3.12rc6 . Thanks! Acked-by: Marek Vasut <marex@denx.de> On M28EVK with NAND device: NAND device: Manufacturer ID: 0x2c, Chip ID: 0xda (Micron MT29F2G08ABAEAWP) NAND device: 256MiB, SLC, page size: 2048, OOB size: 64 Tested-by: Marek Vasut <marex@denx.de> Cheers!
On Fri, 2013-10-25 at 21:33 -0400, Huang Shijie wrote: > > I am ok with your patch. but we will meet a compiler warning, since > the set_geometry_by_ecc_info() is not referenced. Yeah, I predicted that but at this stage with *hours* to catch Linux and get it into 3.12, I think that's the better option. I'll note it in the commit comment. Thanks for the prompt feedback.
On Friday 25 of October 2013 14:29:42 David Woodhouse wrote: > On Fri, 2013-10-25 at 21:33 -0400, Huang Shijie wrote: > > I am ok with your patch. but we will meet a compiler warning, since > > the set_geometry_by_ecc_info() is not referenced. > > Yeah, I predicted that but at this stage with *hours* to catch Linux and > get it into 3.12, I think that's the better option. I'll note it in the > commit comment. > > Thanks for the prompt feedback. Might __maybe_unused help here ?
On Fri, 2013-10-25 at 21:41 -0400, Huang Shijie wrote: > On Fri, Oct 25, 2013 at 02:29:42PM +0100, David Woodhouse wrote: > > On Fri, 2013-10-25 at 21:33 -0400, Huang Shijie wrote: > > > > > > I am ok with your patch. but we will meet a compiler warning, since > > > the set_geometry_by_ecc_info() is not referenced. > > > > Yeah, I predicted that but at this stage with *hours* to catch Linux and > > get it into 3.12, I think that's the better option. I'll note it in the > > commit comment. > > ok. Please add my ack for your formal patch: > Acked-by: Huang Shijie <shijie8@gmail.com> I've just pushed this to the tip of the tree, replacing the version that Brian had put there. Brian, are you still happy to send the pull request to Linus or do you want me to?
On Fri, Oct 25, 2013 at 7:08 AM, David Woodhouse <dwmw2@infradead.org> wrote: > On Fri, 2013-10-25 at 21:41 -0400, Huang Shijie wrote: >> On Fri, Oct 25, 2013 at 02:29:42PM +0100, David Woodhouse wrote: >> > On Fri, 2013-10-25 at 21:33 -0400, Huang Shijie wrote: >> > > >> > > I am ok with your patch. but we will meet a compiler warning, since >> > > the set_geometry_by_ecc_info() is not referenced. >> > >> > Yeah, I predicted that but at this stage with *hours* to catch Linux and >> > get it into 3.12, I think that's the better option. I'll note it in the >> > commit comment. >> >> ok. Please add my ack for your formal patch: >> Acked-by: Huang Shijie <shijie8@gmail.com> > > I've just pushed this to the tip of the tree, replacing the version that > Brian had put there. A tiny comment on the description. You say: "The "legacy" ECC layout used until 3.11 uses all the OOB area by computing the ECC strength and ECC step size ourselves." This phrase "until 3.11" sounds like the layout changed in 3.11 (which it did not). The rest of the commit message might clear that up, but still... > Brian, are you still happy to send the pull request to Linus or do you > want me to? I'm happy either way, as long as the fixes go in. I'll send it, since I still have the day ahead of me in my timezone. Thanks, Brian
On Fri, Oct 25, 2013 at 01:20:54PM +0100, David Woodhouse wrote: > On Fri, 2013-10-25 at 13:03 +0100, David Woodhouse wrote: > > > > So... what if someone has already shipped the new chips that require > > stronger ECC, without realising that legacy_set_geometry() is > > insufficient? (And is legacy_set_geometry *actually* doing precisely the > > same as 3.10/3.11?) > > Answering my own question: If the required ECC strength is known and the > legacy ECC layout is insufficient, that's caused a failure since commit > 92d0e09abeebd ("mtd: gpmi: add sanity check for the ECC") in 3.9, so I'm > not worried about supporting that. > > And legacy_set_geometry() *is* doing what 3.11 did, verbatim. > > So the question is whether we want this "if legacy is sufficient then > use it else use the new method" that you offer in v2 of the patch, or if > a device-tree property is the better way to do it. > > I'm actually slightly in favour of the device-tree property. But since > 3.12 is imminent I think the *best* option is just to do this to > preserve the 3.11 behaviour, and worry about getting it right for 3.13: Hi David: I am ok with your patch. but we will meet a compiler warning, since the set_geometry_by_ecc_info() is not referenced. thanks Huang Shijie > > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > index 59ab069..a9830ff 100644 > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > @@ -349,7 +349,7 @@ static int legacy_set_geometry(struct gpmi_nand_data *this) > > int common_nfc_set_geometry(struct gpmi_nand_data *this) > { > - return set_geometry_by_ecc_info(this) ? 0 : legacy_set_geometry(this); > + return legacy_set_geometry(this); > } > > struct dma_chan *get_dma_chan(struct gpmi_nand_data *this) > > > -- > dwmw2 > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/
On Fri, Oct 25, 2013 at 02:29:42PM +0100, David Woodhouse wrote: > On Fri, 2013-10-25 at 21:33 -0400, Huang Shijie wrote: > > > > I am ok with your patch. but we will meet a compiler warning, since > > the set_geometry_by_ecc_info() is not referenced. > > Yeah, I predicted that but at this stage with *hours* to catch Linux and > get it into 3.12, I think that's the better option. I'll note it in the > commit comment. ok. Please add my ack for your formal patch: Acked-by: Huang Shijie <shijie8@gmail.com>
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c index 59ab069..a9830ff 100644 --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c @@ -349,7 +349,7 @@ static int legacy_set_geometry(struct gpmi_nand_data *this) int common_nfc_set_geometry(struct gpmi_nand_data *this) { - return set_geometry_by_ecc_info(this) ? 0 : legacy_set_geometry(this); + return legacy_set_geometry(this); } struct dma_chan *get_dma_chan(struct gpmi_nand_data *this)