| Message ID | 20221003094459.107553-2-heinrich.schuchardt@canonical.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_driver: fix error handling | expand |
Hi Heinrich, On Mon, Oct 03, 2022 at 11:44:58AM +0200, Heinrich Schuchardt wrote: > When deleting a device or a handle we must remove the link between the two > to avoid dangling references. > > Provide function efi_unlink_dev() for this purpose. > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> > --- > include/efi_loader.h | 1 + > lib/efi_loader/efi_helper.c | 19 +++++++++++++++++++ > 2 files changed, 20 insertions(+) > > diff --git a/include/efi_loader.h b/include/efi_loader.h > index ad01395b39..5a993b0d2e 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -708,6 +708,7 @@ const char *guid_to_sha_str(const efi_guid_t *guid); > int algo_to_len(const char *algo); > > int efi_link_dev(efi_handle_t handle, struct udevice *dev); > +int efi_unlink_dev(efi_handle_t handle); > > /** > * efi_size_in_pages() - convert size in bytes to size in pages > diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c > index 8ed564e261..c71e87d118 100644 > --- a/lib/efi_loader/efi_helper.c > +++ b/lib/efi_loader/efi_helper.c > @@ -171,3 +171,22 @@ int efi_link_dev(efi_handle_t handle, struct udevice *dev) > handle->dev = dev; > return dev_tag_set_ptr(dev, DM_TAG_EFI, handle); > } > + > +/** > + * efi_unlink_dev() - unlink udevice and handle > + * > + * @handle: EFI handle to unlink > + * > + * Return: 0 on success, negative on failure > + */ > +int efi_unlink_dev(efi_handle_t handle) > +{ > + int ret; > + > + ret = dev_tag_del(handle->dev, DM_TAG_EFI); > + if (ret) > + return ret; Is handle->dev always guaranteed to hold a valid ptr? Would it make sense to add an if here and use this function everywhere instead of sprinkling dev_tag_del() around? > + handle->dev = NULL; > + > + return 0; > +} > -- > 2.37.2 > Thanks /Ilias
On 10/3/22 12:20, Ilias Apalodimas wrote: > Hi Heinrich, > > On Mon, Oct 03, 2022 at 11:44:58AM +0200, Heinrich Schuchardt wrote: >> When deleting a device or a handle we must remove the link between the two >> to avoid dangling references. >> >> Provide function efi_unlink_dev() for this purpose. >> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> >> --- >> include/efi_loader.h | 1 + >> lib/efi_loader/efi_helper.c | 19 +++++++++++++++++++ >> 2 files changed, 20 insertions(+) >> >> diff --git a/include/efi_loader.h b/include/efi_loader.h >> index ad01395b39..5a993b0d2e 100644 >> --- a/include/efi_loader.h >> +++ b/include/efi_loader.h >> @@ -708,6 +708,7 @@ const char *guid_to_sha_str(const efi_guid_t *guid); >> int algo_to_len(const char *algo); >> >> int efi_link_dev(efi_handle_t handle, struct udevice *dev); >> +int efi_unlink_dev(efi_handle_t handle); >> >> /** >> * efi_size_in_pages() - convert size in bytes to size in pages >> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c >> index 8ed564e261..c71e87d118 100644 >> --- a/lib/efi_loader/efi_helper.c >> +++ b/lib/efi_loader/efi_helper.c >> @@ -171,3 +171,22 @@ int efi_link_dev(efi_handle_t handle, struct udevice *dev) >> handle->dev = dev; >> return dev_tag_set_ptr(dev, DM_TAG_EFI, handle); >> } >> + >> +/** >> + * efi_unlink_dev() - unlink udevice and handle >> + * >> + * @handle: EFI handle to unlink >> + * >> + * Return: 0 on success, negative on failure >> + */ >> +int efi_unlink_dev(efi_handle_t handle) >> +{ >> + int ret; >> + >> + ret = dev_tag_del(handle->dev, DM_TAG_EFI); >> + if (ret) >> + return ret; > > Is handle->dev always guaranteed to hold a valid ptr? dev_tag_del() returns -EINVAL if !dev. We should not check twice. > Would it make sense to add an if here and use this function everywhere > instead of sprinkling dev_tag_del() around? We should move the calls in lib/efi_loader/efi_disk.c to efi_delete_handle() to make sure that we clean up. Best regards Heinrich > >> + handle->dev = NULL; >> + >> + return 0; >> +} >> -- >> 2.37.2 >> > > Thanks > /Ilias
diff --git a/include/efi_loader.h b/include/efi_loader.h index ad01395b39..5a993b0d2e 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -708,6 +708,7 @@ const char *guid_to_sha_str(const efi_guid_t *guid); int algo_to_len(const char *algo); int efi_link_dev(efi_handle_t handle, struct udevice *dev); +int efi_unlink_dev(efi_handle_t handle); /** * efi_size_in_pages() - convert size in bytes to size in pages diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c index 8ed564e261..c71e87d118 100644 --- a/lib/efi_loader/efi_helper.c +++ b/lib/efi_loader/efi_helper.c @@ -171,3 +171,22 @@ int efi_link_dev(efi_handle_t handle, struct udevice *dev) handle->dev = dev; return dev_tag_set_ptr(dev, DM_TAG_EFI, handle); } + +/** + * efi_unlink_dev() - unlink udevice and handle + * + * @handle: EFI handle to unlink + * + * Return: 0 on success, negative on failure + */ +int efi_unlink_dev(efi_handle_t handle) +{ + int ret; + + ret = dev_tag_del(handle->dev, DM_TAG_EFI); + if (ret) + return ret; + handle->dev = NULL; + + return 0; +}
When deleting a device or a handle we must remove the link between the two to avoid dangling references. Provide function efi_unlink_dev() for this purpose. Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> --- include/efi_loader.h | 1 + lib/efi_loader/efi_helper.c | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+)