Hi Rob, On Tue, Aug 8, 2017 at 6:48 PM, Rob Clark <robdcl...@gmail.com> wrote: > On Tue, Aug 8, 2017 at 6:42 AM, Bin Meng <bmeng...@gmail.com> wrote: >> Hi Rob, >> >> On Tue, Aug 8, 2017 at 3:51 AM, Rob Clark <robdcl...@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 <robdcl...@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...@gmail.com> Regards, Bin _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot