On 2017-07-18 16:50:15 +0200, Hans Verkuil wrote:
> On 18/07/17 16:39, Niklas Söderlund wrote:
> > 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(¬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.
>
> Ah, I missed that. Sorry about that.
>
> But regardless of that the device_release_driver(d) isn't called anymore.
> It's not clear at all to me whether that is a problem or not.
You are right I missed that, thanks for pointing it out, please see
bellow.
>
> >
> >>
> >> 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 :-)
>
> Would it help to simplify this function to:
>
> dev = kvmalloc_array(n_subdev, sizeof(*dev), GFP_KERNEL);
> if (!dev) {
> dev_err(notifier->v4l2_dev->dev,
> "Failed to allocate device cache!\n");
>
> mutex_lock(&list_lock);
>
> list_del(¬ifier->list);
>
> /* this assumes device_release_driver(d) isn't necessary */
> list_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) {
> if (notifier->unbind)
> notifier->unbind(notifier, sd, sd->asd);
>
> v4l2_async_cleanup(sd);
> }
>
> mutex_unlock(&list_lock);
> return;
> }
>
> ...and here the code where dev is non-NULL...
>
> Yes, there is some code duplication, but it is a lot easier to understand.
I be fine with this, or simply aborting with -ENOMEM if the allocation
fails. If the allocation fails I say we are in a lot of trouble anyhow,
as Geert pointed out the kernel would already printed a warning and
invoked the OOM-killer.
If you are OK with it I will rework the next version of this series to
introduce this behavior. Let me know what you think.
>
> Regards,
>
> Hans
>
> >
> >>
> >> 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
