From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from galahad.ideasonboard.com ([185.26.127.97]:44076 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752118AbdLQNKp (ORCPT ); Sun, 17 Dec 2017 08:10:45 -0500 Subject: Re: [PATCH 4/5] v4l2: async: Postpone subdev_notifier registration To: Jacopo Mondi , sakari.ailus@linux.intel.com Cc: niklas.soderlund@ragnatech.se, laurent.pinchart@ideasonboard.com, linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org References: <1513189580-32202-1-git-send-email-jacopo+renesas@jmondi.org> <1513189580-32202-5-git-send-email-jacopo+renesas@jmondi.org> From: Kieran Bingham Reply-To: kieran.bingham@ideasonboard.com Message-ID: Date: Sun, 17 Dec 2017 13:10:41 +0000 MIME-Version: 1.0 In-Reply-To: <1513189580-32202-5-git-send-email-jacopo+renesas@jmondi.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 8bit Sender: linux-media-owner@vger.kernel.org List-ID: 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 > --- > 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 > #include > > +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 >