[PATCH 0/2] efi: remove error in efi_disk_probe/efi_disk_remove
Proposed serie after investigate crash: - board stm32mp157c-dk2, including a USB HUB 4 ports - 2 USB key on the USB HUB (same PID/VID) - multiple command usb start/usb stop
Without these patches, U-Boot failed to probe / failed to unbind the 2nd key and crash in USB stack, usb_find_usb2_hub_address_port()
When the probe for USB child failed, the unbind failed also. For example when PSCI stack can't handle 2 devices with the same EFI handle based on PIDVID for USB device.
On the "usb stop" command, the USB tree becomes invalid as the EFI stack forbids to remove the USB devices, the USB are still present (checked with "dm tree" command).
On the next USB start, on USB scan, when the USB devices children of USB HUB are added dynamically, the USB stack crashes...
I propose to remove the return error in efi_disk_probe/efi_disk_remove and to replace them by log_error => even if EFI can't export the devices, the device should be available for U-Boot proper and the probe should be complete properly (the 2nd USB keys are see in dm tree in the example)
Sequence to reproduce the issue with 2 identical USB key
STM32MP> usb start && usb tree && usb stop && usb start && usb tree && usb stop && usb start && usb tree && usb stop
starting USB... Bus usb@5800d000: USB EHCI 1.00 scanning bus usb@5800d000 for devices... Adding disk for usb_mass_storage.lun0 failed (err=-2147483628/0x80000014) device 'usb_mass_storage.lun0' failed to unbind 3 USB Device(s) found device 'usb_mass_storage.lun0' failed to unbind scanning usb for storage devices... 2 Storage Device(s) found USB device tree: 1 Hub (480 Mb/s, 0mA) | u-boot EHCI Host Controller | +-2 Hub (480 Mb/s, 2mA) | +-3 Mass Storage (480 Mb/s, 200mA) | Generic Mass Storage 81ED9AA7 | stopping USB.. device 'usb_mass_storage.lun0' failed to unbind device 'usb_mass_storage' failed to unbind device 'usb_hub' failed to unbind starting USB... Bus usb@5800d000: USB EHCI 1.00 scanning bus usb@5800d000 for devices... Adding disk for usb_mass_storage.lun0 failed (err=-2147483628/0x80000014) device 'usb_mass_storage.lun0' failed to unbind 3 USB Device(s) found device 'usb_mass_storage.lun0' failed to unbind scanning usb for storage devices... 2 Storage Device(s) found USB device tree: 1 Hub (480 Mb/s, 0mA) | u-boot EHCI Host Controller | +-2 Hub (480 Mb/s, 2mA) | +-3 Mass Storage (480 Mb/s, 200mA) | Generic Mass Storage 81ED9AA7 | stopping USB.. starting USB... Bus usb@5800d000: scanning bus usb@5800d000 for devices... data abort pc : [<ddb3e7b6>] lr : [<ddb3e7b7>] reloc pc : [<c01227b6>] lr : [<c01227b7>] sp : dbafa708 ip : dbb54cc0 fp : dbafa780 r10: dbafac40 r9 : dbb19e80 r8 : 00000000 r7 : dbafa727 r6 : dbafa726 r5 : dbb40fc0 r4 : dbafac40 r3 : 00000001 r2 : dbafa726 r1 : dbafa727 r0 : 00000000 Flags: nZCv IRQs off FIQs off Mode SVC_32 (T) Code: 592c 4628 f008 ff1d (6843) 2b03 Resetting CPU ...
After the 2 patches, with the 2 SAME keys on the USB HUB the EFI handle is not created, with error in trace, BUT the USB key is available in U-Boot proper.
STM32MP> usb start && usb tree && usb stop starting USB... Bus usb@5800d000: USB EHCI 1.00 scanning bus usb@5800d000 for devices... Adding disk for usb_mass_storage.lun0 failed (err=-2147483628/0x80000014) efi_disk_create_raw usb_mass_storage.lun0 failed (-2) 4 USB Device(s) found scanning usb for storage devices... 2 Storage Device(s) found USB device tree: 1 Hub (480 Mb/s, 0mA) | u-boot EHCI Host Controller | +-2 Hub (480 Mb/s, 2mA) | +-3 Mass Storage (480 Mb/s, 200mA) | Generic Mass Storage 81ED9AA7 | +-4 Mass Storage (480 Mb/s, 200mA) Generic Mass Storage C3EAEAD2
stopping USB.. efi_disk_remove failed for usb_mass_storage.lun0 uclass 22 (-1) efi_disk_remove failed for usb_mass_storage.lun0:1 uclass 73 (-1)
Patrick Delaunay (2): efi: remove error in efi_disk_probe efi: remove error in efi_disk_remove
lib/efi_loader/efi_disk.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)
EFI has no reason to block the dm core device_probe() in the callback efi_disk_probe() registered with EVT_DM_POST_PROBE.
This patch avoids to have error in DM core on device_probe()
ret = device_notify(dev, EVT_DM_POST_PROBE);
only because EFI is not able to create its instance/handle.
For example on usb start, when the SAME KEY (PID/VID) is present on 2 ports of the USB HUB, the 2nd key have the same EFI device path with the call stack:
efi_disk_probe() efi_disk_create_raw() efi_disk_add_dev() efi_install_multiple_protocol_interfaces() EFI_ALREADY_STARTED
In case of error in probe, the 2nd key is unbound and deactivated for the next usb commands even if the limitation is only for EFI.
This patch removes any return error in probe event callback; if something occurs in EFI registration, the device is still probed.
Signed-off-by: Patrick Delaunay patrick.delaunay@foss.st.com ---
lib/efi_loader/efi_disk.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index d2256713a8e7..8d53ba3bd27e 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -677,14 +677,18 @@ int efi_disk_probe(void *ctx, struct event *event) desc = dev_get_uclass_plat(dev); if (desc->uclass_id != UCLASS_EFI_LOADER) { ret = efi_disk_create_raw(dev, agent_handle); - if (ret) - return -1; + if (ret) { + log_err("efi_disk_create_raw %s failed (%d)\n", + dev->name, ret); + return 0; + } }
device_foreach_child(child, dev) { ret = efi_disk_create_part(child, agent_handle); if (ret) - return -1; + log_err("efi_disk_create_part %s failed (%d)\n", + dev->name, ret); }
return 0;
On 3/8/23 14:26, Patrick Delaunay wrote:
EFI has no reason to block the dm core device_probe() in the callback efi_disk_probe() registered with EVT_DM_POST_PROBE.
This patch avoids to have error in DM core on device_probe()
ret = device_notify(dev, EVT_DM_POST_PROBE);
only because EFI is not able to create its instance/handle.
This should only occur if we are out of memory or if you call efi_disk_probe() twice for the same device.
For example on usb start, when the SAME KEY (PID/VID) is present on 2 ports of the USB HUB, the 2nd key have the same EFI device path with the call stack:
We need the HUB device with its USB port in the device path.
The way we currently create device paths is not good. We should traverse the dm tree to the root and create a node for each dm device. The code code for creating the individual nodes should be moved to uclasses.
@Simon: is that ok for you?
efi_disk_probe() efi_disk_create_raw() efi_disk_add_dev() efi_install_multiple_protocol_interfaces() EFI_ALREADY_STARTED
If we create the same device path for two USB devices, this is a bug we must fix.
In case of error in probe, the 2nd key is unbound and deactivated for the next usb commands even if the limitation is only for EFI.
This patch removes any return error in probe event callback; if something occurs in EFI registration, the device is still probed.
Signed-off-by: Patrick Delaunay patrick.delaunay@foss.st.com
lib/efi_loader/efi_disk.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index d2256713a8e7..8d53ba3bd27e 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -677,14 +677,18 @@ int efi_disk_probe(void *ctx, struct event *event) desc = dev_get_uclass_plat(dev); if (desc->uclass_id != UCLASS_EFI_LOADER) { ret = efi_disk_create_raw(dev, agent_handle);
if (ret)return -1;
if (ret) {log_err("efi_disk_create_raw %s failed (%d)\n",dev->name, ret);
This isn't a message a non-developer can easily understand.
return 0;}}
device_foreach_child(child, dev) { ret = efi_disk_create_part(child, agent_handle); if (ret)
return -1;
log_err("efi_disk_create_part %s failed (%d)\n",
ditto.
Best regards
Heinrich
dev->name, ret);}
return 0;
Hi,
On 3/9/23 09:57, Heinrich Schuchardt wrote:
On 3/8/23 14:26, Patrick Delaunay wrote:
EFI has no reason to block the dm core device_probe() in the callback efi_disk_probe() registered with EVT_DM_POST_PROBE.
This patch avoids to have error in DM core on device_probe()
ret = device_notify(dev, EVT_DM_POST_PROBE);
only because EFI is not able to create its instance/handle.
This should only occur if we are out of memory or if you call efi_disk_probe() twice for the same device.
OK
For example on usb start, when the SAME KEY (PID/VID) is present on 2 ports of the USB HUB, the 2nd key have the same EFI device path with the call stack:
We need the HUB device with its USB port in the device path.
ok
struct efi_device_path_usb_class { struct efi_device_path dp; u16 vendor_id; u16 product_id; u8 device_class; u8 device_subclass; u8 device_protocol; } __packed;
So a correction need to be done in ./lib/efi_loader/efi_device_path.c:dp_fill()
case UCLASS_MASS_STORAGE: case UCLASS_USB_HUB:
and ./lib/efi_loader/efi_device_path_to_text.c::dp_msging()
case DEVICE_PATH_SUB_TYPE_MSG_USB_CLASS
to add USB port or other identifier (usb dev number for example) to identify each device
and not only use PID/VID as today.
for example use device ID as it is done
UCLASS_NVME => dp->hba_port = desc->devnum;
UCLASS_IDE => dp->logical_unit_number = desc->devnum;
The way we currently create device paths is not good. We should traverse the dm tree to the root and create a node for each dm device. The code code for creating the individual nodes should be moved to uclasses.
I think that the USB port number can be found in USB DM in usb_device: udev->portnr
PS: hub_address can be also found with udev->parent->devnum;
@Simon: is that ok for you?
efi_disk_probe() efi_disk_create_raw() efi_disk_add_dev() efi_install_multiple_protocol_interfaces() EFI_ALREADY_STARTED
If we create the same device path for two USB devices, this is a bug we must fix.
OK,
so you can forget my serie
In case of error in probe, the 2nd key is unbound and deactivated for the next usb commands even if the limitation is only for EFI.
This patch removes any return error in probe event callback; if something occurs in EFI registration, the device is still probed.
Signed-off-by: Patrick Delaunay patrick.delaunay@foss.st.com
lib/efi_loader/efi_disk.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index d2256713a8e7..8d53ba3bd27e 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -677,14 +677,18 @@ int efi_disk_probe(void *ctx, struct event *event) desc = dev_get_uclass_plat(dev); if (desc->uclass_id != UCLASS_EFI_LOADER) { ret = efi_disk_create_raw(dev, agent_handle); - if (ret) - return -1; + if (ret) { + log_err("efi_disk_create_raw %s failed (%d)\n", + dev->name, ret);
This isn't a message a non-developer can easily understand.
+ return 0; + } }
device_foreach_child(child, dev) { ret = efi_disk_create_part(child, agent_handle); if (ret) - return -1; + log_err("efi_disk_create_part %s failed (%d)\n",
ditto.
Best regards
Heinrich
+ dev->name, ret); }
return 0;
Patrick
EFI has no reason to block the driver remove when the associated EFI resources failed to be released.
This patch avoids DM issue when an EFI resource can't be released, for example if this resource wasn't created, for duplicated device name (error EFI_ALREADY_STARTED).
Without this patch, the U-Boot device tree is not updated for "usb stop" command because EFI stack can't free a resource; in usb_stop(), the remove operation is stopped on first device_remove() error, including a device_notify() error on any child.
And this remove error, returned by usb_stop(), is not managed in cmd/usb.c and the next "usb start" command cause a crash because all the USB devices need to be released before the next USB scan.
Signed-off-by: Patrick Delaunay patrick.delaunay@foss.st.com ---
lib/efi_loader/efi_disk.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 8d53ba3bd27e..22a0035dcde2 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -767,16 +767,20 @@ int efi_disk_remove(void *ctx, struct event *event) { enum uclass_id id; struct udevice *dev; + int ret = 0;
dev = event->data.dm.dev; id = device_get_uclass_id(dev);
if (id == UCLASS_BLK) - return efi_disk_delete_raw(dev); + ret = efi_disk_delete_raw(dev); else if (id == UCLASS_PARTITION) - return efi_disk_delete_part(dev); - else - return 0; + ret = efi_disk_delete_part(dev); + + if (ret) + log_err("%s failed for %s uclass %u (%d)\n", __func__, dev->name, id, ret); + + return 0; }
/**
On 3/8/23 14:26, Patrick Delaunay wrote:
EFI has no reason to block the driver remove when the associated EFI resources failed to be released.
This patch avoids DM issue when an EFI resource can't be released, for example if this resource wasn't created, for duplicated device name (error EFI_ALREADY_STARTED).
Without this patch, the U-Boot device tree is not updated for "usb stop" command because EFI stack can't free a resource; in usb_stop(), the remove operation is stopped on first device_remove() error, including a device_notify() error on any chil
The typical reason to return an error here is that the EFI device is still in use, i.e. a protocol installed on the EFI handle is opened by a child controller or driver. As long as the EFI handle cannot be removed we must not remove the linked DM device or we corrupt our data model.
Best regards
Heinrich
And this remove error, returned by usb_stop(), is not managed in cmd/usb.c and the next "usb start" command cause a crash because all the USB devices need to be released before the next USB scan.
Signed-off-by: Patrick Delaunay patrick.delaunay@foss.st.com
lib/efi_loader/efi_disk.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 8d53ba3bd27e..22a0035dcde2 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -767,16 +767,20 @@ int efi_disk_remove(void *ctx, struct event *event) { enum uclass_id id; struct udevice *dev;
int ret = 0;
dev = event->data.dm.dev; id = device_get_uclass_id(dev);
if (id == UCLASS_BLK)
return efi_disk_delete_raw(dev);
ret = efi_disk_delete_raw(dev);
return efi_disk_delete_part(dev);- else
return 0;
ret = efi_disk_delete_part(dev);if (ret)
log_err("%s failed for %s uclass %u (%d)\n", __func__, dev->name, id, ret);return 0; }
/**
On 3/9/23 09:54, Heinrich Schuchardt wrote:
On 3/8/23 14:26, Patrick Delaunay wrote:
EFI has no reason to block the driver remove when the associated EFI resources failed to be released.
This patch avoids DM issue when an EFI resource can't be released, for example if this resource wasn't created, for duplicated device name (error EFI_ALREADY_STARTED).
Without this patch, the U-Boot device tree is not updated for "usb stop" command because EFI stack can't free a resource; in usb_stop(), the remove operation is stopped on first device_remove() error, including a device_notify() error on any chil
The typical reason to return an error here is that the EFI device is still in use, i.e. a protocol installed on the EFI handle is opened by a child controller or driver. As long as the EFI handle cannot be removed we must not remove the linked DM device or we corrupt our data model.
Best regards
Heinrich
Ok
I get it now,
Forget my serie
Patrick
On 3/9/23 11:59, Patrick DELAUNAY wrote:
On 3/9/23 09:54, Heinrich Schuchardt wrote:
On 3/8/23 14:26, Patrick Delaunay wrote:
EFI has no reason to block the driver remove when the associated EFI resources failed to be released.
This patch avoids DM issue when an EFI resource can't be released, for example if this resource wasn't created, for duplicated device name (error EFI_ALREADY_STARTED).
Without this patch, the U-Boot device tree is not updated for "usb stop" command because EFI stack can't free a resource; in usb_stop(), the remove operation is stopped on first device_remove() error, including a device_notify() error on any chil
The typical reason to return an error here is that the EFI device is still in use, i.e. a protocol installed on the EFI handle is opened by a child controller or driver. As long as the EFI handle cannot be removed we must not remove the linked DM device or we corrupt our data model.
Best regards
Heinrich
Ok
I get it now,
Forget my serie
Patrick
Hello Patrick,
thanks a lot for pointing out the issues with non-unique device paths.
As it will take some time to clean up the device path generation patch 1 of the series may still make sense to avoid trouble for users using multiple USB devices.
Best regards
Heinrich
participants (3)
-
Heinrich Schuchardt -
Patrick DELAUNAY -
Patrick Delaunay