| Message ID | 20211001050228.55183-24-takahiro.akashi@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | efi_loader: more tightly integrate UEFI disks to device model | expand |
Hi Takahiro, On Thu, 30 Sept 2021 at 23:04, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > This member field in udevice will be used to dereference from udevice > to efi_object (or efi_handle). > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > include/dm/device.h | 4 ++++ > 1 file changed, 4 insertions(+) I think this should be generalised. Can we add a simple API for attaching things to devices? Something like: config DM_TAG bool "Support tags attached to devices" enum dm_tag_t { DM_TAG_EFI = 0, DM_TAG_COUNT, }; ret = dev_tag_set_ptr(dev, DM_TAG_EFI, ptr); void *ptr = dev_tag_get_ptr(dev, DM_TAG_EFI); ulong val = dev_tag_get_val(dev, DM_TAG_EFI); Under the hood I think for now we could have a simple list of tags for all of DM: struct dmtag_node { struct list_head sibling; struct udevice *dev; enum dm_tag_t tag; union { void *ptr; ulong val; }; }; This can be useful in other situations, for example I think we need to be able to send an event when a device is probed so that other devices (with tags attached) can take action. But in any case, it makes the API separate from the data structure, so aids refactoring later. If we find that this is slow we can change the impl, but I doubt it will matter fornow. > > diff --git a/include/dm/device.h b/include/dm/device.h > index 0a9718a5b81a..33b09a836f06 100644 > --- a/include/dm/device.h > +++ b/include/dm/device.h > @@ -190,6 +190,10 @@ struct udevice { > #if CONFIG_IS_ENABLED(DM_DMA) > ulong dma_offset; > #endif > +#if CONFIG_IS_ENABLED(EFI_LOADER) > + /* link to efi_object */ > + void *efi_obj; > +#endif > }; > > /** > -- > 2.33.0 > Regards, Simon
Simon, On Sun, Oct 10, 2021 at 08:14:18AM -0600, Simon Glass wrote: > Hi Takahiro, > > On Thu, 30 Sept 2021 at 23:04, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: > > > > This member field in udevice will be used to dereference from udevice > > to efi_object (or efi_handle). > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > include/dm/device.h | 4 ++++ > > 1 file changed, 4 insertions(+) > > I think this should be generalised. > > Can we add a simple API for attaching things to devices? Something like: Ok. > config DM_TAG > bool "Support tags attached to devices" > > enum dm_tag_t { > DM_TAG_EFI = 0, > > DM_TAG_COUNT, > }; > > ret = dev_tag_set_ptr(dev, DM_TAG_EFI, ptr); > > void *ptr = dev_tag_get_ptr(dev, DM_TAG_EFI); > > ulong val = dev_tag_get_val(dev, DM_TAG_EFI); > > Under the hood I think for now we could have a simple list of tags for > all of DM: > > struct dmtag_node { > struct list_head sibling; > struct udevice *dev; > enum dm_tag_t tag; > union { > void *ptr; > ulong val; > }; > }; Just let me make sure; Do you intend that we have a *single* list of tags in the system instead of maintaining a list *per udevice*? -Takahiro Akashi > This can be useful in other situations, for example I think we need to > be able to send an event when a device is probed so that other devices > (with tags attached) can take action. But in any case, it makes the > API separate from the data structure, so aids refactoring later. > > If we find that this is slow we can change the impl, but I doubt it > will matter fornow. > > > > > diff --git a/include/dm/device.h b/include/dm/device.h > > index 0a9718a5b81a..33b09a836f06 100644 > > --- a/include/dm/device.h > > +++ b/include/dm/device.h > > @@ -190,6 +190,10 @@ struct udevice { > > #if CONFIG_IS_ENABLED(DM_DMA) > > ulong dma_offset; > > #endif > > +#if CONFIG_IS_ENABLED(EFI_LOADER) > > + /* link to efi_object */ > > + void *efi_obj; > > +#endif > > }; > > > > /** > > -- > > 2.33.0 > > > > Regards, > Simon
Hi Takahiro, On Mon, 11 Oct 2021 at 00:43, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > Simon, > > On Sun, Oct 10, 2021 at 08:14:18AM -0600, Simon Glass wrote: > > Hi Takahiro, > > > > On Thu, 30 Sept 2021 at 23:04, AKASHI Takahiro > > <takahiro.akashi@linaro.org> wrote: > > > > > > This member field in udevice will be used to dereference from udevice > > > to efi_object (or efi_handle). > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > --- > > > include/dm/device.h | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > I think this should be generalised. > > > > Can we add a simple API for attaching things to devices? Something like: > > Ok. > > > > config DM_TAG > > bool "Support tags attached to devices" > > > > enum dm_tag_t { > > DM_TAG_EFI = 0, > > > > DM_TAG_COUNT, > > }; > > > > ret = dev_tag_set_ptr(dev, DM_TAG_EFI, ptr); > > > > void *ptr = dev_tag_get_ptr(dev, DM_TAG_EFI); > > > > ulong val = dev_tag_get_val(dev, DM_TAG_EFI); > > > > Under the hood I think for now we could have a simple list of tags for > > all of DM: > > > > struct dmtag_node { > > struct list_head sibling; > > struct udevice *dev; > > enum dm_tag_t tag; > > union { > > void *ptr; > > ulong val; > > }; > > }; > > Just let me make sure; Do you intend that we have a *single* list of tags > in the system instead of maintaining a list *per udevice*? Yes I would prefer not to have a list per udevice, although the API could be adjusted to iterate through all tags for a particular udevice, if that is needed (dev_tag_first...() dev_tag_next...(). Looking at some of your other patches I think you might need to support multiple tags for EFI, if there are different things. But perhaps a list is necesary. > > -Takahiro Akashi > > > > This can be useful in other situations, for example I think we need to > > be able to send an event when a device is probed so that other devices > > (with tags attached) can take action. But in any case, it makes the > > API separate from the data structure, so aids refactoring later. > > > > If we find that this is slow we can change the impl, but I doubt it > > will matter fornow. > > [..] Regards, Simon
On 10/11/21 16:54, Simon Glass wrote: > Hi Takahiro, > > On Mon, 11 Oct 2021 at 00:43, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: >> >> Simon, >> >> On Sun, Oct 10, 2021 at 08:14:18AM -0600, Simon Glass wrote: >>> Hi Takahiro, >>> >>> On Thu, 30 Sept 2021 at 23:04, AKASHI Takahiro >>> <takahiro.akashi@linaro.org> wrote: >>>> >>>> This member field in udevice will be used to dereference from udevice >>>> to efi_object (or efi_handle). >>>> >>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>>> --- >>>> include/dm/device.h | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>> >>> I think this should be generalised. >>> >>> Can we add a simple API for attaching things to devices? Something like: >> >> Ok. >> >> >>> config DM_TAG >>> bool "Support tags attached to devices" >>> >>> enum dm_tag_t { >>> DM_TAG_EFI = 0, >>> >>> DM_TAG_COUNT, >>> }; >>> >>> ret = dev_tag_set_ptr(dev, DM_TAG_EFI, ptr); >>> >>> void *ptr = dev_tag_get_ptr(dev, DM_TAG_EFI); >>> >>> ulong val = dev_tag_get_val(dev, DM_TAG_EFI); >>> >>> Under the hood I think for now we could have a simple list of tags for >>> all of DM: >>> >>> struct dmtag_node { >>> struct list_head sibling; >>> struct udevice *dev; >>> enum dm_tag_t tag; >>> union { >>> void *ptr; >>> ulong val; >>> }; >>> }; >> >> Just let me make sure; Do you intend that we have a *single* list of tags >> in the system instead of maintaining a list *per udevice*? > > Yes I would prefer not to have a list per udevice, although the API > could be adjusted to iterate through all tags for a particular > udevice, if that is needed (dev_tag_first...() dev_tag_next...(). There will never be more than one UEFI handle for one udevice. We need a single field that points to the the handle if such a handle exists. But there will be devices for which UEFI protocols don't exist and where we need no handle. In this case the value can be NULL. Why should we complicate the picture with a list of tags? Best regards Heinrich > > Looking at some of your other patches I think you might need to > support multiple tags for EFI, if there are different things. But > perhaps a list is necesary. > >> >> -Takahiro Akashi >> >> >>> This can be useful in other situations, for example I think we need to >>> be able to send an event when a device is probed so that other devices >>> (with tags attached) can take action. But in any case, it makes the >>> API separate from the data structure, so aids refactoring later. >>> >>> If we find that this is slow we can change the impl, but I doubt it >>> will matter fornow. >>> > [..] > > Regards, > Simon >
Hi Heinrich, On Mon, 11 Oct 2021 at 09:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > On 10/11/21 16:54, Simon Glass wrote: > > Hi Takahiro, > > > > On Mon, 11 Oct 2021 at 00:43, AKASHI Takahiro > > <takahiro.akashi@linaro.org> wrote: > >> > >> Simon, > >> > >> On Sun, Oct 10, 2021 at 08:14:18AM -0600, Simon Glass wrote: > >>> Hi Takahiro, > >>> > >>> On Thu, 30 Sept 2021 at 23:04, AKASHI Takahiro > >>> <takahiro.akashi@linaro.org> wrote: > >>>> > >>>> This member field in udevice will be used to dereference from udevice > >>>> to efi_object (or efi_handle). > >>>> > >>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >>>> --- > >>>> include/dm/device.h | 4 ++++ > >>>> 1 file changed, 4 insertions(+) > >>> > >>> I think this should be generalised. > >>> > >>> Can we add a simple API for attaching things to devices? Something like: > >> > >> Ok. > >> > >> > >>> config DM_TAG > >>> bool "Support tags attached to devices" > >>> > >>> enum dm_tag_t { > >>> DM_TAG_EFI = 0, > >>> > >>> DM_TAG_COUNT, > >>> }; > >>> > >>> ret = dev_tag_set_ptr(dev, DM_TAG_EFI, ptr); > >>> > >>> void *ptr = dev_tag_get_ptr(dev, DM_TAG_EFI); > >>> > >>> ulong val = dev_tag_get_val(dev, DM_TAG_EFI); > >>> > >>> Under the hood I think for now we could have a simple list of tags for > >>> all of DM: > >>> > >>> struct dmtag_node { > >>> struct list_head sibling; > >>> struct udevice *dev; > >>> enum dm_tag_t tag; > >>> union { > >>> void *ptr; > >>> ulong val; > >>> }; > >>> }; > >> > >> Just let me make sure; Do you intend that we have a *single* list of tags > >> in the system instead of maintaining a list *per udevice*? > > > > Yes I would prefer not to have a list per udevice, although the API > > could be adjusted to iterate through all tags for a particular > > udevice, if that is needed (dev_tag_first...() dev_tag_next...(). > > There will never be more than one UEFI handle for one udevice. > We need a single field that points to the the handle if such a handle > exists. But there will be devices for which UEFI protocols don't exist > and where we need no handle. In this case the value can be NULL. > > Why should we complicate the picture with a list of tags? Let's not talk about complexity while we are discussing UEFI :-) There are other cases where we need to add info to a device. We cover almost all the cases with the uclass-private, plat and priv data attached to each device. But in some cases that is not enough, as with EFI. I have hit this before in a few other places but have tried to work around it rather than extending driver model and adding to the already-large struct udevice. But I think we are at the end of the road on that. I'd also like to look at how much (for example) uclass-plat data is used for devices, in case it would be more efficient to move it to a tag model. I should also point out you are talking about the implementation rather than the API. We can always change the impl later, so long as we have a suitable API. > > > > Looking at some of your other patches I think you might need to > > support multiple tags for EFI, if there are different things. But > > perhaps a list is necesary. > > > >> > >> -Takahiro Akashi > >> > >> > >>> This can be useful in other situations, for example I think we need to > >>> be able to send an event when a device is probed so that other devices > >>> (with tags attached) can take action. But in any case, it makes the > >>> API separate from the data structure, so aids refactoring later. > >>> > >>> If we find that this is slow we can change the impl, but I doubt it > >>> will matter fornow. > >>> Regards, Simon
On Mon, Oct 11, 2021 at 10:09:19AM -0600, Simon Glass wrote: > Hi Heinrich, > > On Mon, 11 Oct 2021 at 09:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > On 10/11/21 16:54, Simon Glass wrote: > > > Hi Takahiro, > > > > > > On Mon, 11 Oct 2021 at 00:43, AKASHI Takahiro > > > <takahiro.akashi@linaro.org> wrote: > > >> > > >> Simon, > > >> > > >> On Sun, Oct 10, 2021 at 08:14:18AM -0600, Simon Glass wrote: > > >>> Hi Takahiro, > > >>> > > >>> On Thu, 30 Sept 2021 at 23:04, AKASHI Takahiro > > >>> <takahiro.akashi@linaro.org> wrote: > > >>>> > > >>>> This member field in udevice will be used to dereference from udevice > > >>>> to efi_object (or efi_handle). > > >>>> > > >>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > >>>> --- > > >>>> include/dm/device.h | 4 ++++ > > >>>> 1 file changed, 4 insertions(+) > > >>> > > >>> I think this should be generalised. > > >>> > > >>> Can we add a simple API for attaching things to devices? Something like: > > >> > > >> Ok. > > >> > > >> > > >>> config DM_TAG > > >>> bool "Support tags attached to devices" > > >>> > > >>> enum dm_tag_t { > > >>> DM_TAG_EFI = 0, > > >>> > > >>> DM_TAG_COUNT, > > >>> }; > > >>> > > >>> ret = dev_tag_set_ptr(dev, DM_TAG_EFI, ptr); > > >>> > > >>> void *ptr = dev_tag_get_ptr(dev, DM_TAG_EFI); > > >>> > > >>> ulong val = dev_tag_get_val(dev, DM_TAG_EFI); > > >>> > > >>> Under the hood I think for now we could have a simple list of tags for > > >>> all of DM: > > >>> > > >>> struct dmtag_node { > > >>> struct list_head sibling; > > >>> struct udevice *dev; > > >>> enum dm_tag_t tag; > > >>> union { > > >>> void *ptr; > > >>> ulong val; > > >>> }; > > >>> }; > > >> > > >> Just let me make sure; Do you intend that we have a *single* list of tags > > >> in the system instead of maintaining a list *per udevice*? > > > > > > Yes I would prefer not to have a list per udevice, although the API > > > could be adjusted to iterate through all tags for a particular > > > udevice, if that is needed (dev_tag_first...() dev_tag_next...(). > > > > There will never be more than one UEFI handle for one udevice. > > We need a single field that points to the the handle if such a handle > > exists. But there will be devices for which UEFI protocols don't exist > > and where we need no handle. In this case the value can be NULL. > > > > Why should we complicate the picture with a list of tags? > > Let's not talk about complexity while we are discussing UEFI :-) > > There are other cases where we need to add info to a device. We cover > almost all the cases with the uclass-private, plat and priv data > attached to each device. But in some cases that is not enough, While I'm not sure whether it is "not enough", I used to think of using 'priv_auto' (or per_device_auto of UCLASS) to hold a pointer to efi_object, but we might see a conflicting situation in the future where some driver may also want to use 'priv_auto' for their own purpose. That is why I added an extra member to udevice. # The real benefit might be to keep the size of udevice unchanged? -Takahiro Akashi > as with > EFI. I have hit this before in a few other places but have tried to > work around it rather than extending driver model and adding to the > already-large struct udevice. But I think we are at the end of the > road on that. > > I'd also like to look at how much (for example) uclass-plat data is > used for devices, in case it would be more efficient to move it to a > tag model. > > I should also point out you are talking about the implementation > rather than the API. We can always change the impl later, so long as > we have a suitable API. > > > > > > > Looking at some of your other patches I think you might need to > > > support multiple tags for EFI, if there are different things. But > > > perhaps a list is necesary. > > > > > >> > > >> -Takahiro Akashi > > >> > > >> > > >>> This can be useful in other situations, for example I think we need to > > >>> be able to send an event when a device is probed so that other devices > > >>> (with tags attached) can take action. But in any case, it makes the > > >>> API separate from the data structure, so aids refactoring later. > > >>> > > >>> If we find that this is slow we can change the impl, but I doubt it > > >>> will matter fornow. > > >>> > > Regards, > Simon
Hi Takahiro, On Mon, 11 Oct 2021 at 20:09, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > On Mon, Oct 11, 2021 at 10:09:19AM -0600, Simon Glass wrote: > > Hi Heinrich, > > > > On Mon, 11 Oct 2021 at 09:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > > > > > On 10/11/21 16:54, Simon Glass wrote: > > > > Hi Takahiro, > > > > > > > > On Mon, 11 Oct 2021 at 00:43, AKASHI Takahiro > > > > <takahiro.akashi@linaro.org> wrote: > > > >> > > > >> Simon, > > > >> > > > >> On Sun, Oct 10, 2021 at 08:14:18AM -0600, Simon Glass wrote: > > > >>> Hi Takahiro, > > > >>> > > > >>> On Thu, 30 Sept 2021 at 23:04, AKASHI Takahiro > > > >>> <takahiro.akashi@linaro.org> wrote: > > > >>>> > > > >>>> This member field in udevice will be used to dereference from udevice > > > >>>> to efi_object (or efi_handle). > > > >>>> > > > >>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > >>>> --- > > > >>>> include/dm/device.h | 4 ++++ > > > >>>> 1 file changed, 4 insertions(+) > > > >>> > > > >>> I think this should be generalised. > > > >>> > > > >>> Can we add a simple API for attaching things to devices? Something like: > > > >> > > > >> Ok. > > > >> > > > >> > > > >>> config DM_TAG > > > >>> bool "Support tags attached to devices" > > > >>> > > > >>> enum dm_tag_t { > > > >>> DM_TAG_EFI = 0, > > > >>> > > > >>> DM_TAG_COUNT, > > > >>> }; > > > >>> > > > >>> ret = dev_tag_set_ptr(dev, DM_TAG_EFI, ptr); > > > >>> > > > >>> void *ptr = dev_tag_get_ptr(dev, DM_TAG_EFI); > > > >>> > > > >>> ulong val = dev_tag_get_val(dev, DM_TAG_EFI); > > > >>> > > > >>> Under the hood I think for now we could have a simple list of tags for > > > >>> all of DM: > > > >>> > > > >>> struct dmtag_node { > > > >>> struct list_head sibling; > > > >>> struct udevice *dev; > > > >>> enum dm_tag_t tag; > > > >>> union { > > > >>> void *ptr; > > > >>> ulong val; > > > >>> }; > > > >>> }; > > > >> > > > >> Just let me make sure; Do you intend that we have a *single* list of tags > > > >> in the system instead of maintaining a list *per udevice*? > > > > > > > > Yes I would prefer not to have a list per udevice, although the API > > > > could be adjusted to iterate through all tags for a particular > > > > udevice, if that is needed (dev_tag_first...() dev_tag_next...(). > > > > > > There will never be more than one UEFI handle for one udevice. > > > We need a single field that points to the the handle if such a handle > > > exists. But there will be devices for which UEFI protocols don't exist > > > and where we need no handle. In this case the value can be NULL. > > > > > > Why should we complicate the picture with a list of tags? > > > > Let's not talk about complexity while we are discussing UEFI :-) > > > > There are other cases where we need to add info to a device. We cover > > almost all the cases with the uclass-private, plat and priv data > > attached to each device. But in some cases that is not enough, > > While I'm not sure whether it is "not enough", I used to think of using > 'priv_auto' (or per_device_auto of UCLASS) to hold a pointer to efi_object, > but we might see a conflicting situation in the future where some driver > may also want to use 'priv_auto' for their own purpose. > That is why I added an extra member to udevice. Yes indeed, we are finding a few situations where there are not enough places to put data attached to devices. > > # The real benefit might be to keep the size of udevice unchanged? Yes, although I hope we can actually reduce it. Needs some analysis though. > > -Takahiro Akashi > > > as with > > EFI. I have hit this before in a few other places but have tried to > > work around it rather than extending driver model and adding to the > > already-large struct udevice. But I think we are at the end of the > > road on that. > > > > I'd also like to look at how much (for example) uclass-plat data is > > used for devices, in case it would be more efficient to move it to a > > tag model. > > > > I should also point out you are talking about the implementation > > rather than the API. We can always change the impl later, so long as > > we have a suitable API. > > > > > > > > > > Looking at some of your other patches I think you might need to > > > > support multiple tags for EFI, if there are different things. But > > > > perhaps a list is necesary. > > > > > > > >> > > > >> -Takahiro Akashi > > > >> > > > >> > > > >>> This can be useful in other situations, for example I think we need to > > > >>> be able to send an event when a device is probed so that other devices > > > >>> (with tags attached) can take action. But in any case, it makes the > > > >>> API separate from the data structure, so aids refactoring later. > > > >>> > > > >>> If we find that this is slow we can change the impl, but I doubt it > > > >>> will matter fornow. > > > >>> Regards, SImon
diff --git a/include/dm/device.h b/include/dm/device.h index 0a9718a5b81a..33b09a836f06 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -190,6 +190,10 @@ struct udevice { #if CONFIG_IS_ENABLED(DM_DMA) ulong dma_offset; #endif +#if CONFIG_IS_ENABLED(EFI_LOADER) + /* link to efi_object */ + void *efi_obj; +#endif }; /**
This member field in udevice will be used to dereference from udevice to efi_object (or efi_handle). Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- include/dm/device.h | 4 ++++ 1 file changed, 4 insertions(+) -- 2.33.0