| Message ID | 1513189580-32202-5-git-send-email-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jacopo, On Wed, Dec 13, 2017 at 07:26:19PM +0100, 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. > > 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. Let me rephrase to make sure I understand the problem correctly --- A sub-device notifier is registered but the notifier's sub-device is not registered yet, and finding a match for this notifier leads, to, well problems. Is that the reason for this patch? I think there could be simpler solutions to address this. I wonder if we could simply check for sub-device notifier's v4l2_dev field, and fail in matching if it's not set. v4l2_device_register_subdev() would still need to proceed with calling v4l2_async_notifier_try_all_subdevs() and v4l2_async_notifier_try_complete() if that was the case. What do you think? > > 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 > @@ -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; > } > > 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 > + * 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 >
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@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 > @@ -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 >
On 17/12/17 13:10, Kieran Bingham wrote: > 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. Oh - I hit send and *then* my mail client wakes up and tells me Sakari reviewed two days ago. -- Kieran
Hi Sakari, On Fri, Dec 15, 2017 at 05:20:40PM +0200, Sakari Ailus wrote: > Hi Jacopo, > > On Wed, Dec 13, 2017 at 07:26:19PM +0100, 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. > > > > 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. > > Let me rephrase to make sure I understand the problem correctly --- > > A sub-device notifier is registered but the notifier's sub-device is not > registered yet, and finding a match for this notifier leads, to, well > problems. Is that the reason for this patch? Almost :) I never encountered problems registering the sub-notifier, but instead identifying it when trying to debug what's happening in v4l2-async. I had a lot of "(null)" notifiers, and that makes debug particularly painful, as in example I had unexpected matches between a subdev and a "(null)" notifier and it's pretty hard find out what's going wrong. So I post-poned registratration (and consequentially matching with the available subdevices) to a time where I know it can be identified. > > I think there could be simpler solutions to address this. > > I wonder if we could simply check for sub-device notifier's v4l2_dev field, > and fail in matching if it's not set. v4l2_device_register_subdev() would > still need to proceed with calling v4l2_async_notifier_try_all_subdevs() > and v4l2_async_notifier_try_complete() if that was the case. > > What do you think? v4l2_dev is only set in root notifiers, we maybe can check for a valid "struct device *" and refuse notifiers without an initialized one in "__v4l2_async_notifier_register()". And you're actually right here, when later the subdevice owning the just refused sub-notifier gets registered (and eventually matched) its sub-notifier will be processed anyhow, and this makes hunk "@@ -548,6 +551,20 @@" of my patch unnecessary. I will test and simplify it. Thanks > > > > > 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 > > @@ -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; > > } > > > > 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 > > + * 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 > > > > -- > Sakari Ailus > sakari.ailus@linux.intel.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]
Hi Laurent, On Sun, Dec 17, 2017 at 07:03:17PM +0200, Laurent Pinchart wrote: > 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 ? 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. 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. > > > 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 >
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+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]
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; } 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 + * 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; }; /**
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. 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(-) -- 2.7.4