[PATCH v4] mtd: parsers: ofpart: add workaround for #size-cells 0

[PATCH v4] mtd: parsers: ofpart: add workaround for #size-cells 0

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

Add a mechanism to handle the case in which partitions are present as
direct child of the nand controller node and #size-cells is set to <0>.

This could happen if the nand-controller node in the DTS is supposed to
have #size-cells set to 0, but for some historical reason/bug it was set
to 1 in the past, and the firmware (e.g. U-Boot) is adding the partition
as direct children of the nand-controller defaulting to #size-cells
being to 1.

This prevents a real boot failure on colibri-imx7 that happened during v6.1
development cycles.

Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
I do not expect this patch to be backported to stable, however I would expect
that we do not backport nand-controller dts cleanups neither.

v4:
 fixed wrong English spelling in the comment

v3:
 minor formatting change, removed not needed new-line and space. 

v2:
 fixup size-cells only when partitions are direct children of the nand-controller
 completely revised commit message, comments and warning print
 use pr_warn instead of pr_warn_once
 added Reviewed-by Greg
 removed cc:stable@ and fixes tag, since the problematic commit was reverted
---
 drivers/mtd/parsers/ofpart_core.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/mtd/parsers/ofpart_core.c b/drivers/mtd/parsers/ofpart_core.c
