| Message ID | 20230308142555.1.I43130d8c0b1b4b863e2cbd9bcb26e07e44e5e235@changeid |
|---|---|
| State | Changes Requested |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi: remove error in efi_disk_probe/efi_disk_remove | expand |
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
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;
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(-)