[U-Boot] [PATCH] usb: kbd: Fix dangling pointers on probe fail

[Rob Clark]

On Thu, Oct 5, 2017 at 7:48 PM, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Rob,
>
> On Wed, Oct 4, 2017 at 1:31 AM, Rob Clark <robdclark at 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 at 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