Re: [PATCH v13 13/25] v4l: async: Allow async notifier register call succeed with no subdevs

[Sakari Ailus]

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 <sakari.ai...@linux.intel.com>
> > > Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> 
> 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(&notifier->waiting);
> > >   INIT_LIST_HEAD(&notifier->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(&notifier->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