Re: [PATCH] m68k: fix command-line parsing when passed from u-boot
Hi Angelo,
On 05/09/18 05:41, Angelo Dureghello wrote:
please drop this patch.
Issue is not related to additional spaces in the command
line, nor to init.data initializaitons.
It seems qiute hard to track down, issue disappears if i
just add some lines of code in uboot.c or setup.c.
Thanks for the update. Will do.
Regards
Greg
Continuing on this.
Regards,
Angelo Dureghello
On Sat, Sep 01, 2018 at 03:16:21AM +0200, Angelo Dureghello wrote:
Without MMU, when CONFIG_UBOOT is set, and CONFIG_BOOTPARAM
is not set, a wrong command-line was produced (boot hangs,
no console), due to an initial erroneus space appended to the
command line in process_uboot_commandline().
In MMU mode, the m68k_command_line array was not initially
terminated to zero, and process_uboot_commandline() was still
producing an invalid command-line (boot hangs, no console).
Signed-off-by: Angelo Dureghello
---
arch/m68k/kernel/setup_mm.c | 1 +
arch/m68k/kernel/setup_no.c | 2 ++
arch/m68k/kernel/uboot.c| 16 ++--
3 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/arch/m68k/kernel/setup_mm.c b/arch/m68k/kernel/setup_mm.c
index 5d3596c180f9..8fc2999f11fe 100644
--- a/arch/m68k/kernel/setup_mm.c
+++ b/arch/m68k/kernel/setup_mm.c
@@ -265,6 +265,7 @@ void __init setup_arch(char **cmdline_p)
init_mm.end_data = (unsigned long)_edata;
init_mm.brk = (unsigned long)_end;
+ m68k_command_line[0] = 0;
#if defined(CONFIG_BOOTPARAM)
strncpy(m68k_command_line, CONFIG_BOOTPARAM_STRING, CL_SIZE);
m68k_command_line[CL_SIZE - 1] = 0;
diff --git a/arch/m68k/kernel/setup_no.c b/arch/m68k/kernel/setup_no.c
index cfd5475bfc31..d65bb433583c 100644
--- a/arch/m68k/kernel/setup_no.c
+++ b/arch/m68k/kernel/setup_no.c
@@ -94,6 +94,8 @@ void __init setup_arch(char **cmdline_p)
init_mm.end_data = (unsigned long) &_edata;
init_mm.brk = (unsigned long) 0;
+ command_line[0] = 0;
+
config_BSP(_line[0], sizeof(command_line));
#if defined(CONFIG_BOOTPARAM)
diff --git a/arch/m68k/kernel/uboot.c b/arch/m68k/kernel/uboot.c
index b29c3b241e1b..c4045bbe0a8c 100644
--- a/arch/m68k/kernel/uboot.c
+++ b/arch/m68k/kernel/uboot.c
@@ -92,13 +92,17 @@ __init void process_uboot_commandline(char *commandp, int
size)
{
int len, n;
+ len = size;
+
n = strnlen(commandp, size);
- commandp += n;
- len = size - n;
- if (len) {
- /* Add the whitespace separator */
- *commandp++ = ' ';
- len--;
+ if (n) {
+ commandp += n;
+ len -= n;
+ if (len) {
+ /* Add the whitespace separator */
+ *commandp++ = ' ';
+ len--;
+ }
}
parse_uboot_commandline(commandp, len);
--
2.18.0
Re: [PATCH] m68k: fix command-line parsing when passed from u-boot
Hi Greg,
please drop this patch.
Issue is not related to additional spaces in the command
line, nor to init.data initializaitons.
It seems qiute hard to track down, issue disappears if i
just add some lines of code in uboot.c or setup.c.
Continuing on this.
Regards,
Angelo Dureghello
On Sat, Sep 01, 2018 at 03:16:21AM +0200, Angelo Dureghello wrote:
> Without MMU, when CONFIG_UBOOT is set, and CONFIG_BOOTPARAM
> is not set, a wrong command-line was produced (boot hangs,
> no console), due to an initial erroneus space appended to the
> command line in process_uboot_commandline().
>
> In MMU mode, the m68k_command_line array was not initially
> terminated to zero, and process_uboot_commandline() was still
> producing an invalid command-line (boot hangs, no console).
>
> Signed-off-by: Angelo Dureghello
> ---
> arch/m68k/kernel/setup_mm.c | 1 +
> arch/m68k/kernel/setup_no.c | 2 ++
> arch/m68k/kernel/uboot.c| 16 ++--
> 3 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/arch/m68k/kernel/setup_mm.c b/arch/m68k/kernel/setup_mm.c
> index 5d3596c180f9..8fc2999f11fe 100644
> --- a/arch/m68k/kernel/setup_mm.c
> +++ b/arch/m68k/kernel/setup_mm.c
> @@ -265,6 +265,7 @@ void __init setup_arch(char **cmdline_p)
> init_mm.end_data = (unsigned long)_edata;
> init_mm.brk = (unsigned long)_end;
>
> + m68k_command_line[0] = 0;
> #if defined(CONFIG_BOOTPARAM)
> strncpy(m68k_command_line, CONFIG_BOOTPARAM_STRING, CL_SIZE);
> m68k_command_line[CL_SIZE - 1] = 0;
> diff --git a/arch/m68k/kernel/setup_no.c b/arch/m68k/kernel/setup_no.c
> index cfd5475bfc31..d65bb433583c 100644
> --- a/arch/m68k/kernel/setup_no.c
> +++ b/arch/m68k/kernel/setup_no.c
> @@ -94,6 +94,8 @@ void __init setup_arch(char **cmdline_p)
> init_mm.end_data = (unsigned long) &_edata;
> init_mm.brk = (unsigned long) 0;
>
> + command_line[0] = 0;
> +
> config_BSP(_line[0], sizeof(command_line));
>
> #if defined(CONFIG_BOOTPARAM)
> diff --git a/arch/m68k/kernel/uboot.c b/arch/m68k/kernel/uboot.c
> index b29c3b241e1b..c4045bbe0a8c 100644
> --- a/arch/m68k/kernel/uboot.c
> +++ b/arch/m68k/kernel/uboot.c
> @@ -92,13 +92,17 @@ __init void process_uboot_commandline(char *commandp, int
> size)
> {
> int len, n;
>
> + len = size;
> +
> n = strnlen(commandp, size);
> - commandp += n;
> - len = size - n;
> - if (len) {
> - /* Add the whitespace separator */
> - *commandp++ = ' ';
> - len--;
> + if (n) {
> + commandp += n;
> + len -= n;
> + if (len) {
> + /* Add the whitespace separator */
> + *commandp++ = ' ';
> + len--;
> + }
> }
>
> parse_uboot_commandline(commandp, len);
> --
> 2.18.0
>
Re: [PATCH] m68k: fix command-line parsing when passed from u-boot
Hi Geert,
On Sat, Sep 01, 2018 at 09:23:42AM +0200, Geert Uytterhoeven wrote:
> Hi Angelo,
>
> On Sat, Sep 1, 2018 at 3:17 AM Angelo Dureghello wrote:
> > Without MMU, when CONFIG_UBOOT is set, and CONFIG_BOOTPARAM
> > is not set, a wrong command-line was produced (boot hangs,
> > no console), due to an initial erroneus space appended to the
> > command line in process_uboot_commandline().
> >
> > In MMU mode, the m68k_command_line array was not initially
> > terminated to zero, and process_uboot_commandline() was still
> > producing an invalid command-line (boot hangs, no console).
>
> It should be all zeroes?
>
> > --- a/arch/m68k/kernel/setup_mm.c
> > +++ b/arch/m68k/kernel/setup_mm.c
> > @@ -265,6 +265,7 @@ void __init setup_arch(char **cmdline_p)
> > init_mm.end_data = (unsigned long)_edata;
> > init_mm.brk = (unsigned long)_end;
> >
> > + m68k_command_line[0] = 0;
>
> This should not be needed:
>
> static char m68k_command_line[CL_SIZE] __initdata;
>
> I.e. m68k_command_line[] should be all zeroes.
>
> Is there a bug in the linker script?
>
Ops, right, but without the zero termination the final commandline
results invalid producing no console.
Investigating further so.
Just noticed some other initialization to 0 ...
void (*mach_sched_init) (irq_handler_t handler) __initdata = NULL;
/* machine dependent irq functions */
void (*mach_init_IRQ) (void) __initdata = NULL;
Noticed another strange thing, if i trace the commandline with
if (m68k_command_line[0] != 0)
pr_info("%02x %02x %02x %02x %02x %02x %02x %02x\n",
m68k_command_line[0],
m68k_command_line[1],
m68k_command_line[2],
m68k_command_line[3],
m68k_command_line[4],
m68k_command_line[5],
m68k_command_line[6],
m68k_command_line[7]);
#if defined(CONFIG_BOOTPARAM)
strncpy(m68k_command_line, CONFIG_BOOTPARAM_STRING, CL_SIZE);
...
Then i see all 00 and command line is set correctly and the
system boots. So it seems the fact i just refer to the array fixes
the things ...
> > #if defined(CONFIG_BOOTPARAM)
> > strncpy(m68k_command_line, CONFIG_BOOTPARAM_STRING, CL_SIZE);
> > m68k_command_line[CL_SIZE - 1] = 0;
> > diff --git a/arch/m68k/kernel/setup_no.c b/arch/m68k/kernel/setup_no.c
> > index cfd5475bfc31..d65bb433583c 100644
> > --- a/arch/m68k/kernel/setup_no.c
> > +++ b/arch/m68k/kernel/setup_no.c
> > @@ -94,6 +94,8 @@ void __init setup_arch(char **cmdline_p)
> > init_mm.end_data = (unsigned long) &_edata;
> > init_mm.brk = (unsigned long) 0;
> >
> > + command_line[0] = 0;
>
> Likewise:
>
> char __initdata command_line[COMMAND_LINE_SIZE];
>
I just added this for conformity with the above MMU case. But this issue
is only visible in the CONFIG_MMU.
> > +
> > config_BSP(_line[0], sizeof(command_line));
> >
> > #if defined(CONFIG_BOOTPARAM)
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
Re: [PATCH] m68k: fix command-line parsing when passed from u-boot
Hi Angelo,
On Sat, Sep 1, 2018 at 3:17 AM Angelo Dureghello wrote:
> Without MMU, when CONFIG_UBOOT is set, and CONFIG_BOOTPARAM
> is not set, a wrong command-line was produced (boot hangs,
> no console), due to an initial erroneus space appended to the
> command line in process_uboot_commandline().
>
> In MMU mode, the m68k_command_line array was not initially
> terminated to zero, and process_uboot_commandline() was still
> producing an invalid command-line (boot hangs, no console).
It should be all zeroes?
> --- a/arch/m68k/kernel/setup_mm.c
> +++ b/arch/m68k/kernel/setup_mm.c
> @@ -265,6 +265,7 @@ void __init setup_arch(char **cmdline_p)
> init_mm.end_data = (unsigned long)_edata;
> init_mm.brk = (unsigned long)_end;
>
> + m68k_command_line[0] = 0;
This should not be needed:
static char m68k_command_line[CL_SIZE] __initdata;
I.e. m68k_command_line[] should be all zeroes.
Is there a bug in the linker script?
> #if defined(CONFIG_BOOTPARAM)
> strncpy(m68k_command_line, CONFIG_BOOTPARAM_STRING, CL_SIZE);
> m68k_command_line[CL_SIZE - 1] = 0;
> diff --git a/arch/m68k/kernel/setup_no.c b/arch/m68k/kernel/setup_no.c
> index cfd5475bfc31..d65bb433583c 100644
> --- a/arch/m68k/kernel/setup_no.c
> +++ b/arch/m68k/kernel/setup_no.c
> @@ -94,6 +94,8 @@ void __init setup_arch(char **cmdline_p)
> init_mm.end_data = (unsigned long) &_edata;
> init_mm.brk = (unsigned long) 0;
>
> + command_line[0] = 0;
Likewise:
char __initdata command_line[COMMAND_LINE_SIZE];
> +
> config_BSP(_line[0], sizeof(command_line));
>
> #if defined(CONFIG_BOOTPARAM)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
