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

[Sakari Ailus <sakari.ailus@xxxxxx>]

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.ailus@xxxxxxxxxxxxxxx>
> > > Acked-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> 
> 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.ailus@xxxxxx