> Hi,
First off, sorry for missing this, and thanks to Andrew for pointing it
out to me. You might want to use the tool, scripts/get_maintainer.pl
for who to know to cc: for patches like this, so I don't miss it.
> I'm seeing a driver crash in its shutdown routine because it's
> touching some uninitialized state. It turns out that the driver's
> probe routine was still running [for the same device]. There also
> appears to be an issue in the remove path, where device_shutdown()
> checks the dev->driver pointer and uses it later, with seemingly
> nothing to guarantee that it doesn't change.
What type of driver is having this problem? What type of bus is it on?
Usually the bus prevents this from happening with its own serialization.
> Shouldn't we synchronize the shutdown routine with probe/remove to
> prevent such races?
Normally, yes, and for some reason, I thought we already were doing
that.
> The patch below should take care of these races.
Does this patch solve your problem? Care to show me the oops you get
without it?
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index e28ce98..f2c63c6 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1823,6 +1823,9 @@ void device_shutdown(void)
> pm_runtime_get_noresume(dev);
> pm_runtime_barrier(dev);
>
> + if (dev->parent) /* Needed for USB */
> + device_lock(dev->parent);
> + device_lock(dev);
The parent thing is "interesting", and if we are going to have to start
duplicating this logic in different parts of the driver core, we should
wrap it up in a common function, right?
thanks,
greg k-h
Hi,
On Fri, May 18, 2012 at 12:59 PM, Greg KH <[email protected]> wrote:
>> Hi,
>
> First off, sorry for missing this, and thanks to Andrew for pointing it
> out to me. ?You might want to use the tool, scripts/get_maintainer.pl
> for who to know to cc: for patches like this, so I don't miss it.
>
>> I'm seeing a driver crash in its shutdown routine because it's
>> touching some uninitialized state. It turns out that the driver's
>> probe routine was still running [for the same device]. There also
>> appears to be an issue in the remove path, where device_shutdown()
>> checks the dev->driver pointer and uses it later, with seemingly
>> nothing to guarantee that it doesn't change.
>
> What type of driver is having this problem? ?What type of bus is it on?
> Usually the bus prevents this from happening with its own serialization.
Looks it is a generic problem.
There are two races, one is between .probe and .shutdown, and another
is between .release and .shutdown, see below:
void device_shutdown(void)
......
/* Don't allow any more runtime suspends */
pm_runtime_get_noresume(dev);
pm_runtime_barrier(dev);
if (dev->bus && dev->bus->shutdown) {
dev_dbg(dev, "shutdown\n");
dev->bus->shutdown(dev);
} else if (dev->driver && dev->driver->shutdown) { /*line-driver*/
dev_dbg(dev, "shutdown\n");
dev->driver->shutdown(dev); /*line-shut*/
}
......
If dev->driver is just set(really_probe) before 'line-driver' and .probe is
not executed before 'line-shut', the .shutdown may touch a uninitialized
device.
Also if dev->driver is just cleared(__device_release_driver) after "line-driver"
and before "line-shut", null pointer will be referenced and oops will
be triggered.
>> Shouldn't we synchronize the shutdown routine with probe/remove to
>> prevent such races?
>
> Normally, yes, and for some reason, I thought we already were doing
> that.
Looks the races are still there.
>> The patch below should take care of these races.
>
> Does this patch solve your problem? ?Care to show me the oops you get
> without it?
>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index e28ce98..f2c63c6 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -1823,6 +1823,9 @@ void device_shutdown(void)
>> ? ? ? ? ? ? ? ? pm_runtime_get_noresume(dev);
>> ? ? ? ? ? ? ? ? pm_runtime_barrier(dev);
>>
>> + ? ? ? ? ? ? ? if (dev->parent) ? ? ? ?/* Needed for USB */
>> + ? ? ? ? ? ? ? ? ? ? ? device_lock(dev->parent);
>> + ? ? ? ? ? ? ? device_lock(dev);
Looks the above makes sense to serialize .shutdown with
.probe and .release.
Thanks,
--
Ming Lei
On Sun, May 20, 2012 at 10:08:08PM +0800, Ming Lei wrote:
> Hi,
>
> On Fri, May 18, 2012 at 12:59 PM, Greg KH <[email protected]> wrote:
> >> Hi,
> >
> > First off, sorry for missing this, and thanks to Andrew for pointing it
> > out to me. ?You might want to use the tool, scripts/get_maintainer.pl
> > for who to know to cc: for patches like this, so I don't miss it.
> >
> >> I'm seeing a driver crash in its shutdown routine because it's
> >> touching some uninitialized state. It turns out that the driver's
> >> probe routine was still running [for the same device]. There also
> >> appears to be an issue in the remove path, where device_shutdown()
> >> checks the dev->driver pointer and uses it later, with seemingly
> >> nothing to guarantee that it doesn't change.
> >
> > What type of driver is having this problem? ?What type of bus is it on?
> > Usually the bus prevents this from happening with its own serialization.
>
> Looks it is a generic problem.
>
> There are two races, one is between .probe and .shutdown, and another
> is between .release and .shutdown, see below:
>
>
> void device_shutdown(void)
>
> ......
> /* Don't allow any more runtime suspends */
> pm_runtime_get_noresume(dev);
> pm_runtime_barrier(dev);
>
> if (dev->bus && dev->bus->shutdown) {
> dev_dbg(dev, "shutdown\n");
> dev->bus->shutdown(dev);
> } else if (dev->driver && dev->driver->shutdown) { /*line-driver*/
> dev_dbg(dev, "shutdown\n");
> dev->driver->shutdown(dev); /*line-shut*/
> }
> ......
>
> If dev->driver is just set(really_probe) before 'line-driver' and .probe is
> not executed before 'line-shut', the .shutdown may touch a uninitialized
> device.
>
> Also if dev->driver is just cleared(__device_release_driver) after "line-driver"
> and before "line-shut", null pointer will be referenced and oops will
> be triggered.
And how can that happen with a real bus? Don't we have a lock
somewhere per-bus that should be protecting this type of thing (sorry,
can't dig through the code at the moment, on the road...)
> >> Shouldn't we synchronize the shutdown routine with probe/remove to
> >> prevent such races?
> >
> > Normally, yes, and for some reason, I thought we already were doing
> > that.
>
> Looks the races are still there.
How come no one has ever hit them in the past 10 years? What am I
missing here?
> >> The patch below should take care of these races.
> >
> > Does this patch solve your problem? ?Care to show me the oops you get
> > without it?
> >
> >> diff --git a/drivers/base/core.c b/drivers/base/core.c
> >> index e28ce98..f2c63c6 100644
> >> --- a/drivers/base/core.c
> >> +++ b/drivers/base/core.c
> >> @@ -1823,6 +1823,9 @@ void device_shutdown(void)
> >> ? ? ? ? ? ? ? ? pm_runtime_get_noresume(dev);
> >> ? ? ? ? ? ? ? ? pm_runtime_barrier(dev);
> >>
> >> + ? ? ? ? ? ? ? if (dev->parent) ? ? ? ?/* Needed for USB */
> >> + ? ? ? ? ? ? ? ? ? ? ? device_lock(dev->parent);
> >> + ? ? ? ? ? ? ? device_lock(dev);
>
> Looks the above makes sense to serialize .shutdown with
> .probe and .release.
Let me look at the code when I get back in a few days, but I really
thought we already had a lock protecting all of this...
thanks,
greg k-h
Cc pm list because it is related with PM.
Hi Greg,
On Mon, May 21, 2012 at 3:51 AM, Greg KH <[email protected]> wrote:
>
> And how can that happen with a real bus? ?Don't we have a lock
The races may be triggered when one device is just probed(triggered
by plug) or released(triggered by unplug) at the same time of running
reboot/poweroff.
> somewhere per-bus that should be protecting this type of thing (sorry,
> can't dig through the code at the moment, on the road...)
device_shutdown is called with only holding reboot_mutex, so I think no
any protection on dev->driver there.
>
> How come no one has ever hit them in the past 10 years? ?What am I
> missing here?
The window is so small that maybe it is very very difficult to trigger
the races, :-)
But looks Wedson is luck enough to observe it.
>> Looks the above makes sense to serialize .shutdown with
>> .probe and .release.
>
> Let me look at the code when I get back in a few days, but I really
> thought we already had a lock protecting all of this...
Also the previous patch don't cover the .runtime_resume races with
.probe or .release, so the right fix may be below:
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 346be8b..cbc8bd2 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1820,6 +1820,11 @@ void device_shutdown(void)
list_del_init(&dev->kobj.entry);
spin_unlock(&devices_kset->list_lock);
+ /*hold lock[s] to avoid races with .probe/.release*/
+ if (dev->parent)
+ device_lock(dev->parent);
+ device_lock(dev);
+
/* Don't allow any more runtime suspends */
pm_runtime_get_noresume(dev);
pm_runtime_barrier(dev);
@@ -1831,6 +1836,9 @@ void device_shutdown(void)
dev_dbg(dev, "shutdown\n");
dev->driver->shutdown(dev);
}
+ device_unlock(dev);
+ if (dev->parent)
+ device_unlock(dev->parent);
put_device(dev);
spin_lock(&devices_kset->list_lock);
Another candidate fix is to register a reboot notifier in driver core to prevent
driver from being bound or unbound from start of reboot/shutdown, but looks
not easy as the way of holding device locks.
Thanks,
--
Ming Lei
On Mon, 21 May 2012, Ming Lei wrote:
> Cc pm list because it is related with PM.
>
> Hi Greg,
>
> On Mon, May 21, 2012 at 3:51 AM, Greg KH <[email protected]> wrote:
>
> >
> > And how can that happen with a real bus? ?Don't we have a lock
>
> The races may be triggered when one device is just probed(triggered
> by plug) or released(triggered by unplug) at the same time of running
> reboot/poweroff.
>
> > somewhere per-bus that should be protecting this type of thing (sorry,
> > can't dig through the code at the moment, on the road...)
>
> device_shutdown is called with only holding reboot_mutex, so I think no
> any protection on dev->driver there.
>
> >
> > How come no one has ever hit them in the past 10 years? ?What am I
> > missing here?
>
> The window is so small that maybe it is very very difficult to trigger
> the races, :-)
> But looks Wedson is luck enough to observe it.
>
> >> Looks the above makes sense to serialize .shutdown with
> >> .probe and .release.
> >
> > Let me look at the code when I get back in a few days, but I really
> > thought we already had a lock protecting all of this...
>
> Also the previous patch don't cover the .runtime_resume races with
> .probe or .release, so the right fix may be below:
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 346be8b..cbc8bd2 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1820,6 +1820,11 @@ void device_shutdown(void)
> list_del_init(&dev->kobj.entry);
> spin_unlock(&devices_kset->list_lock);
>
> + /*hold lock[s] to avoid races with .probe/.release*/
> + if (dev->parent)
> + device_lock(dev->parent);
> + device_lock(dev);
> +
> /* Don't allow any more runtime suspends */
> pm_runtime_get_noresume(dev);
> pm_runtime_barrier(dev);
> @@ -1831,6 +1836,9 @@ void device_shutdown(void)
> dev_dbg(dev, "shutdown\n");
> dev->driver->shutdown(dev);
> }
> + device_unlock(dev);
> + if (dev->parent)
> + device_unlock(dev->parent);
> put_device(dev);
>
> spin_lock(&devices_kset->list_lock);
>
> Another candidate fix is to register a reboot notifier in driver core to prevent
> driver from being bound or unbound from start of reboot/shutdown, but looks
> not easy as the way of holding device locks.
I'd guess it was done this way so that the shutdown task wouldn't have
to wait for a buggy driver that didn't want to release the device lock
(or that crashed while holding the lock).
It's not clear that the reboot notifier approach would work. What
about probes that had already started when notifier was called?
Alan Stern
On Tue, May 22, 2012 at 2:29 AM, Alan Stern <[email protected]> wrote:
> On Mon, 21 May 2012, Ming Lei wrote:
>> Another candidate fix is to register a reboot notifier in driver core to prevent
>> driver from being bound or unbound from start of reboot/shutdown, but looks
>> not easy as the way of holding device locks.
>
> I'd guess it was done this way so that the shutdown task wouldn't have
> to wait for a buggy driver that didn't want to release the device lock
> (or that crashed while holding the lock).
Maybe, so I understand you agree on adding the device lock as did
in the patch, don't I?
Also we can add below line before acquiring device lock to help
toubleshoot buggy driver.
dev_dbg(dev, "acquiring device lock for shutdown\n");
>
> It's not clear that the reboot notifier approach would work. ?What
> about probes that had already started when notifier was called?
Looks holding device lock is still needed in the notifier callback for
handling the case if reboot notifier approach is taken.
IMO, the way of holding device lock is better than reboot notifier approach.
Thanks,
--
Ming Lei
On Tue, 22 May 2012, Ming Lei wrote:
> On Tue, May 22, 2012 at 2:29 AM, Alan Stern <[email protected]> wrote:
> > On Mon, 21 May 2012, Ming Lei wrote:
> >> Another candidate fix is to register a reboot notifier in driver core to prevent
> >> driver from being bound or unbound from start of reboot/shutdown, but looks
> >> not easy as the way of holding device locks.
> >
> > I'd guess it was done this way so that the shutdown task wouldn't have
> > to wait for a buggy driver that didn't want to release the device lock
> > (or that crashed while holding the lock).
>
> Maybe, so I understand you agree on adding the device lock as did
> in the patch, don't I?
I don't know. It depends on the intention behind the shutdown
callback. Maybe each driver is supposed to be responsible for doing
its own locking.
Do you think that a buggy driver should be able to prevent the system
from shutting down?
> Also we can add below line before acquiring device lock to help
> toubleshoot buggy driver.
>
> dev_dbg(dev, "acquiring device lock for shutdown\n");
>
> >
> > It's not clear that the reboot notifier approach would work. ?What
> > about probes that had already started when notifier was called?
>
> Looks holding device lock is still needed in the notifier callback for
> handling the case if reboot notifier approach is taken.
>
> IMO, the way of holding device lock is better than reboot notifier approach.
Yes, it is better. The question is, is it right?
Alan Stern
Alan Stern <[email protected]> writes:
> On Tue, 22 May 2012, Ming Lei wrote:
>
>> On Tue, May 22, 2012 at 2:29 AM, Alan Stern <[email protected]> wrote:
>> > On Mon, 21 May 2012, Ming Lei wrote:
>> >> Another candidate fix is to register a reboot notifier in driver core to prevent
>> >> driver from being bound or unbound from start of reboot/shutdown, but looks
>> >> not easy as the way of holding device locks.
You might also be able to look at system_state and simply not do any
hotplug work if the state is neither SYSTEM_BOOTING or SYSTEM_RUNNING.
>> > I'd guess it was done this way so that the shutdown task wouldn't have
>> > to wait for a buggy driver that didn't want to release the device lock
>> > (or that crashed while holding the lock).
>>
>> Maybe, so I understand you agree on adding the device lock as did
>> in the patch, don't I?
>
> I don't know. It depends on the intention behind the shutdown
> callback. Maybe each driver is supposed to be responsible for doing
> its own locking.
>
> Do you think that a buggy driver should be able to prevent the system
> from shutting down?
The original intent of the shutdown callback was to just the hardware
part of the device shutdown and not do muck with kernel data structures
because just the device portion should be more reliable and was all
that is needed.
Given the current minimal usage of the device shutdown callback I'm not
convinced the original reasoning made sense but shrug. So we might
want to reorg things and merge remove and shutdown. I missed the start
of this thread so I don't know how ambitious everyone is.
Eric
On Wed, May 23, 2012 at 3:16 AM, Eric W. Biederman
<[email protected]> wrote:
> Alan Stern <[email protected]> writes:
>
>> On Tue, 22 May 2012, Ming Lei wrote:
>>
>>> On Tue, May 22, 2012 at 2:29 AM, Alan Stern <[email protected]> wrote:
>>> > On Mon, 21 May 2012, Ming Lei wrote:
>>> >> Another candidate fix is to register a reboot notifier in driver core to prevent
>>> >> driver from being bound or unbound from start of reboot/shutdown, but looks
>>> >> not easy as the way of holding device locks.
>
> You might also be able to look at system_state and simply not do any
> hotplug work if the state is neither SYSTEM_BOOTING or SYSTEM_RUNNING.
As Alan pointed, we need to handle the case of device probing or releasing
at the same time of starting shutdown, so maybe waiting for completing of
device probe or release is needed if device lock is to be avoided.
>
>>> > I'd guess it was done this way so that the shutdown task wouldn't have
>>> > to wait for a buggy driver that didn't want to release the device lock
>>> > (or that crashed while holding the lock).
>>>
>>> Maybe, so I understand you agree on adding the device lock as did
>>> in the patch, don't I?
>>
>> I don't know. ?It depends on the intention behind the shutdown
>> callback. ?Maybe each driver is supposed to be responsible for doing
>> its own locking.
>>
>> Do you think that a buggy driver should be able to prevent the system
>> from shutting down?
IMO, the buggy driver should be fixed first, not only in .probe or .release, but
also in .runtime_resume, all may affect shutting down.
>
> The original intent of the shutdown callback was to just the hardware
> part of the device shutdown and not do muck with kernel data structures
> because just the device portion should be more reliable and was all
> that is needed.
The .shutdown callback pointer is got from device->driver, which is
changed in probe and release path. Also runtime PM thing has been
involved into shutting down recently, so looks not only hardware parts
are involved now.
>
> Given the current minimal usage of the device shutdown callback I'm not
> convinced the original reasoning made sense but shrug. ?So we might
> want to reorg things and merge remove and shutdown. ?I missed the start
> of this thread so I don't know how ambitious everyone is.
Generally remove/release may take much more time than shutdown only, which
may slow down the 'power off'. Also some device may require special handling
in .shutdown, such as, network driver may need to enable WoL or change power
state in .shutdown, so it is not easy to merge remove with shutdown safely.
Thanks,
--
Ming Lei
On Wed, 23 May 2012, Ming Lei wrote:
> >> Do you think that a buggy driver should be able to prevent the system
> >> from shutting down?
>
> IMO, the buggy driver should be fixed first, not only in .probe or .release, but
> also in .runtime_resume, all may affect shutting down.
>
> >
> > The original intent of the shutdown callback was to just the hardware
> > part of the device shutdown and not do muck with kernel data structures
> > because just the device portion should be more reliable and was all
> > that is needed.
>
> The .shutdown callback pointer is got from device->driver, which is
> changed in probe and release path. Also runtime PM thing has been
> involved into shutting down recently, so looks not only hardware parts
> are involved now.
This is a tricky question. Overall I think you're probably right.
It's certainly true that holding the device lock across the shutdown
callback is the easiest and most reliable way to prevent these races.
Alan Stern
On Wed, May 23, 2012 at 11:06 PM, Alan Stern <[email protected]> wrote:
> On Wed, 23 May 2012, Ming Lei wrote:
>> The .shutdown callback pointer is got from device->driver, which is
>> changed in probe and release path. Also runtime PM thing has been
>> involved into shutting down recently, so looks not only hardware parts
>> are involved now.
>
> This is a tricky question. ?Overall I think you're probably right.
>
> It's certainly true that holding the device lock across the shutdown
> callback is the easiest and most reliable way to prevent these races.
But holding device lock across .shutdown is very inefficient because
most of devices' driver have not shutdown callback, so I think it is better
to fix the race by prevent driver core from probing or releasing once
shutdown is started.
How about the below patch?
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 346be8b..fc70930 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1803,6 +1803,18 @@ void device_shutdown(void)
{
struct device *dev;
+ /*wait for completing of device probing and releasing*/
+ printk("%s: wait for completing of devices' probing and"
+ "releasing...");
+
+ /*
+ * order between writing system_state and read probe/release
+ * counter.
+ */
+ smp_mb();
+ wait_for_device_probe_release();
+ printk("OK\n");
+
spin_lock(&devices_kset->list_lock);
/*
* Walk the devices list backward, shutting down each in turn.
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 1b1cbb5..8328383 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -245,6 +245,7 @@ int device_bind_driver(struct device *dev)
EXPORT_SYMBOL_GPL(device_bind_driver);
static atomic_t probe_count = ATOMIC_INIT(0);
+static atomic_t release_count = ATOMIC_INIT(0);
static DECLARE_WAIT_QUEUE_HEAD(probe_waitqueue);
static int really_probe(struct device *dev, struct device_driver *drv)
@@ -252,6 +253,10 @@ static int really_probe(struct device *dev,
struct device_driver *drv)
int ret = 0;
atomic_inc(&probe_count);
+ smp_mb();
+ if (system_in_shutdown())
+ goto done;
+
pr_debug("bus: '%s': %s: probing driver %s with device %s\n",
drv->bus->name, __func__, drv->name, dev_name(dev));
WARN_ON(!list_empty(&dev->devres_head));
@@ -336,6 +341,19 @@ void wait_for_device_probe(void)
EXPORT_SYMBOL_GPL(wait_for_device_probe);
/**
+ * wait_for_device_probe_release
+ * Wait for device probing and releasing to be completed.
+ */
+void wait_for_device_probe_release(void)
+{
+ /* wait for complete devices' probing and releasing*/
+ wait_event(probe_waitqueue, atomic_read(&probe_count) == 0 &&
+ atomic_read(&release_count) == 0);
+ async_synchronize_full();
+}
+EXPORT_SYMBOL_GPL(wait_for_device_probe_release);
+
+/**
* driver_probe_device - attempt to bind device & driver together
* @drv: driver to bind a device to
* @dev: device to try to bind to the driver
@@ -470,6 +488,11 @@ static void __device_release_driver(struct device *dev)
drv = dev->driver;
if (drv) {
+ atomic_inc(&release_count);
+ smp_mb();
+ if (system_in_shutdown())
+ goto exit;
+
pm_runtime_get_sync(dev);
driver_sysfs_remove(dev);
@@ -492,7 +515,9 @@ static void __device_release_driver(struct device *dev)
blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
BUS_NOTIFY_UNBOUND_DRIVER,
dev);
-
+exit:
+ atomic_dec(&release_count);
+ wake_up(&probe_waitqueue);
}
}
diff --git a/include/linux/device.h b/include/linux/device.h
index 161d962..3fdf782 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -243,6 +243,7 @@ extern struct device_driver *driver_find(const char *name,
struct bus_type *bus);
extern int driver_probe_done(void);
extern void wait_for_device_probe(void);
+extern void wait_for_device_probe_release(void);
/* sysfs interface for exporting driver attributes */
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 64bdeeb8..3cf4f36 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -378,6 +378,12 @@ extern enum system_states {
SYSTEM_SUSPEND_DISK,
} system_state;
+static inline int system_in_shutdown(void)
+{
+ return (system_state >= SYSTEM_HALT) &&
+ (system_state <= SYSTEM_RESTART);
+}
+
#define TAINT_PROPRIETARY_MODULE 0
#define TAINT_FORCED_MODULE 1
#define TAINT_UNSAFE_SMP 2
Thanks,
--
Ming Lei
On Thu, May 24, 2012 at 09:39:46AM +0800, Ming Lei wrote:
> On Wed, May 23, 2012 at 11:06 PM, Alan Stern <[email protected]> wrote:
> > On Wed, 23 May 2012, Ming Lei wrote:
> >> The .shutdown callback pointer is got from device->driver, which is
> >> changed in probe and release path. Also runtime PM thing has been
> >> involved into shutting down recently, so looks not only hardware parts
> >> are involved now.
> >
> > This is a tricky question. ?Overall I think you're probably right.
> >
> > It's certainly true that holding the device lock across the shutdown
> > callback is the easiest and most reliable way to prevent these races.
>
> But holding device lock across .shutdown is very inefficient because
> most of devices' driver have not shutdown callback, so I think it is better
> to fix the race by prevent driver core from probing or releasing once
> shutdown is started.
>
> How about the below patch?
How about waiting for the original poster to respond as to exactly how
they are hitting this race before doing anything?
greg k-h
On Thu, May 24, 2012 at 10:14 AM, Greg KH <[email protected]> wrote:
>
> How about waiting for the original poster to respond as to exactly how
> they are hitting this race before doing anything?
Sure, :-)
Thanks,
--
Ming Lei
On Wed, 23 May 2012, Greg KH wrote:
> On Thu, May 24, 2012 at 09:39:46AM +0800, Ming Lei wrote:
> > On Wed, May 23, 2012 at 11:06 PM, Alan Stern <[email protected]> wrote:
> > > On Wed, 23 May 2012, Ming Lei wrote:
> > >> The .shutdown callback pointer is got from device->driver, which is
> > >> changed in probe and release path. Also runtime PM thing has been
> > >> involved into shutting down recently, so looks not only hardware parts
> > >> are involved now.
> > >
> > > This is a tricky question. ?Overall I think you're probably right.
> > >
> > > It's certainly true that holding the device lock across the shutdown
> > > callback is the easiest and most reliable way to prevent these races.
> >
> > But holding device lock across .shutdown is very inefficient because
> > most of devices' driver have not shutdown callback, so I think it is better
The code there is racy already. It does:
} else if (dev->driver && dev->driver->shutdown) {
without any locking protection. If the driver is unbound while this
statement runs then dev->driver could be non-NULL for the first test
and NULL for the second.
> > to fix the race by prevent driver core from probing or releasing once
> > shutdown is started.
> >
> > How about the below patch?
>
> How about waiting for the original poster to respond as to exactly how
> they are hitting this race before doing anything?
In addition, the patch is too complicated. For this type of
synchronization you should use SRCU. See
Documentation/RCU/whatisRCU.txt and related files.
Alan Stern
On Thu, May 24, 2012 at 10:37 PM, Alan Stern <[email protected]> wrote:
>
> The code there is racy already. ?It does:
>
> ? ? ? ? ? ? ? ?} else if (dev->driver && dev->driver->shutdown) {
>
> without any locking protection. ?If the driver is unbound while this
> statement runs then dev->driver could be non-NULL for the first test
> and NULL for the second.
Yes, I missed this one, :-)
>
>> > to fix the race by prevent driver core from probing or releasing once
>> > shutdown is started.
>> >
>> > How about the below patch?
>>
>> How about waiting for the original poster to respond as to exactly how
>> they are hitting this race before doing anything?
>
> In addition, the patch is too complicated. ?For this type of
> synchronization you should use SRCU. ?See
> Documentation/RCU/whatisRCU.txt and related files.
Yes, the synchronization should be a many reader vs. one
writer problem, RCU should be suitable.
Looks we think alike, :-)
I have studied RCU yesterday, but was afraid that may introduce
much more code, so not applied it in the patch. Will study it further
to figure out a new version.
Thanks,
--
Ming Lei