Re: [PATCH 4/5] v4l2: async: Postpone subdev_notifier registration

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jacopo,

Thank you for the patch.

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 ?

> 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+renesas@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