Hi Sakari,
On Monday, 18 December 2017 01:33:56 EET Sakari Ailus wrote:
> On Sun, Dec 17, 2017 at 07:03:17PM +0200, Laurent Pinchart wrote:
> > On Wednesday, 13 December 2017 20:26:19 EET Jacopo Mondi wrote:
> >> Currently, subdevice notifiers are tested against all available
> >> subdevices as soon as they get registered. It often happens anyway
> >> that the subdevice they are connected to is not yet initialized, as
> >> it usually gets registered later in drivers' code. This makes debug
> >> of v4l2_async particularly painful, as identifying a notifier with
> >> an unitialized subdevice is tricky as they don't have a valid
> >> 'struct device *' or 'struct fwnode_handle *' to be identified with.
> >>
> >> In order to make sure that the notifier's subdevices is initialized
> >> when the notifier is tesed against available subdevices post-pone the
> >> actual notifier registration at subdevice registration time.
> >
> > Aren't you piling yet another hack on top of an already dirty framework ?
> > How about fixing the root cause of the issue and ensuring that subdevs
> > are properly initialized when the notifier is registered ?
>
> The problem domain is quite complex --- there are multiple drivers working
> with multiple objects each here, and things can happen in a different order
> --- which needs to be supported but is sometimes missed in design.
>
> In this case the problem is that the sub-device is only registered after
> the related notifier is. If you did that the other way around, the V4L2
> async framework could well find that everything is done and proceed to call
> the complete callback, just before the async sub-device notifier is
> registered.
Sure, I understand that, but can't we guarantee that we initialize enough of
the v4l2_subdev structure before registering the notifier while keeping the
same order of notifier and subdev registration ?
> Perhaps this is, once again, a sign that we should really ditch the
> complete callback. I'd hope we could find consensus on that. It's a lot of
> trouble to support this and I feel it's an entirely arfiticial construct
> that does not really solve a problem it's intended to.
I agree. It's at least time to refactor the API, as it has grown into a
complex piece of code with an intricate and difficult to follow execution
path, without in my opinion any clear benefit of such an approach.
> >> It is worth noting that post-poning registration of a subdevice notifier
> >> does not impact on the completion of the notifiers chain, as even if a
> >> subdev notifier completes as soon as it gets registered, the complete()
> >> call chain cannot be upscaled as long as the subdevice the notifiers
> >> belongs to is not registered.
> >>
> >> Also, it is now safe to access a notifier 'struct device *' as we're now
> >> sure it is properly initialized when the notifier is actually
> >> registered.
> >>
> >> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
> >> ---
> >>
> >> drivers/media/v4l2-core/v4l2-async.c | 65 +++++++++++++++++++-----------
> >> include/media/v4l2-async.h | 2 ++
> >> 2 files changed, 43 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> >> b/drivers/media/v4l2-core/v4l2-async.c index 0a1bf1d..c13a781 100644
> >> --- a/drivers/media/v4l2-core/v4l2-async.c
> >> +++ b/drivers/media/v4l2-core/v4l2-async.c
> >
> > [snip]
> >
> >> @@ -548,6 +551,20 @@ int v4l2_async_register_subdev(struct v4l2_subdev
> >> *sd)
> >> sd->fwnode = dev_fwnode(sd->dev);
> >> }
> >>
> >> + /*
> >> + * If the subdevice has an unregisterd notifier, it's now time
> >> + * to register it.
> >> + */
> >> + subdev_notifier = sd->subdev_notifier;
> >> + if (subdev_notifier && !subdev_notifier->registered) {
> >> + ret = __v4l2_async_notifier_register(subdev_notifier);
> >> + if (ret) {
> >> + sd->fwnode = NULL;
> >> + subdev_notifier->owner = NULL;
> >> + return ret;
> >> + }
> >> + }
> >
> > This is the part I like the least in this patch set. The
> > v4l2_subdev::subdev_notifier field should really disappear, there's no
> > reason to limit subdevs to a single notifier. Implicit registration of
> > notifiers is a dirty hack in my opinion.
> >
> >> mutex_lock(&list_lock);
> >>
> >> INIT_LIST_HEAD(&sd->async_list);
> >
> > [snip]
--
Regards,
Laurent Pinchart
