[PATCH v4 05/12] usb: Avoid unbinding devices in use by bootflows

[Heinrich Schuchardt]

On 11/15/23 17:23, Heinrich Schuchardt wrote:
> On 11/15/23 16:50, Simon Glass wrote:
>> Hi Heinrich,
>>
>> On Sun, 12 Nov 2023 at 16:35, Heinrich Schuchardt <xypron.glpk at gmx.de>
>> wrote:
>>>
>>>
>>>
>>> Am 12. November 2023 22:20:50 MEZ schrieb Simon Glass
>>> <sjg at chromium.org>:
>>>> Hi Heinrich,
>>>>
>>>> On Sun, 12 Nov 2023 at 13:32, Heinrich Schuchardt
>>>> <xypron.glpk at gmx.de> wrote:
>>>>>
>>>>>
>>>>>
>>>>> Am 12. November 2023 21:02:42 MEZ schrieb Simon Glass
>>>>> <sjg at chromium.org>:
>>>>>> When a USB device is unbound, it causes any bootflows attached to
>>>>>> it to
>>>>>> be removed, via a call to bootdev_clear_bootflows() from
>>>>>> bootdev_pre_unbind(). This obviously makes it impossible to boot the
>>>>>> bootflow.
>>>>>>
>>>>>> However, when booting a bootflow that relies on USB, usb_stop() is
>>>>>> called, which unbinds the device. For EFI, this happens in
>>>>>> efi_exit_boot_services() which means that the bootflow disappears
>>>>>> before it is finished with.
>>>>>
>>>>> After ExitBootServices() no driver of U-Boot can be used as the
>>>>> operating system is in charge.
>>>>>
>>>>> Any bootflow inside U-Boot is terminated by definition when
>>>>> reaching ExitBootServices.
>>>>>
>>>>>>
>>>>>> There is no need to unbind all the USB devices just to quiesce them.
>>>>>> Add a new usb_pause() call which removes them but leaves them bound.
>>>>>
>>>>> As U-Boot must not access any device after ExitBootServices() it is
>>>>> unclear to me what you want to achieve.
>>>>
>>>> I can't remember exactly what is needed from the bootflow, but
>>>> something is. Perhaps the kernel, or the cmdline, or fdt? It would
>>>> have been better if I had mentioned this explicitly,  But then this
>>>> patch has been sitting around for ages...
>>>>
>>>> In any case, the intent of exit-boot-services is not to free all the
>>>> memory used, since some of it may have been used to hold data that the
>>>> app continues to use.
>>>
>>> The EFI application reads the memory map and receives an ID which it
>>> passes to ExitBootServiced() after this point any memory not marked
>>> as EFI runtime can be used by the EFI app. This includes the memory
>>> that harbors the U-Boot USB drivers. Therefore no drivers can be used
>>> here.
>>>
>>> Starting the EFI app via StartImage() must  terminate any running
>>> U-Boot "boot flow".
>>>
>>> After ExitBootServices() the EFI application cannot return to U-Boot
>>> except for SetVirtualAdressMspsetting which relocates the EFI runtime.
>>>
>>> Bootflows and U-Boot drivers have no meaning after ExitBootServices().
>>>
>>>
>>>
>>>>
>>>> Also there is U-Boot code between when the devices are unbound and
>>>> when U-Boot actually exits back to the app.
>>>>
>>>> This hang was 100% repeatable on brya (an x86 Chromebook) and it took
>>>> quite a while to find.
>>>
>>> We need a better understanding of the problem that you experience to
>>> find an adequate solution. Why does removing all devices lead to
>>> hanging the system?
>>
>> Are you able to test this? With your better knowledge of EFI you might
>> be able to figure out something else that is going on. But in my case
>> it causes some memory to be freed (perhaps the memory holding the EFI
>> app?), which is then overwritten by something else being malloc()'d,
>
> The memory for the EFI app is not assigned via malloc(). It is allocated
> by AllocatedPages().
>
> Reading "some memory freed" does not give me confidence that the problem
> is sufficiently analyzed.
>
>> so the boot hangs. It is hard to see what is going on after the app
>> starts.
>>
>> This patch was sent almost two months ago and fixes a real bug. The
>> first few versions attracted no comment now it is being blocked
>> because you don't understand how it fixes anything.
>
> Do you understand why unbinding the devices causes the problem?
>
>>
>> I can get the hardware up again and try this but it will take a while.
>
> Digging a bit deeper seems to be the right approach.

Re: bug - bootflow: grub efi crashes when bootflow selected explicitly
https://lore.kernel.org/u-boot/CAHc5_t1H39YV=HSSa7TCEdzRctAX+zibkx1vsuwEW-raOL9TcA@mail.gmail.com/

points to a probable root cause:

https://github.com/u-boot/u-boot/blob/master/boot/bootflow.c#L470
free(bflow->buf)

In the EFI boot bflow->buf points to $kernel_addr_r which never was
allocated and therefore must not be freed.

Best regards

Heinrich

>
>>
>> Even without the hang, this still fixes a bug. We should be using
>> device_remove() to quiesce devices. There is no need to unbind them.
>
> Is this what the patch does?
>
>>
>> BTW another patch that suffered a similar fate was [1]. I just applied
>> it based on a review from Bin.
>
> Please, ping Ilias and me if you don't get the feedback that you deserve.
>
> Best regards
>
> Heinrich
>
>>
>> [..]
>>
>> Regards,
>> Simon
>>
>> [1]
>> https://patchwork.ozlabs.org/project/uboot/patch/20231001191444.v3.1.I9f7f373d00947c704aeae0088dfedd8df07fab60@changeid/
>