| Message ID | 20190115025522.12060-4-takahiro.akashi@linaro.org |
|---|---|
| State | Superseded |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | cmd: add efitool for efi environment | expand |
On 1/15/19 3:55 AM, AKASHI Takahiro wrote: > This new field will be shown as a driver's name in "efitool drivers" > command. We can have drivers supplied by U-Boot and drivers supplied by an EFI binary that we recently installed via the bootefi command. A driver installed via the bootefi command will not have allocated memory for the extra fields. So you cannot use the name field in your "efitool drivers" command. Best regards Heinrich > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > include/efi_driver.h | 1 + > lib/efi_driver/efi_uclass.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/include/efi_driver.h b/include/efi_driver.h > index 840483a416a4..ee8867816094 100644 > --- a/include/efi_driver.h > +++ b/include/efi_driver.h > @@ -34,6 +34,7 @@ struct efi_driver_ops { > * This structure adds internal fields to the driver binding protocol. > */ > struct efi_driver_binding_extended_protocol { > + const char *name; > struct efi_driver_binding_protocol bp; > const struct efi_driver_ops *ops; > }; > diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c > index bb86ffd399c3..8bbaa02d490e 100644 > --- a/lib/efi_driver/efi_uclass.c > +++ b/lib/efi_driver/efi_uclass.c > @@ -271,6 +271,7 @@ static efi_status_t efi_add_driver(struct driver *drv) > bp->bp.stop = efi_uc_stop; > bp->bp.version = 0xffffffff; > bp->ops = drv->ops; > + bp->name = drv->name; > > ret = efi_create_handle(&bp->bp.driver_binding_handle); > if (ret != EFI_SUCCESS) { >
You raised a couple of questions to me. On Tue, Jan 15, 2019 at 04:41:08AM +0100, Heinrich Schuchardt wrote: > On 1/15/19 3:55 AM, AKASHI Takahiro wrote: > > This new field will be shown as a driver's name in "efitool drivers" > > command. > > We can have drivers supplied by U-Boot I assume that what you mention here is a UCLASS_EFI driver. What's the problem is; efi_add_driver() adds EFI_DRIVER_BINDING_PROTOCOL with *efi_driver_binding_extended_protocol* interface, which is NOT compatible with EFI_DRIVER_BINDING_PROTOCOL. On the other hand, for example, in your efi_selftest_controller test a test driver is set up by installing EFI_DRIVER_BINDING_PROTOCOL with EFI_DRIVER_BINDING_PROTOCOL interface. So we have no way to distinguish the two cases(handles) and cannot deal with them properly. > and drivers supplied by an EFI > binary that we recently installed via the bootefi command. > > A driver installed via the bootefi command will not have allocated > memory for the extra fields. There is no good example of driver of such kind. I don't know how we can retrieve a meaningful "driver name." > So you cannot use the name field in your "efitool drivers" command. Any suggestion? Thanks, -Takahiro Akashi > > Best regards > > Heinrich > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > include/efi_driver.h | 1 + > > lib/efi_driver/efi_uclass.c | 1 + > > 2 files changed, 2 insertions(+) > > > > diff --git a/include/efi_driver.h b/include/efi_driver.h > > index 840483a416a4..ee8867816094 100644 > > --- a/include/efi_driver.h > > +++ b/include/efi_driver.h > > @@ -34,6 +34,7 @@ struct efi_driver_ops { > > * This structure adds internal fields to the driver binding protocol. > > */ > > struct efi_driver_binding_extended_protocol { > > + const char *name; > > struct efi_driver_binding_protocol bp; > > const struct efi_driver_ops *ops; > > }; > > diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c > > index bb86ffd399c3..8bbaa02d490e 100644 > > --- a/lib/efi_driver/efi_uclass.c > > +++ b/lib/efi_driver/efi_uclass.c > > @@ -271,6 +271,7 @@ static efi_status_t efi_add_driver(struct driver *drv) > > bp->bp.stop = efi_uc_stop; > > bp->bp.version = 0xffffffff; > > bp->ops = drv->ops; > > + bp->name = drv->name; > > > > ret = efi_create_handle(&bp->bp.driver_binding_handle); > > if (ret != EFI_SUCCESS) { > > >
On 1/17/19 6:33 AM, AKASHI Takahiro wrote: > You raised a couple of questions to me. > > On Tue, Jan 15, 2019 at 04:41:08AM +0100, Heinrich Schuchardt wrote: >> On 1/15/19 3:55 AM, AKASHI Takahiro wrote: >>> This new field will be shown as a driver's name in "efitool drivers" >>> command. >> >> We can have drivers supplied by U-Boot > > I assume that what you mention here is a UCLASS_EFI driver. > > What's the problem is; > efi_add_driver() adds EFI_DRIVER_BINDING_PROTOCOL with > *efi_driver_binding_extended_protocol* interface, which is NOT compatible > with EFI_DRIVER_BINDING_PROTOCOL. > On the other hand, for example, in your efi_selftest_controller test > a test driver is set up by installing EFI_DRIVER_BINDING_PROTOCOL > with EFI_DRIVER_BINDING_PROTOCOL interface. > > So we have no way to distinguish the two cases(handles) and cannot > deal with them properly. Correct. It is allowable to add private fields to a protocol. EDK2 does the same. But we cannot make any assumptions about the private fields here. > >> and drivers supplied by an EFI >> binary that we recently installed via the bootefi command. >> >> A driver installed via the bootefi command will not have allocated >> memory for the extra fields. > > There is no good example of driver of such kind. > I don't know how we can retrieve a meaningful "driver name." The UEFI spec does not foresee any name field. The interesting information is: on which handle did the driver install which protocol. This information could be gathered by looping over all protocols on all handles and looking for an OpenProtocolInformation where the driver is the controller. For me it would be fine if we leave that to a later patch. Best regards Heinrich > >> So you cannot use the name field in your "efitool drivers" command. > > Any suggestion? > > Thanks, > -Takahiro Akashi > >> >> Best regards >> >> Heinrich >> >>> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>> --- >>> include/efi_driver.h | 1 + >>> lib/efi_driver/efi_uclass.c | 1 + >>> 2 files changed, 2 insertions(+) >>> >>> diff --git a/include/efi_driver.h b/include/efi_driver.h >>> index 840483a416a4..ee8867816094 100644 >>> --- a/include/efi_driver.h >>> +++ b/include/efi_driver.h >>> @@ -34,6 +34,7 @@ struct efi_driver_ops { >>> * This structure adds internal fields to the driver binding protocol. >>> */ >>> struct efi_driver_binding_extended_protocol { >>> + const char *name; >>> struct efi_driver_binding_protocol bp; >>> const struct efi_driver_ops *ops; >>> }; >>> diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c >>> index bb86ffd399c3..8bbaa02d490e 100644 >>> --- a/lib/efi_driver/efi_uclass.c >>> +++ b/lib/efi_driver/efi_uclass.c >>> @@ -271,6 +271,7 @@ static efi_status_t efi_add_driver(struct driver *drv) >>> bp->bp.stop = efi_uc_stop; >>> bp->bp.version = 0xffffffff; >>> bp->ops = drv->ops; >>> + bp->name = drv->name; >>> >>> ret = efi_create_handle(&bp->bp.driver_binding_handle); >>> if (ret != EFI_SUCCESS) { >>> >> >
On Thu, Jan 17, 2019 at 07:58:17AM +0100, Heinrich Schuchardt wrote: > On 1/17/19 6:33 AM, AKASHI Takahiro wrote: > > You raised a couple of questions to me. > > > > On Tue, Jan 15, 2019 at 04:41:08AM +0100, Heinrich Schuchardt wrote: > >> On 1/15/19 3:55 AM, AKASHI Takahiro wrote: > >>> This new field will be shown as a driver's name in "efitool drivers" > >>> command. > >> > >> We can have drivers supplied by U-Boot > > > > I assume that what you mention here is a UCLASS_EFI driver. > > > > What's the problem is; > > efi_add_driver() adds EFI_DRIVER_BINDING_PROTOCOL with > > *efi_driver_binding_extended_protocol* interface, which is NOT compatible > > with EFI_DRIVER_BINDING_PROTOCOL. > > On the other hand, for example, in your efi_selftest_controller test > > a test driver is set up by installing EFI_DRIVER_BINDING_PROTOCOL > > with EFI_DRIVER_BINDING_PROTOCOL interface. > > > > So we have no way to distinguish the two cases(handles) and cannot > > deal with them properly. > > Correct. It is allowable to add private fields to a protocol. EDK2 does > the same. But we cannot make any assumptions about the private fields here. I misunderstood something here, but anyhow, > > > >> and drivers supplied by an EFI > >> binary that we recently installed via the bootefi command. > >> > >> A driver installed via the bootefi command will not have allocated > >> memory for the extra fields. > > > > There is no good example of driver of such kind. > > I don't know how we can retrieve a meaningful "driver name." > > The UEFI spec does not foresee any name field. Edk2's shell does get a driver's name by using COMPONENT_NAME2_PROTOCOL, and we'd better do the same way. So "<NULL>" will be printed with my patch for now. -Takahiro Akashi > The interesting information is: on which handle did the driver install > which protocol. > > This information could be gathered by looping over all protocols on all > handles and looking for an OpenProtocolInformation where the driver is > the controller. > > For me it would be fine if we leave that to a later patch. > > Best regards > > Heinrich > > > > >> So you cannot use the name field in your "efitool drivers" command. > > > > Any suggestion? > > > > Thanks, > > -Takahiro Akashi > > > >> > >> Best regards > >> > >> Heinrich > >> > >>> > >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >>> --- > >>> include/efi_driver.h | 1 + > >>> lib/efi_driver/efi_uclass.c | 1 + > >>> 2 files changed, 2 insertions(+) > >>> > >>> diff --git a/include/efi_driver.h b/include/efi_driver.h > >>> index 840483a416a4..ee8867816094 100644 > >>> --- a/include/efi_driver.h > >>> +++ b/include/efi_driver.h > >>> @@ -34,6 +34,7 @@ struct efi_driver_ops { > >>> * This structure adds internal fields to the driver binding protocol. > >>> */ > >>> struct efi_driver_binding_extended_protocol { > >>> + const char *name; > >>> struct efi_driver_binding_protocol bp; > >>> const struct efi_driver_ops *ops; > >>> }; > >>> diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c > >>> index bb86ffd399c3..8bbaa02d490e 100644 > >>> --- a/lib/efi_driver/efi_uclass.c > >>> +++ b/lib/efi_driver/efi_uclass.c > >>> @@ -271,6 +271,7 @@ static efi_status_t efi_add_driver(struct driver *drv) > >>> bp->bp.stop = efi_uc_stop; > >>> bp->bp.version = 0xffffffff; > >>> bp->ops = drv->ops; > >>> + bp->name = drv->name; > >>> > >>> ret = efi_create_handle(&bp->bp.driver_binding_handle); > >>> if (ret != EFI_SUCCESS) { > >>> > >> > > >
diff --git a/include/efi_driver.h b/include/efi_driver.h index 840483a416a4..ee8867816094 100644 --- a/include/efi_driver.h +++ b/include/efi_driver.h @@ -34,6 +34,7 @@ struct efi_driver_ops { * This structure adds internal fields to the driver binding protocol. */ struct efi_driver_binding_extended_protocol { + const char *name; struct efi_driver_binding_protocol bp; const struct efi_driver_ops *ops; }; diff --git a/lib/efi_driver/efi_uclass.c b/lib/efi_driver/efi_uclass.c index bb86ffd399c3..8bbaa02d490e 100644 --- a/lib/efi_driver/efi_uclass.c +++ b/lib/efi_driver/efi_uclass.c @@ -271,6 +271,7 @@ static efi_status_t efi_add_driver(struct driver *drv) bp->bp.stop = efi_uc_stop; bp->bp.version = 0xffffffff; bp->ops = drv->ops; + bp->name = drv->name; ret = efi_create_handle(&bp->bp.driver_binding_handle); if (ret != EFI_SUCCESS) {
This new field will be shown as a driver's name in "efitool drivers" command. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- include/efi_driver.h | 1 + lib/efi_driver/efi_uclass.c | 1 + 2 files changed, 2 insertions(+)