Message ID | 1472136366.1628907.705944009.76C1558A@webmail.messagingengine.com (mailing list archive) |
---|---|
State | Not Applicable, archived |
Delegated to: | Hans Verkuil |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from <linux-media-owner@vger.kernel.org>) id 1bcwZp-0001HM-Vr; Thu, 25 Aug 2016 15:28:22 +0000 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.84_2/mailfrontend-5) with esmtp id 1bcwZn-0005DL-8T; Thu, 25 Aug 2016 17:28:21 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933792AbcHYP2F (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Thu, 25 Aug 2016 11:28:05 -0400 Received: from out2-smtp.messagingengine.com ([66.111.4.26]:35687 "EHLO out2-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933796AbcHYP2D (ORCPT <rfc822;linux-media@vger.kernel.org>); Thu, 25 Aug 2016 11:28:03 -0400 Received: from compute7.internal (compute7.nyi.internal [10.202.2.47]) by mailout.nyi.internal (Postfix) with ESMTP id EDB0B205C8; Thu, 25 Aug 2016 10:46:06 -0400 (EDT) Received: from web1 ([10.202.2.211]) by compute7.internal (MEProxy); Thu, 25 Aug 2016 10:46:07 -0400 DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=fastmail.com; h=cc :content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-sasl-enc :x-sasl-enc; s=mesmtp; bh=Hahqa/9DPbU/1Tgg4D6MhIIxIQ8=; b=jtb8y3 iRHnXZ5l+ahBJgL2i1Qdwj+07IHmxXferfWur7KOEIXEOnXx9EIPaH/X4mkqmSKJ /9EbLyO7fiKfqeO8z+ZIM5kl618bM9a4I5EtfBXNbwFtrt8MXggzB4K4vMdtW8oG aCmEQfoQEuNWnKMgP7xJEeYdIvQqFR9nmrts8= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-sasl-enc:x-sasl-enc; s=smtpout; bh=Hahqa/9DPbU/1Tg g4D6MhIIxIQ8=; b=d+rd/UZzr6dcVoXVHcvxYLacMdELS37VuUMvoQCRKX0g+/w ckpdd80bQP0X6AK30+C9axw2k/Z2aB+cnygSyOpPDDCsbwmiKJ7t9VTmGaE5Gq6d 5FQszlR0shtALatQ7KnHkt1OU5jraOaEy8p4xjfEvYmjT/2SROG0sTAmL2E8= Received: by mailuser.nyi.internal (Postfix, from userid 99) id C292F6A4BD; Thu, 25 Aug 2016 10:46:06 -0400 (EDT) Message-Id: <1472136366.1628907.705944009.76C1558A@webmail.messagingengine.com> X-Sasl-Enc: 6phudXRe+A4MXjImNo5Kpd4b9WOnk7L8tsDzYv8e6Mxp 1472136366 From: Andrey Utkin <andrey_utkin@fastmail.com> To: Mauro Carvalho Chehab <mchehab@s-opensource.com> Cc: Linux Media Mailing List <linux-media@vger.kernel.org>, Mauro Carvalho Chehab <mchehab@infradead.org>, Bluecherry Maintainers <maintainers@bluecherrydvr.com>, Andrey Utkin <andrey.utkin@corp.bluecherry.net>, Mauro Carvalho Chehab <mchehab@kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain X-Mailer: MessagingEngine.com Webmail Interface - ajax-5ae55197 Subject: Re: [PATCH 1/2] [media] tw5864-core: remove double irq lock code Date: Thu, 25 Aug 2016 17:46:06 +0300 In-Reply-To: <c5f789d7d85f4c4b6bcdb2b1674d6495f05ada42.1472056235.git.mchehab@s-opensource.com> References: <c5f789d7d85f4c4b6bcdb2b1674d6495f05ada42.1472056235.git.mchehab@s-opensource.com> Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2016.8.25.152417 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_2000_2999 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, DKIM_SIGNATURE 0, IN_REP_TO 0, LEGITIMATE_NEGATE 0, LEGITIMATE_SIGNS 0, MSG_THREAD 0, NO_URI_HTTPS 0, REFERENCES 0, SINGLE_URI_IN_BODY 0, URI_ENDS_IN_HTML 0, WEBMAIL_SOURCE 0, WEBMAIL_XMAILER 0, __ANY_URI 0, __BOUNCE_CHALLENGE_SUBJ 0, __BOUNCE_NDR_SUBJ_EXEMPT 0, __CC_NAME 0, __CC_NAME_DIFF_FROM_ACC 0, __CC_REAL_NAMES 0, __CP_URI_IN_BODY 0, __CT 0, __CTE 0, __CT_TEXT_PLAIN 0, __DATE_TZ_RU 0, __FORWARDED_MSG 0, __FRAUD_WEBMAIL 0, __FRAUD_WEBMAIL_FROM 0, __HAS_CC_HDR 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_X_MAILER 0, __HAS_X_MAILING_LIST 0, __IN_REP_TO 0, __MIME_TEXT_ONLY 0, __MIME_VERSION 0, __MULTIPLE_RCPTS_CC_X2 0, __PHISH_SPEAR_STRUCTURE_1 0, __PHISH_SPEAR_STRUCTURE_2 0, __REFERENCES 0, __SANE_MSGID 0, __SINGLE_URI_TEXT 0, __SUBJ_ALPHA_END 0, __SUBJ_ALPHA_NEGATE 0, __TO_MALFORMED_2 0, __TO_NAME 0, __TO_NAME_DIFF_FROM_ACC 0, __TO_REAL_NAMES 0, __URI_IN_BODY 0, __URI_NO_WWW 0, __URI_NS , __URI_WITH_PATH 0' |
Commit Message
Andrey Utkin
Aug. 25, 2016, 2:46 p.m. UTC
For some reason (maybe "unlisted recipients"?), my reply didn't get through to maillists. Resending my reply. On Wed, Aug 24, 2016, at 19:30, Mauro Carvalho Chehab wrote: > As warned by smatch: > drivers/media/pci/tw5864/tw5864-core.c:160 tw5864_h264_isr() error: double lock 'irqsave:flags' > drivers/media/pci/tw5864/tw5864-core.c:174 tw5864_h264_isr() error: double unlock 'irqsave:flags' > > Remove the IRQ duplicated lock. > > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> > --- > drivers/media/pci/tw5864/tw5864-core.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/media/pci/tw5864/tw5864-core.c > b/drivers/media/pci/tw5864/tw5864-core.c > index 440cd7bb8d04..e3d884e963c0 100644 > --- a/drivers/media/pci/tw5864/tw5864-core.c > +++ b/drivers/media/pci/tw5864/tw5864-core.c > @@ -157,12 +157,10 @@ static void tw5864_h264_isr(struct tw5864_dev *dev) > > cur_frame = next_frame; > > - spin_lock_irqsave(&input->slock, flags); > input->frame_seqno++; > input->frame_gop_seqno++; > if (input->frame_gop_seqno >= input->gop) > input->frame_gop_seqno = 0; > - spin_unlock_irqrestore(&input->slock, flags); > } else { > dev_err(&dev->pci->dev, > "Skipped frame on input %d because all buffers busy\n", Thank you very much for catching this issue, but NACK on the patch. These two lock operations are on different spinlocks. One for device, another one for input (a subordinate entity of device). What is superfluous here is second _irqsave. Also "flags" variable reuse is wrong. So what would be right, in my opinion, is the following (going to submit this patch): "Skipped frame on input %d because all buffers busy\n", -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Comments
Hi Mauro, Have you received my letter which is cited below? I also tried to get in touch with you on #linuxtv. On Thu, Aug 25, 2016 at 05:46:06PM +0300, Andrey Utkin wrote: > For some reason (maybe "unlisted recipients"?), my reply didn't get > through to maillists. Resending my reply. > > On Wed, Aug 24, 2016, at 19:30, Mauro Carvalho Chehab wrote: > > As warned by smatch: > > drivers/media/pci/tw5864/tw5864-core.c:160 tw5864_h264_isr() error: double lock 'irqsave:flags' > > drivers/media/pci/tw5864/tw5864-core.c:174 tw5864_h264_isr() error: double unlock 'irqsave:flags' > > > > Remove the IRQ duplicated lock. > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> > > --- > > drivers/media/pci/tw5864/tw5864-core.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/media/pci/tw5864/tw5864-core.c > > b/drivers/media/pci/tw5864/tw5864-core.c > > index 440cd7bb8d04..e3d884e963c0 100644 > > --- a/drivers/media/pci/tw5864/tw5864-core.c > > +++ b/drivers/media/pci/tw5864/tw5864-core.c > > @@ -157,12 +157,10 @@ static void tw5864_h264_isr(struct tw5864_dev *dev) > > > > cur_frame = next_frame; > > > > - spin_lock_irqsave(&input->slock, flags); > > input->frame_seqno++; > > input->frame_gop_seqno++; > > if (input->frame_gop_seqno >= input->gop) > > input->frame_gop_seqno = 0; > > - spin_unlock_irqrestore(&input->slock, flags); > > } else { > > dev_err(&dev->pci->dev, > > "Skipped frame on input %d because all buffers busy\n", > > > Thank you very much for catching this issue, but NACK on the patch. > > These two lock operations are on different spinlocks. One for device, > another > one for input (a subordinate entity of device). What is superfluous here > is > second _irqsave. Also "flags" variable reuse is wrong. So what would be > right, > in my opinion, is the following (going to submit this patch): > > diff --git a/drivers/media/pci/tw5864/tw5864-core.c > b/drivers/media/pci/tw5864/tw5864-core.c > index 440cd7b..1d43b96 100644 > --- a/drivers/media/pci/tw5864/tw5864-core.c > +++ b/drivers/media/pci/tw5864/tw5864-core.c > @@ -157,12 +157,12 @@ static void tw5864_h264_isr(struct tw5864_dev > *dev) > > cur_frame = next_frame; > > - spin_lock_irqsave(&input->slock, flags); > + spin_lock(&input->slock); > input->frame_seqno++; > input->frame_gop_seqno++; > if (input->frame_gop_seqno >= input->gop) > input->frame_gop_seqno = 0; > - spin_unlock_irqrestore(&input->slock, flags); > + spin_unlock(&input->slock); > } else { > dev_err(&dev->pci->dev, > "Skipped frame on input %d because all buffers > busy\n", > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Just received a notification from patchwork that my alternative patch on this was rejected as not applicable. Thanks to Hans Verkuil - he has applied a reversion of Mauro's wrong patch and also applied my patch in his repo, git://linuxtv.org/hverkuil/media_tree.git (branch for-v4.9c). As I see the wrong patch is in git://linuxtv.org/media_tree.git master branch, and is considered "merged", but I would love to see that wrong patch (and reversion of it) dropped before it gets into linux-next (I don't know whether media tree has a tradition to not reset branches). -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/pci/tw5864/tw5864-core.c b/drivers/media/pci/tw5864/tw5864-core.c index 440cd7b..1d43b96 100644 --- a/drivers/media/pci/tw5864/tw5864-core.c +++ b/drivers/media/pci/tw5864/tw5864-core.c @@ -157,12 +157,12 @@ static void tw5864_h264_isr(struct tw5864_dev *dev) cur_frame = next_frame; - spin_lock_irqsave(&input->slock, flags); + spin_lock(&input->slock); input->frame_seqno++; input->frame_gop_seqno++; if (input->frame_gop_seqno >= input->gop) input->frame_gop_seqno = 0; - spin_unlock_irqrestore(&input->slock, flags); + spin_unlock(&input->slock); } else { dev_err(&dev->pci->dev,