On Sun, 18 Mar 2018 05:51:50 PDT (-0700), robh@xxxxxxxxxx wrote:
On Wed, Mar 14, 2018 at 09:31:05AM -0700, Palmer Dabbelt wrote:
Systems that boot without a chosen node in the device tree should still
respect the command lines that are built into the kernel. This patch
avoids bailing out of the command line argument parsing code when there
is no chosen node in the device tree. This is necessary to boot
straight to a root file system (ie, without an initramfs)
The intent here is that the only functional change is to copy
CONFIG_CMDLINE into data if both of them are valid (ie, CONFIG_CMDLINE
is defined and data is non-null) on systems where there is no chosen
device tree node. I don't actually know what the return values do so I
just preserved them.
Thanks to Trung and Moritz for finding the bug during our ELC hackathon
(and providing the first fix), and Michal for suggesting this fix (which
is cleaner than what I was doing). I've given this very minimal
testing: it fixes the RISC-V bug (in conjunction with a patch to move
from COMMANDLINE_OVERRIDE to COMMANDLINE_FORCE), still works on systems
with and without the chosen node, and builds on ARM64.
CC: Michael J Clark <mjc@xxxxxxxxxx>
CC: Trung Tran <trung.tran@xxxxxxxxx>
CC: Moritz Fischer <mdf@xxxxxxxxxx>
Signed-off-by: Palmer Dabbelt <palmer@xxxxxxxxxx>
---
drivers/of/fdt.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 84aa9d676375..60241b1cb024 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1084,9 +1084,12 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
pr_debug("search \"chosen\", depth: %d, uname: %s\n", depth, uname);
- if (depth != 1 || !data ||
+ if (!data)
+ goto no_data;
Just "return 0" here.
+
+ if (depth != 1 ||
(strcmp(uname, "chosen") != 0 && strcmp(uname, "chosen@0") != 0))
- return 0;
+ goto no_chosen;
early_init_dt_check_for_initrd(node);
@@ -1117,6 +1120,13 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
/* break now */
return 1;
+
+no_chosen:
+#ifdef CONFIG_CMDLINE
+ strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
Best case, this is going to needlessly copy the string on every single
node that is not /chosen.
Worst case, I think this changes behavior. For example, first you copy
CONFIG_CMDLINE into data, then on a later iteration, you strlcat
CONFIG_CMDLINE into data if CONFIG_CMDLINE_EXTEND is enable. There could
also be some unintended behavior if data has a string to start with.
Ah, sorry, I guess I wasn't paying attention -- I assumed this was only called
for the relevant node (like the rest of the device tree stuff), and that chosel
was just somewhere inside it.
I'd really like to see this function re-written to just find the /chosen
node and then handle each property one by one. The iterating approach is
silly. I assume it predates libfdt and we didn't have nice functions to
find nodes by path (or any other way).
I'm working on a patch to re-structure this function. Will send it out
in the next day.
+#endif
+no_data:
+ return 0;
}
#ifdef CONFIG_HAVE_MEMBLOCK
--
2.16.1
Thanks. Do you mind CCing me on it? Then I'll figure out a way to make sure
our command lines still work.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
