* [U-Boot] [PATCH] usb: kbd: Fix dangling pointers on probe fail
@ 2017-10-03 17:31 Rob Clark
2017-10-05 8:38 ` Peter Robinson
2017-10-05 23:48 ` Bin Meng
0 siblings, 2 replies; 4+ messages in thread
From: Rob Clark @ 2017-10-03 17:31 UTC (permalink / raw)
To: u-boot
If probe fails, we should unregister the stdio device, else we leave
dangling pointers to the parent 'struct usb_device'.
Signed-off-by: Rob Clark <robdclark@gmail.com>
---
I finally got around to debugging why things explode so badly without
fixing usb_kbd vs. iomux[1] (which this patch applies on top of).
[1] https://patchwork.ozlabs.org/patch/818881/
common/usb_kbd.c | 30 ++++++++++++++++++++++++------
1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index 4c3ad95fca..82ad93f6ca 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -535,22 +535,40 @@ static int probe_usb_keyboard(struct usb_device *dev)
error = iomux_doenv(stdin, stdinname);
free(newstdin);
if (error)
- return error;
+ goto unregister_stdio;
} else {
/* Check if this is the standard input device. */
- if (strcmp(stdinname, DEVNAME))
- return 1;
+ if (strcmp(stdinname, DEVNAME)) {
+ error = -1;
+ goto unregister_stdio;
+ }
/* Reassign the console */
- if (overwrite_console())
- return 1;
+ if (overwrite_console()) {
+ error = -1;
+ goto unregister_stdio;
+ }
error = console_assign(stdin, DEVNAME);
if (error)
- return error;
+ goto unregister_stdio;
}
return 0;
+
+unregister_stdio:
+ /*
+ * If probe fails, the device will be removed.. leaving dangling
+ * pointers if the stdio device is not unregistered. If u-boot
+ * is built without stdio_deregister(), just pretend to succeed
+ * in order to avoid dangling pointers.
+ */
+#if CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER)
+ stdio_deregister(DEVNAME, 1);
+ return error;
+#else
+ return 0;
+#endif
}
#ifndef CONFIG_DM_USB
--
2.13.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH] usb: kbd: Fix dangling pointers on probe fail
2017-10-03 17:31 [U-Boot] [PATCH] usb: kbd: Fix dangling pointers on probe fail Rob Clark
@ 2017-10-05 8:38 ` Peter Robinson
2017-10-05 23:48 ` Bin Meng
1 sibling, 0 replies; 4+ messages in thread
From: Peter Robinson @ 2017-10-05 8:38 UTC (permalink / raw)
To: u-boot
On Tue, Oct 3, 2017 at 6:31 PM, Rob Clark <robdclark@gmail.com> wrote:
> If probe fails, we should unregister the stdio device, else we leave
> dangling pointers to the parent 'struct usb_device'.
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
Tested-by: Peter Robinson <pbrobinson@gmail.com>
Tested on RPi3 and a couple of other devices.
> ---
> I finally got around to debugging why things explode so badly without
> fixing usb_kbd vs. iomux[1] (which this patch applies on top of).
>
> [1] https://patchwork.ozlabs.org/patch/818881/
>
> common/usb_kbd.c | 30 ++++++++++++++++++++++++------
> 1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> index 4c3ad95fca..82ad93f6ca 100644
> --- a/common/usb_kbd.c
> +++ b/common/usb_kbd.c
> @@ -535,22 +535,40 @@ static int probe_usb_keyboard(struct usb_device *dev)
> error = iomux_doenv(stdin, stdinname);
> free(newstdin);
> if (error)
> - return error;
> + goto unregister_stdio;
> } else {
> /* Check if this is the standard input device. */
> - if (strcmp(stdinname, DEVNAME))
> - return 1;
> + if (strcmp(stdinname, DEVNAME)) {
> + error = -1;
> + goto unregister_stdio;
> + }
>
> /* Reassign the console */
> - if (overwrite_console())
> - return 1;
> + if (overwrite_console()) {
> + error = -1;
> + goto unregister_stdio;
> + }
>
> error = console_assign(stdin, DEVNAME);
> if (error)
> - return error;
> + goto unregister_stdio;
> }
>
> return 0;
> +
> +unregister_stdio:
> + /*
> + * If probe fails, the device will be removed.. leaving dangling
> + * pointers if the stdio device is not unregistered. If u-boot
> + * is built without stdio_deregister(), just pretend to succeed
> + * in order to avoid dangling pointers.
> + */
> +#if CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER)
> + stdio_deregister(DEVNAME, 1);
> + return error;
> +#else
> + return 0;
> +#endif
> }
>
> #ifndef CONFIG_DM_USB
> --
> 2.13.5
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH] usb: kbd: Fix dangling pointers on probe fail
2017-10-03 17:31 [U-Boot] [PATCH] usb: kbd: Fix dangling pointers on probe fail Rob Clark
2017-10-05 8:38 ` Peter Robinson
@ 2017-10-05 23:48 ` Bin Meng
2017-10-06 1:35 ` Rob Clark
1 sibling, 1 reply; 4+ messages in thread
From: Bin Meng @ 2017-10-05 23:48 UTC (permalink / raw)
To: u-boot
Hi Rob,
On Wed, Oct 4, 2017 at 1:31 AM, Rob Clark <robdclark@gmail.com> wrote:
> If probe fails, we should unregister the stdio device, else we leave
> dangling pointers to the parent 'struct usb_device'.
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
> I finally got around to debugging why things explode so badly without
> fixing usb_kbd vs. iomux[1] (which this patch applies on top of).
>
> [1] https://patchwork.ozlabs.org/patch/818881/
>
> common/usb_kbd.c | 30 ++++++++++++++++++++++++------
> 1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
> index 4c3ad95fca..82ad93f6ca 100644
> --- a/common/usb_kbd.c
> +++ b/common/usb_kbd.c
> @@ -535,22 +535,40 @@ static int probe_usb_keyboard(struct usb_device *dev)
> error = iomux_doenv(stdin, stdinname);
> free(newstdin);
> if (error)
> - return error;
> + goto unregister_stdio;
> } else {
> /* Check if this is the standard input device. */
> - if (strcmp(stdinname, DEVNAME))
> - return 1;
> + if (strcmp(stdinname, DEVNAME)) {
> + error = -1;
Could we use some meaningful errno?
> + goto unregister_stdio;
> + }
>
> /* Reassign the console */
> - if (overwrite_console())
> - return 1;
> + if (overwrite_console()) {
> + error = -1;
> + goto unregister_stdio;
> + }
>
> error = console_assign(stdin, DEVNAME);
> if (error)
> - return error;
> + goto unregister_stdio;
> }
>
> return 0;
> +
> +unregister_stdio:
> + /*
> + * If probe fails, the device will be removed.. leaving dangling
> + * pointers if the stdio device is not unregistered. If u-boot
nits: U-Boot
> + * is built without stdio_deregister(), just pretend to succeed
> + * in order to avoid dangling pointers.
> + */
> +#if CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER)
> + stdio_deregister(DEVNAME, 1);
> + return error;
> +#else
> + return 0;
Don't understand why 'return 0' here if probe fails...
> +#endif
> }
>
> #ifndef CONFIG_DM_USB
> --
Regards,
Bin
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH] usb: kbd: Fix dangling pointers on probe fail
2017-10-05 23:48 ` Bin Meng
@ 2017-10-06 1:35 ` Rob Clark
0 siblings, 0 replies; 4+ messages in thread
From: Rob Clark @ 2017-10-06 1:35 UTC (permalink / raw)
To: u-boot
On Thu, Oct 5, 2017 at 7:48 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Rob,
>
> On Wed, Oct 4, 2017 at 1:31 AM, Rob Clark <robdclark@gmail.com> wrote:
>> If probe fails, we should unregister the stdio device, else we leave
>> dangling pointers to the parent 'struct usb_device'.
>>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> ---
>> I finally got around to debugging why things explode so badly without
>> fixing usb_kbd vs. iomux[1] (which this patch applies on top of).
>>
>> [1] https://patchwork.ozlabs.org/patch/818881/
>>
>> common/usb_kbd.c | 30 ++++++++++++++++++++++++------
>> 1 file changed, 24 insertions(+), 6 deletions(-)
>>
>> diff --git a/common/usb_kbd.c b/common/usb_kbd.c
>> index 4c3ad95fca..82ad93f6ca 100644
>> --- a/common/usb_kbd.c
>> +++ b/common/usb_kbd.c
>> @@ -535,22 +535,40 @@ static int probe_usb_keyboard(struct usb_device *dev)
>> error = iomux_doenv(stdin, stdinname);
>> free(newstdin);
>> if (error)
>> - return error;
>> + goto unregister_stdio;
>> } else {
>> /* Check if this is the standard input device. */
>> - if (strcmp(stdinname, DEVNAME))
>> - return 1;
>> + if (strcmp(stdinname, DEVNAME)) {
>> + error = -1;
>
> Could we use some meaningful errno?
suggestions welcome.. I wasn't sure what to pick so I just went with <0
>> + goto unregister_stdio;
>> + }
>>
>> /* Reassign the console */
>> - if (overwrite_console())
>> - return 1;
>> + if (overwrite_console()) {
>> + error = -1;
>> + goto unregister_stdio;
>> + }
>>
>> error = console_assign(stdin, DEVNAME);
>> if (error)
>> - return error;
>> + goto unregister_stdio;
>> }
>>
>> return 0;
>> +
>> +unregister_stdio:
>> + /*
>> + * If probe fails, the device will be removed.. leaving dangling
>> + * pointers if the stdio device is not unregistered. If u-boot
>
> nits: U-Boot
>
>> + * is built without stdio_deregister(), just pretend to succeed
>> + * in order to avoid dangling pointers.
>> + */
>> +#if CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER)
>> + stdio_deregister(DEVNAME, 1);
>> + return error;
>> +#else
>> + return 0;
>
> Don't understand why 'return 0' here if probe fails...
see above comment, basically we can't fail after we've registered the
stdio device unless we can remove it. Suggestions welcome on better
wording for the comment if it wasn't clear
BR,
-R
>> +#endif
>> }
>>
>> #ifndef CONFIG_DM_USB
>> --
>
> Regards,
> Bin
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-10-06 1:35 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-03 17:31 [U-Boot] [PATCH] usb: kbd: Fix dangling pointers on probe fail Rob Clark
2017-10-05 8:38 ` Peter Robinson
2017-10-05 23:48 ` Bin Meng
2017-10-06 1:35 ` Rob Clark
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox