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() ;-)
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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
