Hi Jacopo,
Thank you for the patch,
This seems like a good thing to do at a glance here, but I'll leave contextual
judgement like that to Sakari.
A few minor grammatical nits here and a question on locking.
On 13/12/17 18:26, 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
uninitialized
> '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.
>
> It is worth noting that post-poning registration of a subdevice notifier
postponing is not hyphenated.
> 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
Upscaled? Is this the right word here ? perhaps 'processed'?
"the complete() call chain cannot be process before the subdevice to which the
notifiers belong has been registered"
> 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@xxxxxxxxxx>
> ---
> 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
> @@ -25,6 +25,13 @@
> #include <media/v4l2-fwnode.h>
> #include <media/v4l2-subdev.h>
>
> +static struct device *v4l2_async_notifier_dev(
> + struct v4l2_async_notifier *notifier)
> +{
> + return notifier->v4l2_dev ? notifier->v4l2_dev->dev :
> + notifier->sd->dev;
> +}
> +
> static int v4l2_async_notifier_call_bound(struct v4l2_async_notifier *n,
> struct v4l2_subdev *subdev,
> struct v4l2_async_subdev *asd)
> @@ -124,19 +131,6 @@ static struct v4l2_async_subdev *v4l2_async_find_match(
> return NULL;
> }
>
> -/* Find the sub-device notifier registered by a sub-device driver. */
> -static struct v4l2_async_notifier *v4l2_async_find_subdev_notifier(
> - struct v4l2_subdev *sd)
> -{
> - struct v4l2_async_notifier *n;
> -
> - list_for_each_entry(n, ¬ifier_list, list)
> - if (n->sd == sd)
> - return n;
> -
> - return NULL;
> -}
> -
> /* Get v4l2_device related to the notifier if one can be found. */
> static struct v4l2_device *v4l2_async_notifier_find_v4l2_dev(
> struct v4l2_async_notifier *notifier)
> @@ -160,7 +154,7 @@ static bool v4l2_async_notifier_can_complete(
>
> list_for_each_entry(sd, ¬ifier->done, async_list) {
> struct v4l2_async_notifier *subdev_notifier =
> - v4l2_async_find_subdev_notifier(sd);
> + sd->subdev_notifier;
>
> if (subdev_notifier &&
> !v4l2_async_notifier_can_complete(subdev_notifier))
> @@ -228,7 +222,7 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
> /*
> * See if the sub-device has a notifier. If not, return here.
> */
> - subdev_notifier = v4l2_async_find_subdev_notifier(sd);
> + subdev_notifier = sd->subdev_notifier;
> if (!subdev_notifier || subdev_notifier->parent)
> return 0;
>
> @@ -294,7 +288,7 @@ static void v4l2_async_notifier_unbind_all_subdevs(
>
> list_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) {
> struct v4l2_async_notifier *subdev_notifier =
> - v4l2_async_find_subdev_notifier(sd);
> + sd->subdev_notifier;
>
> if (subdev_notifier)
> v4l2_async_notifier_unbind_all_subdevs(subdev_notifier);
> @@ -371,8 +365,7 @@ static bool v4l2_async_notifier_fwnode_has_async_subdev(
>
> static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> {
> - struct device *dev =
> - notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
> + struct device *dev = v4l2_async_notifier_dev(notifier);
> struct v4l2_async_subdev *asd;
> int ret;
> int i;
> @@ -383,6 +376,8 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> INIT_LIST_HEAD(¬ifier->waiting);
> INIT_LIST_HEAD(¬ifier->done);
>
> + notifier->owner = dev_fwnode(dev);
> +
> mutex_lock(&list_lock);
>
> for (i = 0; i < notifier->num_subdevs; i++) {
> @@ -421,6 +416,7 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
>
> /* Keep also completed notifiers on the list */
> list_add(¬ifier->list, ¬ifier_list);
> + notifier->registered = true;
>
> mutex_unlock(&list_lock);
>
> @@ -447,7 +443,7 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> return -EINVAL;
>
> notifier->v4l2_dev = v4l2_dev;
> - notifier->owner = dev_fwnode(v4l2_dev->dev);
> + notifier->registered = false;
>
> ret = __v4l2_async_notifier_register(notifier);
> if (ret)
> @@ -466,7 +462,11 @@ int v4l2_async_subdev_notifier_register(struct v4l2_subdev *sd,
> return -EINVAL;
>
> notifier->sd = sd;
> - notifier->owner = dev_fwnode(sd->dev);
> + sd->subdev_notifier = notifier;
> + notifier->registered = false;
> +
> + if (!sd->dev || !sd->fwnode)
> + return 0;
>
> ret = __v4l2_async_notifier_register(notifier);
> if (ret)
> @@ -482,12 +482,15 @@ static void __v4l2_async_notifier_unregister(
> if (!notifier || (!notifier->v4l2_dev && !notifier->sd))
> return;
>
> - v4l2_async_notifier_unbind_all_subdevs(notifier);
> + if (notifier->registered) {
> + v4l2_async_notifier_unbind_all_subdevs(notifier);
> + list_del(¬ifier->list);
> + }
>
> notifier->sd = NULL;
> notifier->v4l2_dev = NULL;
> -
> - list_del(¬ifier->list);
> + notifier->owner = NULL;
> + notifier->registered = false;
Do we need any locking of the notifier object now that it uses a flag and an
'asynchronous' registration ?
It might be OK ... but I haven't processed through the whole usage yet.
> }
>
> void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> @@ -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
unregistered
--
Regards
Kieran
> + * 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;
> + }
> + }
> +
> mutex_lock(&list_lock);
>
> INIT_LIST_HEAD(&sd->async_list);
> @@ -589,7 +606,7 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> * Complete failed. Unbind the sub-devices bound through registering
> * this async sub-device.
> */
> - subdev_notifier = v4l2_async_find_subdev_notifier(sd);
> + subdev_notifier = sd->subdev_notifier;
> if (subdev_notifier)
> v4l2_async_notifier_unbind_all_subdevs(subdev_notifier);
>
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index a15c01d..6ab04ad 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -110,6 +110,7 @@ struct v4l2_async_notifier_operations {
> * @waiting: list of struct v4l2_async_subdev, waiting for their drivers
> * @done: list of struct v4l2_subdev, already probed
> * @list: member in a global list of notifiers
> + * @registered: notifier registered complete flag
> */
> struct v4l2_async_notifier {
> const struct v4l2_async_notifier_operations *ops;
> @@ -123,6 +124,7 @@ struct v4l2_async_notifier {
> struct list_head waiting;
> struct list_head done;
> struct list_head list;
> + bool registered;
> };
>
> /**
> --
> 2.7.4
>
