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+renesas@xxxxxxxxxxxx>
> > ---
> > 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(¬ifier->list);
> >
> > list_for_each_entry_safe(sd, tmp, ¬ifier->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
