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

[Bin Meng]

Hi Rob,

On Tue, Aug 8, 2017 at 6:48 PM, Rob Clark <robdclark at gmail.com> wrote:
> 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() ;-)
>

Ah yes, I misread the codes. It's free(newstdin) not free(stdinname).
Thanks for the clarification.

Reviewed-by: Bin Meng <bmeng.cn at gmail.com>

Regards,
Bin