Re: [RFC PATCH] RFC: Sketch of dm event notification

AKASHI Takahiro
2021-11-25T02:57:26Z

Hi Simon,

On Wed, Nov 24, 2021 at 05:12:45PM -0700, Simon Glass wrote:
> Hi Takahiro,
>
> On Tue, 16 Nov 2021 at 20:03, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > On Tue, Nov 16, 2021 at 07:48:59PM -0700, Simon Glass wrote:
> > > Hi Takahiro,
> > >
> > > On Tue, 16 Nov 2021 at 19:31, AKASHI Takahiro
> > > <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > Simon,
> > > >
> > > > On Mon, Nov 01, 2021 at 03:41:50PM +0900, AKASHI Takahiro wrote:
> > > > > Hello Simon,
> > > > >
> > > > > On Sun, Oct 31, 2021 at 09:07:01PM -0600, Simon Glass wrote:
> > > > > > This is a patch for Takahiro to look at, just for illustration. Not ready
> > > > > > for review.
> > > > >
> > > > > Thank you for posting the draft.
> > > > > At a first glance, it looks good and I don't see any specific issue
> > > > > with your implementation.
> > > >
> > > > I said OK and functionally it should work well, but I now have
> > > > some concerns:
> > > > 1) In my current implementation, I use post_probe/pre_remove hooks
> > > > of BLK device to invoke efi callback functions. In this sense,
> > > > event(POST_PROBE/ PRE_REMOVE) seems to be a duplicated feature
> > > > in some way.
> > >
> > > We should not be calling EFI functions from a BLK devince, unless it
> > > is an EFI block device, i.e. we should provide hooks for EFI or any
> > > other interested party to use.
> >
> > Even for now, part_init() is called in post_probe() (and
> > blk_create_partitions(), which is to create another type of
> > uclass device, in my patch as well).
> > So I'm not sure what functions can and should be called
> > and what not in those hooks.
> >
> > Do you want to convert part_init() to an event subscriber
> > along with my efi hooks?
>
> You could do. From my reading of the code it makes sense to do that.

Ok, but I will put this change on hold for now.

One of problems with the proposed framework of event notification
is that we cannot deterministically specify the order of callback
functions when we want to add callbacks for a number of subsystems
to a particular event.
For instance, we have callbacks for efi_disk creations as well as
partitions (part_init() & blk_create_partitions()) at POST_PROBE of
BLK devices, they should (logically) be called in this order but
currently no definite guarantee.
If we utilise a callback to create efi_disk's for "partition" devices
as well, this situation can get more complicated.
# Why? If the order of invocations is the other way around, efi_disks
# for partitions might be created before an efi_disk for a whole disk.

-Takahiro Akashi

> Regards,
> Simon
>
>
> >
> > -Takahiro Akashi
> >
> >
> > > > 2) For the rest of uclass devices which don't utilise events yet,
> > > > device_notify() is nothing but an overhead as it always tries to
> > > > go through a list of event hooks.
> > >
> > > Yes that's true, but it can be optimised to avoid useless
> > > searches/calls, since subscribers need to specify an even they are
> > > interested in.
> > >
> > > >
> > > > Event notification can be more than just a dm feature, but ...
> > > > What's your thought here?
> > >
> > > Yes, more than just a DM feature. For example I think it could help
> > > with the initcalls we have in board_f and board_r.
> > >
> > > Regards,
> > > Simon
> > >
> > > >
> > > > -Takahiro Akashi
> > > >
> > > > > Since my code has already added DM_TAG support, I'm looking forward for
> > > > > getting your final patch.
> > > > >
> > > > > The only remaining issue is *modeling* partitions :)
> > > > >
> > > > > -Takahiro Akashi
> > > > >
> > > > > > To run the test:
> > > > > >
> > > > > > ./u-boot -T -c "ut common test_event_probe"
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > ---
> > >
> > > [..]
> > >
> > > Regards,
> > > Simon