| Message ID | 20230308142555.2.Ie4c0c26b9f9564443ab4eb36059d16195af15fe5@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 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); > 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/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
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; } /**
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(-)