| Message ID | 20220927024728.28447-1-chris.packham@alliedtelesis.co.nz |
|---|---|
| State | Accepted |
| Headers | show |
| Series | mtd: rawnand: marvell: Use correct logic for nand-keep-config | expand |
On 27/09/22 15:47, Chris Packham wrote: > From: Tony O'Brien <tony.obrien@alliedtelesis.co.nz> > > Originally the absence of the marvell,nand-keep-config property caused > the setup_data_interface function to be provided. However when > setup_data_interface was moved into nand_controller_ops the logic was > unintentionally inverted. Update the logic so that only if the > marvell,nand-keep-config property is present the bootloader NAND config > kept. > > Fixes: 7a08dbaedd36 ("mtd: rawnand: Move ->setup_data_interface() to nand_controller_ops") > Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz> > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > --- > > Notes: > I think this is a bug that's been lurking for 4 years or so. I'm not > sure that's particularly long in the life of an embedded device but it > does make me wonder if there have been other bug reports about it. > > We noticed this because we had a bootloader that used maxed out NAND > timings which made the time it took the kernel to do anything on the > file system longer than we expected. I think there might be a similar logic inversion bug in drivers/mtd/nand/raw/denali.c but I lack the ability to test for that platform. > drivers/mtd/nand/raw/marvell_nand.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c > index 2455a581fd70..b248c5f657d5 100644 > --- a/drivers/mtd/nand/raw/marvell_nand.c > +++ b/drivers/mtd/nand/raw/marvell_nand.c > @@ -2672,7 +2672,7 @@ static int marvell_nand_chip_init(struct device *dev, struct marvell_nfc *nfc, > chip->controller = &nfc->controller; > nand_set_flash_node(chip, np); > > - if (!of_property_read_bool(np, "marvell,nand-keep-config")) > + if (of_property_read_bool(np, "marvell,nand-keep-config")) > chip->options |= NAND_KEEP_TIMINGS; > > mtd = nand_to_mtd(chip);
Hi All, On 27/09/22 15:47, Chris Packham wrote: > From: Tony O'Brien <tony.obrien@alliedtelesis.co.nz> > > Originally the absence of the marvell,nand-keep-config property caused > the setup_data_interface function to be provided. However when > setup_data_interface was moved into nand_controller_ops the logic was > unintentionally inverted. Update the logic so that only if the > marvell,nand-keep-config property is present the bootloader NAND config > kept. > > Fixes: 7a08dbaedd36 ("mtd: rawnand: Move ->setup_data_interface() to nand_controller_ops") > Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz> > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> Just following up on this. I know things have probably been busy with the 6.0 release but it's been a week so I figured I'd give this a bump. > --- > > Notes: > I think this is a bug that's been lurking for 4 years or so. I'm not > sure that's particularly long in the life of an embedded device but it > does make me wonder if there have been other bug reports about it. > > We noticed this because we had a bootloader that used maxed out NAND > timings which made the time it took the kernel to do anything on the > file system longer than we expected. > > drivers/mtd/nand/raw/marvell_nand.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c > index 2455a581fd70..b248c5f657d5 100644 > --- a/drivers/mtd/nand/raw/marvell_nand.c > +++ b/drivers/mtd/nand/raw/marvell_nand.c > @@ -2672,7 +2672,7 @@ static int marvell_nand_chip_init(struct device *dev, struct marvell_nfc *nfc, > chip->controller = &nfc->controller; > nand_set_flash_node(chip, np); > > - if (!of_property_read_bool(np, "marvell,nand-keep-config")) > + if (of_property_read_bool(np, "marvell,nand-keep-config")) > chip->options |= NAND_KEEP_TIMINGS; > > mtd = nand_to_mtd(chip);
On Mon, 3 Oct 2022 21:46:31 +0000 Chris Packham <Chris.Packham@alliedtelesis.co.nz> wrote: > Hi All, > > On 27/09/22 15:47, Chris Packham wrote: > > From: Tony O'Brien <tony.obrien@alliedtelesis.co.nz> > > > > Originally the absence of the marvell,nand-keep-config property caused > > the setup_data_interface function to be provided. However when > > setup_data_interface was moved into nand_controller_ops the logic was > > unintentionally inverted. Update the logic so that only if the > > marvell,nand-keep-config property is present the bootloader NAND config > > kept. > > > > Fixes: 7a08dbaedd36 ("mtd: rawnand: Move ->setup_data_interface() to nand_controller_ops") > > Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz> > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > > Just following up on this. I know things have probably been busy with > the 6.0 release but it's been a week so I figured I'd give this a bump. Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > > > --- > > > > Notes: > > I think this is a bug that's been lurking for 4 years or so. I'm not > > sure that's particularly long in the life of an embedded device but it > > does make me wonder if there have been other bug reports about it. > > > > We noticed this because we had a bootloader that used maxed out NAND > > timings which made the time it took the kernel to do anything on the > > file system longer than we expected. > > > > drivers/mtd/nand/raw/marvell_nand.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c > > index 2455a581fd70..b248c5f657d5 100644 > > --- a/drivers/mtd/nand/raw/marvell_nand.c > > +++ b/drivers/mtd/nand/raw/marvell_nand.c > > @@ -2672,7 +2672,7 @@ static int marvell_nand_chip_init(struct device *dev, struct marvell_nfc *nfc, > > chip->controller = &nfc->controller; > > nand_set_flash_node(chip, np); > > > > - if (!of_property_read_bool(np, "marvell,nand-keep-config")) > > + if (of_property_read_bool(np, "marvell,nand-keep-config")) > > chip->options |= NAND_KEEP_TIMINGS; > > > > mtd = nand_to_mtd(chip)
Hi Chris, Chris.Packham@alliedtelesis.co.nz wrote on Mon, 3 Oct 2022 21:46:31 +0000: > Hi All, > > On 27/09/22 15:47, Chris Packham wrote: > > From: Tony O'Brien <tony.obrien@alliedtelesis.co.nz> > > > > Originally the absence of the marvell,nand-keep-config property caused > > the setup_data_interface function to be provided. However when > > setup_data_interface was moved into nand_controller_ops the logic was > > unintentionally inverted. Update the logic so that only if the > > marvell,nand-keep-config property is present the bootloader NAND config > > kept. > > > > Fixes: 7a08dbaedd36 ("mtd: rawnand: Move ->setup_data_interface() to nand_controller_ops") > > Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz> > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > > Just following up on this. I know things have probably been busy with > the 6.0 release but it's been a week so I figured I'd give this a bump. I was just off the past week :) I will queue it soon. > > > --- > > > > Notes: > > I think this is a bug that's been lurking for 4 years or so. I'm not > > sure that's particularly long in the life of an embedded device but it > > does make me wonder if there have been other bug reports about it. I don't remember any... Indeed this must be fixed. > > We noticed this because we had a bootloader that used maxed out NAND > > timings which made the time it took the kernel to do anything on the > > file system longer than we expected. > > > > drivers/mtd/nand/raw/marvell_nand.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c > > index 2455a581fd70..b248c5f657d5 100644 > > --- a/drivers/mtd/nand/raw/marvell_nand.c > > +++ b/drivers/mtd/nand/raw/marvell_nand.c > > @@ -2672,7 +2672,7 @@ static int marvell_nand_chip_init(struct device *dev, struct marvell_nfc *nfc, > > chip->controller = &nfc->controller; > > nand_set_flash_node(chip, np); > > > > - if (!of_property_read_bool(np, "marvell,nand-keep-config")) > > + if (of_property_read_bool(np, "marvell,nand-keep-config")) > > chip->options |= NAND_KEEP_TIMINGS; > > > > mtd = nand_to_mtd(chip) Thanks, Miquèl
Hi Chris, Chris.Packham@alliedtelesis.co.nz wrote on Tue, 27 Sep 2022 02:54:40 +0000: > On 27/09/22 15:47, Chris Packham wrote: > > From: Tony O'Brien <tony.obrien@alliedtelesis.co.nz> > > > > Originally the absence of the marvell,nand-keep-config property caused > > the setup_data_interface function to be provided. However when > > setup_data_interface was moved into nand_controller_ops the logic was > > unintentionally inverted. Update the logic so that only if the > > marvell,nand-keep-config property is present the bootloader NAND config > > kept. > > > > Fixes: 7a08dbaedd36 ("mtd: rawnand: Move ->setup_data_interface() to nand_controller_ops") > > Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz> > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > > --- > > > > Notes: > > I think this is a bug that's been lurking for 4 years or so. I'm not > > sure that's particularly long in the life of an embedded device but it > > does make me wonder if there have been other bug reports about it. > > > > We noticed this because we had a bootloader that used maxed out NAND > > timings which made the time it took the kernel to do anything on the > > file system longer than we expected. > > I think there might be a similar logic inversion bug in > drivers/mtd/nand/raw/denali.c but I lack the ability to test for that > platform. Agreed, the denali driver has the same issue. Could you please send a patch? > > > drivers/mtd/nand/raw/marvell_nand.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c > > index 2455a581fd70..b248c5f657d5 100644 > > --- a/drivers/mtd/nand/raw/marvell_nand.c > > +++ b/drivers/mtd/nand/raw/marvell_nand.c > > @@ -2672,7 +2672,7 @@ static int marvell_nand_chip_init(struct device *dev, struct marvell_nfc *nfc, > > chip->controller = &nfc->controller; > > nand_set_flash_node(chip, np); > > > > - if (!of_property_read_bool(np, "marvell,nand-keep-config")) > > + if (of_property_read_bool(np, "marvell,nand-keep-config")) > > chip->options |= NAND_KEEP_TIMINGS; > > > > mtd = nand_to_mtd(chip) Thanks, Miquèl
On 4/10/22 23:02, Miquel Raynal wrote: > Hi Chris, > > Chris.Packham@alliedtelesis.co.nz wrote on Tue, 27 Sep 2022 02:54:40 > +0000: > >> On 27/09/22 15:47, Chris Packham wrote: >>> From: Tony O'Brien <tony.obrien@alliedtelesis.co.nz> >>> >>> Originally the absence of the marvell,nand-keep-config property caused >>> the setup_data_interface function to be provided. However when >>> setup_data_interface was moved into nand_controller_ops the logic was >>> unintentionally inverted. Update the logic so that only if the >>> marvell,nand-keep-config property is present the bootloader NAND config >>> kept. >>> >>> Fixes: 7a08dbaedd36 ("mtd: rawnand: Move ->setup_data_interface() to nand_controller_ops") >>> Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz> >>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> >>> --- >>> >>> Notes: >>> I think this is a bug that's been lurking for 4 years or so. I'm not >>> sure that's particularly long in the life of an embedded device but it >>> does make me wonder if there have been other bug reports about it. >>> >>> We noticed this because we had a bootloader that used maxed out NAND >>> timings which made the time it took the kernel to do anything on the >>> file system longer than we expected. >> I think there might be a similar logic inversion bug in >> drivers/mtd/nand/raw/denali.c but I lack the ability to test for that >> platform. > Agreed, the denali driver has the same issue. Could you please send a > patch? Sure although it'll be compile tested only. >>> drivers/mtd/nand/raw/marvell_nand.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c >>> index 2455a581fd70..b248c5f657d5 100644 >>> --- a/drivers/mtd/nand/raw/marvell_nand.c >>> +++ b/drivers/mtd/nand/raw/marvell_nand.c >>> @@ -2672,7 +2672,7 @@ static int marvell_nand_chip_init(struct device *dev, struct marvell_nfc *nfc, >>> chip->controller = &nfc->controller; >>> nand_set_flash_node(chip, np); >>> >>> - if (!of_property_read_bool(np, "marvell,nand-keep-config")) >>> + if (of_property_read_bool(np, "marvell,nand-keep-config")) >>> chip->options |= NAND_KEEP_TIMINGS; >>> >>> mtd = nand_to_mtd(chip) > > Thanks, > Miquèl
On 5/10/22 08:41, Chris Packham wrote: > > On 4/10/22 23:02, Miquel Raynal wrote: >> Hi Chris, >> >> Chris.Packham@alliedtelesis.co.nz wrote on Tue, 27 Sep 2022 02:54:40 >> +0000: >> >>> On 27/09/22 15:47, Chris Packham wrote: >>>> From: Tony O'Brien <tony.obrien@alliedtelesis.co.nz> >>>> >>>> Originally the absence of the marvell,nand-keep-config property caused >>>> the setup_data_interface function to be provided. However when >>>> setup_data_interface was moved into nand_controller_ops the logic was >>>> unintentionally inverted. Update the logic so that only if the >>>> marvell,nand-keep-config property is present the bootloader NAND >>>> config >>>> kept. >>>> >>>> Fixes: 7a08dbaedd36 ("mtd: rawnand: Move ->setup_data_interface() >>>> to nand_controller_ops") >>>> Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz> >>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> >>>> --- >>>> >>>> Notes: >>>> I think this is a bug that's been lurking for 4 years or so. >>>> I'm not >>>> sure that's particularly long in the life of an embedded >>>> device but it >>>> does make me wonder if there have been other bug reports >>>> about it. >>>> We noticed this because we had a bootloader that used >>>> maxed out NAND >>>> timings which made the time it took the kernel to do anything >>>> on the >>>> file system longer than we expected. >>> I think there might be a similar logic inversion bug in >>> drivers/mtd/nand/raw/denali.c but I lack the ability to test for that >>> platform. >> Agreed, the denali driver has the same issue. Could you please send a >> patch? > Sure although it'll be compile tested only. Actually looks like it was already fixed in commit d311e0c27b8f ("mtd: rawnand: denali: get ->setup_data_interface() working again"). >>>> drivers/mtd/nand/raw/marvell_nand.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/mtd/nand/raw/marvell_nand.c >>>> b/drivers/mtd/nand/raw/marvell_nand.c >>>> index 2455a581fd70..b248c5f657d5 100644 >>>> --- a/drivers/mtd/nand/raw/marvell_nand.c >>>> +++ b/drivers/mtd/nand/raw/marvell_nand.c >>>> @@ -2672,7 +2672,7 @@ static int marvell_nand_chip_init(struct >>>> device *dev, struct marvell_nfc *nfc, >>>> chip->controller = &nfc->controller; >>>> nand_set_flash_node(chip, np); >>>> - if (!of_property_read_bool(np, "marvell,nand-keep-config")) >>>> + if (of_property_read_bool(np, "marvell,nand-keep-config")) >>>> chip->options |= NAND_KEEP_TIMINGS; >>>> mtd = nand_to_mtd(chip) >> >> Thanks, >> Miquèl
Hi Chris, Chris.Packham@alliedtelesis.co.nz wrote on Tue, 4 Oct 2022 21:21:37 +0000: > On 5/10/22 08:41, Chris Packham wrote: > > > > On 4/10/22 23:02, Miquel Raynal wrote: > >> Hi Chris, > >> > >> Chris.Packham@alliedtelesis.co.nz wrote on Tue, 27 Sep 2022 02:54:40 > >> +0000: > >> > >>> On 27/09/22 15:47, Chris Packham wrote: > >>>> From: Tony O'Brien <tony.obrien@alliedtelesis.co.nz> > >>>> > >>>> Originally the absence of the marvell,nand-keep-config property caused > >>>> the setup_data_interface function to be provided. However when > >>>> setup_data_interface was moved into nand_controller_ops the logic was > >>>> unintentionally inverted. Update the logic so that only if the > >>>> marvell,nand-keep-config property is present the bootloader NAND > >>>> config > >>>> kept. > >>>> > >>>> Fixes: 7a08dbaedd36 ("mtd: rawnand: Move ->setup_data_interface() > >>>> to nand_controller_ops") > >>>> Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz> > >>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > >>>> --- > >>>> > >>>> Notes: > >>>> I think this is a bug that's been lurking for 4 years or so. > >>>> I'm not > >>>> sure that's particularly long in the life of an embedded > >>>> device but it > >>>> does make me wonder if there have been other bug reports > >>>> about it. > >>>> We noticed this because we had a bootloader that used > >>>> maxed out NAND > >>>> timings which made the time it took the kernel to do anything > >>>> on the > >>>> file system longer than we expected. > >>> I think there might be a similar logic inversion bug in > >>> drivers/mtd/nand/raw/denali.c but I lack the ability to test for that > >>> platform. > >> Agreed, the denali driver has the same issue. Could you please send a > >> patch? > > Sure although it'll be compile tested only. > Actually looks like it was already fixed in commit d311e0c27b8f ("mtd: > rawnand: denali: get ->setup_data_interface() working again"). Ok, perfect. Thanks, Miquèl
On Tue, 2022-09-27 at 02:47:28 UTC, Chris Packham wrote: > From: Tony O'Brien <tony.obrien@alliedtelesis.co.nz> > > Originally the absence of the marvell,nand-keep-config property caused > the setup_data_interface function to be provided. However when > setup_data_interface was moved into nand_controller_ops the logic was > unintentionally inverted. Update the logic so that only if the > marvell,nand-keep-config property is present the bootloader NAND config > kept. > > Fixes: 7a08dbaedd36 ("mtd: rawnand: Move ->setup_data_interface() to nand_controller_ops") > Signed-off-by: Tony O'Brien <tony.obrien@alliedtelesis.co.nz> > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/fixes, thanks. Miquel
diff --git a/drivers/mtd/nand/raw/marvell_nand.c b/drivers/mtd/nand/raw/marvell_nand.c index 2455a581fd70..b248c5f657d5 100644 --- a/drivers/mtd/nand/raw/marvell_nand.c +++ b/drivers/mtd/nand/raw/marvell_nand.c @@ -2672,7 +2672,7 @@ static int marvell_nand_chip_init(struct device *dev, struct marvell_nfc *nfc, chip->controller = &nfc->controller; nand_set_flash_node(chip, np); - if (!of_property_read_bool(np, "marvell,nand-keep-config")) + if (of_property_read_bool(np, "marvell,nand-keep-config")) chip->options |= NAND_KEEP_TIMINGS; mtd = nand_to_mtd(chip);