On Tue, Jul 25, 2017 at 12:39 PM, Heinrich Schuchardt <xypron.g...@gmx.de> wrote: > On 07/25/2017 02:21 PM, Rob Clark wrote: >> In some cases it is useful to defer creation of the protocol interface >> object. So add back an optional ->open() hook that is used if >> protcol_interface is NULL. >> >> I've slightly simplified the fxn ptr signature to remove unneeded args, >> and so compiler will complain if patches that used the "old way" are, >> and which do not need this extra complexity, are rebased. >> >> Signed-off-by: Rob Clark <robdcl...@gmail.com> >> --- >> v2: also handle this properly in locate_protocol as agraf pointed out >> >> Note that I might be misunderstanding what the EFI application expects >> when looking up device-path on a file handle.. if this is only expected >> to be the device path up to the partition object (and not including the >> file-path part of the path), then maybe ->open() isn't *as* important >> as I thought it would be. OTOH I think it would be cleaner to use for >> loaded_image_info_obj and bootefi_device_obj.. currently I have to >> patch in the real boot device-path in efi_set_bootdev(), which is >> messy and fragile. >> >> I don't fully understand Heinrich's concerns, especially since the >> EFI_OPEN_PROTOCOL_INFORMATION_ENTRY stuff looks like it only cares >> about already opened protocols, which should already have their open() >> called. (And either way, since open() is optional maybe it is enough >> to just not use it for any efiobj's where it would be problematic?) >> >> So tl;dr, if this is really going to be a problem for some things that >> Heinrich has got in the pipeline, maybe (pending clarification of a >> file handle's device-path) we can get by without this. But if it is >> not a problem, then it certainly seems useful in a few places. >> >> include/efi_loader.h | 9 ++++++++- >> lib/efi_loader/efi_boottime.c | 30 ++++++++++++++++++++++++------ >> 2 files changed, 32 insertions(+), 7 deletions(-) >> >> diff --git a/include/efi_loader.h b/include/efi_loader.h >> index 48eeb4cdd8..a9888afb04 100644 >> --- a/include/efi_loader.h >> +++ b/include/efi_loader.h >> @@ -41,10 +41,17 @@ extern unsigned int __efi_runtime_rel_start, >> __efi_runtime_rel_stop; >> /* >> * When the UEFI payload wants to open a protocol on an object to get its >> * interface (usually a struct with callback functions), this struct maps >> the >> - * protocol GUID to the respective protocol interface */ >> + * protocol GUID to the respective protocol interface. >> + * >> + * The optional ->open() fxn can be used for cases where the protocol >> + * interface is constructed on-demand, and is called if protocol_interface >> + * is NULL. >> + */ >> struct efi_handler { >> const efi_guid_t *guid; >> void *protocol_interface; >> + efi_status_t (EFIAPI *open)(void *handle, const efi_guid_t *protocol, >> + void **protocol_interface); >> }; >> >> /* >> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c >> index 7d45c18ff7..80d9024a53 100644 >> --- a/lib/efi_loader/efi_boottime.c >> +++ b/lib/efi_loader/efi_boottime.c >> @@ -903,6 +903,23 @@ out: >> return EFI_EXIT(r); >> } >> >> +static efi_status_t open_protocol(struct efi_object *efiobj, >> + struct efi_handler *handler, >> + void **protocol_interface) >> +{ >> + efi_status_t ret = EFI_SUCCESS; >> + >> + if (!handler->protocol_interface) { >> + ret = handler->open(efiobj->handle, >> + handler->guid, >> + &handler->protocol_interface); >> + } >> + *protocol_interface = >> + handler->protocol_interface; >> + >> + return ret; >> +} >> + >> static efi_status_t EFIAPI efi_locate_protocol(efi_guid_t *protocol, >> void *registration, >> void **protocol_interface) >> @@ -925,9 +942,10 @@ static efi_status_t EFIAPI >> efi_locate_protocol(efi_guid_t *protocol, >> if (!handler->guid) >> continue; >> if (!guidcmp(handler->guid, protocol)) { >> - *protocol_interface = >> - handler->protocol_interface; >> - return EFI_EXIT(EFI_SUCCESS); >> + efi_status_t ret; >> + ret = open_protocol(efiobj, handler, >> + protocol_interface); > > NAK > > With your design a driver could decide that on the first call of > open_protocol it exposes a protocol and on the second it does not.
Not true, please read the patch again. ->open() is only called the first time. > Or LocateHandleBuffer return an object meant to support a protocol and > then opening is refused. > > The UEFI standard assumes that the supported protocols can be reliably > be determined once and for all. > > If anything, please, use an init function of type: > void init(efi_guid guid); > Simply pass the protocol guid to the init function of the handler. > > How many ms and kiB do you think you will save when starting grub with > deferred initialization? I did confirm that the device-path for an file object should include the file path. So this also avoids needing to construct a device path on each file open, which will most likely never be accessed. BR, -R > Best regards > > Heinrich > > >> + return EFI_EXIT(ret); >> } >> } >> } >> @@ -1061,12 +1079,12 @@ static efi_status_t EFIAPI efi_open_protocol( >> if (!hprotocol) >> continue; >> if (!guidcmp(hprotocol, protocol)) { >> + r = EFI_SUCCESS; >> if (attributes != >> EFI_OPEN_PROTOCOL_TEST_PROTOCOL) { >> - *protocol_interface = >> - handler->protocol_interface; >> + r = open_protocol(efiobj, handler, >> + protocol_interface); >> } >> - r = EFI_SUCCESS; >> goto out; >> } >> } >> > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot