[U-Boot] [PATCH v4] usb: kbd: don't fail with iomux

[Rob Clark]

On Tue, Aug 8, 2017 at 6:42 AM, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Rob,
>
> On Tue, Aug 8, 2017 at 3:51 AM, Rob Clark <robdclark at 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 at 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