index 192190c42fc8..e7b8e9d0a910 100644
--- a/drivers/mtd/parsers/ofpart_core.c
+++ b/drivers/mtd/parsers/ofpart_core.c
@@ -122,6 +122,25 @@ static int parse_fixed_partitions(struct mtd_info *master,
 
 		a_cells = of_n_addr_cells(pp);
 		s_cells = of_n_size_cells(pp);
+		if (!dedicated && s_cells == 0) {
+			/*
+			 * This is a ugly workaround to not create
+			 * regression on devices that are still creating
+			 * partitions as direct children of the nand controller.
+			 * This can happen in case the nand controller node has
+			 * #size-cells equal to 0 and the firmware (e.g.
+			 * U-Boot) just add the partitions there assuming
+			 * 32-bit addressing.
+			 *
+			 * If you get this warning your firmware and/or DTS
+			 * should be really fixed.
+			 *
+			 * This is working only for devices smaller than 4GiB.
+			 */
+			pr_warn("%s: ofpart partition %pOF (%pOF) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.\n",
+				master->name, pp, mtd_node);
+			s_cells = 1;
+		}
 		if (len / 4 != a_cells + s_cells) {
 			pr_debug("%s: ofpart partition %pOF (%pOF) error parsing reg property.\n",
 				 master->name, pp,
-- 
2.25.1

Re: [PATCH v4] mtd: parsers: ofpart: add workaround for #size-cells 0

[Greg Kroah-Hartman]

On Tue, Jan 24, 2023 at 11:44:44AM +0100, Francesco Dolcini wrote:
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> Add a mechanism to handle the case in which partitions are present as
> direct child of the nand controller node and #size-cells is set to <0>.
> 
> This could happen if the nand-controller node in the DTS is supposed to
> have #size-cells set to 0, but for some historical reason/bug it was set
> to 1 in the past, and the firmware (e.g. U-Boot) is adding the partition
> as direct children of the nand-controller defaulting to #size-cells
> being to 1.
> 
> This prevents a real boot failure on colibri-imx7 that happened during v6.1
> development cycles.
> 
> Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> I do not expect this patch to be backported to stable, however I would expect
> that we do not backport nand-controller dts cleanups neither.
> 
> v4:
>  fixed wrong English spelling in the comment
> 
> v3:
>  minor formatting change, removed not needed new-line and space. 
> 
> v2:
>  fixup size-cells only when partitions are direct children of the nand-controller
>  completely revised commit message, comments and warning print
>  use pr_warn instead of pr_warn_once
>  added Reviewed-by Greg
>  removed cc:stable@ and fixes tag, since the problematic commit was reverted
> ---
>  drivers/mtd/parsers/ofpart_core.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/mtd/parsers/ofpart_core.c b/drivers/mtd/parsers/ofpart_core.c
> index 192190c42fc8..e7b8e9d0a910 100644
> --- a/drivers/mtd/parsers/ofpart_core.c
> +++ b/drivers/mtd/parsers/ofpart_core.c
> @@ -122,6 +122,25 @@ static int parse_fixed_partitions(struct mtd_info *master,
>  
>  		a_cells = of_n_addr_cells(pp);
>  		s_cells = of_n_size_cells(pp);
> +		if (!dedicated && s_cells == 0) {
> +			/*
> +			 * This is a ugly workaround to not create
> +			 * regression on devices that are still creating
> +			 * partitions as direct children of the nand controller.
> +			 * This can happen in case the nand controller node has
> +			 * #size-cells equal to 0 and the firmware (e.g.
> +			 * U-Boot) just add the partitions there assuming
> +			 * 32-bit addressing.
> +			 *
> +			 * If you get this warning your firmware and/or DTS
> +			 * should be really fixed.
> +			 *
> +			 * This is working only for devices smaller than 4GiB.
> +			 */
> +			pr_warn("%s: ofpart partition %pOF (%pOF) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.\n",
> +				master->name, pp, mtd_node);

This is a driver, always use dev_*() calls, not pr_*() calls so that we
know what is being referred to exactly.

I take back my "reviewed-by" line above, please fix this up to not need
pr_warn, but to use dev_warn() instead.

thanks,

greg k-h

Re: [PATCH v4] mtd: parsers: ofpart: add workaround for #size-cells 0

[Francesco Dolcini]

Hello Miquel, Greg and all

On Tue, Jan 24, 2023 at 04:38:59PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Jan 24, 2023 at 11:44:44AM +0100, Francesco Dolcini wrote:
> > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > 
> > Add a mechanism to handle the case in which partitions are present as
> > direct child of the nand controller node and #size-cells is set to <0>.
> > 
> > This could happen if the nand-controller node in the DTS is supposed to
> > have #size-cells set to 0, but for some historical reason/bug it was set
> > to 1 in the past, and the firmware (e.g. U-Boot) is adding the partition
> > as direct children of the nand-controller defaulting to #size-cells
> > being to 1.
> > 
> > This prevents a real boot failure on colibri-imx7 that happened during v6.1
> > development cycles.
> > 
> > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > I do not expect this patch to be backported to stable, however I would expect
> > that we do not backport nand-controller dts cleanups neither.
> > 
> > v4:
> >  fixed wrong English spelling in the comment
> > 
> > v3:
> >  minor formatting change, removed not needed new-line and space. 
> > 
> > v2:
> >  fixup size-cells only when partitions are direct children of the nand-controller
> >  completely revised commit message, comments and warning print
> >  use pr_warn instead of pr_warn_once
> >  added Reviewed-by Greg
> >  removed cc:stable@ and fixes tag, since the problematic commit was reverted
> > ---
> >  drivers/mtd/parsers/ofpart_core.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/drivers/mtd/parsers/ofpart_core.c b/drivers/mtd/parsers/ofpart_core.c
> > index 192190c42fc8..e7b8e9d0a910 100644
> > --- a/drivers/mtd/parsers/ofpart_core.c
> > +++ b/drivers/mtd/parsers/ofpart_core.c
> > @@ -122,6 +122,25 @@ static int parse_fixed_partitions(struct mtd_info *master,
> >  
> >  		a_cells = of_n_addr_cells(pp);
> >  		s_cells = of_n_size_cells(pp);
> > +		if (!dedicated && s_cells == 0) {
> > +			/*
> > +			 * This is a ugly workaround to not create
> > +			 * regression on devices that are still creating
> > +			 * partitions as direct children of the nand controller.
> > +			 * This can happen in case the nand controller node has
> > +			 * #size-cells equal to 0 and the firmware (e.g.
> > +			 * U-Boot) just add the partitions there assuming
> > +			 * 32-bit addressing.
> > +			 *
> > +			 * If you get this warning your firmware and/or DTS
> > +			 * should be really fixed.
> > +			 *
> > +			 * This is working only for devices smaller than 4GiB.
> > +			 */
> > +			pr_warn("%s: ofpart partition %pOF (%pOF) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.\n",
> > +				master->name, pp, mtd_node);
> 
> This is a driver, always use dev_*() calls, not pr_*() calls so that we
> know what is being referred to exactly.

Is this reasonable here? Where can I get the struct device?

In general this file uses only pr_* debug API and messages are about OF
nodes/properties, not about a device.

> I take back my "reviewed-by" line above, please fix this up to not need
> pr_warn, but to use dev_warn() instead.

Francesco

Re: [PATCH v4] mtd: parsers: ofpart: add workaround for #size-cells 0

[Miquel Raynal]

Hi Greg,

francesco@dolcini.it wrote on Wed, 25 Jan 2023 22:06:57 +0100:

> Hello Miquel, Greg and all
> 
> On Tue, Jan 24, 2023 at 04:38:59PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Jan 24, 2023 at 11:44:44AM +0100, Francesco Dolcini wrote:  
> > > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > 
> > > Add a mechanism to handle the case in which partitions are present as
> > > direct child of the nand controller node and #size-cells is set to <0>.
> > > 
> > > This could happen if the nand-controller node in the DTS is supposed to
> > > have #size-cells set to 0, but for some historical reason/bug it was set
> > > to 1 in the past, and the firmware (e.g. U-Boot) is adding the partition
> > > as direct children of the nand-controller defaulting to #size-cells
> > > being to 1.
> > > 
> > > This prevents a real boot failure on colibri-imx7 that happened during v6.1
> > > development cycles.
> > > 
> > > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> > > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > ---
> > > I do not expect this patch to be backported to stable, however I would expect
> > > that we do not backport nand-controller dts cleanups neither.
> > > 
> > > v4:
> > >  fixed wrong English spelling in the comment
> > > 
> > > v3:
> > >  minor formatting change, removed not needed new-line and space. 
> > > 
> > > v2:
> > >  fixup size-cells only when partitions are direct children of the nand-controller
> > >  completely revised commit message, comments and warning print
> > >  use pr_warn instead of pr_warn_once
> > >  added Reviewed-by Greg
> > >  removed cc:stable@ and fixes tag, since the problematic commit was reverted
> > > ---
> > >  drivers/mtd/parsers/ofpart_core.c | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > > 
> > > diff --git a/drivers/mtd/parsers/ofpart_core.c b/drivers/mtd/parsers/ofpart_core.c
> > > index 192190c42fc8..e7b8e9d0a910 100644
> > > --- a/drivers/mtd/parsers/ofpart_core.c
> > > +++ b/drivers/mtd/parsers/ofpart_core.c
> > > @@ -122,6 +122,25 @@ static int parse_fixed_partitions(struct mtd_info *master,
> > >  
> > >  		a_cells = of_n_addr_cells(pp);
> > >  		s_cells = of_n_size_cells(pp);
> > > +		if (!dedicated && s_cells == 0) {
> > > +			/*
> > > +			 * This is a ugly workaround to not create
> > > +			 * regression on devices that are still creating
> > > +			 * partitions as direct children of the nand controller.
> > > +			 * This can happen in case the nand controller node has
> > > +			 * #size-cells equal to 0 and the firmware (e.g.
> > > +			 * U-Boot) just add the partitions there assuming
> > > +			 * 32-bit addressing.
> > > +			 *
> > > +			 * If you get this warning your firmware and/or DTS
> > > +			 * should be really fixed.
> > > +			 *
> > > +			 * This is working only for devices smaller than 4GiB.
> > > +			 */
> > > +			pr_warn("%s: ofpart partition %pOF (%pOF) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.\n",
> > > +				master->name, pp, mtd_node);  
> > 
> > This is a driver, always use dev_*() calls, not pr_*() calls so that we
> > know what is being referred to exactly.  
> 
> Is this reasonable here? Where can I get the struct device?
> 
> In general this file uses only pr_* debug API and messages are about OF
> nodes/properties, not about a device.

I'm also skeptical here, this is not a device driver, it's a generic
parser and it seems more appropriate to warn about an of node rather
than a struct device.

MTD devices inherit from struct device (mtd->dev) which I guess
might be used here. The bus infrastructure device
(mtd->device->parents) is less appropriate as it sometimes points at the
controller (raw NAND) and sometimes at the spi device (SPI-NAND, SPI
NOR).

pr_warn is fine here IMHO, but if Greg insist, switch it to dev_warn, I
don't mind. Maybe it is worth testing that dev_warn still displays an
easy-to-understand message in that case.

> > I take back my "reviewed-by" line above, please fix this up to not need
> > pr_warn, but to use dev_warn() instead.  
> 
> Francesco


Thanks,
Miquèl

Re: [PATCH v4] mtd: parsers: ofpart: add workaround for #size-cells 0

[Greg Kroah-Hartman]

On Wed, Jan 25, 2023 at 10:06:57PM +0100, Francesco Dolcini wrote:
> Hello Miquel, Greg and all
> 
> On Tue, Jan 24, 2023 at 04:38:59PM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Jan 24, 2023 at 11:44:44AM +0100, Francesco Dolcini wrote:
> > > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > 
> > > Add a mechanism to handle the case in which partitions are present as
> > > direct child of the nand controller node and #size-cells is set to <0>.
> > > 
> > > This could happen if the nand-controller node in the DTS is supposed to
> > > have #size-cells set to 0, but for some historical reason/bug it was set
> > > to 1 in the past, and the firmware (e.g. U-Boot) is adding the partition
> > > as direct children of the nand-controller defaulting to #size-cells
> > > being to 1.
> > > 
> > > This prevents a real boot failure on colibri-imx7 that happened during v6.1
> > > development cycles.
> > > 
> > > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> > > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > ---
> > > I do not expect this patch to be backported to stable, however I would expect
> > > that we do not backport nand-controller dts cleanups neither.
> > > 
> > > v4:
> > >  fixed wrong English spelling in the comment
> > > 
> > > v3:
> > >  minor formatting change, removed not needed new-line and space. 
> > > 
> > > v2:
> > >  fixup size-cells only when partitions are direct children of the nand-controller
> > >  completely revised commit message, comments and warning print
> > >  use pr_warn instead of pr_warn_once
> > >  added Reviewed-by Greg
> > >  removed cc:stable@ and fixes tag, since the problematic commit was reverted
> > > ---
> > >  drivers/mtd/parsers/ofpart_core.c | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > > 
> > > diff --git a/drivers/mtd/parsers/ofpart_core.c b/drivers/mtd/parsers/ofpart_core.c
> > > index 192190c42fc8..e7b8e9d0a910 100644
> > > --- a/drivers/mtd/parsers/ofpart_core.c
> > > +++ b/drivers/mtd/parsers/ofpart_core.c
> > > @@ -122,6 +122,25 @@ static int parse_fixed_partitions(struct mtd_info *master,
> > >  
> > >  		a_cells = of_n_addr_cells(pp);
> > >  		s_cells = of_n_size_cells(pp);
> > > +		if (!dedicated && s_cells == 0) {
> > > +			/*
> > > +			 * This is a ugly workaround to not create
> > > +			 * regression on devices that are still creating
> > > +			 * partitions as direct children of the nand controller.
> > > +			 * This can happen in case the nand controller node has
> > > +			 * #size-cells equal to 0 and the firmware (e.g.
> > > +			 * U-Boot) just add the partitions there assuming
> > > +			 * 32-bit addressing.
> > > +			 *
> > > +			 * If you get this warning your firmware and/or DTS
> > > +			 * should be really fixed.
> > > +			 *
> > > +			 * This is working only for devices smaller than 4GiB.
> > > +			 */
> > > +			pr_warn("%s: ofpart partition %pOF (%pOF) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.\n",
> > > +				master->name, pp, mtd_node);
> > 
> > This is a driver, always use dev_*() calls, not pr_*() calls so that we
> > know what is being referred to exactly.
> 
> Is this reasonable here? Where can I get the struct device?

Walk back up the call chain, there has to be a device somewhere
controlling this, right?

> In general this file uses only pr_* debug API and messages are about OF
> nodes/properties, not about a device.

OF nodes and properties are part of a device's properties :)

thanks,

greg k-h

Re: [PATCH v4] mtd: parsers: ofpart: add workaround for #size-cells 0

[Miquel Raynal]

Hi Greg,

gregkh@linuxfoundation.org wrote on Thu, 26 Jan 2023 10:01:02 +0100:

> On Wed, Jan 25, 2023 at 10:06:57PM +0100, Francesco Dolcini wrote:
> > Hello Miquel, Greg and all
> > 
> > On Tue, Jan 24, 2023 at 04:38:59PM +0100, Greg Kroah-Hartman wrote:  
> > > On Tue, Jan 24, 2023 at 11:44:44AM +0100, Francesco Dolcini wrote:  
> > > > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > > 
> > > > Add a mechanism to handle the case in which partitions are present as
> > > > direct child of the nand controller node and #size-cells is set to <0>.
> > > > 
> > > > This could happen if the nand-controller node in the DTS is supposed to
> > > > have #size-cells set to 0, but for some historical reason/bug it was set
> > > > to 1 in the past, and the firmware (e.g. U-Boot) is adding the partition
> > > > as direct children of the nand-controller defaulting to #size-cells
> > > > being to 1.
> > > > 
> > > > This prevents a real boot failure on colibri-imx7 that happened during v6.1
> > > > development cycles.
> > > > 
> > > > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> > > > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > ---
> > > > I do not expect this patch to be backported to stable, however I would expect
> > > > that we do not backport nand-controller dts cleanups neither.
> > > > 
> > > > v4:
> > > >  fixed wrong English spelling in the comment
> > > > 
> > > > v3:
> > > >  minor formatting change, removed not needed new-line and space. 
> > > > 
> > > > v2:
> > > >  fixup size-cells only when partitions are direct children of the nand-controller
> > > >  completely revised commit message, comments and warning print
> > > >  use pr_warn instead of pr_warn_once
> > > >  added Reviewed-by Greg
> > > >  removed cc:stable@ and fixes tag, since the problematic commit was reverted
> > > > ---
> > > >  drivers/mtd/parsers/ofpart_core.c | 19 +++++++++++++++++++
> > > >  1 file changed, 19 insertions(+)
> > > > 
> > > > diff --git a/drivers/mtd/parsers/ofpart_core.c b/drivers/mtd/parsers/ofpart_core.c
> > > > index 192190c42fc8..e7b8e9d0a910 100644
> > > > --- a/drivers/mtd/parsers/ofpart_core.c
> > > > +++ b/drivers/mtd/parsers/ofpart_core.c
> > > > @@ -122,6 +122,25 @@ static int parse_fixed_partitions(struct mtd_info *master,
> > > >  
> > > >  		a_cells = of_n_addr_cells(pp);
> > > >  		s_cells = of_n_size_cells(pp);
> > > > +		if (!dedicated && s_cells == 0) {
> > > > +			/*
> > > > +			 * This is a ugly workaround to not create
> > > > +			 * regression on devices that are still creating
> > > > +			 * partitions as direct children of the nand controller.
> > > > +			 * This can happen in case the nand controller node has
> > > > +			 * #size-cells equal to 0 and the firmware (e.g.
> > > > +			 * U-Boot) just add the partitions there assuming
> > > > +			 * 32-bit addressing.
> > > > +			 *
> > > > +			 * If you get this warning your firmware and/or DTS
> > > > +			 * should be really fixed.
> > > > +			 *
> > > > +			 * This is working only for devices smaller than 4GiB.
> > > > +			 */
> > > > +			pr_warn("%s: ofpart partition %pOF (%pOF) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.\n",
> > > > +				master->name, pp, mtd_node);  
> > > 
> > > This is a driver, always use dev_*() calls, not pr_*() calls so that we
> > > know what is being referred to exactly.  
> > 
> > Is this reasonable here? Where can I get the struct device?  
> 
> Walk back up the call chain, there has to be a device somewhere
> controlling this, right?
> 
> > In general this file uses only pr_* debug API and messages are about OF
> > nodes/properties, not about a device.  
> 
> OF nodes and properties are part of a device's properties :)

Yes but the warning comes from a wrong DT description, hence it felt
better suited to warn against the node name which is easily identifiable
in a text file and must be fixed rather than the device which is a pure
software component.

Anyway, Francesco, please show us the resultant line and if it feels
meaningful enough we'll take the dev_warn approach.

Thanks,
Miquèl

Re: [PATCH v4] mtd: parsers: ofpart: add workaround for #size-cells 0

[Francesco Dolcini]

On Thu, Jan 26, 2023 at 10:12:04AM +0100, Miquel Raynal wrote:
> gregkh@linuxfoundation.org wrote on Thu, 26 Jan 2023 10:01:02 +0100:
> 
> > On Wed, Jan 25, 2023 at 10:06:57PM +0100, Francesco Dolcini wrote:
> > > Hello Miquel, Greg and all
> > > 
> > > On Tue, Jan 24, 2023 at 04:38:59PM +0100, Greg Kroah-Hartman wrote:  
> > > > On Tue, Jan 24, 2023 at 11:44:44AM +0100, Francesco Dolcini wrote:  
> > > > > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > > > 
> > > > > Add a mechanism to handle the case in which partitions are present as
> > > > > direct child of the nand controller node and #size-cells is set to <0>.
> > > > > 
> > > > > This could happen if the nand-controller node in the DTS is supposed to
> > > > > have #size-cells set to 0, but for some historical reason/bug it was set
> > > > > to 1 in the past, and the firmware (e.g. U-Boot) is adding the partition
> > > > > as direct children of the nand-controller defaulting to #size-cells
> > > > > being to 1.
> > > > > 
> > > > > This prevents a real boot failure on colibri-imx7 that happened during v6.1
> > > > > development cycles.
> > > > > 
> > > > > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> > > > > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> > > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > ---
> > > > > I do not expect this patch to be backported to stable, however I would expect
> > > > > that we do not backport nand-controller dts cleanups neither.
> > > > > 
> > > > > v4:
> > > > >  fixed wrong English spelling in the comment
> > > > > 
> > > > > v3:
> > > > >  minor formatting change, removed not needed new-line and space. 
> > > > > 
> > > > > v2:
> > > > >  fixup size-cells only when partitions are direct children of the nand-controller
> > > > >  completely revised commit message, comments and warning print
> > > > >  use pr_warn instead of pr_warn_once
> > > > >  added Reviewed-by Greg
> > > > >  removed cc:stable@ and fixes tag, since the problematic commit was reverted
> > > > > ---
> > > > >  drivers/mtd/parsers/ofpart_core.c | 19 +++++++++++++++++++
> > > > >  1 file changed, 19 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/mtd/parsers/ofpart_core.c b/drivers/mtd/parsers/ofpart_core.c
> > > > > index 192190c42fc8..e7b8e9d0a910 100644
> > > > > --- a/drivers/mtd/parsers/ofpart_core.c
> > > > > +++ b/drivers/mtd/parsers/ofpart_core.c
> > > > > @@ -122,6 +122,25 @@ static int parse_fixed_partitions(struct mtd_info *master,
> > > > >  
> > > > >  		a_cells = of_n_addr_cells(pp);
> > > > >  		s_cells = of_n_size_cells(pp);
> > > > > +		if (!dedicated && s_cells == 0) {
> > > > > +			/*
> > > > > +			 * This is a ugly workaround to not create
> > > > > +			 * regression on devices that are still creating
> > > > > +			 * partitions as direct children of the nand controller.
> > > > > +			 * This can happen in case the nand controller node has
> > > > > +			 * #size-cells equal to 0 and the firmware (e.g.
> > > > > +			 * U-Boot) just add the partitions there assuming
> > > > > +			 * 32-bit addressing.
> > > > > +			 *
> > > > > +			 * If you get this warning your firmware and/or DTS
> > > > > +			 * should be really fixed.
> > > > > +			 *
> > > > > +			 * This is working only for devices smaller than 4GiB.
> > > > > +			 */
> > > > > +			pr_warn("%s: ofpart partition %pOF (%pOF) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.\n",
> > > > > +				master->name, pp, mtd_node);  
> > > > 
> > > > This is a driver, always use dev_*() calls, not pr_*() calls so that we
> > > > know what is being referred to exactly.  
> > > 
> > > Is this reasonable here? Where can I get the struct device?  
> > 
> > Walk back up the call chain, there has to be a device somewhere
> > controlling this, right?
> > 
> > > In general this file uses only pr_* debug API and messages are about OF
> > > nodes/properties, not about a device.  
> > 
> > OF nodes and properties are part of a device's properties :)
> 
> Yes but the warning comes from a wrong DT description, hence it felt
> better suited to warn against the node name which is easily identifiable
> in a text file and must be fixed rather than the device which is a pure
> software component.
> 
> Anyway, Francesco, please show us the resultant line and if it feels
> meaningful enough we'll take the dev_warn approach.

So, I tried, but I guess I failed.

Both

  dev_warn(&mtd_get_master(master)->dev, ...);

and

  dev_warn(&master->dev, ...);

are NULL.


(null): gpmi-nand: ofpart partition /soc/nand-controller@33002000/partition@0 (/soc/nand-controller@33002000) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.

while the current v4 is just:

gpmi-nand: ofpart partition /soc/nand-controller@33002000/partition@0 (/soc/nand-controller@33002000) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.

on a colibri-imx7.

Any advice?

Francesco

Re: [PATCH v4] mtd: parsers: ofpart: add workaround for #size-cells 0

[Miquel Raynal]

Hi Francesco,

francesco@dolcini.it wrote on Thu, 2 Feb 2023 12:33:34 +0100:

> On Thu, Jan 26, 2023 at 10:12:04AM +0100, Miquel Raynal wrote:
> > gregkh@linuxfoundation.org wrote on Thu, 26 Jan 2023 10:01:02 +0100:
> >   
> > > On Wed, Jan 25, 2023 at 10:06:57PM +0100, Francesco Dolcini wrote:  
> > > > Hello Miquel, Greg and all
> > > > 
> > > > On Tue, Jan 24, 2023 at 04:38:59PM +0100, Greg Kroah-Hartman wrote:    
> > > > > On Tue, Jan 24, 2023 at 11:44:44AM +0100, Francesco Dolcini wrote:    
> > > > > > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > > > > 
> > > > > > Add a mechanism to handle the case in which partitions are present as
> > > > > > direct child of the nand controller node and #size-cells is set to <0>.
> > > > > > 
> > > > > > This could happen if the nand-controller node in the DTS is supposed to
> > > > > > have #size-cells set to 0, but for some historical reason/bug it was set
> > > > > > to 1 in the past, and the firmware (e.g. U-Boot) is adding the partition
> > > > > > as direct children of the nand-controller defaulting to #size-cells
> > > > > > being to 1.
> > > > > > 
> > > > > > This prevents a real boot failure on colibri-imx7 that happened during v6.1
> > > > > > development cycles.
> > > > > > 
> > > > > > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> > > > > > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> > > > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > > > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > > ---
> > > > > > I do not expect this patch to be backported to stable, however I would expect
> > > > > > that we do not backport nand-controller dts cleanups neither.
> > > > > > 
> > > > > > v4:
> > > > > >  fixed wrong English spelling in the comment
> > > > > > 
> > > > > > v3:
> > > > > >  minor formatting change, removed not needed new-line and space. 
> > > > > > 
> > > > > > v2:
> > > > > >  fixup size-cells only when partitions are direct children of the nand-controller
> > > > > >  completely revised commit message, comments and warning print
> > > > > >  use pr_warn instead of pr_warn_once
> > > > > >  added Reviewed-by Greg
> > > > > >  removed cc:stable@ and fixes tag, since the problematic commit was reverted
> > > > > > ---
> > > > > >  drivers/mtd/parsers/ofpart_core.c | 19 +++++++++++++++++++
> > > > > >  1 file changed, 19 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/mtd/parsers/ofpart_core.c b/drivers/mtd/parsers/ofpart_core.c
> > > > > > index 192190c42fc8..e7b8e9d0a910 100644
> > > > > > --- a/drivers/mtd/parsers/ofpart_core.c
> > > > > > +++ b/drivers/mtd/parsers/ofpart_core.c
> > > > > > @@ -122,6 +122,25 @@ static int parse_fixed_partitions(struct mtd_info *master,
> > > > > >  
> > > > > >  		a_cells = of_n_addr_cells(pp);
> > > > > >  		s_cells = of_n_size_cells(pp);
> > > > > > +		if (!dedicated && s_cells == 0) {
> > > > > > +			/*
> > > > > > +			 * This is a ugly workaround to not create
> > > > > > +			 * regression on devices that are still creating
> > > > > > +			 * partitions as direct children of the nand controller.
> > > > > > +			 * This can happen in case the nand controller node has
> > > > > > +			 * #size-cells equal to 0 and the firmware (e.g.
> > > > > > +			 * U-Boot) just add the partitions there assuming
> > > > > > +			 * 32-bit addressing.
> > > > > > +			 *
> > > > > > +			 * If you get this warning your firmware and/or DTS
> > > > > > +			 * should be really fixed.
> > > > > > +			 *
> > > > > > +			 * This is working only for devices smaller than 4GiB.
> > > > > > +			 */
> > > > > > +			pr_warn("%s: ofpart partition %pOF (%pOF) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.\n",
> > > > > > +				master->name, pp, mtd_node);    
> > > > > 
> > > > > This is a driver, always use dev_*() calls, not pr_*() calls so that we
> > > > > know what is being referred to exactly.    
> > > > 
> > > > Is this reasonable here? Where can I get the struct device?    
> > > 
> > > Walk back up the call chain, there has to be a device somewhere
> > > controlling this, right?
> > >   
> > > > In general this file uses only pr_* debug API and messages are about OF
> > > > nodes/properties, not about a device.    
> > > 
> > > OF nodes and properties are part of a device's properties :)  
> > 
> > Yes but the warning comes from a wrong DT description, hence it felt
> > better suited to warn against the node name which is easily identifiable
> > in a text file and must be fixed rather than the device which is a pure
> > software component.
> > 
> > Anyway, Francesco, please show us the resultant line and if it feels
> > meaningful enough we'll take the dev_warn approach.  
> 
> So, I tried, but I guess I failed.
> 
> Both
> 
>   dev_warn(&mtd_get_master(master)->dev, ...);
> 
> and
> 
>   dev_warn(&master->dev, ...);
> 
> are NULL.

mtd->dev (in raw NAND) is populated by the controller drivers, so the
master mtd device is pointing to the bus "struct device" in its
dev.parent field. This happens at the end of the probe of the
controller, after setting the dev entry, so we expect the name of the
controller to appear.

> (null): gpmi-nand: ofpart partition /soc/nand-controller@33002000/partition@0 (/soc/nand-controller@33002000) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.

Second field looks right, first field does not (bus or class id?) I
have no idea why it has not been populated at this point (end of the
controller probe). But it's not a big deal, at least we have the device
name, so it's ok for me.

> while the current v4 is just:
> 
> gpmi-nand: ofpart partition /soc/nand-controller@33002000/partition@0 (/soc/nand-controller@33002000) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.
> 
> on a colibri-imx7.
> 
> Any advice?
> 
> Francesco
> 


Thanks,
Miquèl

Re: [PATCH v4] mtd: parsers: ofpart: add workaround for #size-cells 0

[Francesco Dolcini]

Il 3 febbraio 2023 16:12:02 CET, Miquel Raynal <miquel.raynal@bootlin.com> ha scritto:
>Hi Francesco,
>
>francesco@dolcini.it wrote on Thu, 2 Feb 2023 12:33:34 +0100:
>
>> On Thu, Jan 26, 2023 at 10:12:04AM +0100, Miquel Raynal wrote:
>> > gregkh@linuxfoundation.org wrote on Thu, 26 Jan 2023 10:01:02 +0100:
>> >   
>> > > On Wed, Jan 25, 2023 at 10:06:57PM +0100, Francesco Dolcini wrote:  
>> > > > Hello Miquel, Greg and all
>> > > > 
>> > > > On Tue, Jan 24, 2023 at 04:38:59PM +0100, Greg Kroah-Hartman wrote:    
>> > > > > On Tue, Jan 24, 2023 at 11:44:44AM +0100, Francesco Dolcini wrote:    
>> > > > > > From: Francesco Dolcini <francesco.dolcini@toradex.com>
>> > > > > > 
>> > > > > > Add a mechanism to handle the case in which partitions are present as
>> > > > > > direct child of the nand controller node and #size-cells is set to <0>.
>> > > > > > 
>> > > > > > This could happen if the nand-controller node in the DTS is supposed to
>> > > > > > have #size-cells set to 0, but for some historical reason/bug it was set
>> > > > > > to 1 in the past, and the firmware (e.g. U-Boot) is adding the partition
>> > > > > > as direct children of the nand-controller defaulting to #size-cells
>> > > > > > being to 1.
>> > > > > > 
>> > > > > > This prevents a real boot failure on colibri-imx7 that happened during v6.1
>> > > > > > development cycles.
>> > > > > > 
>> > > > > > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
>> > > > > > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
>> > > > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
>> > > > > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> > > > > > ---
>> > > > > > I do not expect this patch to be backported to stable, however I would expect
>> > > > > > that we do not backport nand-controller dts cleanups neither.
>> > > > > > 
>> > > > > > v4:
>> > > > > >  fixed wrong English spelling in the comment
>> > > > > > 
>> > > > > > v3:
>> > > > > >  minor formatting change, removed not needed new-line and space. 
>> > > > > > 
>> > > > > > v2:
>> > > > > >  fixup size-cells only when partitions are direct children of the nand-controller
>> > > > > >  completely revised commit message, comments and warning print
>> > > > > >  use pr_warn instead of pr_warn_once
>> > > > > >  added Reviewed-by Greg
>> > > > > >  removed cc:stable@ and fixes tag, since the problematic commit was reverted
>> > > > > > ---
>> > > > > >  drivers/mtd/parsers/ofpart_core.c | 19 +++++++++++++++++++
>> > > > > >  1 file changed, 19 insertions(+)
>> > > > > > 
>> > > > > > diff --git a/drivers/mtd/parsers/ofpart_core.c b/drivers/mtd/parsers/ofpart_core.c
>> > > > > > index 192190c42fc8..e7b8e9d0a910 100644
>> > > > > > --- a/drivers/mtd/parsers/ofpart_core.c
>> > > > > > +++ b/drivers/mtd/parsers/ofpart_core.c
>> > > > > > @@ -122,6 +122,25 @@ static int parse_fixed_partitions(struct mtd_info *master,
>> > > > > >  
>> > > > > >  		a_cells = of_n_addr_cells(pp);
>> > > > > >  		s_cells = of_n_size_cells(pp);
>> > > > > > +		if (!dedicated && s_cells == 0) {
>> > > > > > +			/*
>> > > > > > +			 * This is a ugly workaround to not create
>> > > > > > +			 * regression on devices that are still creating
>> > > > > > +			 * partitions as direct children of the nand controller.
>> > > > > > +			 * This can happen in case the nand controller node has
>> > > > > > +			 * #size-cells equal to 0 and the firmware (e.g.
>> > > > > > +			 * U-Boot) just add the partitions there assuming
>> > > > > > +			 * 32-bit addressing.
>> > > > > > +			 *
>> > > > > > +			 * If you get this warning your firmware and/or DTS
>> > > > > > +			 * should be really fixed.
>> > > > > > +			 *
>> > > > > > +			 * This is working only for devices smaller than 4GiB.
>> > > > > > +			 */
>> > > > > > +			pr_warn("%s: ofpart partition %pOF (%pOF) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.\n",
>> > > > > > +				master->name, pp, mtd_node);    
>> > > > > 
>> > > > > This is a driver, always use dev_*() calls, not pr_*() calls so that we
>> > > > > know what is being referred to exactly.    
>> > > > 
>> > > > Is this reasonable here? Where can I get the struct device?    
>> > > 
>> > > Walk back up the call chain, there has to be a device somewhere
>> > > controlling this, right?
>> > >   
>> > > > In general this file uses only pr_* debug API and messages are about OF
>> > > > nodes/properties, not about a device.    
>> > > 
>> > > OF nodes and properties are part of a device's properties :)  
>> > 
>> > Yes but the warning comes from a wrong DT description, hence it felt
>> > better suited to warn against the node name which is easily identifiable
>> > in a text file and must be fixed rather than the device which is a pure
>> > software component.
>> > 
>> > Anyway, Francesco, please show us the resultant line and if it feels
>> > meaningful enough we'll take the dev_warn approach.  
>> 
>> So, I tried, but I guess I failed.
>> 
>> Both
>> 
>>   dev_warn(&mtd_get_master(master)->dev, ...);
>> 
>> and
>> 
>>   dev_warn(&master->dev, ...);
>> 
>> are NULL.
>
>mtd->dev (in raw NAND) is populated by the controller drivers, so the
>master mtd device is pointing to the bus "struct device" in its
>dev.parent field. This happens at the end of the probe of the
>controller, after setting the dev entry, so we expect the name of the
>controller to appear.
>
>> (null): gpmi-nand: ofpart partition /soc/nand-controller@33002000/partition@0 (/soc/nand-controller@33002000) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.
>
>Second field looks right, first field does not (bus or class id?) I
>have no idea why it has not been populated at this point (end of the
>controller probe). But it's not a big deal, at least we have the device
>name, so it's ok for me.

If I understand correctly you are fine with the current patch v4 that just print (on colibri-imx7)


gpmi-nand: ofpart partition /soc/nand-controller@33002000/partition@0 (/soc/nand-controller@33002000) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.

Correct? Do you want a v5 with the reviewed-by greg removed or you remove it yourself?

Francesco

Re: [PATCH v4] mtd: parsers: ofpart: add workaround for #size-cells 0

[Miquel Raynal]

Hi Francesco,

francesco@dolcini.it wrote on Fri, 03 Feb 2023 19:03:27 +0100:

> Il 3 febbraio 2023 16:12:02 CET, Miquel Raynal <miquel.raynal@bootlin.com> ha scritto:
> >Hi Francesco,
> >
> >francesco@dolcini.it wrote on Thu, 2 Feb 2023 12:33:34 +0100:
> >  
> >> On Thu, Jan 26, 2023 at 10:12:04AM +0100, Miquel Raynal wrote:  
> >> > gregkh@linuxfoundation.org wrote on Thu, 26 Jan 2023 10:01:02 +0100:
> >> >     
> >> > > On Wed, Jan 25, 2023 at 10:06:57PM +0100, Francesco Dolcini wrote:    
> >> > > > Hello Miquel, Greg and all
> >> > > > 
> >> > > > On Tue, Jan 24, 2023 at 04:38:59PM +0100, Greg Kroah-Hartman wrote:      
> >> > > > > On Tue, Jan 24, 2023 at 11:44:44AM +0100, Francesco Dolcini wrote:      
> >> > > > > > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> >> > > > > > 
> >> > > > > > Add a mechanism to handle the case in which partitions are present as
> >> > > > > > direct child of the nand controller node and #size-cells is set to <0>.
> >> > > > > > 
> >> > > > > > This could happen if the nand-controller node in the DTS is supposed to
> >> > > > > > have #size-cells set to 0, but for some historical reason/bug it was set
> >> > > > > > to 1 in the past, and the firmware (e.g. U-Boot) is adding the partition
> >> > > > > > as direct children of the nand-controller defaulting to #size-cells
> >> > > > > > being to 1.
> >> > > > > > 
> >> > > > > > This prevents a real boot failure on colibri-imx7 that happened during v6.1
> >> > > > > > development cycles.
> >> > > > > > 
> >> > > > > > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> >> > > > > > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> >> > > > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> >> > > > > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> > > > > > ---
> >> > > > > > I do not expect this patch to be backported to stable, however I would expect
> >> > > > > > that we do not backport nand-controller dts cleanups neither.
> >> > > > > > 
> >> > > > > > v4:
> >> > > > > >  fixed wrong English spelling in the comment
> >> > > > > > 
> >> > > > > > v3:
> >> > > > > >  minor formatting change, removed not needed new-line and space. 
> >> > > > > > 
> >> > > > > > v2:
> >> > > > > >  fixup size-cells only when partitions are direct children of the nand-controller
> >> > > > > >  completely revised commit message, comments and warning print
> >> > > > > >  use pr_warn instead of pr_warn_once
> >> > > > > >  added Reviewed-by Greg
> >> > > > > >  removed cc:stable@ and fixes tag, since the problematic commit was reverted
> >> > > > > > ---
> >> > > > > >  drivers/mtd/parsers/ofpart_core.c | 19 +++++++++++++++++++
> >> > > > > >  1 file changed, 19 insertions(+)
> >> > > > > > 
> >> > > > > > diff --git a/drivers/mtd/parsers/ofpart_core.c b/drivers/mtd/parsers/ofpart_core.c
> >> > > > > > index 192190c42fc8..e7b8e9d0a910 100644
> >> > > > > > --- a/drivers/mtd/parsers/ofpart_core.c
> >> > > > > > +++ b/drivers/mtd/parsers/ofpart_core.c
> >> > > > > > @@ -122,6 +122,25 @@ static int parse_fixed_partitions(struct mtd_info *master,
> >> > > > > >  
> >> > > > > >  		a_cells = of_n_addr_cells(pp);
> >> > > > > >  		s_cells = of_n_size_cells(pp);
> >> > > > > > +		if (!dedicated && s_cells == 0) {
> >> > > > > > +			/*
> >> > > > > > +			 * This is a ugly workaround to not create
> >> > > > > > +			 * regression on devices that are still creating
> >> > > > > > +			 * partitions as direct children of the nand controller.
> >> > > > > > +			 * This can happen in case the nand controller node has
> >> > > > > > +			 * #size-cells equal to 0 and the firmware (e.g.
> >> > > > > > +			 * U-Boot) just add the partitions there assuming
> >> > > > > > +			 * 32-bit addressing.
> >> > > > > > +			 *
> >> > > > > > +			 * If you get this warning your firmware and/or DTS
> >> > > > > > +			 * should be really fixed.
> >> > > > > > +			 *
> >> > > > > > +			 * This is working only for devices smaller than 4GiB.
> >> > > > > > +			 */
> >> > > > > > +			pr_warn("%s: ofpart partition %pOF (%pOF) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.\n",
> >> > > > > > +				master->name, pp, mtd_node);      
> >> > > > > 
> >> > > > > This is a driver, always use dev_*() calls, not pr_*() calls so that we
> >> > > > > know what is being referred to exactly.      
> >> > > > 
> >> > > > Is this reasonable here? Where can I get the struct device?      
> >> > > 
> >> > > Walk back up the call chain, there has to be a device somewhere
> >> > > controlling this, right?
> >> > >     
> >> > > > In general this file uses only pr_* debug API and messages are about OF
> >> > > > nodes/properties, not about a device.      
> >> > > 
> >> > > OF nodes and properties are part of a device's properties :)    
> >> > 
> >> > Yes but the warning comes from a wrong DT description, hence it felt
> >> > better suited to warn against the node name which is easily identifiable
> >> > in a text file and must be fixed rather than the device which is a pure
> >> > software component.
> >> > 
> >> > Anyway, Francesco, please show us the resultant line and if it feels
> >> > meaningful enough we'll take the dev_warn approach.    
> >> 
> >> So, I tried, but I guess I failed.
> >> 
> >> Both
> >> 
> >>   dev_warn(&mtd_get_master(master)->dev, ...);
> >> 
> >> and
> >> 
> >>   dev_warn(&master->dev, ...);
> >> 
> >> are NULL.  
> >
> >mtd->dev (in raw NAND) is populated by the controller drivers, so the
> >master mtd device is pointing to the bus "struct device" in its
> >dev.parent field. This happens at the end of the probe of the
> >controller, after setting the dev entry, so we expect the name of the
> >controller to appear.
> >  
> >> (null): gpmi-nand: ofpart partition /soc/nand-controller@33002000/partition@0 (/soc/nand-controller@33002000) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.  
> >
> >Second field looks right, first field does not (bus or class id?) I
> >have no idea why it has not been populated at this point (end of the
> >controller probe). But it's not a big deal, at least we have the device
> >name, so it's ok for me.  
> 
> If I understand correctly you are fine with the current patch v4 that just print (on colibri-imx7)

Well, yes I was fine with it, but no that's not what I meant.

AFAIS, using dev_warn() starts with "(null): gpmi-nand:" while pr_warn
started with "gpmi-nand:", so for me the main and relevant information
is present, that's all what I care about.

Greg cared about the API used so I believe he would prefer us to use
dev_warn().

It is unusual to apply a patch on which someone active in the community
answered negatively as long as his concerns can be addressed, so I
would like Greg to either state that he is fine with the pr_warn or you
to send a v5 using dev_warn() and him to send his R-by back.

If this happens early next week I'll take the patch for my next PR.

> gpmi-nand: ofpart partition /soc/nand-controller@33002000/partition@0 (/soc/nand-controller@33002000) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.
> 
> Correct? Do you want a v5 with the reviewed-by greg removed or you remove it yourself?
> 
> Francesco
> 


Thanks,
Miquèl

Re: [PATCH v4] mtd: parsers: ofpart: add workaround for #size-cells 0

[Francesco Dolcini]

Il 3 febbraio 2023 19:16:23 CET, Miquel Raynal <miquel.raynal@bootlin.com> ha scritto:
>Hi Francesco,
>
>francesco@dolcini.it wrote on Fri, 03 Feb 2023 19:03:27 +0100:
>
>> Il 3 febbraio 2023 16:12:02 CET, Miquel Raynal <miquel.raynal@bootlin.com> ha scritto:
>> >Hi Francesco,
>> >
>> >francesco@dolcini.it wrote on Thu, 2 Feb 2023 12:33:34 +0100:
>> >  
>> >> On Thu, Jan 26, 2023 at 10:12:04AM +0100, Miquel Raynal wrote:  
>> >> > gregkh@linuxfoundation.org wrote on Thu, 26 Jan 2023 10:01:02 +0100:
>> >> >     
>> >> > > On Wed, Jan 25, 2023 at 10:06:57PM +0100, Francesco Dolcini wrote:    
>> >> > > > Hello Miquel, Greg and all
>> >> > > > 
>> >> > > > On Tue, Jan 24, 2023 at 04:38:59PM +0100, Greg Kroah-Hartman wrote:      
>> >> > > > > On Tue, Jan 24, 2023 at 11:44:44AM +0100, Francesco Dolcini wrote:      
>> >> > > > > > From: Francesco Dolcini <francesco.dolcini@toradex.com>
>> >> > > > > > 
>> >> > > > > > Add a mechanism to handle the case in which partitions are present as
>> >> > > > > > direct child of the nand controller node and #size-cells is set to <0>.
>> >> > > > > > 
>> >> > > > > > This could happen if the nand-controller node in the DTS is supposed to
>> >> > > > > > have #size-cells set to 0, but for some historical reason/bug it was set
>> >> > > > > > to 1 in the past, and the firmware (e.g. U-Boot) is adding the partition
>> >> > > > > > as direct children of the nand-controller defaulting to #size-cells
>> >> > > > > > being to 1.
>> >> > > > > > 
>> >> > > > > > This prevents a real boot failure on colibri-imx7 that happened during v6.1
>> >> > > > > > development cycles.
>> >> > > > > > 
>> >> > > > > > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
>> >> > > > > > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
>> >> > > > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
>> >> > > > > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> >> > > > > > ---
>> >> > > > > > I do not expect this patch to be backported to stable, however I would expect
>> >> > > > > > that we do not backport nand-controller dts cleanups neither.
>> >> > > > > > 
>> >> > > > > > v4:
>> >> > > > > >  fixed wrong English spelling in the comment
>> >> > > > > > 
>> >> > > > > > v3:
>> >> > > > > >  minor formatting change, removed not needed new-line and space. 
>> >> > > > > > 
>> >> > > > > > v2:
>> >> > > > > >  fixup size-cells only when partitions are direct children of the nand-controller
>> >> > > > > >  completely revised commit message, comments and warning print
>> >> > > > > >  use pr_warn instead of pr_warn_once
>> >> > > > > >  added Reviewed-by Greg
>> >> > > > > >  removed cc:stable@ and fixes tag, since the problematic commit was reverted
>> >> > > > > > ---
>> >> > > > > >  drivers/mtd/parsers/ofpart_core.c | 19 +++++++++++++++++++
>> >> > > > > >  1 file changed, 19 insertions(+)
>> >> > > > > > 
>> >> > > > > > diff --git a/drivers/mtd/parsers/ofpart_core.c b/drivers/mtd/parsers/ofpart_core.c
>> >> > > > > > index 192190c42fc8..e7b8e9d0a910 100644
>> >> > > > > > --- a/drivers/mtd/parsers/ofpart_core.c
>> >> > > > > > +++ b/drivers/mtd/parsers/ofpart_core.c
>> >> > > > > > @@ -122,6 +122,25 @@ static int parse_fixed_partitions(struct mtd_info *master,
>> >> > > > > >  
>> >> > > > > >  		a_cells = of_n_addr_cells(pp);
>> >> > > > > >  		s_cells = of_n_size_cells(pp);
>> >> > > > > > +		if (!dedicated && s_cells == 0) {
>> >> > > > > > +			/*
>> >> > > > > > +			 * This is a ugly workaround to not create
>> >> > > > > > +			 * regression on devices that are still creating
>> >> > > > > > +			 * partitions as direct children of the nand controller.
>> >> > > > > > +			 * This can happen in case the nand controller node has
>> >> > > > > > +			 * #size-cells equal to 0 and the firmware (e.g.
>> >> > > > > > +			 * U-Boot) just add the partitions there assuming
>> >> > > > > > +			 * 32-bit addressing.
>> >> > > > > > +			 *
>> >> > > > > > +			 * If you get this warning your firmware and/or DTS
>> >> > > > > > +			 * should be really fixed.
>> >> > > > > > +			 *
>> >> > > > > > +			 * This is working only for devices smaller than 4GiB.
>> >> > > > > > +			 */
>> >> > > > > > +			pr_warn("%s: ofpart partition %pOF (%pOF) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.\n",
>> >> > > > > > +				master->name, pp, mtd_node);      
>> >> > > > > 
>> >> > > > > This is a driver, always use dev_*() calls, not pr_*() calls so that we
>> >> > > > > know what is being referred to exactly.      
>> >> > > > 
>> >> > > > Is this reasonable here? Where can I get the struct device?      
>> >> > > 
>> >> > > Walk back up the call chain, there has to be a device somewhere
>> >> > > controlling this, right?
>> >> > >     
>> >> > > > In general this file uses only pr_* debug API and messages are about OF
>> >> > > > nodes/properties, not about a device.      
>> >> > > 
>> >> > > OF nodes and properties are part of a device's properties :)    
>> >> > 
>> >> > Yes but the warning comes from a wrong DT description, hence it felt
>> >> > better suited to warn against the node name which is easily identifiable
>> >> > in a text file and must be fixed rather than the device which is a pure
>> >> > software component.
>> >> > 
>> >> > Anyway, Francesco, please show us the resultant line and if it feels
>> >> > meaningful enough we'll take the dev_warn approach.    
>> >> 
>> >> So, I tried, but I guess I failed.
>> >> 
>> >> Both
>> >> 
>> >>   dev_warn(&mtd_get_master(master)->dev, ...);
>> >> 
>> >> and
>> >> 
>> >>   dev_warn(&master->dev, ...);
>> >> 
>> >> are NULL.  
>> >
>> >mtd->dev (in raw NAND) is populated by the controller drivers, so the
>> >master mtd device is pointing to the bus "struct device" in its
>> >dev.parent field. This happens at the end of the probe of the
>> >controller, after setting the dev entry, so we expect the name of the
>> >controller to appear.
>> >  
>> >> (null): gpmi-nand: ofpart partition /soc/nand-controller@33002000/partition@0 (/soc/nand-controller@33002000) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.  
>> >
>> >Second field looks right, first field does not (bus or class id?) I
>> >have no idea why it has not been populated at this point (end of the
>> >controller probe). But it's not a big deal, at least we have the device
>> >name, so it's ok for me.  
>> 
>> If I understand correctly you are fine with the current patch v4 that just print (on colibri-imx7)
>
>Well, yes I was fine with it, but no that's not what I meant.
>
>AFAIS, using dev_warn() starts with "(null): gpmi-nand:" while pr_warn
>started with "gpmi-nand:", so for me the main and relevant information
>is present, that's all what I care about.
>
>Greg cared about the API used so I believe he would prefer us to use
>dev_warn().
>
>It is unusual to apply a patch on which someone active in the community
>answered negatively as long as his concerns can be addressed, so I
>would like Greg to either state that he is fine with the pr_warn or you
>to send a v5 using dev_warn() and him to send his R-by back.

Perfect, I understand and agree.

I was somehow puzzled because changing to dev_warn() on this specific warning message while the whole file already uses pr_warn() just to get a "(null):" prefix in the logs seems a little pointless to me.

Francesco 

Re: [PATCH v4] mtd: parsers: ofpart: add workaround for #size-cells 0

[Greg Kroah-Hartman]

On Fri, Feb 03, 2023 at 07:37:03PM +0100, Francesco Dolcini wrote:
> 
> 
> Il 3 febbraio 2023 19:16:23 CET, Miquel Raynal <miquel.raynal@bootlin.com> ha scritto:
> >Hi Francesco,
> >
> >francesco@dolcini.it wrote on Fri, 03 Feb 2023 19:03:27 +0100:
> >
> >> Il 3 febbraio 2023 16:12:02 CET, Miquel Raynal <miquel.raynal@bootlin.com> ha scritto:
> >> >Hi Francesco,
> >> >
> >> >francesco@dolcini.it wrote on Thu, 2 Feb 2023 12:33:34 +0100:
> >> >  
> >> >> On Thu, Jan 26, 2023 at 10:12:04AM +0100, Miquel Raynal wrote:  
> >> >> > gregkh@linuxfoundation.org wrote on Thu, 26 Jan 2023 10:01:02 +0100:
> >> >> >     
> >> >> > > On Wed, Jan 25, 2023 at 10:06:57PM +0100, Francesco Dolcini wrote:    
> >> >> > > > Hello Miquel, Greg and all
> >> >> > > > 
> >> >> > > > On Tue, Jan 24, 2023 at 04:38:59PM +0100, Greg Kroah-Hartman wrote:      
> >> >> > > > > On Tue, Jan 24, 2023 at 11:44:44AM +0100, Francesco Dolcini wrote:      
> >> >> > > > > > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> >> >> > > > > > 
> >> >> > > > > > Add a mechanism to handle the case in which partitions are present as
> >> >> > > > > > direct child of the nand controller node and #size-cells is set to <0>.
> >> >> > > > > > 
> >> >> > > > > > This could happen if the nand-controller node in the DTS is supposed to
> >> >> > > > > > have #size-cells set to 0, but for some historical reason/bug it was set
> >> >> > > > > > to 1 in the past, and the firmware (e.g. U-Boot) is adding the partition
> >> >> > > > > > as direct children of the nand-controller defaulting to #size-cells
> >> >> > > > > > being to 1.
> >> >> > > > > > 
> >> >> > > > > > This prevents a real boot failure on colibri-imx7 that happened during v6.1
> >> >> > > > > > development cycles.
> >> >> > > > > > 
> >> >> > > > > > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> >> >> > > > > > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> >> >> > > > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> >> >> > > > > > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> >> > > > > > ---
> >> >> > > > > > I do not expect this patch to be backported to stable, however I would expect
> >> >> > > > > > that we do not backport nand-controller dts cleanups neither.
> >> >> > > > > > 
> >> >> > > > > > v4:
> >> >> > > > > >  fixed wrong English spelling in the comment
> >> >> > > > > > 
> >> >> > > > > > v3:
> >> >> > > > > >  minor formatting change, removed not needed new-line and space. 
> >> >> > > > > > 
> >> >> > > > > > v2:
> >> >> > > > > >  fixup size-cells only when partitions are direct children of the nand-controller
> >> >> > > > > >  completely revised commit message, comments and warning print
> >> >> > > > > >  use pr_warn instead of pr_warn_once
> >> >> > > > > >  added Reviewed-by Greg
> >> >> > > > > >  removed cc:stable@ and fixes tag, since the problematic commit was reverted
> >> >> > > > > > ---
> >> >> > > > > >  drivers/mtd/parsers/ofpart_core.c | 19 +++++++++++++++++++
> >> >> > > > > >  1 file changed, 19 insertions(+)
> >> >> > > > > > 
> >> >> > > > > > diff --git a/drivers/mtd/parsers/ofpart_core.c b/drivers/mtd/parsers/ofpart_core.c
> >> >> > > > > > index 192190c42fc8..e7b8e9d0a910 100644
> >> >> > > > > > --- a/drivers/mtd/parsers/ofpart_core.c
> >> >> > > > > > +++ b/drivers/mtd/parsers/ofpart_core.c
> >> >> > > > > > @@ -122,6 +122,25 @@ static int parse_fixed_partitions(struct mtd_info *master,
> >> >> > > > > >  
> >> >> > > > > >  		a_cells = of_n_addr_cells(pp);
> >> >> > > > > >  		s_cells = of_n_size_cells(pp);
> >> >> > > > > > +		if (!dedicated && s_cells == 0) {
> >> >> > > > > > +			/*
> >> >> > > > > > +			 * This is a ugly workaround to not create
> >> >> > > > > > +			 * regression on devices that are still creating
> >> >> > > > > > +			 * partitions as direct children of the nand controller.
> >> >> > > > > > +			 * This can happen in case the nand controller node has
> >> >> > > > > > +			 * #size-cells equal to 0 and the firmware (e.g.
> >> >> > > > > > +			 * U-Boot) just add the partitions there assuming
> >> >> > > > > > +			 * 32-bit addressing.
> >> >> > > > > > +			 *
> >> >> > > > > > +			 * If you get this warning your firmware and/or DTS
> >> >> > > > > > +			 * should be really fixed.
> >> >> > > > > > +			 *
> >> >> > > > > > +			 * This is working only for devices smaller than 4GiB.
> >> >> > > > > > +			 */
> >> >> > > > > > +			pr_warn("%s: ofpart partition %pOF (%pOF) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.\n",
> >> >> > > > > > +				master->name, pp, mtd_node);      
> >> >> > > > > 
> >> >> > > > > This is a driver, always use dev_*() calls, not pr_*() calls so that we
> >> >> > > > > know what is being referred to exactly.      
> >> >> > > > 
> >> >> > > > Is this reasonable here? Where can I get the struct device?      
> >> >> > > 
> >> >> > > Walk back up the call chain, there has to be a device somewhere
> >> >> > > controlling this, right?
> >> >> > >     
> >> >> > > > In general this file uses only pr_* debug API and messages are about OF
> >> >> > > > nodes/properties, not about a device.      
> >> >> > > 
> >> >> > > OF nodes and properties are part of a device's properties :)    
> >> >> > 
> >> >> > Yes but the warning comes from a wrong DT description, hence it felt
> >> >> > better suited to warn against the node name which is easily identifiable
> >> >> > in a text file and must be fixed rather than the device which is a pure
> >> >> > software component.
> >> >> > 
> >> >> > Anyway, Francesco, please show us the resultant line and if it feels
> >> >> > meaningful enough we'll take the dev_warn approach.    
> >> >> 
> >> >> So, I tried, but I guess I failed.
> >> >> 
> >> >> Both
> >> >> 
> >> >>   dev_warn(&mtd_get_master(master)->dev, ...);
> >> >> 
> >> >> and
> >> >> 
> >> >>   dev_warn(&master->dev, ...);
> >> >> 
> >> >> are NULL.  
> >> >
> >> >mtd->dev (in raw NAND) is populated by the controller drivers, so the
> >> >master mtd device is pointing to the bus "struct device" in its
> >> >dev.parent field. This happens at the end of the probe of the
> >> >controller, after setting the dev entry, so we expect the name of the
> >> >controller to appear.
> >> >  
> >> >> (null): gpmi-nand: ofpart partition /soc/nand-controller@33002000/partition@0 (/soc/nand-controller@33002000) #size-cells is wrongly set to <0>, assuming <1> for parsing partitions.  
> >> >
> >> >Second field looks right, first field does not (bus or class id?) I
> >> >have no idea why it has not been populated at this point (end of the
> >> >controller probe). But it's not a big deal, at least we have the device
> >> >name, so it's ok for me.  
> >> 
> >> If I understand correctly you are fine with the current patch v4 that just print (on colibri-imx7)
> >
> >Well, yes I was fine with it, but no that's not what I meant.
> >
> >AFAIS, using dev_warn() starts with "(null): gpmi-nand:" while pr_warn
> >started with "gpmi-nand:", so for me the main and relevant information
> >is present, that's all what I care about.
> >
> >Greg cared about the API used so I believe he would prefer us to use
> >dev_warn().
> >
> >It is unusual to apply a patch on which someone active in the community
> >answered negatively as long as his concerns can be addressed, so I
> >would like Greg to either state that he is fine with the pr_warn or you
> >to send a v5 using dev_warn() and him to send his R-by back.
> 
> Perfect, I understand and agree.
> 
> I was somehow puzzled because changing to dev_warn() on this specific warning message while the whole file already uses pr_warn() just to get a "(null):" prefix in the logs seems a little pointless to me.

Yeah, if the whole rest of the file is this way, a pr_warn() is fine for
now to solve the real problem.  You can resolve the use of dev_warn()
later in a follow-on patch once the (null) issues get resolved (as
that's obviously not ok...)

thanks,

greg k-h

Re: [PATCH v4] mtd: parsers: ofpart: add workaround for #size-cells 0

[Miquel Raynal]

On Tue, 2023-01-24 at 10:44:44 UTC, Francesco Dolcini wrote:
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> Add a mechanism to handle the case in which partitions are present as
> direct child of the nand controller node and #size-cells is set to <0>.
> 
> This could happen if the nand-controller node in the DTS is supposed to
> have #size-cells set to 0, but for some historical reason/bug it was set
> to 1 in the past, and the firmware (e.g. U-Boot) is adding the partition
> as direct children of the nand-controller defaulting to #size-cells
> being to 1.
> 
> This prevents a real boot failure on colibri-imx7 that happened during v6.1
> development cycles.
> 
> Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/
> Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks.

Miquel