From patchwork Mon Nov 5 13:56:40 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dave Stevenson X-Patchwork-Id: 52756 Received: from vger.kernel.org ([209.132.180.67]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from ) id 1gJfNB-0005Wc-SL; Mon, 05 Nov 2018 13:56:58 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729587AbeKEXQr (ORCPT + 1 other); Mon, 5 Nov 2018 18:16:47 -0500 Received: from mx07-00252a01.pphosted.com ([62.209.51.214]:33312 "EHLO mx07-00252a01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729549AbeKEXQr (ORCPT ); Mon, 5 Nov 2018 18:16:47 -0500 Received: from pps.filterd (m0102628.ppops.net [127.0.0.1]) by mx07-00252a01.pphosted.com (8.16.0.27/8.16.0.27) with SMTP id wA5Dqr7W005668 for ; Mon, 5 Nov 2018 13:56:53 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=raspberrypi.org; h=mime-version : references : in-reply-to : from : date : message-id : subject : to : cc : content-type; s=pp; bh=j7eG+mihOj1xs60ne76Zz2Y9R+wmHU1G0UoN7HG6urg=; b=utG36aZG/wodJQTqjFlLqrpJi4Fww+dUHai18sgJ6muGf1bHwNbJHh1NtJHasYRjkXyX hT5A4g8Oxgq6a7bWK2XVcJM0UHnOzP5tr519dbfNc6z1aomqvuKcaaH/+teT23GUOrlM sPukvMOOGWvR7nrKLCPSgEyIktmgDrXbdEEix5eRCLHsn2qN/wV73pxyyhdUZC7qLBGP uMDPm7xO0Ak15bcDZMMJ+lh2azFolu57ERokP0tH8US4yTUEiDabCg/M++/ayOxx/sQt aZoJkCaxg4H85WM42/x/X0ri8CoAodhd2It5koCSGULiI1N2Kwn/UmOc8JNoIDjwW4h+ 0w== Received: from mail-pl1-f199.google.com (mail-pl1-f199.google.com [209.85.214.199]) by mx07-00252a01.pphosted.com with ESMTP id 2nh1y5h1hs-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=OK) for ; Mon, 05 Nov 2018 13:56:53 +0000 Received: by mail-pl1-f199.google.com with SMTP id b3-v6so9979590plr.17 for ; Mon, 05 Nov 2018 05:56:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=raspberrypi.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=j7eG+mihOj1xs60ne76Zz2Y9R+wmHU1G0UoN7HG6urg=; b=GO91Wq9XEj7sIot4oofT37NzAAM/ULTkrdlmAkyhPvdyqJoetMex7ddmomZ0dcXb9Y 3f5IJuyugE3SFVjOzSodjZt8WN4U2+EHPCr/Y7iNEhx9/xq+DYCnv6/hNtKqX1nP8YJ3 HCI9M3VPQpA+AB58Ba+0wz1uh7Ug5KQaeS6+nrf/Ec3ag+kCXV8lfg5ZkbqhJ2AfJCoH T4LaAQVsNYNgaofydpZ1gKAZty56xfTnMHazJWHq2Jl/dJkgDZdzDtVrr+XDKJ2v7w4P BU1F7ErHrqBLUkL2LT+zwyhi3BVG9h+oglPZdv9chMQ3OV2fttXbIzGLtEdf5qNu6Dh8 q/GQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=j7eG+mihOj1xs60ne76Zz2Y9R+wmHU1G0UoN7HG6urg=; b=YZVc9L1wn08zRgDg6l43OtTkup2pH6w8iDT8lBtmW8a3A6+fvcuW7zzkQSQw/WRKuR 4H2EzS9N3IpNyW1qRQin8//cBJbGqv3MWQyVtV1PbSQrwNPBGVM4kOHPbTXs+/oX6kJT BvgXciw2l8WiaUJZxQKOiAX5c4r4omCQ/88Ax5OWk3YsyQUz3X34RUrEghr+gAzWP6gv Ut0w02CJX1rstTJPwZWOmN//GgwjlExI8I3OIIip0ASQ12beeb8Pes+VH6LURK499occ Cfo4kf49XgzK/FIOB1N/viV4aFeDGzn89S/qa5mpwQUdxslUlQPqFsBzO2uEAbiFrZh1 dpng== X-Gm-Message-State: AGRZ1gKwptLWMjgS+2X0nyr+Qd2n+++N5mde69yOHdGyC3ApEtY7AFvH O0qlm1lvzIBVUzkhsAfyABtXsckAkEbWkl92TJvlVP2aOXbkXqEjP6FIjsziN13okEGi2wsmfc0 ZpIFjHWGrmdAaxfP+ugD73Kwr4GaSqpdvCTbOzw== X-Received: by 2002:a17:902:ab83:: with SMTP id f3-v6mr17172248plr.122.1541426211387; Mon, 05 Nov 2018 05:56:51 -0800 (PST) X-Google-Smtp-Source: AJdET5emxAEHplMCSD87VbFDIcw8uSZdFP/ms3Qyh0QKhrAuwNDVYmIBc/v4CwzB0pc2waB14M7O3ioHa/9R4surNfw= X-Received: by 2002:a17:902:ab83:: with SMTP id f3-v6mr17172235plr.122.1541426211097; Mon, 05 Nov 2018 05:56:51 -0800 (PST) MIME-Version: 1.0 References: <085fa404-44ef-8391-3863-1fa90e48187c@xs4all.nl> In-Reply-To: <085fa404-44ef-8391-3863-1fa90e48187c@xs4all.nl> From: Dave Stevenson Date: Mon, 5 Nov 2018 13:56:40 +0000 Message-ID: Subject: Re: VIDIOC_SUBSCRIBE_EVENT for V4L2_EVENT_CTRL regression? To: Hans Verkuil Cc: LMML , Sakari Ailus X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:, , definitions=2018-11-05_08:, , signatures=0 Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org Hi Hans On Mon, 5 Nov 2018 at 13:18, Hans Verkuil 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? 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: 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. This patch has also been merged to stable, so 4.14 is affected as well. Dave 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);