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

[Heinrich Schuchardt]

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.

>
> 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/