[U-Boot] [PATCH] usb: kbd: don't fail with iomux (v3)

[Rob Clark]

On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass <sjg at chromium.org> wrote:
> On 4 August 2017 at 07:16, Bin Meng <bmeng.cn at gmail.com> wrote:
>> Hi Rob,
>>
>> On Fri, Aug 4, 2017 at 8:51 PM, 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
>>>
>>> (the real v3 this time)
>>>
>>
>> As I mentioned, it's the email title, not the commit title with
>> version number. If you prefer format-patch, then use
>> --subject-prefix="PATCH v3"
>>
>>>  common/usb_kbd.c | 15 +++++++++++++++
>>>  1 file changed, 15 insertions(+)
>
> Reviewed-by: Simon Glass <sjg at chromium.org>
>
> Question below
>
>>>
>>> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
>>> index d2d29cc98f..d71eae61ec 100644
>>> --- a/common/usb_kbd.c
>>> +++ b/common/usb_kbd.c
>>> @@ -517,7 +517,22 @@ static int probe_usb_keyboard(struct usb_device *dev)
>>>
>>>         stdinname = getenv("stdin");
>>>  #if CONFIG_IS_ENABLED(CONSOLE_MUX)
>
> Would this work:
>
> if (CONFIG_IS_ENABLED(CONSOLE_MUX) {
>    ..
> }
>
> The #ifdef adds a code path that is not tested, so if possible we
> should try to use the compiler.

I gave this a quick try.. it would require either adding an
unconditional #include <iomux.h> in usb_kbd.c or dropping the #ifdef
CONFIG_CONSOLE_MUX guarding the #include <iomux.h> in console.h.. not
sure which is preferred?

I suspect it would cause problems with -O0 but when I tried
KCFLAGS=-O0 there were enough other unrelated compile errors, that I
guess this isn't a legit use-case.

If you want I can send a v4 that uses "if (CONFIG_IS_ENABLED(...))"
(or a separate patch on top.. in which case, do you mind removing the
"(v3)" in $subject when you apply, or do you prefer I spam list with
yet another resend?)  And in either case let me know what you prefer
about iomux.h

BR,
-R

>>> +       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
>>> --
>
> Regards,
> Simon