Message ID | 4E4F9E86.7030001@yahoo.com (mailing list archive) |
---|---|
State | Rejected, archived |
Headers |
Return-path: <linux-media-owner@vger.kernel.org> Envelope-to: mchehab@infradead.org Delivery-date: Sat, 20 Aug 2011 11:46:23 +0000 Received: from casper.infradead.org [85.118.1.10] by gaivota with IMAP (fetchmail-6.3.20) for <mchehab@localhost> (single-drop); Sat, 20 Aug 2011 04:50:36 -0700 (PDT) Received: from vger.kernel.org ([209.132.180.67]) by casper.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1Quk0N-0007Ye-Ai; Sat, 20 Aug 2011 11:46:23 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753525Ab1HTLqT (ORCPT <rfc822; kmpark@infradead.org> + 1 other); Sat, 20 Aug 2011 07:46:19 -0400 Received: from nm4.bt.bullet.mail.ird.yahoo.com ([212.82.108.235]:43110 "HELO nm4.bt.bullet.mail.ird.yahoo.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1752389Ab1HTLqS (ORCPT <rfc822;linux-media@vger.kernel.org>); Sat, 20 Aug 2011 07:46:18 -0400 Received: from [212.82.108.231] by nm4.bt.bullet.mail.ird.yahoo.com with NNFMP; 20 Aug 2011 11:46:17 -0000 Received: from [212.82.108.226] by tm4.bt.bullet.mail.ird.yahoo.com with NNFMP; 20 Aug 2011 11:46:17 -0000 Received: from [127.0.0.1] by omp1003.bt.mail.ird.yahoo.com with NNFMP; 20 Aug 2011 11:46:17 -0000 X-Yahoo-Newman-Id: 539015.74068.bm@omp1003.bt.mail.ird.yahoo.com Received: (qmail 65180 invoked from network); 20 Aug 2011 11:46:17 -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:References:In-Reply-To:Content-Type; b=OBKpAgM+O8o3H+itXA+xipjjPmuqOoHU+5C8/SSeTDLw/YoV2yQmmBfyCjbOwutJsGBElzeODkBpVyuGO0QwqPI7F4BwFkdhATOjXaZX+gplbsumkEiOuFwgYGBQlJT3WgYPj3CvKUN0BJ7tcDIB4dAvNQsilqGh3YpRHClU/AM= ; DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s1024; t=1313840777; bh=sndk4HG/tsLFVN0NAJr+Lr6JVDRKmmFShB/m+0R/6To=; h=X-Yahoo-Newman-Property:X-YMail-OSG:X-Yahoo-SMTP:Received:Received:Message-ID:Date:From:User-Agent:MIME-Version:To:CC:Subject:References:In-Reply-To:Content-Type; b=EUjKI0UcQax7WsZ5gBkwelRM0TQTLB3eVgqY0TxJwwwWbc0QcHFqrbjnVjuwGUKurRYBhwFvJ2FL/SppCbw3Q6SeYWaz1memXYcVn+QY+LD2RtfX43+QsCvXQdYQyJGkEHkdLtHfwawK4PbjDRPUxkgqqdyWLKOvWPTMQG52v4Q= X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: eVumnLwVM1lu8M0NNCxY4OsqCWvriPWcV_s.0FmvrBcF977 njIauvIa9egpqK9.blOURQC5dpe3E47GSjkBEiJTcbnes_w6pHegKq3llonb Dkj5t8W848MzRY6NyFkEZFQyxqvHLVJM8.yX0AFDKxtu.pt21jtV_xNBi6Ux 2VGOC9IqaGVYy9tl3B7f8JI0YqzXAWdmO1R5ZoVi79hA7E1OwcG8Nr_redkt rpYD0l9bADl4Wk4MxXQFnOPwqfhGzA7fDG4e1GZeg.O3HWUujw2MggUOmbDS SCWQVKEVNwdlUwM2aWw7GhUJy6lwiOvCzTNMO4o7zBhsMXbeUPqnsoP071Ci ooRdYRQV7SETXBxXR84CBfMW4CmXBwAHSFM7gaqF3c0tZ4ALOkIMSiF3Ld.5 O.Q-- X-Yahoo-SMTP: dMK34oyswBBlfKesWTI5ovDjFOUFE6shtILt.ZXnUEjQHhWq Received: from wellhouse.underworld (rankincj@86.182.26.38 with login) by smtp818.mail.ird.yahoo.com with SMTP; 20 Aug 2011 11:46:17 +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 p7KBkEfX011505 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NOT); Sat, 20 Aug 2011 12:46:16 +0100 Message-ID: <4E4F9E86.7030001@yahoo.com> Date: Sat, 20 Aug 2011 12:46:14 +0100 From: Chris Rankin <rankincj@yahoo.com> User-Agent: Mozilla/5.0 (X11; Linux i686; rv:5.0) Gecko/20110707 Thunderbird/5.0 MIME-Version: 1.0 To: Mauro Carvalho Chehab <mchehab@redhat.com> CC: Devin Heitmueller <dheitmueller@kernellabs.com>, linux-media@vger.kernel.org, Antti Palosaari <crope@iki.fi> Subject: [PATCH 2/2] EM28xx - fix deadlock when unplugging and replugging a DVB adapter References: <4E4D5157.2080406@yahoo.com> <CAGoCfiwk4vy1V7T=Hdz1CsywgWVpWEis0eDoh2Aqju3LYqcHfA@mail.gmail.com> <CAGoCfiw4v-ZsUPmVgOhARwNqjCVK458EV79djD625Sf+8Oghag@mail.gmail.com> <4E4D8DFD.5060800@yahoo.com> <4E4DFA65.4090508@redhat.com> In-Reply-To: <4E4DFA65.4090508@redhat.com> Content-Type: multipart/mixed; boundary="------------080105000203040004080606" Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Commit Message
Chris Rankin
Aug. 20, 2011, 11:46 a.m. UTC
The final patch removes the unplug/replug deadlock by not holding the device
mutex during dvb_init(). However, this mutex has already been locked during
device initialisation by em28xx_usb_probe() and is not released again until all
extensions have been initialised successfully.
The device mutex is not held during either em28xx_register_extension() or
em28xx_unregister_extension() any more. More importantly, I don't believe it can
safely be held by these functions because they must both - by their nature -
acquire the device list mutex before they can iterate through the device list.
In other words, while usb_probe() and usb_disconnect() acquire the device mutex
followed by the device list mutex, the register/unregister_extension() functions
would need to acquire these mutexes in the opposite order. And that sounds like
a potential deadlock.
On the other hand, the new situation is a definite improvement :-).
Signed-off-by: Chris Rankin <rankincj@yahoo.com>
Comments
Em 20-08-2011 04:46, Chris Rankin escreveu: > The final patch removes the unplug/replug deadlock by not holding the device mutex during dvb_init(). However, this mutex has already been locked during device initialisation by em28xx_usb_probe() and is not released again until all extensions have been initialised successfully. No. The extension load can happen after the usb probe phase. In practice, the only case where the extension init will happen together with the usb probe phase is when the em28xx modules are compiled builtin, as the module load is done asynchronously, and generally happens after the em28xx core to load. > The device mutex is not held during either em28xx_register_extension() or em28xx_unregister_extension() any more. More importantly, I don't believe it can safely be held by these functions because they must both - by their nature - acquire the device list mutex before they can iterate through the device list. In other words, while usb_probe() and usb_disconnect() acquire the device mutex followed by the device list mutex, the register/unregister_extension() functions would need to acquire these mutexes in the opposite order. And that sounds like a potential deadlock. > > On the other hand, the new situation is a definite improvement :-). No, it is a regression for a known bug. This patch can cause troubles. The point is that, after initializing the analog part, the device can be accessed via the V4L2 API, while it is still initializing the DVB part. This actually happens in practice, as when udev detects a new device, it opens it and calls VIDIOC_QUERYCAP. So, it ends by having a race issue, as at V4L2 open, or at some analog mode ioctl's, there are calls for em28xx_set_mode(dev, EM28XX_ANALOG_MODE). In order words, if the DVB initialization is still happening, the driver should not allow any V4L2 call, otherwise the DVB detection breaks. Maybe the proper fix would be to change the logic under em28xx_usb_probe() to not hold dev->lock anymore when the device is loading the extensions. > > Signed-off-by: Chris Rankin <rankincj@yahoo.com> > > > EM28xx-replug-deadlock.diff > > > --- linux-3.0/drivers/media/video/em28xx/em28xx-dvb.c.orig 2011-08-19 00:50:41.000000000 +0100 > +++ linux-3.0/drivers/media/video/em28xx/em28xx-dvb.c 2011-08-19 00:51:03.000000000 +0100 > @@ -542,7 +542,6 @@ > dev->dvb = dvb; > dvb->fe[0] = dvb->fe[1] = NULL; > > - mutex_lock(&dev->lock); > em28xx_set_mode(dev, EM28XX_DIGITAL_MODE); > /* init frontend */ > switch (dev->model) { > @@ -711,7 +710,6 @@ > em28xx_info("Successfully loaded em28xx-dvb\n"); > ret: > em28xx_set_mode(dev, EM28XX_SUSPEND); > - mutex_unlock(&dev->lock); > return result; > > out_free: -- 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 Sat, 20/8/11, Mauro Carvalho Chehab <mchehab@redhat.com> wrote: > No. The extension load can happen after the usb probe > phase. In practice, the only case where the extension init will happen > together with the usb probe phase is when the em28xx modules are > compiled builtin It also happens when someone plugs an adapter into the machine when the modules are already loaded. E.g. someone plugging a second adapter in, or unplugging and then replugging the same one. > Maybe the proper fix would be to change the logic under > em28xx_usb_probe() to not hold dev->lock anymore when the device is > loading the extensions. I could certainly write such a patch, although I only have a PCTV 290e adapter to test with. Is this problem unique to the em28xx-dvb module? How does the em28xx-alsa module get away with creating ALSA devices without causing a similar race condition? 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 20-08-2011 07:40, Chris Rankin escreveu: > --- On Sat, 20/8/11, Mauro Carvalho Chehab <mchehab@redhat.com> wrote: >> No. The extension load can happen after the usb probe >> phase. In practice, the only case where the extension init will happen >> together with the usb probe phase is when the em28xx modules are >> compiled builtin > > It also happens when someone plugs an adapter into the machine when the modules are already loaded. E.g. someone plugging a second adapter in, or unplugging and then replugging the same one. Yes. >> Maybe the proper fix would be to change the logic under >> em28xx_usb_probe() to not hold dev->lock anymore when the device is >> loading the extensions. > > I could certainly write such a patch, although I only have a PCTV 290e adapter to test with. We can test it on more devices. > Is this problem unique to the em28xx-dvb module? How does the em28xx-alsa module get > away with creating ALSA devices without causing a similar race condition? It might also affect alsa. Pulseaudio has the bad habit of opening the device. However, the device lock is hold when the driver is changing the lock. It should be noticed that the current mutex lock strategy is a workaround. The proper solution is to work on resource lock library that could be used when the access of a device via one API blocks the access of the same device using another API. We had some discussions about that during the USB mini-summit on last Monday. We'll probably discuss more about that during the Kernel Summit/media workshop. 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
--- linux-3.0/drivers/media/video/em28xx/em28xx-dvb.c.orig 2011-08-19 00:50:41.000000000 +0100 +++ linux-3.0/drivers/media/video/em28xx/em28xx-dvb.c 2011-08-19 00:51:03.000000000 +0100 @@ -542,7 +542,6 @@ dev->dvb = dvb; dvb->fe[0] = dvb->fe[1] = NULL; - mutex_lock(&dev->lock); em28xx_set_mode(dev, EM28XX_DIGITAL_MODE); /* init frontend */ switch (dev->model) { @@ -711,7 +710,6 @@ em28xx_info("Successfully loaded em28xx-dvb\n"); ret: em28xx_set_mode(dev, EM28XX_SUSPEND); - mutex_unlock(&dev->lock); return result; out_free: