I just rediscovered a deadlock in drivers/ieee1394/nodemgr.c which I
thought didn't exist anymore. It's still there, it's just a matter of
timing to trigger this. Quoting myself from
http://bugzilla.kernel.org/show_bug.cgi?id=6706 :
------------------------------------------------------------------------
# modprobe ohci1394 && modprobe -r ohci1394
works.
# modprobe ohci1394 && sleep 1 && modprobe -r ohci1394
gets stuck in uninterruptible sleep on kthread_stop(). This is trying to
stop the knodemgrd which uninterruptibly sleeps on
bus_rescan_devices_helper() meanwhile.
Call trace of the modprobe -r context:
kthread_stop in kernel/kthread.c
nodemgr_remove_host in drivers/ieee1394/nodemgr.c
__unregister_host in drivers/ieee1394/highlevel.c
highlevel_remove_host in drivers/ieee1394/highlevel.c
hpsb_remove_host in drivers/ieee1394/hosts.c
ohci1394_pci_remove in drivers/ieee1394/ohci1394.c
pci_device_remove in pci/pci-driver.c
__device_release_driver in drivers/base/dd.c
driver_detach in drivers/base/dd.c
Call trace of the knodemgrd context:
bus_rescan_devices_helper in drivers/base/bus.c
bus_rescan_devices in drivers/base/bus.c
nodemgr_node_probe in drivers/ieee1394/nodemgr.c
nodemgr_host_thread in drivers/ieee1394/nodemgr.c
It seems the following is the culprit:
Since Linux 2.6.16, bus_rescan_devices_helper takes
down(&dev->parent->sem) if a parent device exists. This is true for all
devices that are managed by nodemgr. (FireWire ud's have ud's or ne's as
parent, and FireWire ne's have hosts as parent.) And yes, the call in
driver_detach to __device_release_driver is enclosed in down(&dev->sem).
------------------------------------------------------------------------
The relevant change to bus_rescan_devices_helper in 2.6.16 is
http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=bf74ad5bc41727d5f2f1c6bedb2c1fac394de731
> commit bf74ad5bc41727d5f2f1c6bedb2c1fac394de731
> Author: Alan Stern <[email protected]>
> Date: Thu Nov 17 16:54:12 2005 -0500
>
> [PATCH] Hold the device's parent's lock during probe and remove
>
> This patch (as604) makes the driver core hold a device's parent's lock
> as well as the device's lock during calls to the probe and remove
> methods in a driver. This facility is needed by USB device drivers,
> owing to the peculiar way USB devices work:
[...]
> I have not tested this patch for conflicts with other subsystems. As
> far as I can see, the only possibility of conflict would lie in the
> bus_rescan_devices pathway, and it seems pretty remote. Nevertheless,
> it would be good for this to get a lot of testing in -mm.
Yes, it's pretty remote but there is indeed one if I'm not entirely
mistaken.
Right now I don't see a sane fix but I will have a few nights sleep over
it...
--
Stefan Richter
-=====-=-==- =-== =--==
http://arcgraph.de/sr/
I wrote:
> http://bugzilla.kernel.org/show_bug.cgi?id=6706 :
...
> Right now I don't see a sane fix but I will have a few nights sleep over
> it...
A couple of reboots and a slightly charred pizza later I found out the
following:
1. If Alan's 2.6.16 patch is reverted, the deadlock is gone as expected.
See bugzilla for the reverting patch.
2. The following patch works too, without the need to revert Alan's
driver core changes.
3. Now that I have an at least unsane (sic) fix for the deadlock, a new
bug in eth1394's remove code was revealed. This is a separate issue
and logged as http://bugzilla.kernel.org/show_bug.cgi?id=7550 .
Please comment on the patch below.
From: Stefan Richter <[email protected]>
Subject: ieee1394: nodemgr: fix deadlock in shutdown
If "modprobe ohci1394" was quickly followed by "modprobe -r ohci1394",
say with 1 second pause in between, the modprobe -r got stuck in
uninterruptible sleep in kthread_stop. At the same time the knodemgrd
slept uninterruptibly in bus_rescan_devices_helper.
This was a regression since Linux 2.6.16,
commit bf74ad5bc41727d5f2f1c6bedb2c1fac394de731
"Hold the device's parent's lock during probe and remove"
The fix lets ieee1394's nodemgr temporarily counteract the driver core's
downed parent->sem. Thus bus_rescan_devices_helper can proceed and
knodemgrd terminates properly.
Signed-off-by: Stefan Richter <[email protected]>
---
drivers/ieee1394/nodemgr.c | 11 +++++++++++
1 files changed, 11 insertions(+)
Index: linux-2.6.19-rc4/drivers/ieee1394/nodemgr.c
===================================================================
--- linux-2.6.19-rc4.orig/drivers/ieee1394/nodemgr.c 2006-11-18 23:31:35.000000000 +0100
+++ linux-2.6.19-rc4/drivers/ieee1394/nodemgr.c 2006-11-19 15:14:50.000000000 +0100
@@ -1873,8 +1873,19 @@ static void nodemgr_remove_host(struct h
{
struct host_info *hi = hpsb_get_hostinfo(&nodemgr_highlevel, host);
+ /* Here comes a potential deadlock. A "modprobe -r ohci1394" calls
+ * nodemgr_remove_host from driver_detach which takes the parent->sem.
+ * Meanwhile, knodemgrd may be running into bus_rescan_devices_helper
+ * which would block on the same semaphore. Therefore lift the
+ * semaphore until knodemgrd exited. */
if (hi) {
+ /* up(&host->device.sem); --- apparently not required */
+ if (host->device.parent)
+ up(&host->device.parent->sem);
kthread_stop(hi->thread);
+ if (host->device.parent)
+ down(&host->device.parent->sem);
+ /* down(&host->device.sem); --- apparently not required */
nodemgr_remove_host_dev(&host->device);
}
}
--
Stefan Richter
-=====-=-==- =-== =--==
http://arcgraph.de/sr/
On Sun, 19 Nov 2006, Stefan Richter wrote:
> I wrote:
> > http://bugzilla.kernel.org/show_bug.cgi?id=6706 :
> ...
> > Right now I don't see a sane fix but I will have a few nights sleep over
> > it...
>
> A couple of reboots and a slightly charred pizza later I found out the
> following:
>
> 1. If Alan's 2.6.16 patch is reverted, the deadlock is gone as expected.
> See bugzilla for the reverting patch.
>
> 2. The following patch works too, without the need to revert Alan's
> driver core changes.
>
> 3. Now that I have an at least unsane (sic) fix for the deadlock, a new
> bug in eth1394's remove code was revealed. This is a separate issue
> and logged as http://bugzilla.kernel.org/show_bug.cgi?id=7550 .
>
> Please comment on the patch below.
>
>
> From: Stefan Richter <[email protected]>
> Subject: ieee1394: nodemgr: fix deadlock in shutdown
>
> If "modprobe ohci1394" was quickly followed by "modprobe -r ohci1394",
> say with 1 second pause in between, the modprobe -r got stuck in
> uninterruptible sleep in kthread_stop. At the same time the knodemgrd
> slept uninterruptibly in bus_rescan_devices_helper.
>
> This was a regression since Linux 2.6.16,
> commit bf74ad5bc41727d5f2f1c6bedb2c1fac394de731
> "Hold the device's parent's lock during probe and remove"
>
> The fix lets ieee1394's nodemgr temporarily counteract the driver core's
> downed parent->sem. Thus bus_rescan_devices_helper can proceed and
> knodemgrd terminates properly.
>
> Signed-off-by: Stefan Richter <[email protected]>
> ---
> drivers/ieee1394/nodemgr.c | 11 +++++++++++
> 1 files changed, 11 insertions(+)
>
> Index: linux-2.6.19-rc4/drivers/ieee1394/nodemgr.c
> ===================================================================
> --- linux-2.6.19-rc4.orig/drivers/ieee1394/nodemgr.c 2006-11-18 23:31:35.000000000 +0100
> +++ linux-2.6.19-rc4/drivers/ieee1394/nodemgr.c 2006-11-19 15:14:50.000000000 +0100
> @@ -1873,8 +1873,19 @@ static void nodemgr_remove_host(struct h
> {
> struct host_info *hi = hpsb_get_hostinfo(&nodemgr_highlevel, host);
>
> + /* Here comes a potential deadlock. A "modprobe -r ohci1394" calls
> + * nodemgr_remove_host from driver_detach which takes the parent->sem.
> + * Meanwhile, knodemgrd may be running into bus_rescan_devices_helper
> + * which would block on the same semaphore. Therefore lift the
> + * semaphore until knodemgrd exited. */
> if (hi) {
> + /* up(&host->device.sem); --- apparently not required */
> + if (host->device.parent)
> + up(&host->device.parent->sem);
> kthread_stop(hi->thread);
> + if (host->device.parent)
> + down(&host->device.parent->sem);
> + /* down(&host->device.sem); --- apparently not required */
> nodemgr_remove_host_dev(&host->device);
> }
> }
Obviously this patch isn't pretty. It's also incorrect, because it
reacquires the parent's semaphore while holding the child's -- that's
another recipe for deadlock.
Knowing nothing at all about ieee1394, I get the feeling that the culprit
here is a strange subsystem design. In fact, I don't understand exactly
what's going wrong. Evidently the rmmod thread owns the locks for both
the host being removed and its parent, and it wants to stop knodemgrd,
which is waiting to acquire the host's parent's lock because it is
attempting to rescan the parent. Is that right?
It doesn't make sense. If knodemgrd is rescanning the parent then the
parent must not have a driver. If it doesn't have a driver, how can it
have children?
Alan Stern
Alan Stern wrote:
> On Sun, 19 Nov 2006, Stefan Richter wrote:
>> @@ -1873,8 +1873,19 @@ static void nodemgr_remove_host(struct h
...
>> if (hi) {
>> + /* up(&host->device.sem); --- apparently not required */
>> + if (host->device.parent)
>> + up(&host->device.parent->sem);
>> kthread_stop(hi->thread);
>> + if (host->device.parent)
>> + down(&host->device.parent->sem);
>> + /* down(&host->device.sem); --- apparently not required */
>> nodemgr_remove_host_dev(&host->device);
>> }
>> }
>
> Obviously this patch isn't pretty. It's also incorrect, because it
> reacquires the parent's semaphore while holding the child's -- that's
> another recipe for deadlock.
>
> Knowing nothing at all about ieee1394, I get the feeling that the culprit
> here is a strange subsystem design. In fact, I don't understand exactly
> what's going wrong. Evidently the rmmod thread owns the locks for both
> the host being removed and its parent, and it wants to stop knodemgrd,
> which is waiting to acquire the host's parent's lock because it is
> attempting to rescan the parent. Is that right?
That's right.
> It doesn't make sense. If knodemgrd is rescanning the parent then the
> parent must not have a driver. If it doesn't have a driver, how can it
> have children?
Well, I don't fully understand the reasoning behind it. (But even less
do I grasp the driver core.) Short story: It may indeed be possible to
get rid of the parent relationship to the host device and of the
rescanning of the host device. Long story:
ieee1394 has:
- struct hpsb_host (contains a struct device and a struct class_device)
ieee1394's nodemgr in addition has:
- struct node_entry (ditto)
- struct unit_directory (ditto)
nodemgr also provides different .release callbacks for each of these
devices and class_devices, an .uevent callback for unit_directory
class_devices and the struct bus_type for the three kinds of device.
All of the existing protocol drivers bind only to unit_directories, so
these are the most important ones as far as driver matching is
concerned. (There is only a little out-of-tree driver, mem1394 for
forensics, which binds differently because it doesn't require any actual
protocol on a remote node.)
A hpsb_host is merely access to controllers; each one controls a bus. On
a bus live a few node_entries, as many as there are nodes with an active
link layer controller. This includes the local host, therefore there is
always at least one node_entry per bus. A node may implement 0, 1 or
more units and presents them in a hierarchical manner. A unit talks a
protocol; and our protocol drivers access such a unit and, if the
protocol has this necessity, the unit's node. (While doing so, it also
accesses the hpsb_host in front of the node.)
If support for eth1394 is configured in, our drivers add a unit to each
of the local nodes which indicate IP over 1394 capability to external
nodes. This unit_directory also matches the eth1394 when the driver core
scans the ieee1394_bus_type.
So in short, I think we could actually live with a minimum sysfs
implementation which exposes only unit directories. (But I may be
missing something --- I'm not quite familiar with the sysfs interfacing
tools, that is udev and hald.) At least device file naming of SBP-2
units wouldn't suffer because udev takes the unique name of SBP-2 units
from a sysfs attribute of the SCSI device, not from the unit_directory's
device.
The /sys/class/net/eth?/device links of eth1394 class devices currently
point to the hpsb_host devices i.e. fw-host devices; maybe they could as
well point to respective unit directories.
So maybe a simple, flat sysfs representation without node_entry devices,
perhaps also without fw-host devices --- or at least a representation
(1) without parent relationship to the fw-host devices and (2) without
ieee1394_bus_type in fw-host devices --- is possible without breaking
userspace. It probably would eliminate the problem which we are
discussing here. This would mean that we loose the ability to bind
protocol drivers to a hpsb_host (and depending on how far the
respresentation is cut down, also the ability to bind protocol drivers
to a node_entry). But as I said I see no actual need for this ability.
--
Stefan Richter
-=====-=-==- =-== =--==
http://arcgraph.de/sr/
On Sunday 19 November 2006 14:54, Alan Stern wrote:
> On Sun, 19 Nov 2006, Stefan Richter wrote:
>
> > I wrote:
> > > http://bugzilla.kernel.org/show_bug.cgi?id=6706 :
> > ...
> > > Right now I don't see a sane fix but I will have a few nights sleep over
> > > it...
> >
> > A couple of reboots and a slightly charred pizza later I found out the
> > following:
> >
> > 1. If Alan's 2.6.16 patch is reverted, the deadlock is gone as expected.
> > See bugzilla for the reverting patch.
> >
> > 2. The following patch works too, without the need to revert Alan's
> > driver core changes.
> >
> > 3. Now that I have an at least unsane (sic) fix for the deadlock, a new
> > bug in eth1394's remove code was revealed. This is a separate issue
> > and logged as http://bugzilla.kernel.org/show_bug.cgi?id=7550 .
> >
> > Please comment on the patch below.
> >
> >
> > From: Stefan Richter <[email protected]>
> > Subject: ieee1394: nodemgr: fix deadlock in shutdown
> >
> > If "modprobe ohci1394" was quickly followed by "modprobe -r ohci1394",
> > say with 1 second pause in between, the modprobe -r got stuck in
> > uninterruptible sleep in kthread_stop. At the same time the knodemgrd
> > slept uninterruptibly in bus_rescan_devices_helper.
> >
> > This was a regression since Linux 2.6.16,
> > commit bf74ad5bc41727d5f2f1c6bedb2c1fac394de731
> > "Hold the device's parent's lock during probe and remove"
> >
> > The fix lets ieee1394's nodemgr temporarily counteract the driver core's
> > downed parent->sem. Thus bus_rescan_devices_helper can proceed and
> > knodemgrd terminates properly.
> >
> > Signed-off-by: Stefan Richter <[email protected]>
> > ---
> > drivers/ieee1394/nodemgr.c | 11 +++++++++++
> > 1 files changed, 11 insertions(+)
> >
> > Index: linux-2.6.19-rc4/drivers/ieee1394/nodemgr.c
> > ===================================================================
> > --- linux-2.6.19-rc4.orig/drivers/ieee1394/nodemgr.c 2006-11-18 23:31:35.000000000 +0100
> > +++ linux-2.6.19-rc4/drivers/ieee1394/nodemgr.c 2006-11-19 15:14:50.000000000 +0100
> > @@ -1873,8 +1873,19 @@ static void nodemgr_remove_host(struct h
> > {
> > struct host_info *hi = hpsb_get_hostinfo(&nodemgr_highlevel, host);
> >
> > + /* Here comes a potential deadlock. A "modprobe -r ohci1394" calls
> > + * nodemgr_remove_host from driver_detach which takes the parent->sem.
> > + * Meanwhile, knodemgrd may be running into bus_rescan_devices_helper
> > + * which would block on the same semaphore. Therefore lift the
> > + * semaphore until knodemgrd exited. */
> > if (hi) {
> > + /* up(&host->device.sem); --- apparently not required */
> > + if (host->device.parent)
> > + up(&host->device.parent->sem);
> > kthread_stop(hi->thread);
> > + if (host->device.parent)
> > + down(&host->device.parent->sem);
> > + /* down(&host->device.sem); --- apparently not required */
> > nodemgr_remove_host_dev(&host->device);
> > }
> > }
>
> Obviously this patch isn't pretty. It's also incorrect, because it
> reacquires the parent's semaphore while holding the child's -- that's
> another recipe for deadlock.
>
> Knowing nothing at all about ieee1394, I get the feeling that the culprit
> here is a strange subsystem design. In fact, I don't understand exactly
> what's going wrong. Evidently the rmmod thread owns the locks for both
> the host being removed and its parent, and it wants to stop knodemgrd,
> which is waiting to acquire the host's parent's lock because it is
> attempting to rescan the parent. Is that right?
>
I was actually looking at the driver core recently and was surprised that
USB crapped all over it with its locking requirements. I don't think its a
good idea to enforce one subsystem's lcoking rules onto everyone.
Would there be alot of objections if we add bus->[un]lock_device() and
hide all uglies there? If methods are not set when bus is registered then
standard dev->sem lock/unlock will be used.
--
Dmitry
On Sun, 19 Nov 2006, Dmitry Torokhov wrote:
> I was actually looking at the driver core recently and was surprised that
> USB crapped all over it with its locking requirements. I don't think its a
> good idea to enforce one subsystem's lcoking rules onto everyone.
I agree. The only reason for doing it that way was I couldn't think of
any other approach.
> Would there be alot of objections if we add bus->[un]lock_device() and
> hide all uglies there? If methods are not set when bus is registered then
> standard dev->sem lock/unlock will be used.
That's a little too simple to work. You can see from examining the
existing code in bus.c and dd.c; there are places where the device needs
to be locked but not the parent, places where the parent needs to be
locked but not the device, and places where both need to be locked.
A slightly different way to accomplish much the same thing might be to
have a per-bus parent_needs_to_be_locked flag (or method). It wouldn't be
quite as elegant, though.
Alan Stern
On Sun, 19 Nov 2006, Stefan Richter wrote:
> > Knowing nothing at all about ieee1394, I get the feeling that the culprit
> > here is a strange subsystem design. In fact, I don't understand exactly
> > what's going wrong. Evidently the rmmod thread owns the locks for both
> > the host being removed and its parent, and it wants to stop knodemgrd,
> > which is waiting to acquire the host's parent's lock because it is
> > attempting to rescan the parent. Is that right?
>
> That's right.
>
> > It doesn't make sense. If knodemgrd is rescanning the parent then the
> > parent must not have a driver. If it doesn't have a driver, how can it
> > have children?
>
> Well, I don't fully understand the reasoning behind it. (But even less
> do I grasp the driver core.) Short story: It may indeed be possible to
> get rid of the parent relationship to the host device and of the
> rescanning of the host device. Long story:
Wait a minute. Above you agreed that the problem was caused by knodemgrd
attempting to rescan the host's _parent_. So which is the focus of the
deadlock: the host or its parent?
> ieee1394 has:
> - struct hpsb_host (contains a struct device and a struct class_device)
> ieee1394's nodemgr in addition has:
> - struct node_entry (ditto)
> - struct unit_directory (ditto)
>
> nodemgr also provides different .release callbacks for each of these
> devices and class_devices, an .uevent callback for unit_directory
> class_devices and the struct bus_type for the three kinds of device.
>
> All of the existing protocol drivers bind only to unit_directories, so
> these are the most important ones as far as driver matching is
> concerned. (There is only a little out-of-tree driver, mem1394 for
> forensics, which binds differently because it doesn't require any actual
> protocol on a remote node.)
>
> A hpsb_host is merely access to controllers; each one controls a bus. On
> a bus live a few node_entries, as many as there are nodes with an active
> link layer controller. This includes the local host, therefore there is
> always at least one node_entry per bus. A node may implement 0, 1 or
> more units and presents them in a hierarchical manner. A unit talks a
> protocol; and our protocol drivers access such a unit and, if the
> protocol has this necessity, the unit's node. (While doing so, it also
> accesses the hpsb_host in front of the node.)
>
> If support for eth1394 is configured in, our drivers add a unit to each
> of the local nodes which indicate IP over 1394 capability to external
> nodes. This unit_directory also matches the eth1394 when the driver core
> scans the ieee1394_bus_type.
>
> So in short, I think we could actually live with a minimum sysfs
> implementation which exposes only unit directories. (But I may be
> missing something --- I'm not quite familiar with the sysfs interfacing
> tools, that is udev and hald.) At least device file naming of SBP-2
> units wouldn't suffer because udev takes the unique name of SBP-2 units
> from a sysfs attribute of the SCSI device, not from the unit_directory's
> device.
>
> The /sys/class/net/eth?/device links of eth1394 class devices currently
> point to the hpsb_host devices i.e. fw-host devices; maybe they could as
> well point to respective unit directories.
>
> So maybe a simple, flat sysfs representation without node_entry devices,
> perhaps also without fw-host devices --- or at least a representation
> (1) without parent relationship to the fw-host devices and (2) without
> ieee1394_bus_type in fw-host devices --- is possible without breaking
> userspace. It probably would eliminate the problem which we are
> discussing here. This would mean that we loose the ability to bind
> protocol drivers to a hpsb_host (and depending on how far the
> respresentation is cut down, also the ability to bind protocol drivers
> to a node_entry). But as I said I see no actual need for this ability.
If I understand all this, you've got an hpsb_host, directly below which
are one or more node_entry's, below each of which may be some
unit_directory's. Right?
But how is this relevant if the problem is caused by knodemgrd trying to
rescan the hpsb_host's parent?
Is the problem caused by the fact that some of these struct device's
aren't bound to a driver? Remember, bus_rescan_devices() will skip over
anything that already has a driver. Could you solve your problem by
adding a do_nothing driver that would bind to these otherwise unused
devices?
Alan Stern
Alan Stern wrote:
> Wait a minute. Above you agreed that the problem was caused by knodemgrd
> attempting to rescan the host's _parent_. So which is the focus of the
> deadlock: the host or its parent?
The parent of the hpsb_host.
ohci1394 works on a pci_dev which contains a dev, let's call it A.
ieee1394 has a hpsb_host which contains a dev, let's call it B. B's
parent is A. Then there is one or more node_entry with dev C whose
parent is B, end unit_directory with dev D whose parent is C.
The bus of devices B, C, D is set to be ieee1394_bus_type, and that's
what knodemgrd is scanning.
knodemgrd blocks on the semaphore of the parent of B because
driver_detach took the semaphore of A (and of the parent of A if there
is one).
[...]
> If I understand all this, you've got an hpsb_host, directly below which
> are one or more node_entry's, below each of which may be some
> unit_directory's. Right?
Right.
> But how is this relevant if the problem is caused by knodemgrd trying to
> rescan the hpsb_host's parent?
It's rescanning just the hpsb_host, but you surely got the picture now.
> Is the problem caused by the fact that some of these struct device's
> aren't bound to a driver? Remember, bus_rescan_devices() will skip over
> anything that already has a driver. Could you solve your problem by
> adding a do_nothing driver that would bind to these otherwise unused
> devices?
Excellently, that's what I will try in a minute. It is surely intended
that the hpsb_host can get a driver bound too, but as I mentioned, we
don't have a driver which needs this capability and I don't foresee any
such driver.
Thanks for the directions.
--
Stefan Richter
-=====-=-==- =-== =-=--
http://arcgraph.de/sr/
On Mon, 20 Nov 2006, Stefan Richter wrote:
> Alan Stern wrote:
> > Wait a minute. Above you agreed that the problem was caused by knodemgrd
> > attempting to rescan the host's _parent_. So which is the focus of the
> > deadlock: the host or its parent?
>
> The parent of the hpsb_host.
>
> ohci1394 works on a pci_dev which contains a dev, let's call it A.
>
> ieee1394 has a hpsb_host which contains a dev, let's call it B. B's
> parent is A. Then there is one or more node_entry with dev C whose
> parent is B, end unit_directory with dev D whose parent is C.
>
> The bus of devices B, C, D is set to be ieee1394_bus_type, and that's
> what knodemgrd is scanning.
>
> knodemgrd blocks on the semaphore of the parent of B because
> driver_detach took the semaphore of A (and of the parent of A if there
> is one).
Okay, I get it.
> > Is the problem caused by the fact that some of these struct device's
> > aren't bound to a driver? Remember, bus_rescan_devices() will skip over
> > anything that already has a driver. Could you solve your problem by
> > adding a do_nothing driver that would bind to these otherwise unused
> > devices?
>
> Excellently, that's what I will try in a minute. It is surely intended
> that the hpsb_host can get a driver bound too, but as I mentioned, we
> don't have a driver which needs this capability and I don't foresee any
> such driver.
>
> Thanks for the directions.
You're welcome. Let me know how it turns out.
Alan Stern
On 20 Nov, Alan Stern wrote:
>> > Is the problem caused by the fact that some of these struct device's
>> > aren't bound to a driver? Remember, bus_rescan_devices() will skip over
>> > anything that already has a driver. Could you solve your problem by
>> > adding a do_nothing driver that would bind to these otherwise unused
>> > devices?
There are three minor problems with this approach:
- some ballast in system memory for the dummy driver struct
- the dummy driver is visible in sysfs
- the (root) user can unbind the driver which will recreate the
preconditions for the deadlock
Nevertheless, here is the patch. I find it ugly, maybe even more so than
my previous up(&dev->parent->sem) hack, but it works as you said.
From: Stefan Richter <[email protected]>
Subject: ieee1394: nodemgr: fix deadlock in shutdown
If "modprobe ohci1394" was quickly followed by "modprobe -r ohci1394",
say with 1 second pause in between, the modprobe -r got stuck in
uninterruptible sleep in kthread_stop. At the same time the knodemgrd
slept uninterruptibly in bus_rescan_devices_helper.
This was a regression since Linux 2.6.16,
commit bf74ad5bc41727d5f2f1c6bedb2c1fac394de731
"Hold the device's parent's lock during probe and remove"
The fix (or rather workaround) adds a dummy driver to the hpsb_host
device. Now bus_rescan_devices_helper won't scan it anymore. This
doesn't hurt since we have no drivers which will bind to these devices
and it is unlikely that there will ever be such a driver.
Signed-off-by: Stefan Richter <[email protected]>
---
drivers/ieee1394/nodemgr.c | 19 ++++++++++++++++---
1 files changed, 16 insertions(+), 3 deletions(-)
Index: linux-2.6.19-rc4/drivers/ieee1394/nodemgr.c
===================================================================
--- linux-2.6.19-rc4.orig/drivers/ieee1394/nodemgr.c 2006-11-21 00:09:25.000000000 +0100
+++ linux-2.6.19-rc4/drivers/ieee1394/nodemgr.c 2006-11-21 01:00:28.000000000 +0100
@@ -259,9 +259,19 @@ static struct device nodemgr_dev_templat
.release = nodemgr_release_ne,
};
+/* The dummy driver prevents the host devices from being scanned. We have no
+ * useful drivers for them yet, and there is a deadlock possible if the driver
+ * core scans the host device while the host's low-level driver, i.e. the host's
+ * parent device, is being removed. */
+static struct device_driver nodemgr_dev_dummy_driver = {
+ .bus = &ieee1394_bus_type,
+ .name = "none",
+};
+
struct device nodemgr_dev_template_host = {
.bus = &ieee1394_bus_type,
.release = nodemgr_release_host,
+ .driver = &nodemgr_dev_dummy_driver,
};
@@ -703,12 +713,15 @@ static int nodemgr_bus_match(struct devi
if (dev->platform_data != &nodemgr_ud_platform_data)
return 0;
- ud = container_of(dev, struct unit_directory, device);
- driver = container_of(drv, struct hpsb_protocol_driver, driver);
+ /* The dummy driver is not wrapped in a hpsb_protocol_driver */
+ if (drv == &nodemgr_dev_dummy_driver)
+ return 0;
+ ud = container_of(dev, struct unit_directory, device);
if (ud->ne->in_limbo || ud->ignore_driver)
return 0;
+ driver = container_of(drv, struct hpsb_protocol_driver, driver);
for (id = driver->id_table; id->match_flags != 0; id++) {
if ((id->match_flags & IEEE1394_MATCH_VENDOR_ID) &&
id->vendor_id != ud->vendor_id)
@@ -1899,7 +1912,7 @@ int init_ieee1394_nodemgr(void)
class_unregister(&nodemgr_ne_class);
return error;
}
-
+ error = driver_register(&nodemgr_dev_dummy_driver);
hpsb_register_highlevel(&nodemgr_highlevel);
return 0;
}
--
Stefan Richter
-=====-=-==- =-== =-=-=
http://arcgraph.de/sr/
On Tue, 21 Nov 2006, Stefan Richter wrote:
> On 20 Nov, Alan Stern wrote:
> >> > Is the problem caused by the fact that some of these struct device's
> >> > aren't bound to a driver? Remember, bus_rescan_devices() will skip over
> >> > anything that already has a driver. Could you solve your problem by
> >> > adding a do_nothing driver that would bind to these otherwise unused
> >> > devices?
>
> There are three minor problems with this approach:
> - some ballast in system memory for the dummy driver struct
> - the dummy driver is visible in sysfs
> - the (root) user can unbind the driver which will recreate the
> preconditions for the deadlock
I agree with all these objections.
The real issue here is the way ieee1394 sets up children of devices with
no driver. On the face of it that is quite illogical: If a device has no
driver, then who can interrogate it to find out about its children?
USB faces a similar situation. In a USB device, all the real work is
actually done by "interfaces". So we set up a device structure for the
USB device itself, plus device structures for each of its interfaces. The
parent structure is bound to a (more or less) dummy driver, which insures
that the child structures are deleted whenever it gets unbound.
Still, maybe some compromise can be reached. Perhaps Dmitry's idea, or
something like it, can be adopted.
Alan Stern
Alan Stern wrote:
> The real issue here is the way ieee1394 sets up children of devices with
> no driver. On the face of it that is quite illogical: If a device has no
> driver, then who can interrogate it to find out about its children?
Perhaps I should turn this dummy driver into an actual representation of
ieee1394 or ieee1394's nodemgr.
> USB faces a similar situation. In a USB device, all the real work is
> actually done by "interfaces". So we set up a device structure for the
> USB device itself, plus device structures for each of its interfaces. The
> parent structure is bound to a (more or less) dummy driver, which insures
> that the child structures are deleted whenever it gets unbound.
I will eventually more thoroughly look at how USB and other subsystems
with device hierarchies do it.
> Still, maybe some compromise can be reached. Perhaps Dmitry's idea, or
> something like it, can be adopted.
Thanks for the input of both of you.
--
Stefan Richter
-=====-=-==- =-== =-=-=
http://arcgraph.de/sr/
If "modprobe ohci1394" was quickly followed by "modprobe -r ohci1394",
say with 1 second pause in between, the modprobe -r got stuck in
uninterruptible sleep in kthread_stop. At the same time the knodemgrd
slept uninterruptibly in bus_rescan_devices_helper. That's because
driver_detach took the semaphore of the PCI device and
bus_rescan_devices_helper wanted to take the semaphore of the FireWire
host device's parent, which is the same semaphore. This was a regression
since Linux 2.6.16, commit bf74ad5bc41727d5f2f1c6bedb2c1fac394de731,
"Hold the device's parent's lock during probe and remove".
The fix (or workaround) adds a dummy driver to the hpsb_host device. Now
bus_rescan_devices_helper won't scan the host device anymore. This
doesn't hurt since we have no drivers which will bind to these devices
and it is unlikely that there will ever be such a driver. The dummy
driver is befittingly presented as a representation of ieee1394 itself.
Fixes: http://bugzilla.kernel.org/show_bug.cgi?id=6706
Signed-off-by: Stefan Richter <[email protected]>
---
There is now a /sys/bus/ieee1394/drivers/ieee1394, whose "bind" and
"unbind" attributes are not welcome. Is there a way to disable them?
drivers/ieee1394/nodemgr.c | 21 ++++++++++++++++++---
1 files changed, 18 insertions(+), 3 deletions(-)
Index: linux/drivers/ieee1394/nodemgr.c
===================================================================
--- linux.orig/drivers/ieee1394/nodemgr.c 2006-11-21 00:09:25.000000000 +0100
+++ linux/drivers/ieee1394/nodemgr.c 2006-11-21 22:32:09.000000000 +0100
@@ -14,6 +14,7 @@
#include <linux/slab.h>
#include <linux/delay.h>
#include <linux/kthread.h>
+#include <linux/module.h>
#include <linux/moduleparam.h>
#include <asm/atomic.h>
@@ -259,9 +260,20 @@ static struct device nodemgr_dev_templat
.release = nodemgr_release_ne,
};
+/* This dummy driver prevents the host devices from being scanned. We have no
+ * useful drivers for them yet, and there would be a deadlock possible if the
+ * driver core scans the host device while the host's low-level driver (i.e.
+ * the host's parent device) is being removed. */
+static struct device_driver nodemgr_mid_layer_driver = {
+ .bus = &ieee1394_bus_type,
+ .name = "ieee1394",
+ .owner = THIS_MODULE,
+};
+
struct device nodemgr_dev_template_host = {
.bus = &ieee1394_bus_type,
.release = nodemgr_release_host,
+ .driver = &nodemgr_mid_layer_driver,
};
@@ -704,11 +716,14 @@ static int nodemgr_bus_match(struct devi
return 0;
ud = container_of(dev, struct unit_directory, device);
- driver = container_of(drv, struct hpsb_protocol_driver, driver);
-
if (ud->ne->in_limbo || ud->ignore_driver)
return 0;
+ /* We only match drivers of type hpsb_protocol_driver */
+ if (drv == &nodemgr_mid_layer_driver)
+ return 0;
+
+ driver = container_of(drv, struct hpsb_protocol_driver, driver);
for (id = driver->id_table; id->match_flags != 0; id++) {
if ((id->match_flags & IEEE1394_MATCH_VENDOR_ID) &&
id->vendor_id != ud->vendor_id)
@@ -1899,7 +1914,7 @@ int init_ieee1394_nodemgr(void)
class_unregister(&nodemgr_ne_class);
return error;
}
-
+ error = driver_register(&nodemgr_mid_layer_driver);
hpsb_register_highlevel(&nodemgr_highlevel);
return 0;
}
On Tue, 21 Nov 2006, Stefan Richter wrote:
> There is now a /sys/bus/ieee1394/drivers/ieee1394, whose "bind" and
> "unbind" attributes are not welcome. Is there a way to disable them?
You can always prevent "bind" from operating by returning an error code
from the driver's probe routine (although it's not clear why you would
want to do that). I don't think there's any way to make the "unbind"
attribute stop working.
You could violate the layering and remove the attribute files directly.
But that would be a race; there would remain a brief interval between the
time the files were created and the time you removed them.
Lastly, you could remove source of your deadlock by having the unbind
routine for the new driver delete all the child device structures. In
fact, just to make things more symmetric and logical you could have the
probe routine create those child devices in the first place!
Alan Stern
Alan Stern wrote:
> On Tue, 21 Nov 2006, Stefan Richter wrote:
>> There is now a /sys/bus/ieee1394/drivers/ieee1394,
(I'll rename it to nodemgr when I commit this patch.)
>> whose "bind" and "unbind" attributes are not welcome. Is there a way
>> to disable them?
>
> You can always prevent "bind" from operating by returning an error code
> from the driver's probe routine (although it's not clear why you would
> want to do that). I don't think there's any way to make the "unbind"
> attribute stop working.
>
> You could violate the layering and remove the attribute files directly.
> But that would be a race; there would remain a brief interval between the
> time the files were created and the time you removed them.
Does this matter if there is no device which can be unbound?
Anyway, I don't think I will go this route unless a real problem with
the attributes turns up.
> Lastly, you could remove source of your deadlock by having the unbind
> routine for the new driver delete all the child device structures.
Hmm, I won't believe you until I actually try it. :-)
> In fact, just to make things more symmetric and logical you could have
> the probe routine create those child devices in the first place!
Sounds good. It's on my .plan now.
--
Stefan Richter
-=====-=-==- =-== =-==-
http://arcgraph.de/sr/