Hi Dave, Hans,
Thanks for reporting the issue!
On Mon, Nov 05, 2018 at 01:56:40PM +0000, Dave Stevenson wrote:
> Hi Hans
>
> On Mon, 5 Nov 2018 at 13:18, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> >
> > On 11/05/2018 01:21 PM, Dave Stevenson wrote:
> > > Hi All
> > >
> > > I'm testing with 4.19 and finding that testEvents in v4l2-compliance
> > > is failing with ""failed to find event for control '%s' type %u", ie
> > > it hasn't got the event for the inital values. This is with the
> > > various BCM2835 drivers that I'm involved with.
> > >
> > > Having looked at the v4l2-core history I tried reverting "ad608fb
> > > media: v4l: event: Prevent freeing event subscriptions while
> > > accessed". The test passes again.
> > >
> > > Enabling all logging, and adding a couple of logging messages at the
> > > beginning and end of v4l2_ctrl_add_event and __v4l2_event_queue_fh
> > > error path, I get the following logs:
> > >
> > > [ 90.629999] v4l2_ctrl_add_event: ctrl a2b86fa8 "User Controls" type
> > > 6, flags 0001
> > > [ 90.630002] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980001, flags=0x1
> > > [ 91.630166] videodev: v4l2_poll: video0: poll: 00000000
> > > [ 91.630311] videodev: v4l2_poll: video0: poll: 00000000
> > > [ 91.630325] video0: VIDIOC_UNSUBSCRIBE_EVENT: type=0x3,
> > > id=0x980001, flags=0x1
> > > [ 91.630396] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1,
> > > flags 0001
> > > [ 91.630403] __v4l2_event_queue_fh: Not subscribed to event type 3 id 0x980001
> > > [ 91.630407] v4l2_ctrl_add_event: ctrl 8f6fcc61 "Brightness" type 1
> > > - initial values queued
> > > [ 91.630409] video0: VIDIOC_SUBSCRIBE_EVENT: type=0x3, id=0x980900, flags=0x1
> > > [ 92.630513] videodev: v4l2_poll: video0: poll: 00000000
> > > [ 92.630660] videodev: v4l2_poll: video0: poll: 00000000
> > > [ 92.630729] videodev: v4l2_release: video0: release
> > >
> > > So __v4l2_event_queue_fh is dropping the event as we aren't subscribed
> > > at the point that the initial values are queued.
> > >
> > > Sorry, I don't have any other devices that support subscribing for
> > > events to hand (uvcvideo passes the test as it reports unsupported). I
> > > don't have a media tree build immediately available either, but I
> > > can't see anything related to this in the recent history. I can go
> > > down that route if needed.
> > > v4l-utils repo was synced today - head at "f9881444e8 cec-compliance:
> > > wake-up on Active Source is warn for <2.0"
> > >
> > > Could someone test on other hardware to confirm that it's not the
> > > drivers I'm using? I'm fairly certain it isn't as that patch does call
> > > sev->ops->add(sev, elems); before list_add(&sev->list,
> > > &fh->subscribed), and that is guaranteed to fail if sev->ops->add
> > > immediately queues an event.
> >
> > Just to pitch in, I got similar issues when I tried to apply that commit
> > to our Cisco code base. I've been away for a week and had no time to look
> > into the cause, but it really appears that that commit was bad.
> >
> > Sakari, can you take a look at this?
Yes.
>
> Thanks for confirming that it's not just me!
>
> Swapping around the order of the list_add and ops->add fixes the
> issue, but I'm not clear enough on whether there is a chance for
> events to have been raised and need clearing to propose a full patch.
> I'm currently running with:
> diff --git a/drivers/media/v4l2-core/v4l2-event.c
> b/drivers/media/v4l2-core/v4l2-event.c
> index a3ef1f5..a997d2e 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -232,18 +234,20 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
> goto out_unlock;
> }
>
> + spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> + list_add(&sev->list, &fh->subscribed);
> + spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
> if (sev->ops && sev->ops->add) {
> ret = sev->ops->add(sev, elems);
> if (ret) {
> + spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> + list_del(&sev->list);
> + spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> kvfree(sev);
> - goto out_unlock;
> }
> }
>
> - spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> - list_add(&sev->list, &fh->subscribed);
> - spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> -
> out_unlock:
> mutex_unlock(&fh->subscribe_lock);
>
> Is there a need to iterate the sev->events list and free any
> potentially pending events there in the ops->add error path?
> We still have the subscribe_lock held, so I don't think that it
> reintroduces the issue that the patch was fixing of unsubscribing
> before subscribed.
The events queued during that time need to be removed. Indeed the issue
appears to be that the add op is called before the event is added to the
list, and therefore the check whether it's subscribed fails.
>
> This patch has also been merged to stable, so 4.14 is affected as well.
Yes, and the earlier stable kernels as well.
--
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx
