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

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

[Rob Clark]

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@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)

 common/usb_kbd.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

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)
+	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
-- 
2.13.0

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

[Bin Meng]

Hi Rob,

On Fri, Aug 4, 2017 at 8:51 PM, Rob Clark <robdclark@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@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(+)
>
> 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)
> +       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,
Bin

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

[Simon Glass]

On 4 August 2017 at 07:16, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Rob,
>
> On Fri, Aug 4, 2017 at 8:51 PM, Rob Clark <robdclark@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@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@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.

>> +       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

[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@chromium.org> wrote:
> On 4 August 2017 at 07:16, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi Rob,
>>
>> On Fri, Aug 4, 2017 at 8:51 PM, Rob Clark <robdclark@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@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@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 think it would, except maybe if someone compiled w/ -O0 (unresolved
symbols at link time).. not sure if that is something we care about?

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

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

[Bin Meng]

Hi Rob,

On Sun, Aug 6, 2017 at 6:41 PM, Rob Clark <robdclark@gmail.com> wrote:
> On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass <sjg@chromium.org> wrote:
>> On 4 August 2017 at 07:16, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> Hi Rob,
>>>
>>> On Fri, Aug 4, 2017 at 8:51 PM, Rob Clark <robdclark@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@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@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 think it would, except maybe if someone compiled w/ -O0 (unresolved
> symbols at link time).. not sure if that is something we care about?

Can you please resend with a commit title without (v3) but email title with v3?

Regards,
Bin

[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@chromium.org> wrote:
> On 4 August 2017 at 07:16, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi Rob,
>>
>> On Fri, Aug 4, 2017 at 8:51 PM, Rob Clark <robdclark@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@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@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

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

[Simon Glass]

Hi Rob,

On 6 August 2017 at 05:58, Rob Clark <robdclark@gmail.com> wrote:
>
> On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass <sjg@chromium.org> wrote:
> > On 4 August 2017 at 07:16, Bin Meng <bmeng.cn@gmail.com> wrote:
> >> Hi Rob,
> >>
> >> On Fri, Aug 4, 2017 at 8:51 PM, Rob Clark <robdclark@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@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@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 don't think we should put #ifdefs around header files #includes so
the first option seems best. There are still some header files in
U-Boot which 'do stuff' like ensure that an option is set, etc. We
should over time fix those up.

The #include in console.h seems wrong as well. It would be good to
move that to console.c
.
.>
> 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.

Yes we have had that for a while. I don't think people use it anymore.

>
> 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

I think a v4 patch is best. But you should not have the version in the
subject there since it will end up in the commit. If you use patman it
will produce the patch correctly.

>
> 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


Regards,
Simon

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

[Rob Clark]

On Sun, Aug 13, 2017 at 11:35 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Rob,
>
> On 6 August 2017 at 05:58, Rob Clark <robdclark@gmail.com> wrote:
>>
>> On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass <sjg@chromium.org> wrote:
>> > On 4 August 2017 at 07:16, Bin Meng <bmeng.cn@gmail.com> wrote:
>> >> Hi Rob,
>> >>
>> >> On Fri, Aug 4, 2017 at 8:51 PM, Rob Clark <robdclark@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@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@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 don't think we should put #ifdefs around header files #includes so
> the first option seems best. There are still some header files in
> U-Boot which 'do stuff' like ensure that an option is set, etc. We
> should over time fix those up.
>
> The #include in console.h seems wrong as well. It would be good to
> move that to console.c
> .
> .>
>> 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.
>
> Yes we have had that for a while. I don't think people use it anymore.
>
>>
>> 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
>
> I think a v4 patch is best. But you should not have the version in the
> subject there since it will end up in the commit. If you use patman it
> will produce the patch correctly.
>

fwiw, I did already send a v4 which went with the first option..

BR,
-R

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

[Simon Glass]

Hi Rob,

On 13 August 2017 at 12:07, Rob Clark <robdclark@gmail.com> wrote:
> On Sun, Aug 13, 2017 at 11:35 AM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Rob,
>>
>> On 6 August 2017 at 05:58, Rob Clark <robdclark@gmail.com> wrote:
>>>
>>> On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass <sjg@chromium.org> wrote:
>>> > On 4 August 2017 at 07:16, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> >> Hi Rob,
>>> >>
>>> >> On Fri, Aug 4, 2017 at 8:51 PM, Rob Clark <robdclark@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@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@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 don't think we should put #ifdefs around header files #includes so
>> the first option seems best. There are still some header files in
>> U-Boot which 'do stuff' like ensure that an option is set, etc. We
>> should over time fix those up.
>>
>> The #include in console.h seems wrong as well. It would be good to
>> move that to console.c
>> .
>> .>
>>> 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.
>>
>> Yes we have had that for a while. I don't think people use it anymore.
>>
>>>
>>> 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
>>
>> I think a v4 patch is best. But you should not have the version in the
>> subject there since it will end up in the commit. If you use patman it
>> will produce the patch correctly.
>>
>
> fwiw, I did already send a v4 which went with the first option..

OK. I can't seem to find it - can you please give me a patchwork link?

Thanks,
Simon