usb: kbd: don't fail with iomux (v3)
PATCH 1/1 usb: kbd: don't fail with iomux (v3)
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).
---
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(+)
common/usb_kbd.c +15 -0
@@ -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
Signature · 1 line
2.13.0
PATCH 1/1 usb: kbd: don't fail with iomux (v3)
Hi Rob,
Quoted 8 lines · Rob Clark
stdin might not be set, which would cause
iomux_doenv()to fail therefore causingprobe_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).
Quoted 8 lines
Signed-off-by Rob Clark <[email protected]>---
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"
Quoted 21 lines
common/usb_kbd.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)common/usb_kbd.c +10
-0@@ -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);
common/usb_kbd.c static int probe_usb_keyboard(struct usb_device *dev)
Quoted 10 lines
common/usb_kbd.c +5
-1+ stdinname = newstdin; + }else if (!stdinname) { + stdinname = devname; + } error =iomux_doenv (stdin, stdinname); +free (newstdin);if (error)return error;#else --
Regards,
Bin
PATCH 1/1 usb: kbd: don't fail with iomux (v3)
Quoted 17 lines · Bin Meng
Hi Rob,
Quoted 14 lines · Rob Clark
stdin might not be set, which would cause
iomux_doenv()to fail therefore causingprobe_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 <[email protected]>---
v2: address Bin's review comments
v3: fix fail with free()ing if usbkbd is already in stdin env variable
pointed out by Simon
Quoted 9 lines
Quoted 2 lines
(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"
Quoted 2 lines
common/usb_kbd.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
Question below
Quoted 9 lines
common/usb_kbd.c static int probe_usb_keyboard(struct usb_device *dev) · line 519
Quoted 9 lines
common/usb_kbd.c +0
-0@@ -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.
Quoted 12 lines
Quoted 12 lines
common/usb_kbd.c +12
-0+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) {
Quoted 8 lines
common/usb_kbd.c static int probe_usb_keyboard(struct usb_device *dev)
Quoted 8 lines
common/usb_kbd.c +3
-1+ stdinname = devname; + } error =iomux_doenv (stdin, stdinname); +free (newstdin);if (error)return error;#else --
Regards,
Simon
PATCH 1/1 usb: kbd: don't fail with iomux (v3)
Quoted 44 lines · Simon Glass
Quoted 17 lines · Bin Meng
Hi Rob,
Quoted 14 lines · Rob Clark
stdin might not be set, which would cause
iomux_doenv()to fail therefore causingprobe_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 <[email protected]>---
v2: address Bin's review comments
v3: fix fail with free()ing if usbkbd is already in stdin env variable
pointed out by SimonQuoted 9 lines
Quoted 2 lines
(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"
Quoted 2 lines
common/usb_kbd.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)Reviewed-by Simon Glass <[email protected]>Question below
Quoted 9 lines
common/usb_kbd.c static int probe_usb_keyboard(struct usb_device *dev) · line 519
Quoted 9 lines
common/usb_kbd.c +0
-0@@ -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:
Quoted 6 lines
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
Quoted 23 lines
Quoted 12 lines
Quoted 12 lines
common/usb_kbd.c +12
-0+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) {Quoted 8 lines
common/usb_kbd.c static int probe_usb_keyboard(struct usb_device *dev)
Quoted 8 lines
common/usb_kbd.c +3
-1+ stdinname = devname; + } error =iomux_doenv (stdin, stdinname); +free (newstdin);if (error)return error;#else --Regards,
Simon
PATCH 1/1 usb: kbd: don't fail with iomux (v3)
Hi Rob,
Quoted 49 lines · Rob Clark
Quoted 48 lines · Simon Glass
Quoted 17 lines · Bin Meng
Hi Rob,
Quoted 14 lines · Rob Clark
stdin might not be set, which would cause
iomux_doenv()to fail therefore causingprobe_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 <[email protected]>---
v2: address Bin's review comments
v3: fix fail with free()ing if usbkbd is already in stdin env variable
pointed out by SimonQuoted 9 lines
Quoted 2 lines
(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"
Quoted 2 lines
common/usb_kbd.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)Reviewed-by Simon Glass <[email protected]>Question below
Quoted 9 lines
common/usb_kbd.c static int probe_usb_keyboard(struct usb_device *dev) · line 519
Quoted 9 lines
common/usb_kbd.c +0
-0@@ -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) { .. }
Quoted 5 lines
Quoted 2 lines
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
PATCH 1/1 usb: kbd: don't fail with iomux (v3)
In reply to Simon GlassQuoted 44 lines · Simon Glass
Quoted 17 lines · Bin Meng
Hi Rob,
Quoted 14 lines · Rob Clark
stdin might not be set, which would cause
iomux_doenv()to fail therefore causingprobe_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 <[email protected]>---
v2: address Bin's review comments
v3: fix fail with free()ing if usbkbd is already in stdin env variable
pointed out by SimonQuoted 9 lines
Quoted 2 lines
(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"
Quoted 2 lines
common/usb_kbd.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)Reviewed-by Simon Glass <[email protected]>Question below
Quoted 9 lines
common/usb_kbd.c static int probe_usb_keyboard(struct usb_device *dev) · line 519
Quoted 9 lines
common/usb_kbd.c +0
-0@@ -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:
Quoted 6 lines
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
Quoted 23 lines
Quoted 12 lines
Quoted 12 lines
common/usb_kbd.c +12
-0+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) {Quoted 8 lines
common/usb_kbd.c static int probe_usb_keyboard(struct usb_device *dev)
Quoted 8 lines
common/usb_kbd.c +3
-1+ stdinname = devname; + } error =iomux_doenv (stdin, stdinname); +free (newstdin);if (error)return error;#else --Regards,
Simon
PATCH 1/1 usb: kbd: don't fail with iomux (v3)
Hi Rob,
Quoted 50 lines · Rob Clark
Quoted 48 lines · Simon Glass
Quoted 17 lines · Bin Meng
Hi Rob,
Quoted 14 lines · Rob Clark
stdin might not be set, which would cause
iomux_doenv()to fail therefore causingprobe_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 <[email protected]>---
v2: address Bin's review comments
v3: fix fail with free()ing if usbkbd is already in stdin env variable
pointed out by SimonQuoted 9 lines
Quoted 2 lines
(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"
Quoted 2 lines
common/usb_kbd.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)Reviewed-by Simon Glass <[email protected]>Question below
Quoted 9 lines
common/usb_kbd.c static int probe_usb_keyboard(struct usb_device *dev) · line 519
Quoted 9 lines
common/usb_kbd.c +0
-0@@ -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) { .. }
Quoted 7 lines
Quoted 2 lines
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> inusb_kbd.cor dropping the #ifdefCONFIG_CONSOLE_MUXguarding the #include <iomux.h> inconsole.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
.
.>
Quoted 3 lines
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.
Quoted 6 lines
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 aboutiomux.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.
Quoted 27 lines
BR,
-RQuoted 23 lines
Quoted 12 lines
Quoted 12 lines
common/usb_kbd.c +12
-0+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) {Quoted 8 lines
common/usb_kbd.c static int probe_usb_keyboard(struct usb_device *dev)
Quoted 8 lines
common/usb_kbd.c +3
-1+ stdinname = devname; + } error =iomux_doenv (stdin, stdinname); +free (newstdin);if (error)return error;#else --Regards,
Simon
Regards,
Simon
PATCH 1/1 usb: kbd: don't fail with iomux (v3)
Quoted 83 lines · Simon Glass
Hi Rob,
Quoted 50 lines · Rob Clark
Quoted 48 lines · Simon Glass
Quoted 17 lines · Bin Meng
Hi Rob,
Quoted 14 lines · Rob Clark
stdin might not be set, which would cause
iomux_doenv()to fail therefore causingprobe_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 <[email protected]>---
v2: address Bin's review comments
v3: fix fail with free()ing if usbkbd is already in stdin env variable
pointed out by SimonQuoted 9 lines
Quoted 2 lines
(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"
Quoted 2 lines
common/usb_kbd.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)Reviewed-by Simon Glass <[email protected]>Question below
Quoted 9 lines
common/usb_kbd.c static int probe_usb_keyboard(struct usb_device *dev) · line 519
Quoted 9 lines
common/usb_kbd.c +0
-0@@ -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) { .. }Quoted 7 lines
Quoted 2 lines
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> inusb_kbd.cor dropping the #ifdefCONFIG_CONSOLE_MUXguarding the #include <iomux.h> inconsole.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.hseems wrong as well. It would be good to move that toconsole.c
.
.>Quoted 3 lines
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.
Quoted 6 lines
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 aboutiomux.h
Quoted 4 lines
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
PATCH 1/1 usb: kbd: don't fail with iomux (v3)
Hi Rob,
Quoted 84 lines · Rob Clark
Quoted 83 lines · Simon Glass
Hi Rob,
Quoted 50 lines · Rob Clark
Quoted 48 lines · Simon Glass
Quoted 17 lines · Bin Meng
Hi Rob,
Quoted 14 lines · Rob Clark
stdin might not be set, which would cause
iomux_doenv()to fail therefore causingprobe_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 <[email protected]>---
v2: address Bin's review comments
v3: fix fail with free()ing if usbkbd is already in stdin env variable
pointed out by SimonQuoted 9 lines
Quoted 2 lines
(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"
Quoted 2 lines
common/usb_kbd.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)Reviewed-by Simon Glass <[email protected]>Question below
Quoted 9 lines
common/usb_kbd.c static int probe_usb_keyboard(struct usb_device *dev) · line 519
Quoted 9 lines
common/usb_kbd.c +0
-0@@ -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) { .. }Quoted 7 lines
Quoted 2 lines
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> inusb_kbd.cor dropping the #ifdefCONFIG_CONSOLE_MUXguarding the #include <iomux.h> inconsole.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.hseems wrong as well. It would be good to move that toconsole.c
.
.>Quoted 3 lines
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.
Quoted 6 lines
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 aboutiomux.h
Quoted 6 lines
Quoted 4 lines
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