[ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ]
Patch ad608fbcf166 changed how events were subscribed to address an issue elsewhere. As a side effect of that change, the "add" callback was called before the event subscription was added to the list of subscribed events, causing the first event queued by the add callback (and possibly other events arriving soon afterwards) to be lost.
Fix this by adding the subscription to the list before calling the "add" callback, and clean up afterwards if that fails.
Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions while accessed")
Reported-by: Dave Stevenson dave.stevenson@raspberrypi.org Signed-off-by: Sakari Ailus sakari.ailus@linux.intel.com Tested-by: Dave Stevenson dave.stevenson@raspberrypi.org Reviewed-by: Hans Verkuil hans.verkuil@cisco.com Tested-by: Hans Verkuil hans.verkuil@cisco.com Cc: stable@vger.kernel.org (for 4.14 and up) Signed-off-by: Mauro Carvalho Chehab mchehab+samsung@kernel.org --- drivers/media/v4l2-core/v4l2-event.c | 43 ++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 19 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c index fcf65f131a2ac..35901f5b3971b 100644 --- a/drivers/media/v4l2-core/v4l2-event.c +++ b/drivers/media/v4l2-core/v4l2-event.c @@ -194,6 +194,22 @@ int v4l2_event_pending(struct v4l2_fh *fh) } EXPORT_SYMBOL_GPL(v4l2_event_pending);
+static void __v4l2_event_unsubscribe(struct v4l2_subscribed_event *sev) +{ + struct v4l2_fh *fh = sev->fh; + unsigned int i; + + lockdep_assert_held(&fh->subscribe_lock); + assert_spin_locked(&fh->vdev->fh_lock); + + /* Remove any pending events for this subscription */ + for (i = 0; i < sev->in_use; i++) { + list_del(&sev->events[sev_pos(sev, i)].list); + fh->navailable--; + } + list_del(&sev->list); +} + int v4l2_event_subscribe(struct v4l2_fh *fh, const struct v4l2_event_subscription *sub, unsigned elems, const struct v4l2_subscribed_event_ops *ops) @@ -225,27 +241,23 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
spin_lock_irqsave(&fh->vdev->fh_lock, flags); found_ev = v4l2_event_subscribed(fh, sub->type, sub->id); + if (!found_ev) + list_add(&sev->list, &fh->subscribed); spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
if (found_ev) { /* Already listening */ kfree(sev); - goto out_unlock; - } - - if (sev->ops && sev->ops->add) { + } else if (sev->ops && sev->ops->add) { ret = sev->ops->add(sev, elems); if (ret) { + spin_lock_irqsave(&fh->vdev->fh_lock, flags); + __v4l2_event_unsubscribe(sev); + spin_unlock_irqrestore(&fh->vdev->fh_lock, flags); kfree(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);
return ret; @@ -280,7 +292,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh, { struct v4l2_subscribed_event *sev; unsigned long flags; - int i;
if (sub->type == V4L2_EVENT_ALL) { v4l2_event_unsubscribe_all(fh); @@ -292,14 +303,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh, spin_lock_irqsave(&fh->vdev->fh_lock, flags);
sev = v4l2_event_subscribed(fh, sub->type, sub->id); - if (sev != NULL) { - /* Remove any pending events for this subscription */ - for (i = 0; i < sev->in_use; i++) { - list_del(&sev->events[sev_pos(sev, i)].list); - fh->navailable--; - } - list_del(&sev->list); - } + if (sev != NULL) + __v4l2_event_unsubscribe(sev);
spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
On Thu, Nov 08, 2018 at 02:03:50PM +0200, Sakari Ailus wrote:
[ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ]
Patch ad608fbcf166 changed how events were subscribed to address an issue elsewhere. As a side effect of that change, the "add" callback was called before the event subscription was added to the list of subscribed events, causing the first event queued by the add callback (and possibly other events arriving soon afterwards) to be lost.
Fix this by adding the subscription to the list before calling the "add" callback, and clean up afterwards if that fails.
Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions while accessed")
Now applied, thanks.
greg k-h
On Thu, 2018-11-08 at 14:03 +0200, Sakari Ailus wrote:
[ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ]
Patch ad608fbcf166 changed how events were subscribed to address an issue elsewhere. As a side effect of that change, the "add" callback was called before the event subscription was added to the list of subscribed events, causing the first event queued by the add callback (and possibly other events arriving soon afterwards) to be lost.
Fix this by adding the subscription to the list before calling the "add" callback, and clean up afterwards if that fails.
[...]
I've queued this up for the next update, thanks.
Ben.
hello, I sent a email about 'can't wake problem' 4 days ago.
Is this problem related with mine?
epoll and vb2_poll: can't wake_up
Sun, Dec 30, 2018, 6:17 PM (4 days ago) to linux-kernel Hello, I encountered a "can't wake_up" problem when use camera on imx6.
if delay some time after 'streamon' the /dev/video0, then add fd through epoll_ctl, then the process can't be waken_up after some time.
I checked both the epoll / vb2_poll(videobuf2_core.c) code.
epoll will pass 'poll_table' structure to vb2_poll, but it only contain valid function pointer when inserting fd.
in vb2_poll, if found new data in done list, it will not call 'poll_wait'. after that, every call to vb2_poll will not contain valid poll_table, which will result in all calling to poll_wait will not work.
so if app can process frames quickly, and found frame data when inserting fd (i.e. poll_wait will not be called or not contain valid function pointer), it will not found valid frame in 'vb2_poll' finally at some time, then call 'poll_wait' to expect be waken up at following vb2_buffer_done, but no good luck.
I also checked the 'videobuf-core.c', there is no this problem.
of course, both epoll and vb2_poll are right by itself side, but the result is we can't get new frames.
I think by epoll's implementation, the user should always call poll_wait.
and it's better to split the two actions: 'wait' and 'poll' both for epoll framework and all epoll users, for example, v4l2.
am I right?
On Thu, Jan 3, 2019 at 4:17 AM Ben Hutchings ben@decadent.org.uk wrote:
On Thu, 2018-11-08 at 14:03 +0200, Sakari Ailus wrote:
[ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ]
Patch ad608fbcf166 changed how events were subscribed to address an issue elsewhere. As a side effect of that change, the "add" callback was called before the event subscription was added to the list of subscribed events, causing the first event queued by the add callback (and possibly other events arriving soon afterwards) to be lost.
Fix this by adding the subscription to the list before calling the "add" callback, and clean up afterwards if that fails.
[...]
I've queued this up for the next update, thanks.
Ben.
-- Ben Hutchings Absolutum obsoletum. (If it works, it's out of date.) - Stafford Beer
On 01/03/2019 01:58 AM, Yi Qingliang wrote:
hello, I sent a email about 'can't wake problem' 4 days ago.
Is this problem related with mine?
No, it's unrelated.
I'll take a look at vb2_poll next week.
Regards,
Hans
epoll and vb2_poll: can't wake_up
Sun, Dec 30, 2018, 6:17 PM (4 days ago) to linux-kernel Hello, I encountered a "can't wake_up" problem when use camera on imx6.
if delay some time after 'streamon' the /dev/video0, then add fd through epoll_ctl, then the process can't be waken_up after some time.
I checked both the epoll / vb2_poll(videobuf2_core.c) code.
epoll will pass 'poll_table' structure to vb2_poll, but it only contain valid function pointer when inserting fd.
in vb2_poll, if found new data in done list, it will not call 'poll_wait'. after that, every call to vb2_poll will not contain valid poll_table, which will result in all calling to poll_wait will not work.
so if app can process frames quickly, and found frame data when inserting fd (i.e. poll_wait will not be called or not contain valid function pointer), it will not found valid frame in 'vb2_poll' finally at some time, then call 'poll_wait' to expect be waken up at following vb2_buffer_done, but no good luck.
I also checked the 'videobuf-core.c', there is no this problem.
of course, both epoll and vb2_poll are right by itself side, but the result is we can't get new frames.
I think by epoll's implementation, the user should always call poll_wait.
and it's better to split the two actions: 'wait' and 'poll' both for epoll framework and all epoll users, for example, v4l2.
am I right?
On Thu, Jan 3, 2019 at 4:17 AM Ben Hutchings ben@decadent.org.uk wrote:
On Thu, 2018-11-08 at 14:03 +0200, Sakari Ailus wrote:
[ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ]
Patch ad608fbcf166 changed how events were subscribed to address an issue elsewhere. As a side effect of that change, the "add" callback was called before the event subscription was added to the list of subscribed events, causing the first event queued by the add callback (and possibly other events arriving soon afterwards) to be lost.
Fix this by adding the subscription to the list before calling the "add" callback, and clean up afterwards if that fails.
[...]
I've queued this up for the next update, thanks.
Ben.
-- Ben Hutchings Absolutum obsoletum. (If it works, it's out of date.) - Stafford Beer
Ok, thanks a lot.
On Thu, Jan 3, 2019 at 6:15 PM Hans Verkuil hverkuil@xs4all.nl wrote:
On 01/03/2019 01:58 AM, Yi Qingliang wrote:
hello, I sent a email about 'can't wake problem' 4 days ago.
Is this problem related with mine?
No, it's unrelated.
I'll take a look at vb2_poll next week.
Regards,
Hansepoll and vb2_poll: can't wake_up
Sun, Dec 30, 2018, 6:17 PM (4 days ago) to linux-kernel Hello, I encountered a "can't wake_up" problem when use camera on imx6.
if delay some time after 'streamon' the /dev/video0, then add fd through epoll_ctl, then the process can't be waken_up after some time.
I checked both the epoll / vb2_poll(videobuf2_core.c) code.
epoll will pass 'poll_table' structure to vb2_poll, but it only contain valid function pointer when inserting fd.
in vb2_poll, if found new data in done list, it will not call 'poll_wait'. after that, every call to vb2_poll will not contain valid poll_table, which will result in all calling to poll_wait will not work.
so if app can process frames quickly, and found frame data when inserting fd (i.e. poll_wait will not be called or not contain valid function pointer), it will not found valid frame in 'vb2_poll' finally at some time, then call 'poll_wait' to expect be waken up at following vb2_buffer_done, but no good luck.
I also checked the 'videobuf-core.c', there is no this problem.
of course, both epoll and vb2_poll are right by itself side, but the result is we can't get new frames.
I think by epoll's implementation, the user should always call poll_wait.
and it's better to split the two actions: 'wait' and 'poll' both for epoll framework and all epoll users, for example, v4l2.
am I right?
On Thu, Jan 3, 2019 at 4:17 AM Ben Hutchings ben@decadent.org.uk wrote:
On Thu, 2018-11-08 at 14:03 +0200, Sakari Ailus wrote:
[ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ]
Patch ad608fbcf166 changed how events were subscribed to address an issue elsewhere. As a side effect of that change, the "add" callback was called before the event subscription was added to the list of subscribed events, causing the first event queued by the add callback (and possibly other events arriving soon afterwards) to be lost.
Fix this by adding the subscription to the list before calling the "add" callback, and clean up afterwards if that fails.
[...]
I've queued this up for the next update, thanks.
Ben.
-- Ben Hutchings Absolutum obsoletum. (If it works, it's out of date.) - Stafford Beer
linux-stable-mirror@lists.linaro.org
-
Ben Hutchings -
Greg KH -
Hans Verkuil -
Sakari Ailus -
Yi Qingliang