Re: [U-Boot] [PATCH v4] usb: kbd: don't fail with iomux
Hi Rob,
On Tue, Aug 8, 2017 at 6:48 PM, Rob Clark wrote:
> On Tue, Aug 8, 2017 at 6:42 AM, Bin Meng wrote:
>> Hi Rob,
>>
>> On Tue, Aug 8, 2017 at 3:51 AM, Rob Clark 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
>>> ---
>>> 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
Regards,
Bin
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v4] usb: kbd: don't fail with iomux
On Tue, Aug 8, 2017 at 6:42 AM, Bin Meng wrote:
> Hi Rob,
>
> On Tue, Aug 8, 2017 at 3:51 AM, Rob Clark 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
>> ---
>> 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
>> -#endif
>>
>> #endif
>> --
>
> Regards,
> Bin
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v4] usb: kbd: don't fail with iomux
Hi Rob,
On Tue, Aug 8, 2017 at 3:51 AM, Rob Clark 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
> ---
> 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.
> + 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
> -#endif
>
> #endif
> --
Regards,
Bin
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
