Re: [PATCH v4 2/3] v4l: async: do not hold list_lock when reprobing devices

[Niklas Söderlund]

Hi Hans,

Thanks for your feedback.

On 2017-07-18 16:22:14 +0200, Hans Verkuil wrote:
> On 17/07/17 18:59, Niklas Söderlund wrote:
> > There is no good reason to hold the list_lock when reprobing the devices
> > and it prevents a clean implementation of subdevice notifiers. Move the
> > actual release of the devices outside of the loop which requires the
> > lock to be held.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c | 29 ++++++++++-------------------
> >  1 file changed, 10 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> > b/drivers/media/v4l2-core/v4l2-async.c
> > index 0acf288d7227ba97..8fc84f7962386ddd 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -206,7 +206,7 @@ void v4l2_async_notifier_unregister(struct 
> > v4l2_async_notifier *notifier)
> >     unsigned int notif_n_subdev = notifier->num_subdevs;
> >     unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS);
> >     struct device **dev;
> > -   int i = 0;
> > +   int i, count = 0;
> >  
> >     if (!notifier->v4l2_dev)
> >             return;
> > @@ -222,37 +222,28 @@ void v4l2_async_notifier_unregister(struct 
> > v4l2_async_notifier *notifier)
> >     list_del(&notifier->list);
> >  
> >     list_for_each_entry_safe(sd, tmp, &notifier->done, async_list) {
> > -           struct device *d;
> > -
> > -           d = get_device(sd->dev);
> > +           if (dev)
> > +                   dev[count] = get_device(sd->dev);
> > +           count++;
> >  
> >             if (notifier->unbind)
> >                     notifier->unbind(notifier, sd, sd->asd);
> >  
> >             v4l2_async_cleanup(sd);
> > +   }
> >  
> > -           /* If we handled USB devices, we'd have to lock the parent too 
> > */
> > -           device_release_driver(d);
> > +   mutex_unlock(&list_lock);
> >  
> > -           /*
> > -            * Store device at the device cache, in order to call
> > -            * put_device() on the final step
> > -            */
> > +   for (i = 0; i < count; i++) {
> > +           /* If we handled USB devices, we'd have to lock the parent too 
> > */
> >             if (dev)
> > -                   dev[i++] = d;
> > -           else
> > -                   put_device(d);
> > +                   device_release_driver(dev[i]);
> 
> This changes the behavior. If the alloc failed, then at least put_device was 
> still called.
> Now that no longer happens.

Yes, but also changes the behavior to also only call get_device() if the 
allocation was successful. So the behavior is kept the same as far as I 
understands it.

> 
> Frankly I don't understand this code, it is in desperate need of some 
> comments explaining
> this whole reprobing thing.

I agree that the code is in need of comments, but I feel a patch that 
separates the v4l2-async work from the re-probing work is a step in the 
right direction :-)

> 
> I have this strong feeling that this function needs to be reworked.

I also strongly agree with this.

> 
> Regards,
> 
>       Hans
> 
> >     }
> >  
> > -   mutex_unlock(&list_lock);
> > -
> >     /*
> >      * Call device_attach() to reprobe devices
> > -    *
> > -    * NOTE: If dev allocation fails, i is 0, and the whole loop won't be
> > -    * executed.
> >      */
> > -   while (i--) {
> > +   for (i = 0; dev && i < count; i++) {
> >             struct device *d = dev[i];
> >  
> >             if (d && device_attach(d) < 0) {
> > 
> 

-- 
Regards,
Niklas Söderlund