| Message ID | 20230206224838.75963-2-francesco@dolcini.it |
|---|---|
| State | New |
| Delegated to: | Dario Binacchi |
| Headers | show |
| Series | fdt: Fix mtparts fixup | expand |
On Mon, Feb 06, 2023 at 11:48:36PM +0100, Francesco Dolcini wrote: > From: Francesco Dolcini <francesco.dolcini@toradex.com> > > Fixup #size-cells value when updating the MTD partitions, this is > required to prevent issues in case the MTD parent set #size-cells to > zero. > This could happen for example in the legacy case in which the partitions > are created as direct child of the mtd node and that specific node has > no children. Recent clean-up on Linux device tree files created a boot > regression on colibri-imx7 [0]. > > This fixup has the limitation to assume 32-bit (#size-cells=1) > addressing, therefore it will not work with device bigger than 4GiB. > > This change also enforce #address-cells to be the same as #size-cells, > this was already silently enforced by fdt_node_set_part_info(), now this > is checked explicitly and partition fixup will just fail in such case. > > When the partition list is static the preferred way to pass the mtd > partition list to the kernel is to either define those in the source DTS > file or use mtdparts= in the command line. > Tweaking the DT from U-Boot should be avoided, unless some dynamic > changes are required, since it proved to be problematic when not > following the evolution of the "standard". > > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/ > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/ > Cc: Marek Vasut <marex@denx.de> > Cc: Miquel Raynal <miquel.raynal@bootlin.com> > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > --- > v2: improved commit message > --- > common/fdt_support.c | 45 ++++++++++++++++++++++++++++++++++---------- > 1 file changed, 35 insertions(+), 10 deletions(-) I'm dropping the linux-mtd list here and adding a bunch more platform maintainers. In general, calling fdt_fixup_mtdparts is the wrong choice. I see we do have a few cases in U-Boot where we're clearly doing something dynamic to the partition table, but it looks like at first glance most callers are using this hook when they should either be having the partition map in the device tree properly (and using one of the appropriate bindings) or passing the map in via the kernel command line. I would like to ask everyone I've added to the list here to please audit your platform and update it as needed. Thanks!
+Stefano On Mon, Feb 06, 2023 at 06:17:31PM -0500, Tom Rini wrote: > On Mon, Feb 06, 2023 at 11:48:36PM +0100, Francesco Dolcini wrote: > > > From: Francesco Dolcini <francesco.dolcini@toradex.com> > > > > Fixup #size-cells value when updating the MTD partitions, this is > > required to prevent issues in case the MTD parent set #size-cells to > > zero. > > This could happen for example in the legacy case in which the partitions > > are created as direct child of the mtd node and that specific node has > > no children. Recent clean-up on Linux device tree files created a boot > > regression on colibri-imx7 [0]. > > > > This fixup has the limitation to assume 32-bit (#size-cells=1) > > addressing, therefore it will not work with device bigger than 4GiB. > > > > This change also enforce #address-cells to be the same as #size-cells, > > this was already silently enforced by fdt_node_set_part_info(), now this > > is checked explicitly and partition fixup will just fail in such case. > > > > When the partition list is static the preferred way to pass the mtd > > partition list to the kernel is to either define those in the source DTS > > file or use mtdparts= in the command line. > > Tweaking the DT from U-Boot should be avoided, unless some dynamic > > changes are required, since it proved to be problematic when not > > following the evolution of the "standard". > > > > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/ > > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/ > > Cc: Marek Vasut <marex@denx.de> > > Cc: Miquel Raynal <miquel.raynal@bootlin.com> > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > --- > > v2: improved commit message > > --- > > common/fdt_support.c | 45 ++++++++++++++++++++++++++++++++++---------- > > 1 file changed, 35 insertions(+), 10 deletions(-) > > I'm dropping the linux-mtd list here and adding a bunch more platform > maintainers. In general, calling fdt_fixup_mtdparts is the wrong choice. > I see we do have a few cases in U-Boot where we're clearly doing > something dynamic to the partition table, but it looks like at first > glance most callers are using this hook when they should either be > having the partition map in the device tree properly (and using one of > the appropriate bindings) or passing the map in via the kernel command > line. I would like to ask everyone I've added to the list here to > please audit your platform and update it as needed. Thanks! Hello Tom, what should we do with this patch? No feedback so far, apart this email from you. Stefano: maybe you can pick patches 2 and 3 ? Francesco
On Tue, Apr 04, 2023 at 06:18:03PM +0200, Francesco Dolcini wrote: > +Stefano > > On Mon, Feb 06, 2023 at 06:17:31PM -0500, Tom Rini wrote: > > On Mon, Feb 06, 2023 at 11:48:36PM +0100, Francesco Dolcini wrote: > > > > > From: Francesco Dolcini <francesco.dolcini@toradex.com> > > > > > > Fixup #size-cells value when updating the MTD partitions, this is > > > required to prevent issues in case the MTD parent set #size-cells to > > > zero. > > > This could happen for example in the legacy case in which the partitions > > > are created as direct child of the mtd node and that specific node has > > > no children. Recent clean-up on Linux device tree files created a boot > > > regression on colibri-imx7 [0]. > > > > > > This fixup has the limitation to assume 32-bit (#size-cells=1) > > > addressing, therefore it will not work with device bigger than 4GiB. > > > > > > This change also enforce #address-cells to be the same as #size-cells, > > > this was already silently enforced by fdt_node_set_part_info(), now this > > > is checked explicitly and partition fixup will just fail in such case. > > > > > > When the partition list is static the preferred way to pass the mtd > > > partition list to the kernel is to either define those in the source DTS > > > file or use mtdparts= in the command line. > > > Tweaking the DT from U-Boot should be avoided, unless some dynamic > > > changes are required, since it proved to be problematic when not > > > following the evolution of the "standard". > > > > > > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/ > > > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/ > > > Cc: Marek Vasut <marex@denx.de> > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com> > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > > --- > > > v2: improved commit message > > > --- > > > common/fdt_support.c | 45 ++++++++++++++++++++++++++++++++++---------- > > > 1 file changed, 35 insertions(+), 10 deletions(-) > > > > I'm dropping the linux-mtd list here and adding a bunch more platform > > maintainers. In general, calling fdt_fixup_mtdparts is the wrong choice. > > I see we do have a few cases in U-Boot where we're clearly doing > > something dynamic to the partition table, but it looks like at first > > glance most callers are using this hook when they should either be > > having the partition map in the device tree properly (and using one of > > the appropriate bindings) or passing the map in via the kernel command > > line. I would like to ask everyone I've added to the list here to > > please audit your platform and update it as needed. Thanks! > > Hello Tom, > what should we do with this patch? No feedback so far, apart this email > from you. > > Stefano: maybe you can pick patches 2 and 3 ? I thought someone chimed in for the STM32 side? But yes, patches 2 and 3 should get picked up for sure.
On Thu, Apr 06, 2023 at 11:25:27AM -0400, Tom Rini wrote: > On Tue, Apr 04, 2023 at 06:18:03PM +0200, Francesco Dolcini wrote: > > +Stefano > > > > On Mon, Feb 06, 2023 at 06:17:31PM -0500, Tom Rini wrote: > > > On Mon, Feb 06, 2023 at 11:48:36PM +0100, Francesco Dolcini wrote: > > > > > > > From: Francesco Dolcini <francesco.dolcini@toradex.com> > > > > > > > > Fixup #size-cells value when updating the MTD partitions, this is > > > > required to prevent issues in case the MTD parent set #size-cells to > > > > zero. > > > > This could happen for example in the legacy case in which the partitions > > > > are created as direct child of the mtd node and that specific node has > > > > no children. Recent clean-up on Linux device tree files created a boot > > > > regression on colibri-imx7 [0]. > > > > > > > > This fixup has the limitation to assume 32-bit (#size-cells=1) > > > > addressing, therefore it will not work with device bigger than 4GiB. > > > > > > > > This change also enforce #address-cells to be the same as #size-cells, > > > > this was already silently enforced by fdt_node_set_part_info(), now this > > > > is checked explicitly and partition fixup will just fail in such case. > > > > > > > > When the partition list is static the preferred way to pass the mtd > > > > partition list to the kernel is to either define those in the source DTS > > > > file or use mtdparts= in the command line. > > > > Tweaking the DT from U-Boot should be avoided, unless some dynamic > > > > changes are required, since it proved to be problematic when not > > > > following the evolution of the "standard". > > > > > > > > Link: https://lore.kernel.org/all/Y4dgBTGNWpM6SQXI@francesco-nb.int.toradex.com/ > > > > Link: https://lore.kernel.org/all/20221202071900.1143950-1-francesco@dolcini.it/ > > > > Cc: Marek Vasut <marex@denx.de> > > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com> > > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > > > --- > > > > v2: improved commit message > > > > --- > > > > common/fdt_support.c | 45 ++++++++++++++++++++++++++++++++++---------- > > > > 1 file changed, 35 insertions(+), 10 deletions(-) > > > > > > I'm dropping the linux-mtd list here and adding a bunch more platform > > > maintainers. In general, calling fdt_fixup_mtdparts is the wrong choice. > > > I see we do have a few cases in U-Boot where we're clearly doing > > > something dynamic to the partition table, but it looks like at first > > > glance most callers are using this hook when they should either be > > > having the partition map in the device tree properly (and using one of > > > the appropriate bindings) or passing the map in via the kernel command > > > line. I would like to ask everyone I've added to the list here to > > > please audit your platform and update it as needed. Thanks! > > > > Hello Tom, > > what should we do with this patch? No feedback so far, apart this email > > from you. > > > > Stefano: maybe you can pick patches 2 and 3 ? > > I thought someone chimed in for the STM32 side? Whoops, yes, I remember the same. However, what about this patch, do we want to merge it? It solves a potential issue and from what I can tell it does not introduce new one. Francesco
diff --git a/common/fdt_support.c b/common/fdt_support.c index dbceec6f2dcc..3aee826e60cf 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -877,27 +877,20 @@ static int fdt_del_partitions(void *blob, int parent_offset) return 0; } -static int fdt_node_set_part_info(void *blob, int parent_offset, +/* This expects #address-cells and #size-cells to have same value */ +static int fdt_node_set_part_info(void *blob, int parent_offset, int sizecell, struct mtd_device *dev) { struct list_head *pentry; struct part_info *part; int off, ndepth = 0; int part_num, ret; - int sizecell; char buf[64]; ret = fdt_del_partitions(blob, parent_offset); if (ret < 0) return ret; - /* - * Check if size/address is 1 or 2 cells. - * We assume #address-cells and #size-cells have same value. - */ - sizecell = fdt_getprop_u32_default_node(blob, parent_offset, - 0, "#size-cells", 1); - /* * Check if it is nand {}; subnode, adjust * the offset in this case @@ -992,6 +985,31 @@ err_prop: return ret; } +static int fdt_mtdparts_cell_cnt(void *fdt, int off) +{ + int sizecell, addrcell; + + sizecell = fdt_getprop_u32_default_node(fdt, off, 0, "#size-cells", 0); + if (sizecell != 1 && sizecell != 2) { + printf("%s: Invalid or missing #size-cells %d value, assuming 1\n", + __func__, sizecell); + + sizecell = 1; + if (fdt_setprop_u32(fdt, off, "#size-cells", sizecell)) + return -1; + } + + addrcell = fdt_getprop_u32_default_node(fdt, off, 0, + "#address-cells", 0); + if (addrcell != sizecell) { + printf("%s: Invalid #address-cells %d != #size-cells %d, aborting\n", + __func__, addrcell, sizecell); + return -1; + } + + return sizecell; +} + /* * Update partitions in nor/nand nodes using info from * mtdparts environment variable. The nodes to update are @@ -1037,12 +1055,19 @@ void fdt_fixup_mtdparts(void *blob, const struct node_info *node_info, dev = device_find(node_info[i].type, idx++); if (dev) { + int cell; + parts = fdt_subnode_offset(blob, noff, "partitions"); if (parts < 0) parts = noff; - if (fdt_node_set_part_info(blob, parts, dev)) + cell = fdt_mtdparts_cell_cnt(blob, parts); + if (cell < 0) + return; + + if (fdt_node_set_part_info(blob, parts, + cell, dev)) return; /* return on error */ } }