Message ID | 4E7E43A2.3020905@yahoo.com (mailing list archive) |
---|---|
State | Accepted, archived |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.72) (envelope-from <linux-media-owner@vger.kernel.org>) id 1R7ZFc-0000rg-UM; Sat, 24 Sep 2011 22:55:32 +0200 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.75/mailfrontend-2) with esmtp id 1R7ZFc-00006R-GP; Sat, 24 Sep 2011 22:55:08 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752307Ab1IXUzE (ORCPT <rfc822;patchwork@linuxtv.org> + 5 others); Sat, 24 Sep 2011 16:55:04 -0400 Received: from nm3.bt.bullet.mail.ukl.yahoo.com ([217.146.183.201]:42543 "HELO nm3.bt.bullet.mail.ukl.yahoo.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1752015Ab1IXUzC (ORCPT <rfc822;linux-media@vger.kernel.org>); Sat, 24 Sep 2011 16:55:02 -0400 Received: from [217.146.183.198] by nm3.bt.bullet.mail.ukl.yahoo.com with NNFMP; 24 Sep 2011 20:55:01 -0000 Received: from [217.146.183.206] by tm4.bt.bullet.mail.ukl.yahoo.com with NNFMP; 24 Sep 2011 20:55:01 -0000 Received: from [127.0.0.1] by omp1004.bt.mail.ukl.yahoo.com with NNFMP; 24 Sep 2011 20:55:01 -0000 X-Yahoo-Newman-Id: 850435.6728.bm@omp1004.bt.mail.ukl.yahoo.com Received: (qmail 74152 invoked from network); 24 Sep 2011 20:55:01 -0000 DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=s1024; d=yahoo.com; h=DKIM-Signature:X-Yahoo-Newman-Property:X-YMail-OSG:X-Yahoo-SMTP:Received:Received:Message-ID:Date:From:User-Agent:MIME-Version:To:CC:Subject:Content-Type; b=h+obJtFYQr9561kfzOU70+3VyaxwK0S0Uku6rV15HMoz2jrpkYu4lcjR2ufdpzVwYQaVA5CtBO8hWQ0DypCSDto4uxmgH16Nx0Vrb2IpLSuHCcbh9oN8BlR/QBe4YKU6I/kyA9joSqVlhXqby98bJn9OWf9rGkgdneAhSZsg9eg= ; DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s1024; t=1316897701; bh=0OOui5k90RSvflt7MqAlOowyUnXYj7klm8CdkKIfw6Q=; h=X-Yahoo-Newman-Property:X-YMail-OSG:X-Yahoo-SMTP:Received:Received:Message-ID:Date:From:User-Agent:MIME-Version:To:CC:Subject:Content-Type; b=XK02ijT3HEqBx1ppG9T/gCXmWDosl59552Js5o9xO+s8wQ8kCkgYcRbw4gnnxe/fj0R1wtJsxV0zAr9Tgzsor23d1F8hIjpeUyYn057HMArJYYXXZLBTpIphDCkogHOmhm9dMWkJQGzVqqhyReIlORzhxnrHsH/bMsR+SW9gyWs= X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: Ejl1LmsVM1lkx8dPdr62IzPrnYNcBQvCFECDYqbn_Pvpemb qne6knxDPIWQblLdK4TK46UDQgE6ElWRMnfVBs1g9Q8diiObLO04U773JzXg F9HkWl9CGibeTcLRh25_pbki3TaNpP9t6_v9hNOUB0fNZnWgR1S6TKFcpdX7 xgt4Kj9KyS1fJfWHegGei6In.jWABr2W8Tg0R88yx6sSzgFXAuKORclmw.EA A.YLeo1.gwacO.CqSZOz3yBseJ7jGB0E6enI6x91H0lLxYYgk_ajwB7cQ9Gv 6knjPYQjgWAJs6cWURkCPGuuPMcc3Jdi4UwSO5_78eR2YP3qodRQfpjC6qYO kmZQBJ1S0HDUw.ZsO0NC9fkgrPYncVqsuFAVxcBi6Q2hZrjRyTjyBndXUeP7 Dy_sl X-Yahoo-SMTP: dMK34oyswBBlfKesWTI5ovDjFOUFE6shtILt.ZXnUEjQHhWq Received: from wellhouse.underworld (rankincj@86.178.153.128 with login) by smtp817.mail.ukl.yahoo.com with SMTP; 24 Sep 2011 20:55:00 +0000 GMT Received: from volcano.underworld (volcano.underworld [192.168.0.3]) by wellhouse.underworld (8.14.3/8.14.3/Debian-5+lenny1) with ESMTP id p8OKswID027515 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NOT); Sat, 24 Sep 2011 21:55:00 +0100 Message-ID: <4E7E43A2.3020905@yahoo.com> Date: Sat, 24 Sep 2011 21:54:58 +0100 From: Chris Rankin <rankincj@yahoo.com> User-Agent: Mozilla/5.0 (X11; Linux i686; rv:6.0.2) Gecko/20110906 Thunderbird/6.0.2 MIME-Version: 1.0 To: Mauro Carvalho Chehab <mchehab@redhat.com> CC: linux-media@vger.kernel.org Subject: [PATCH v3] EM28xx - fix deadlock when unplugging and replugging a DVB adapter Content-Type: multipart/mixed; boundary="------------020807060000050501050004" 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: 5.6.1.2065439, Antispam-Engine: 2.7.2.376379, Antispam-Data: 2011.9.24.204515 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' FORGED_FROM_YAHOO 0.1, MIME_TEXT_ONLY_MP_MIXED 0.05, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_2000_2999 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, ECARD_KNOWN_DOMAINS 0, __ANY_URI 0, __BAT_BOUNDARY 0, __CP_URI_IN_BODY 0, __CT 0, __CTYPE_HAS_BOUNDARY 0, __CTYPE_MULTIPART 0, __CTYPE_MULTIPART_MIXED 0, __DOMAINKEYS_YAHOO 0, __FRAUD_BODY_WEBMAIL 0, __FRAUD_WEBMAIL 0, __FRAUD_WEBMAIL_FROM 0, __FROM_YAHOO 0, __HAS_MSGID 0, __HAS_X_MAILING_LIST 0, __MIME_TEXT_ONLY 0, __MIME_VERSION 0, __MOZILLA_MSGID 0, __PHISH_SPEAR_STRUCTURE_1 0, __SANE_MSGID 0, __SUBJ_ALPHA_END 0, __TO_MALFORMED_2 0, __URI_NO_WWW 0, __URI_NS , __USER_AGENT 0' X-LSpam-Score: 1.1 (+) X-LSpam-Report: No, score=1.1 required=5.0 tests=BAYES_00=-1.9, FREEMAIL_FROM=0.001, KB_DATE_CONTAINS_TAB=2.751, RCVD_IN_DNSWL_MED=-2.3, TAB_IN_FROM=2.494, T_DKIM_INVALID=0.01, T_TVD_MIME_EPI=0.01, UNPARSEABLE_RELAY=0.001 autolearn=no |
Commit Message
Chris Rankin
Sept. 24, 2011, 8:54 p.m. UTC
Mauro,
Excuse me - I put my brain in backwards today and sent you a reverse diff by
accident! Reresending...
----------------------------------------------------------------------------
This fixes the deadlock that occurs with either multiple PCTV 290e adapters or
when a single PCTV 290e adapter is replugged.
For DVB devices, the device lock must now *not* be held when adding/removing
either a device or an extension to the respective lists. (Because
em28xx_init_dvb() will want to take the lock instead).
Conversely, for Audio-Only devices, the device lock *must* be held when
adding/removing either a device or an extension to the respective lists.
Signed-off-by: Chris Rankin <ranki...@yahoo.com>
Cheers,
Chris
Comments
Em 24-09-2011 17:54, Chris Rankin escreveu: > This fixes the deadlock that occurs with either multiple PCTV 290e adapters or when a single PCTV 290e adapter is replugged. > > For DVB devices, the device lock must now *not* be held when adding/removing either a device or an extension to the respective lists. (Because em28xx_init_dvb() will want to take the lock instead). > > Conversely, for Audio-Only devices, the device lock *must* be held when adding/removing either a device or an extension to the respective lists. > > Signed-off-by: Chris Rankin <ranki...@yahoo.com> Ok, I've applied it, but it doesn't sound a good idea to me to do: + mutex_unlock(&dev->lock); em28xx_init_extension(dev); + mutex_lock(&dev->lock); I'll later test it with some hardware and see how well this behaves in practice. Thanks, Mauro -- 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
On 25/09/11 13:49, Mauro Carvalho Chehab wrote: > Ok, I've applied it, but it doesn't sound a good idea to me to do: > > + mutex_unlock(&dev->lock); > em28xx_init_extension(dev); > + mutex_lock(&dev->lock); > > I'll later test it with some hardware and see how well this behaves > in practice. No, I don't like it either. But since a kernel mutex isn't recursive, I can't think of a better solution at the moment. Cheers, Chris -- 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
--- On Sun, 25/9/11, Mauro Carvalho Chehab <mchehab@redhat.com> wrote: > Ok, I've applied it, but it doesn't sound a good idea to me > to do: > > + mutex_unlock(&dev->lock); > em28xx_init_extension(dev); > + mutex_lock(&dev->lock); > Yes, I suppose it's the logical equivalent of moving the em28xx_init_extension(dev) call from em28xx_init_dev(), and placing it immediately after the final mutex_unlock(&dev->lock) call in em28xx_usb_probe() instead. Which would be cleaner, quite frankly. Which stage of the v4l2 initialisation triggers the race with udev? v4l2_device_register()? Cheers, Chris -- 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
Em 25-09-2011 16:28, Chris Rankin escreveu: > --- On Sun, 25/9/11, Mauro Carvalho Chehab <mchehab@redhat.com> wrote: >> Ok, I've applied it, but it doesn't sound a good idea to me >> to do: >> >> + mutex_unlock(&dev->lock); >> em28xx_init_extension(dev); >> + mutex_lock(&dev->lock); >> > > Yes, I suppose it's the logical equivalent of moving the em28xx_init_extension(dev) call from em28xx_init_dev(), and placing it immediately after the final mutex_unlock(&dev->lock) call in em28xx_usb_probe() instead. Which would be cleaner, quite frankly. > > Which stage of the v4l2 initialisation triggers the race with udev? v4l2_device_register()? Yes. Just after creating a device, udev tries to access it. This bug is more sensitive on multi-CPU machines, as udev may run on another CPU. > > Cheers, > Chris > -- 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
--- On Sun, 25/9/11, Mauro Carvalho Chehab <mchehab@redhat.com> wrote: > Yes. Just after creating a device, udev tries to access it. > This bug is more sensitive on multi-CPU machines, as udev may run on > another CPU. Heh, I have a hyper-threaded quad-core here. I suspect that counts as "multi-CPU" :-). However, I think we can agree that the first "plugging" event is not causing problems (in the modular case). The interesting thing about this first event is that it requests that the em28xx_dvb module be loaded, which in turn means that em28xx_init_extension() cannot invoke the dvb_init() function during em28xx_usb_probe(), thus avoiding the deadlock. So one of the following sequences of events must be occurring instead: Either: 1) em28xx_usb_probe() runs 2) em28xx_dvb module loads, invoking em28xx_register_extension() and dvb_init() 3) udev runs for V4L nodes Or: 1) em28xx_usb_probe() runs 2) udev runs for V4L nodes 3) em28xx_dvb module loads, invoking em28xx_register_extension() and dvb_init() The steps in both of these sequences are serialised by the dev->lock mutex. So wouldn't moving em28xx_init_extension() out of em28xx_init_dev() to the bottom of em28xx_usb_probe() (after the dev->lock mutex has been unlocked) be logically identical to the case where the em28xx-dvb module is loaded? Cheers, Chris -- 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
--- linux/drivers/media/video/em28xx/em28xx-cards.c.orig 2011-09-24 21:42:43.000000000 +0100 +++ linux/drivers/media/video/em28xx/em28xx-cards.c 2011-09-24 21:48:56.000000000 +0100 @@ -3005,7 +3005,9 @@ goto fail; } + mutex_unlock(&dev->lock); em28xx_init_extension(dev); + mutex_lock(&dev->lock); /* Save some power by putting tuner to sleep */ v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0); @@ -3301,10 +3303,10 @@ em28xx_release_resources(dev); } - em28xx_close_extension(dev); - mutex_unlock(&dev->lock); + em28xx_close_extension(dev); + if (!dev->users) { kfree(dev->alt_max_pkt_size); kfree(dev);