| Message ID | 20170807195129.10638-1-robdclark@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
Hi Rob, On Tue, Aug 8, 2017 at 3:51 AM, Rob Clark <robdclark@gmail.com> wrote: > stdin might not be set, which would cause iomux_doenv() to fail > therefore causing probe_usb_keyboard() to fail. Furthermore if we do > have iomux enabled, the sensible thing (in terms of user experience) > would be to simply add ourselves to the list of stdin devices. > > This fixes an issue with usbkbd on dragonboard410c with distro- > bootcmd, where stdin is not set (so stdinname is null). > > Signed-off-by: Rob Clark <robdclark@gmail.com> > --- > v2: address Bin's review comments > v3: fix fail with free()ing if usbkbd is already in stdin env variable > pointed out by Simon > v4: use if (CONFIG_IS_ENABLED(CONSOLE_MUX)) { ... } > > common/usb_kbd.c | 46 +++++++++++++++++++++++++++++++--------------- > include/console.h | 2 -- > 2 files changed, 31 insertions(+), 17 deletions(-) > > diff --git a/common/usb_kbd.c b/common/usb_kbd.c > index d2d29cc98f..8edeb6c4f5 100644 > --- a/common/usb_kbd.c > +++ b/common/usb_kbd.c > @@ -516,23 +516,39 @@ static int probe_usb_keyboard(struct usb_device *dev) > return error; > > stdinname = getenv("stdin"); > -#if CONFIG_IS_ENABLED(CONSOLE_MUX) > - error = iomux_doenv(stdin, stdinname); > - if (error) > - return error; > -#else > - /* Check if this is the standard input device. */ > - if (strcmp(stdinname, DEVNAME)) > - return 1; > + if (CONFIG_IS_ENABLED(CONSOLE_MUX)) { > + char *devname = DEVNAME; > + char *newstdin = NULL; > + /* > + * stdin might not be set yet.. either way, with console- > + * mux the sensible thing to do is add ourselves to the > + * list of stdio devices: > + */ > + if (stdinname && !strstr(stdinname, DEVNAME)) { > + newstdin = malloc(strlen(stdinname) + > + strlen(","DEVNAME) + 1); > + sprintf(newstdin, "%s,"DEVNAME, stdinname); > + stdinname = newstdin; > + } else if (!stdinname) { > + stdinname = devname; > + } > + error = iomux_doenv(stdin, stdinname); > + free(newstdin); Sorry I should have asked this before: isn't free(devname) OK? In previous version, it has a test logic to see whether free is needed. > + if (error) > + return error; > + } else { > + /* Check if this is the standard input device. */ > + if (strcmp(stdinname, DEVNAME)) > + return 1; > > - /* Reassign the console */ > - if (overwrite_console()) > - return 1; > + /* Reassign the console */ > + if (overwrite_console()) > + return 1; > > - error = console_assign(stdin, DEVNAME); > - if (error) > - return error; > -#endif > + error = console_assign(stdin, DEVNAME); > + if (error) > + return error; > + } > > return 0; > } > diff --git a/include/console.h b/include/console.h > index cea29ed6dc..7dfd36d7d1 100644 > --- a/include/console.h > +++ b/include/console.h > @@ -57,8 +57,6 @@ int console_announce_r(void); > /* > * CONSOLE multiplexing. > */ > -#ifdef CONFIG_CONSOLE_MUX > #include <iomux.h> > -#endif > > #endif > -- Regards, Bin
On Tue, Aug 8, 2017 at 6:42 AM, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Rob, > > On Tue, Aug 8, 2017 at 3:51 AM, Rob Clark <robdclark@gmail.com> wrote: >> stdin might not be set, which would cause iomux_doenv() to fail >> therefore causing probe_usb_keyboard() to fail. Furthermore if we do >> have iomux enabled, the sensible thing (in terms of user experience) >> would be to simply add ourselves to the list of stdin devices. >> >> This fixes an issue with usbkbd on dragonboard410c with distro- >> bootcmd, where stdin is not set (so stdinname is null). >> >> Signed-off-by: Rob Clark <robdclark@gmail.com> >> --- >> v2: address Bin's review comments >> v3: fix fail with free()ing if usbkbd is already in stdin env variable >> pointed out by Simon >> v4: use if (CONFIG_IS_ENABLED(CONSOLE_MUX)) { ... } >> >> common/usb_kbd.c | 46 +++++++++++++++++++++++++++++++--------------- >> include/console.h | 2 -- >> 2 files changed, 31 insertions(+), 17 deletions(-) >> >> diff --git a/common/usb_kbd.c b/common/usb_kbd.c >> index d2d29cc98f..8edeb6c4f5 100644 >> --- a/common/usb_kbd.c >> +++ b/common/usb_kbd.c >> @@ -516,23 +516,39 @@ static int probe_usb_keyboard(struct usb_device *dev) >> return error; >> >> stdinname = getenv("stdin"); >> -#if CONFIG_IS_ENABLED(CONSOLE_MUX) >> - error = iomux_doenv(stdin, stdinname); >> - if (error) >> - return error; >> -#else >> - /* Check if this is the standard input device. */ >> - if (strcmp(stdinname, DEVNAME)) >> - return 1; >> + if (CONFIG_IS_ENABLED(CONSOLE_MUX)) { >> + char *devname = DEVNAME; >> + char *newstdin = NULL; >> + /* >> + * stdin might not be set yet.. either way, with console- >> + * mux the sensible thing to do is add ourselves to the >> + * list of stdio devices: >> + */ >> + if (stdinname && !strstr(stdinname, DEVNAME)) { >> + newstdin = malloc(strlen(stdinname) + >> + strlen(","DEVNAME) + 1); >> + sprintf(newstdin, "%s,"DEVNAME, stdinname); >> + stdinname = newstdin; >> + } else if (!stdinname) { >> + stdinname = devname; >> + } >> + error = iomux_doenv(stdin, stdinname); >> + free(newstdin); > > Sorry I should have asked this before: isn't free(devname) OK? In > previous version, it has a test logic to see whether free is needed. it was in the first version of my patch, until I added the &&!strstr() bit to avoid adding usbkbd a second time to stdin if stdin already contained usbkbd. Now we have a case where stdinname is the ptr returned from getenv() which we probably don't want to free() ;-) BR, -R >> + if (error) >> + return error; >> + } else { >> + /* Check if this is the standard input device. */ >> + if (strcmp(stdinname, DEVNAME)) >> + return 1; >> >> - /* Reassign the console */ >> - if (overwrite_console()) >> - return 1; >> + /* Reassign the console */ >> + if (overwrite_console()) >> + return 1; >> >> - error = console_assign(stdin, DEVNAME); >> - if (error) >> - return error; >> -#endif >> + error = console_assign(stdin, DEVNAME); >> + if (error) >> + return error; >> + } >> >> return 0; >> } >> diff --git a/include/console.h b/include/console.h >> index cea29ed6dc..7dfd36d7d1 100644 >> --- a/include/console.h >> +++ b/include/console.h >> @@ -57,8 +57,6 @@ int console_announce_r(void); >> /* >> * CONSOLE multiplexing. >> */ >> -#ifdef CONFIG_CONSOLE_MUX >> #include <iomux.h> >> -#endif >> >> #endif >> -- > > Regards, > Bin
Hi Rob, On Tue, Aug 8, 2017 at 6:48 PM, Rob Clark <robdclark@gmail.com> wrote: > On Tue, Aug 8, 2017 at 6:42 AM, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Rob, >> >> On Tue, Aug 8, 2017 at 3:51 AM, Rob Clark <robdclark@gmail.com> wrote: >>> stdin might not be set, which would cause iomux_doenv() to fail >>> therefore causing probe_usb_keyboard() to fail. Furthermore if we do >>> have iomux enabled, the sensible thing (in terms of user experience) >>> would be to simply add ourselves to the list of stdin devices. >>> >>> This fixes an issue with usbkbd on dragonboard410c with distro- >>> bootcmd, where stdin is not set (so stdinname is null). >>> >>> Signed-off-by: Rob Clark <robdclark@gmail.com> >>> --- >>> v2: address Bin's review comments >>> v3: fix fail with free()ing if usbkbd is already in stdin env variable >>> pointed out by Simon >>> v4: use if (CONFIG_IS_ENABLED(CONSOLE_MUX)) { ... } >>> >>> common/usb_kbd.c | 46 +++++++++++++++++++++++++++++++--------------- >>> include/console.h | 2 -- >>> 2 files changed, 31 insertions(+), 17 deletions(-) >>> >>> diff --git a/common/usb_kbd.c b/common/usb_kbd.c >>> index d2d29cc98f..8edeb6c4f5 100644 >>> --- a/common/usb_kbd.c >>> +++ b/common/usb_kbd.c >>> @@ -516,23 +516,39 @@ static int probe_usb_keyboard(struct usb_device *dev) >>> return error; >>> >>> stdinname = getenv("stdin"); >>> -#if CONFIG_IS_ENABLED(CONSOLE_MUX) >>> - error = iomux_doenv(stdin, stdinname); >>> - if (error) >>> - return error; >>> -#else >>> - /* Check if this is the standard input device. */ >>> - if (strcmp(stdinname, DEVNAME)) >>> - return 1; >>> + if (CONFIG_IS_ENABLED(CONSOLE_MUX)) { >>> + char *devname = DEVNAME; >>> + char *newstdin = NULL; >>> + /* >>> + * stdin might not be set yet.. either way, with console- >>> + * mux the sensible thing to do is add ourselves to the >>> + * list of stdio devices: >>> + */ >>> + if (stdinname && !strstr(stdinname, DEVNAME)) { >>> + newstdin = malloc(strlen(stdinname) + >>> + strlen(","DEVNAME) + 1); >>> + sprintf(newstdin, "%s,"DEVNAME, stdinname); >>> + stdinname = newstdin; >>> + } else if (!stdinname) { >>> + stdinname = devname; >>> + } >>> + error = iomux_doenv(stdin, stdinname); >>> + free(newstdin); >> >> Sorry I should have asked this before: isn't free(devname) OK? In >> previous version, it has a test logic to see whether free is needed. > > it was in the first version of my patch, until I added the &&!strstr() > bit to avoid adding usbkbd a second time to stdin if stdin already > contained usbkbd. Now we have a case where stdinname is the ptr > returned from getenv() which we probably don't want to free() ;-) > Ah yes, I misread the codes. It's free(newstdin) not free(stdinname). Thanks for the clarification. Reviewed-by: Bin Meng <bmeng.cn@gmail.com> Regards, Bin
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index d2d29cc98f..8edeb6c4f5 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -516,23 +516,39 @@ static int probe_usb_keyboard(struct usb_device *dev) return error; stdinname = getenv("stdin"); -#if CONFIG_IS_ENABLED(CONSOLE_MUX) - error = iomux_doenv(stdin, stdinname); - if (error) - return error; -#else - /* Check if this is the standard input device. */ - if (strcmp(stdinname, DEVNAME)) - return 1; + if (CONFIG_IS_ENABLED(CONSOLE_MUX)) { + char *devname = DEVNAME; + char *newstdin = NULL; + /* + * stdin might not be set yet.. either way, with console- + * mux the sensible thing to do is add ourselves to the + * list of stdio devices: + */ + if (stdinname && !strstr(stdinname, DEVNAME)) { + newstdin = malloc(strlen(stdinname) + + strlen(","DEVNAME) + 1); + sprintf(newstdin, "%s,"DEVNAME, stdinname); + stdinname = newstdin; + } else if (!stdinname) { + stdinname = devname; + } + error = iomux_doenv(stdin, stdinname); + free(newstdin); + if (error) + return error; + } else { + /* Check if this is the standard input device. */ + if (strcmp(stdinname, DEVNAME)) + return 1; - /* Reassign the console */ - if (overwrite_console()) - return 1; + /* Reassign the console */ + if (overwrite_console()) + return 1; - error = console_assign(stdin, DEVNAME); - if (error) - return error; -#endif + error = console_assign(stdin, DEVNAME); + if (error) + return error; + } return 0; } diff --git a/include/console.h b/include/console.h index cea29ed6dc..7dfd36d7d1 100644 --- a/include/console.h +++ b/include/console.h @@ -57,8 +57,6 @@ int console_announce_r(void); /* * CONSOLE multiplexing. */ -#ifdef CONFIG_CONSOLE_MUX #include <iomux.h> -#endif #endif
stdin might not be set, which would cause iomux_doenv() to fail therefore causing probe_usb_keyboard() to fail. Furthermore if we do have iomux enabled, the sensible thing (in terms of user experience) would be to simply add ourselves to the list of stdin devices. This fixes an issue with usbkbd on dragonboard410c with distro- bootcmd, where stdin is not set (so stdinname is null). Signed-off-by: Rob Clark <robdclark@gmail.com> --- v2: address Bin's review comments v3: fix fail with free()ing if usbkbd is already in stdin env variable pointed out by Simon v4: use if (CONFIG_IS_ENABLED(CONSOLE_MUX)) { ... } common/usb_kbd.c | 46 +++++++++++++++++++++++++++++++--------------- include/console.h | 2 -- 2 files changed, 31 insertions(+), 17 deletions(-)