Re: [PATCH v13 13/25] v4l: async: Allow async notifier register call succeed with no subdevs
Hi Laurent, On Tue, Sep 19, 2017 at 08:54:12PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Tuesday, 19 September 2017 18:03:48 EEST Sakari Ailus wrote: > > On Tue, Sep 19, 2017 at 05:58:32PM +0300, Sakari Ailus wrote: > > >> This skips adding the notifier to the notifier_list. Won't this result > > >> in an oops when calling list_del(¬ifier->list) in > > >> v4l2_async_notifier_unregister() ? > > > > > > Good point. I'll add initialising the list head to the register function, > > > with an appropriate comment. > > > > I'll set v4l2_dev NULL instead; no tricks with lists needed. > > Shouldn't the notifier still be added to the notifier_list ? Would there be any benefit of that? The notifier's v4l2_dev field is also used to determine whether the notifier is registered currently. If the notifier is added to the notifier list, we need to remove it in unregistration as well. -- Sakari Ailus sakari.ai...@linux.intel.com
Re: [PATCH v13 13/25] v4l: async: Allow async notifier register call succeed with no subdevs
Hi Sakari, On Tuesday, 19 September 2017 18:03:48 EEST Sakari Ailus wrote: > On Tue, Sep 19, 2017 at 05:58:32PM +0300, Sakari Ailus wrote: > >> This skips adding the notifier to the notifier_list. Won't this result > >> in an oops when calling list_del(¬ifier->list) in > >> v4l2_async_notifier_unregister() ? > > > > Good point. I'll add initialising the list head to the register function, > > with an appropriate comment. > > I'll set v4l2_dev NULL instead; no tricks with lists needed. Shouldn't the notifier still be added to the notifier_list ? -- Regards, Laurent Pinchart
Re: [PATCH v13 13/25] v4l: async: Allow async notifier register call succeed with no subdevs
On Tue, Sep 19, 2017 at 05:58:32PM +0300, Sakari Ailus wrote: > > This skips adding the notifier to the notifier_list. Won't this result in > > an > > oops when calling list_del(¬ifier->list) in > > v4l2_async_notifier_unregister() ? > > Good point. I'll add initialising the list head to the register function, > with an appropriate comment. I'll set v4l2_dev NULL instead; no tricks with lists needed. -- Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH v13 13/25] v4l: async: Allow async notifier register call succeed with no subdevs
Hi Laurent,
On Tue, Sep 19, 2017 at 03:52:02PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Tuesday, 19 September 2017 15:04:43 EEST Laurent Pinchart wrote:
> > Hi Sakari,
> >
> > Thank you for the patch.
> >
> > On Friday, 15 September 2017 17:17:12 EEST Sakari Ailus wrote:
> > > The information on how many async sub-devices would be bindable to a
> > > notifier is typically dependent on information from platform firmware and
> > > it's not driver's business to be aware of that.
> > >
> > > Many V4L2 main drivers are perfectly usable (and useful) without async
> > > sub-devices and so if there aren't any around, just proceed call the
> > > notifier's complete callback immediately without registering the notifier
> > > itself.
> > >
> > > If a driver needs to check whether there are async sub-devices available,
> > > it can be done by inspecting the notifier's num_subdevs field which tells
> > > the number of async sub-devices.
> > >
> > > Signed-off-by: Sakari Ailus
> > > Acked-by: Hans Verkuil
> >
> > Reviewed-by: Laurent Pinchart
>
> I take this back.
>
> > > ---
> > >
> > > drivers/media/v4l2-core/v4l2-async.c | 6 --
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > > b/drivers/media/v4l2-core/v4l2-async.c index 9895b610e2a0..4be2f16af051
> > > 100644
> > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > @@ -170,14 +170,16 @@ int v4l2_async_notifier_register(struct v4l2_device
> > > *v4l2_dev, struct v4l2_async_subdev *asd;
> > >
> > > int i;
> > >
> > > - if (!v4l2_dev || !notifier->num_subdevs ||
> > > - notifier->num_subdevs > V4L2_MAX_SUBDEVS)
> > > + if (!v4l2_dev || notifier->num_subdevs > V4L2_MAX_SUBDEVS)
> > >
> > > return -EINVAL;
> > >
> > > notifier->v4l2_dev = v4l2_dev;
> > > INIT_LIST_HEAD(¬ifier->waiting);
> > > INIT_LIST_HEAD(¬ifier->done);
> > >
> > > + if (!notifier->num_subdevs)
> > > + return v4l2_async_notifier_call_complete(notifier);
> > > +
>
> This skips adding the notifier to the notifier_list. Won't this result in an
> oops when calling list_del(¬ifier->list) in
> v4l2_async_notifier_unregister() ?
Good point. I'll add initialising the list head to the register function,
with an appropriate comment.
>
> > > for (i = 0; i < notifier->num_subdevs; i++) {
> > >
> > > asd = notifier->subdevs[i];
>
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH v13 13/25] v4l: async: Allow async notifier register call succeed with no subdevs
Hi Sakari,
On Tuesday, 19 September 2017 15:04:43 EEST Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Friday, 15 September 2017 17:17:12 EEST Sakari Ailus wrote:
> > The information on how many async sub-devices would be bindable to a
> > notifier is typically dependent on information from platform firmware and
> > it's not driver's business to be aware of that.
> >
> > Many V4L2 main drivers are perfectly usable (and useful) without async
> > sub-devices and so if there aren't any around, just proceed call the
> > notifier's complete callback immediately without registering the notifier
> > itself.
> >
> > If a driver needs to check whether there are async sub-devices available,
> > it can be done by inspecting the notifier's num_subdevs field which tells
> > the number of async sub-devices.
> >
> > Signed-off-by: Sakari Ailus
> > Acked-by: Hans Verkuil
>
> Reviewed-by: Laurent Pinchart
I take this back.
> > ---
> >
> > drivers/media/v4l2-core/v4l2-async.c | 6 --
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > b/drivers/media/v4l2-core/v4l2-async.c index 9895b610e2a0..4be2f16af051
> > 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -170,14 +170,16 @@ int v4l2_async_notifier_register(struct v4l2_device
> > *v4l2_dev, struct v4l2_async_subdev *asd;
> >
> > int i;
> >
> > - if (!v4l2_dev || !notifier->num_subdevs ||
> > - notifier->num_subdevs > V4L2_MAX_SUBDEVS)
> > + if (!v4l2_dev || notifier->num_subdevs > V4L2_MAX_SUBDEVS)
> >
> > return -EINVAL;
> >
> > notifier->v4l2_dev = v4l2_dev;
> > INIT_LIST_HEAD(¬ifier->waiting);
> > INIT_LIST_HEAD(¬ifier->done);
> >
> > + if (!notifier->num_subdevs)
> > + return v4l2_async_notifier_call_complete(notifier);
> > +
This skips adding the notifier to the notifier_list. Won't this result in an
oops when calling list_del(¬ifier->list) in
v4l2_async_notifier_unregister() ?
> > for (i = 0; i < notifier->num_subdevs; i++) {
> >
> > asd = notifier->subdevs[i];
--
Regards,
Laurent Pinchart
Re: [PATCH v13 13/25] v4l: async: Allow async notifier register call succeed with no subdevs
Hi Sakari,
Thank you for the patch.
On Friday, 15 September 2017 17:17:12 EEST Sakari Ailus wrote:
> The information on how many async sub-devices would be bindable to a
> notifier is typically dependent on information from platform firmware and
> it's not driver's business to be aware of that.
>
> Many V4L2 main drivers are perfectly usable (and useful) without async
> sub-devices and so if there aren't any around, just proceed call the
> notifier's complete callback immediately without registering the notifier
> itself.
>
> If a driver needs to check whether there are async sub-devices available,
> it can be done by inspecting the notifier's num_subdevs field which tells
> the number of async sub-devices.
>
> Signed-off-by: Sakari Ailus
> Acked-by: Hans Verkuil
Reviewed-by: Laurent Pinchart
> ---
> drivers/media/v4l2-core/v4l2-async.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index 9895b610e2a0..4be2f16af051
> 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -170,14 +170,16 @@ int v4l2_async_notifier_register(struct v4l2_device
> *v4l2_dev, struct v4l2_async_subdev *asd;
> int i;
>
> - if (!v4l2_dev || !notifier->num_subdevs ||
> - notifier->num_subdevs > V4L2_MAX_SUBDEVS)
> + if (!v4l2_dev || notifier->num_subdevs > V4L2_MAX_SUBDEVS)
> return -EINVAL;
>
> notifier->v4l2_dev = v4l2_dev;
> INIT_LIST_HEAD(¬ifier->waiting);
> INIT_LIST_HEAD(¬ifier->done);
>
> + if (!notifier->num_subdevs)
> + return v4l2_async_notifier_call_complete(notifier);
> +
> for (i = 0; i < notifier->num_subdevs; i++) {
> asd = notifier->subdevs[i];
--
Regards,
Laurent Pinchart
[PATCH v13 13/25] v4l: async: Allow async notifier register call succeed with no subdevs
The information on how many async sub-devices would be bindable to a
notifier is typically dependent on information from platform firmware and
it's not driver's business to be aware of that.
Many V4L2 main drivers are perfectly usable (and useful) without async
sub-devices and so if there aren't any around, just proceed call the
notifier's complete callback immediately without registering the notifier
itself.
If a driver needs to check whether there are async sub-devices available,
it can be done by inspecting the notifier's num_subdevs field which tells
the number of async sub-devices.
Signed-off-by: Sakari Ailus
Acked-by: Hans Verkuil
---
drivers/media/v4l2-core/v4l2-async.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-async.c
b/drivers/media/v4l2-core/v4l2-async.c
index 9895b610e2a0..4be2f16af051 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -170,14 +170,16 @@ int v4l2_async_notifier_register(struct v4l2_device
*v4l2_dev,
struct v4l2_async_subdev *asd;
int i;
- if (!v4l2_dev || !notifier->num_subdevs ||
- notifier->num_subdevs > V4L2_MAX_SUBDEVS)
+ if (!v4l2_dev || notifier->num_subdevs > V4L2_MAX_SUBDEVS)
return -EINVAL;
notifier->v4l2_dev = v4l2_dev;
INIT_LIST_HEAD(¬ifier->waiting);
INIT_LIST_HEAD(¬ifier->done);
+ if (!notifier->num_subdevs)
+ return v4l2_async_notifier_call_complete(notifier);
+
for (i = 0; i < notifier->num_subdevs; i++) {
asd = notifier->subdevs[i];
--
2.11.0
