* [BUGFIX 1/2] PCI/PM: Fix deadlock when unbind device if its parent in D3cold
@ 2012-10-24 6:54 Huang Ying
2012-10-24 6:54 ` [BUGFIX 2/2] PCI/PM: Resume device before shutdown Huang Ying
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Huang Ying @ 2012-10-24 6:54 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, linux-pci, linux-pm, Rafael J. Wysocki, Huang Ying,
Zhang Yanmin
If a PCI device and its parents are put into D3cold, unbinding the
device will trigger deadlock as follow:
- driver_unbind
- device_release_driver
- device_lock(dev) <--- previous lock here
- __device_release_driver
- pm_runtime_get_sync
...
- rpm_resume(dev)
- rpm_resume(dev->parent)
...
- pci_pm_runtime_resume
...
- pci_set_power_state
- __pci_start_power_transition
- pci_wakeup_bus(dev->parent->subordinate)
- pci_walk_bus
- device_lock(dev) <--- dead lock here
If we do not do device_lock in pci_walk_bus, we can avoid dead lock.
Device_lock in pci_walk_bus is introduced in commit:
d71374dafbba7ec3f67371d3b7e9f6310a588808, corresponding email thread
is: https://lkml.org/lkml/2006/5/26/38. The patch author Zhang Yanmin
said device_lock is added to pci_walk_bus because:
Some error handling functions call pci_walk_bus. For example, PCIe
aer. Here we lock the device, so the driver wouldn't detach from the
device, as the cb might call driver's callback function.
So I fixed the dead lock as follow:
- remove device_lock from pci_walk_bus
- add device_lock into callback if callback will call driver's callback
I checked pci_walk_bus users one by one, and found only PCIe aer needs
device lock.
Signed-off-by: Huang Ying <ying.huang@intel.com>
Cc: Zhang Yanmin <yanmin.zhang@intel.com>
---
drivers/pci/bus.c | 3 ---
drivers/pci/pcie/aer/aerdrv_core.c | 20 ++++++++++++++++----
2 files changed, 16 insertions(+), 7 deletions(-)
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -320,10 +320,7 @@ void pci_walk_bus(struct pci_bus *top, i
} else
next = dev->bus_list.next;
- /* Run device routines with the device locked */
- device_lock(&dev->dev);
retval = cb(dev, userdata);
- device_unlock(&dev->dev);
if (retval)
break;
}
--- a/drivers/pci/pcie/aer/aerdrv_core.c
+++ b/drivers/pci/pcie/aer/aerdrv_core.c
@@ -213,6 +213,7 @@ static int report_error_detected(struct
struct aer_broadcast_data *result_data;
result_data = (struct aer_broadcast_data *) data;
+ device_lock(&dev->dev);
dev->error_state = result_data->state;
if (!dev->driver ||
@@ -231,12 +232,14 @@ static int report_error_detected(struct
dev->driver ?
"no AER-aware driver" : "no driver");
}
- return 0;
+ goto out;
}
err_handler = dev->driver->err_handler;
vote = err_handler->error_detected(dev, result_data->state);
result_data->result = merge_result(result_data->result, vote);
+out:
+ device_unlock(&dev->dev);
return 0;
}
@@ -247,14 +250,17 @@ static int report_mmio_enabled(struct pc
struct aer_broadcast_data *result_data;
result_data = (struct aer_broadcast_data *) data;
+ device_lock(&dev->dev);
if (!dev->driver ||
!dev->driver->err_handler ||
!dev->driver->err_handler->mmio_enabled)
- return 0;
+ goto out;
err_handler = dev->driver->err_handler;
vote = err_handler->mmio_enabled(dev);
result_data->result = merge_result(result_data->result, vote);
+out:
+ device_unlock(&dev->dev);
return 0;
}
@@ -265,14 +271,17 @@ static int report_slot_reset(struct pci_
struct aer_broadcast_data *result_data;
result_data = (struct aer_broadcast_data *) data;
+ device_lock(&dev->dev);
if (!dev->driver ||
!dev->driver->err_handler ||
!dev->driver->err_handler->slot_reset)
- return 0;
+ goto out;
err_handler = dev->driver->err_handler;
vote = err_handler->slot_reset(dev);
result_data->result = merge_result(result_data->result, vote);
+out:
+ device_unlock(&dev->dev);
return 0;
}
@@ -280,15 +289,18 @@ static int report_resume(struct pci_dev
{
const struct pci_error_handlers *err_handler;
+ device_lock(&dev->dev);
dev->error_state = pci_channel_io_normal;
if (!dev->driver ||
!dev->driver->err_handler ||
!dev->driver->err_handler->resume)
- return 0;
+ goto out;
err_handler = dev->driver->err_handler;
err_handler->resume(dev);
+out:
+ device_unlock(&dev->dev);
return 0;
}
^ permalink raw reply [flat|nested] 15+ messages in thread* [BUGFIX 2/2] PCI/PM: Resume device before shutdown
2012-10-24 6:54 [BUGFIX 1/2] PCI/PM: Fix deadlock when unbind device if its parent in D3cold Huang Ying
@ 2012-10-24 6:54 ` Huang Ying
2012-10-24 21:01 ` Rafael J. Wysocki
2012-11-02 16:52 ` Bjorn Helgaas
2012-10-24 21:01 ` [BUGFIX 1/2] PCI/PM: Fix deadlock when unbind device if its parent in D3cold Rafael J. Wysocki
2012-11-02 16:50 ` Bjorn Helgaas
2 siblings, 2 replies; 15+ messages in thread
From: Huang Ying @ 2012-10-24 6:54 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, linux-pci, linux-pm, Rafael J. Wysocki, Huang Ying
Some actions during shutdown need device to be in D0 state, such as
MSI shutdown etc, so resume device before shutdown.
Signed-off-by: Huang Ying <ying.huang@intel.com>
---
drivers/pci/pci-driver.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -398,6 +398,8 @@ static void pci_device_shutdown(struct d
struct pci_dev *pci_dev = to_pci_dev(dev);
struct pci_driver *drv = pci_dev->driver;
+ pm_runtime_resume(dev);
+
if (drv && drv->shutdown)
drv->shutdown(pci_dev);
pci_msi_shutdown(pci_dev);
@@ -408,16 +410,6 @@ static void pci_device_shutdown(struct d
* continue to do DMA
*/
pci_disable_device(pci_dev);
-
- /*
- * Devices may be enabled to wake up by runtime PM, but they need not
- * be supposed to wake up the system from its "power off" state (e.g.
- * ACPI S5). Therefore disable wakeup for all devices that aren't
- * supposed to wake up the system at this point. The state argument
- * will be ignored by pci_enable_wake().
- */
- if (!device_may_wakeup(dev))
- pci_enable_wake(pci_dev, PCI_UNKNOWN, false);
}
#ifdef CONFIG_PM
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [BUGFIX 2/2] PCI/PM: Resume device before shutdown
2012-10-24 6:54 ` [BUGFIX 2/2] PCI/PM: Resume device before shutdown Huang Ying
@ 2012-10-24 21:01 ` Rafael J. Wysocki
2012-11-02 16:52 ` Bjorn Helgaas
1 sibling, 0 replies; 15+ messages in thread
From: Rafael J. Wysocki @ 2012-10-24 21:01 UTC (permalink / raw)
To: Huang Ying; +Cc: Bjorn Helgaas, linux-kernel, linux-pci, linux-pm
On Wednesday 24 of October 2012 14:54:14 Huang Ying wrote:
> Some actions during shutdown need device to be in D0 state, such as
> MSI shutdown etc, so resume device before shutdown.
>
> Signed-off-by: Huang Ying <ying.huang@intel.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/pci/pci-driver.c | 12 ++----------
> 1 file changed, 2 insertions(+), 10 deletions(-)
>
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -398,6 +398,8 @@ static void pci_device_shutdown(struct d
> struct pci_dev *pci_dev = to_pci_dev(dev);
> struct pci_driver *drv = pci_dev->driver;
>
> + pm_runtime_resume(dev);
> +
> if (drv && drv->shutdown)
> drv->shutdown(pci_dev);
> pci_msi_shutdown(pci_dev);
> @@ -408,16 +410,6 @@ static void pci_device_shutdown(struct d
> * continue to do DMA
> */
> pci_disable_device(pci_dev);
> -
> - /*
> - * Devices may be enabled to wake up by runtime PM, but they need not
> - * be supposed to wake up the system from its "power off" state (e.g.
> - * ACPI S5). Therefore disable wakeup for all devices that aren't
> - * supposed to wake up the system at this point. The state argument
> - * will be ignored by pci_enable_wake().
> - */
> - if (!device_may_wakeup(dev))
> - pci_enable_wake(pci_dev, PCI_UNKNOWN, false);
> }
>
> #ifdef CONFIG_PM
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [BUGFIX 2/2] PCI/PM: Resume device before shutdown
2012-10-24 6:54 ` [BUGFIX 2/2] PCI/PM: Resume device before shutdown Huang Ying
2012-10-24 21:01 ` Rafael J. Wysocki
@ 2012-11-02 16:52 ` Bjorn Helgaas
2012-11-02 20:26 ` Rafael J. Wysocki
2012-11-03 5:05 ` Huang Ying
1 sibling, 2 replies; 15+ messages in thread
From: Bjorn Helgaas @ 2012-11-02 16:52 UTC (permalink / raw)
To: Huang Ying; +Cc: linux-kernel, linux-pci, linux-pm, Rafael J. Wysocki
On Wed, Oct 24, 2012 at 12:54 AM, Huang Ying <ying.huang@intel.com> wrote:
> Some actions during shutdown need device to be in D0 state, such as
> MSI shutdown etc, so resume device before shutdown.
Is there a problem report or bugzilla for this issue? What are the
symptoms by which a user could figure out that he needs this fix?
Should this be put in the stable tree as well? If so, for v3.6 only?
> Signed-off-by: Huang Ying <ying.huang@intel.com>
> ---
> drivers/pci/pci-driver.c | 12 ++----------
> 1 file changed, 2 insertions(+), 10 deletions(-)
>
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -398,6 +398,8 @@ static void pci_device_shutdown(struct d
> struct pci_dev *pci_dev = to_pci_dev(dev);
> struct pci_driver *drv = pci_dev->driver;
>
> + pm_runtime_resume(dev);
> +
> if (drv && drv->shutdown)
> drv->shutdown(pci_dev);
> pci_msi_shutdown(pci_dev);
> @@ -408,16 +410,6 @@ static void pci_device_shutdown(struct d
> * continue to do DMA
> */
> pci_disable_device(pci_dev);
> -
> - /*
> - * Devices may be enabled to wake up by runtime PM, but they need not
> - * be supposed to wake up the system from its "power off" state (e.g.
> - * ACPI S5). Therefore disable wakeup for all devices that aren't
> - * supposed to wake up the system at this point. The state argument
> - * will be ignored by pci_enable_wake().
> - */
> - if (!device_may_wakeup(dev))
> - pci_enable_wake(pci_dev, PCI_UNKNOWN, false);
> }
>
> #ifdef CONFIG_PM
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [BUGFIX 2/2] PCI/PM: Resume device before shutdown
2012-11-02 16:52 ` Bjorn Helgaas
@ 2012-11-02 20:26 ` Rafael J. Wysocki
2012-11-03 5:05 ` Huang Ying
1 sibling, 0 replies; 15+ messages in thread
From: Rafael J. Wysocki @ 2012-11-02 20:26 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Huang Ying, linux-kernel, linux-pci, linux-pm
On Friday, November 02, 2012 10:52:45 AM Bjorn Helgaas wrote:
> On Wed, Oct 24, 2012 at 12:54 AM, Huang Ying <ying.huang@intel.com> wrote:
> > Some actions during shutdown need device to be in D0 state, such as
> > MSI shutdown etc, so resume device before shutdown.
>
> Is there a problem report or bugzilla for this issue? What are the
> symptoms by which a user could figure out that he needs this fix?
>
> Should this be put in the stable tree as well? If so, for v3.6 only?
Yes, it should be -stable for v3.6.y.
Thanks,
Rafael
> > Signed-off-by: Huang Ying <ying.huang@intel.com>
> > ---
> > drivers/pci/pci-driver.c | 12 ++----------
> > 1 file changed, 2 insertions(+), 10 deletions(-)
> >
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -398,6 +398,8 @@ static void pci_device_shutdown(struct d
> > struct pci_dev *pci_dev = to_pci_dev(dev);
> > struct pci_driver *drv = pci_dev->driver;
> >
> > + pm_runtime_resume(dev);
> > +
> > if (drv && drv->shutdown)
> > drv->shutdown(pci_dev);
> > pci_msi_shutdown(pci_dev);
> > @@ -408,16 +410,6 @@ static void pci_device_shutdown(struct d
> > * continue to do DMA
> > */
> > pci_disable_device(pci_dev);
> > -
> > - /*
> > - * Devices may be enabled to wake up by runtime PM, but they need not
> > - * be supposed to wake up the system from its "power off" state (e.g.
> > - * ACPI S5). Therefore disable wakeup for all devices that aren't
> > - * supposed to wake up the system at this point. The state argument
> > - * will be ignored by pci_enable_wake().
> > - */
> > - if (!device_may_wakeup(dev))
> > - pci_enable_wake(pci_dev, PCI_UNKNOWN, false);
> > }
> >
> > #ifdef CONFIG_PM
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [BUGFIX 2/2] PCI/PM: Resume device before shutdown
2012-11-02 16:52 ` Bjorn Helgaas
2012-11-02 20:26 ` Rafael J. Wysocki
@ 2012-11-03 5:05 ` Huang Ying
2012-11-03 17:21 ` Bjorn Helgaas
1 sibling, 1 reply; 15+ messages in thread
From: Huang Ying @ 2012-11-03 5:05 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-kernel, linux-pci, linux-pm, Rafael J. Wysocki
On Fri, 2012-11-02 at 10:52 -0600, Bjorn Helgaas wrote:
> On Wed, Oct 24, 2012 at 12:54 AM, Huang Ying <ying.huang@intel.com> wrote:
> > Some actions during shutdown need device to be in D0 state, such as
> > MSI shutdown etc, so resume device before shutdown.
>
> Is there a problem report or bugzilla for this issue? What are the
> symptoms by which a user could figure out that he needs this fix?
No bugzilla for this issue. This issue will cause the corresponding
device lost in kexeced kernel.
Best Regards,
Huang Ying
> Should this be put in the stable tree as well? If so, for v3.6 only?
>
> > Signed-off-by: Huang Ying <ying.huang@intel.com>
> > ---
> > drivers/pci/pci-driver.c | 12 ++----------
> > 1 file changed, 2 insertions(+), 10 deletions(-)
> >
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -398,6 +398,8 @@ static void pci_device_shutdown(struct d
> > struct pci_dev *pci_dev = to_pci_dev(dev);
> > struct pci_driver *drv = pci_dev->driver;
> >
> > + pm_runtime_resume(dev);
> > +
> > if (drv && drv->shutdown)
> > drv->shutdown(pci_dev);
> > pci_msi_shutdown(pci_dev);
> > @@ -408,16 +410,6 @@ static void pci_device_shutdown(struct d
> > * continue to do DMA
> > */
> > pci_disable_device(pci_dev);
> > -
> > - /*
> > - * Devices may be enabled to wake up by runtime PM, but they need not
> > - * be supposed to wake up the system from its "power off" state (e.g.
> > - * ACPI S5). Therefore disable wakeup for all devices that aren't
> > - * supposed to wake up the system at this point. The state argument
> > - * will be ignored by pci_enable_wake().
> > - */
> > - if (!device_may_wakeup(dev))
> > - pci_enable_wake(pci_dev, PCI_UNKNOWN, false);
> > }
> >
> > #ifdef CONFIG_PM
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [BUGFIX 2/2] PCI/PM: Resume device before shutdown
2012-11-03 5:05 ` Huang Ying
@ 2012-11-03 17:21 ` Bjorn Helgaas
2012-11-04 3:44 ` Huang Ying
0 siblings, 1 reply; 15+ messages in thread
From: Bjorn Helgaas @ 2012-11-03 17:21 UTC (permalink / raw)
To: Huang Ying; +Cc: linux-kernel, linux-pci, linux-pm, Rafael J. Wysocki
On Fri, Nov 2, 2012 at 11:05 PM, Huang Ying <ying.huang@intel.com> wrote:
> On Fri, 2012-11-02 at 10:52 -0600, Bjorn Helgaas wrote:
>> On Wed, Oct 24, 2012 at 12:54 AM, Huang Ying <ying.huang@intel.com> wrote:
>> > Some actions during shutdown need device to be in D0 state, such as
>> > MSI shutdown etc, so resume device before shutdown.
>>
>> Is there a problem report or bugzilla for this issue? What are the
>> symptoms by which a user could figure out that he needs this fix?
>
> No bugzilla for this issue. This issue will cause the corresponding
> device lost in kexeced kernel.
So would the following be accurate changelog text?
Without this patch, a device may not work correctly after a kexec
because the new kernel expects devices to be in D0.
>> Should this be put in the stable tree as well? If so, for v3.6 only?
What about the stable tree?
>> > Signed-off-by: Huang Ying <ying.huang@intel.com>
>> > ---
>> > drivers/pci/pci-driver.c | 12 ++----------
>> > 1 file changed, 2 insertions(+), 10 deletions(-)
>> >
>> > --- a/drivers/pci/pci-driver.c
>> > +++ b/drivers/pci/pci-driver.c
>> > @@ -398,6 +398,8 @@ static void pci_device_shutdown(struct d
>> > struct pci_dev *pci_dev = to_pci_dev(dev);
>> > struct pci_driver *drv = pci_dev->driver;
>> >
>> > + pm_runtime_resume(dev);
>> > +
>> > if (drv && drv->shutdown)
>> > drv->shutdown(pci_dev);
>> > pci_msi_shutdown(pci_dev);
>> > @@ -408,16 +410,6 @@ static void pci_device_shutdown(struct d
>> > * continue to do DMA
>> > */
>> > pci_disable_device(pci_dev);
>> > -
>> > - /*
>> > - * Devices may be enabled to wake up by runtime PM, but they need not
>> > - * be supposed to wake up the system from its "power off" state (e.g.
>> > - * ACPI S5). Therefore disable wakeup for all devices that aren't
>> > - * supposed to wake up the system at this point. The state argument
>> > - * will be ignored by pci_enable_wake().
>> > - */
>> > - if (!device_may_wakeup(dev))
>> > - pci_enable_wake(pci_dev, PCI_UNKNOWN, false);
>> > }
>> >
>> > #ifdef CONFIG_PM
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [BUGFIX 2/2] PCI/PM: Resume device before shutdown
2012-11-03 17:21 ` Bjorn Helgaas
@ 2012-11-04 3:44 ` Huang Ying
0 siblings, 0 replies; 15+ messages in thread
From: Huang Ying @ 2012-11-04 3:44 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-kernel, linux-pci, linux-pm, Rafael J. Wysocki
On Sat, 2012-11-03 at 11:21 -0600, Bjorn Helgaas wrote:
> On Fri, Nov 2, 2012 at 11:05 PM, Huang Ying <ying.huang@intel.com> wrote:
> > On Fri, 2012-11-02 at 10:52 -0600, Bjorn Helgaas wrote:
> >> On Wed, Oct 24, 2012 at 12:54 AM, Huang Ying <ying.huang@intel.com> wrote:
> >> > Some actions during shutdown need device to be in D0 state, such as
> >> > MSI shutdown etc, so resume device before shutdown.
> >>
> >> Is there a problem report or bugzilla for this issue? What are the
> >> symptoms by which a user could figure out that he needs this fix?
> >
> > No bugzilla for this issue. This issue will cause the corresponding
> > device lost in kexeced kernel.
>
> So would the following be accurate changelog text?
>
> Without this patch, a device may not work correctly after a kexec
> because the new kernel expects devices to be in D0.
The issue I encountered is
Without this patch, a device may not be enumerated after a kexec
because the corresponding bridge is not in D0, so that
configuration space of the device is not accessible.
> >> Should this be put in the stable tree as well? If so, for v3.6 only?
>
> What about the stable tree?
Yes. This patch should be for v3.6 stable tree only.
Best Regards,
Huang Ying
> >> > Signed-off-by: Huang Ying <ying.huang@intel.com>
> >> > ---
> >> > drivers/pci/pci-driver.c | 12 ++----------
> >> > 1 file changed, 2 insertions(+), 10 deletions(-)
> >> >
> >> > --- a/drivers/pci/pci-driver.c
> >> > +++ b/drivers/pci/pci-driver.c
> >> > @@ -398,6 +398,8 @@ static void pci_device_shutdown(struct d
> >> > struct pci_dev *pci_dev = to_pci_dev(dev);
> >> > struct pci_driver *drv = pci_dev->driver;
> >> >
> >> > + pm_runtime_resume(dev);
> >> > +
> >> > if (drv && drv->shutdown)
> >> > drv->shutdown(pci_dev);
> >> > pci_msi_shutdown(pci_dev);
> >> > @@ -408,16 +410,6 @@ static void pci_device_shutdown(struct d
> >> > * continue to do DMA
> >> > */
> >> > pci_disable_device(pci_dev);
> >> > -
> >> > - /*
> >> > - * Devices may be enabled to wake up by runtime PM, but they need not
> >> > - * be supposed to wake up the system from its "power off" state (e.g.
> >> > - * ACPI S5). Therefore disable wakeup for all devices that aren't
> >> > - * supposed to wake up the system at this point. The state argument
> >> > - * will be ignored by pci_enable_wake().
> >> > - */
> >> > - if (!device_may_wakeup(dev))
> >> > - pci_enable_wake(pci_dev, PCI_UNKNOWN, false);
> >> > }
> >> >
> >> > #ifdef CONFIG_PM
> >
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [BUGFIX 1/2] PCI/PM: Fix deadlock when unbind device if its parent in D3cold
2012-10-24 6:54 [BUGFIX 1/2] PCI/PM: Fix deadlock when unbind device if its parent in D3cold Huang Ying
2012-10-24 6:54 ` [BUGFIX 2/2] PCI/PM: Resume device before shutdown Huang Ying
@ 2012-10-24 21:01 ` Rafael J. Wysocki
2012-11-02 16:50 ` Bjorn Helgaas
2 siblings, 0 replies; 15+ messages in thread
From: Rafael J. Wysocki @ 2012-10-24 21:01 UTC (permalink / raw)
To: Huang Ying; +Cc: Bjorn Helgaas, linux-kernel, linux-pci, linux-pm, Zhang Yanmin
On Wednesday 24 of October 2012 14:54:13 Huang Ying wrote:
> If a PCI device and its parents are put into D3cold, unbinding the
> device will trigger deadlock as follow:
>
> - driver_unbind
> - device_release_driver
> - device_lock(dev) <--- previous lock here
> - __device_release_driver
> - pm_runtime_get_sync
> ...
> - rpm_resume(dev)
> - rpm_resume(dev->parent)
> ...
> - pci_pm_runtime_resume
> ...
> - pci_set_power_state
> - __pci_start_power_transition
> - pci_wakeup_bus(dev->parent->subordinate)
> - pci_walk_bus
> - device_lock(dev) <--- dead lock here
>
>
> If we do not do device_lock in pci_walk_bus, we can avoid dead lock.
> Device_lock in pci_walk_bus is introduced in commit:
> d71374dafbba7ec3f67371d3b7e9f6310a588808, corresponding email thread
> is: https://lkml.org/lkml/2006/5/26/38. The patch author Zhang Yanmin
> said device_lock is added to pci_walk_bus because:
>
> Some error handling functions call pci_walk_bus. For example, PCIe
> aer. Here we lock the device, so the driver wouldn't detach from the
> device, as the cb might call driver's callback function.
>
> So I fixed the dead lock as follow:
>
> - remove device_lock from pci_walk_bus
> - add device_lock into callback if callback will call driver's callback
>
> I checked pci_walk_bus users one by one, and found only PCIe aer needs
> device lock.
>
> Signed-off-by: Huang Ying <ying.huang@intel.com>
> Cc: Zhang Yanmin <yanmin.zhang@intel.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/pci/bus.c | 3 ---
> drivers/pci/pcie/aer/aerdrv_core.c | 20 ++++++++++++++++----
> 2 files changed, 16 insertions(+), 7 deletions(-)
>
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -320,10 +320,7 @@ void pci_walk_bus(struct pci_bus *top, i
> } else
> next = dev->bus_list.next;
>
> - /* Run device routines with the device locked */
> - device_lock(&dev->dev);
> retval = cb(dev, userdata);
> - device_unlock(&dev->dev);
> if (retval)
> break;
> }
> --- a/drivers/pci/pcie/aer/aerdrv_core.c
> +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> @@ -213,6 +213,7 @@ static int report_error_detected(struct
> struct aer_broadcast_data *result_data;
> result_data = (struct aer_broadcast_data *) data;
>
> + device_lock(&dev->dev);
> dev->error_state = result_data->state;
>
> if (!dev->driver ||
> @@ -231,12 +232,14 @@ static int report_error_detected(struct
> dev->driver ?
> "no AER-aware driver" : "no driver");
> }
> - return 0;
> + goto out;
> }
>
> err_handler = dev->driver->err_handler;
> vote = err_handler->error_detected(dev, result_data->state);
> result_data->result = merge_result(result_data->result, vote);
> +out:
> + device_unlock(&dev->dev);
> return 0;
> }
>
> @@ -247,14 +250,17 @@ static int report_mmio_enabled(struct pc
> struct aer_broadcast_data *result_data;
> result_data = (struct aer_broadcast_data *) data;
>
> + device_lock(&dev->dev);
> if (!dev->driver ||
> !dev->driver->err_handler ||
> !dev->driver->err_handler->mmio_enabled)
> - return 0;
> + goto out;
>
> err_handler = dev->driver->err_handler;
> vote = err_handler->mmio_enabled(dev);
> result_data->result = merge_result(result_data->result, vote);
> +out:
> + device_unlock(&dev->dev);
> return 0;
> }
>
> @@ -265,14 +271,17 @@ static int report_slot_reset(struct pci_
> struct aer_broadcast_data *result_data;
> result_data = (struct aer_broadcast_data *) data;
>
> + device_lock(&dev->dev);
> if (!dev->driver ||
> !dev->driver->err_handler ||
> !dev->driver->err_handler->slot_reset)
> - return 0;
> + goto out;
>
> err_handler = dev->driver->err_handler;
> vote = err_handler->slot_reset(dev);
> result_data->result = merge_result(result_data->result, vote);
> +out:
> + device_unlock(&dev->dev);
> return 0;
> }
>
> @@ -280,15 +289,18 @@ static int report_resume(struct pci_dev
> {
> const struct pci_error_handlers *err_handler;
>
> + device_lock(&dev->dev);
> dev->error_state = pci_channel_io_normal;
>
> if (!dev->driver ||
> !dev->driver->err_handler ||
> !dev->driver->err_handler->resume)
> - return 0;
> + goto out;
>
> err_handler = dev->driver->err_handler;
> err_handler->resume(dev);
> +out:
> + device_unlock(&dev->dev);
> return 0;
> }
>
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [BUGFIX 1/2] PCI/PM: Fix deadlock when unbind device if its parent in D3cold
2012-10-24 6:54 [BUGFIX 1/2] PCI/PM: Fix deadlock when unbind device if its parent in D3cold Huang Ying
2012-10-24 6:54 ` [BUGFIX 2/2] PCI/PM: Resume device before shutdown Huang Ying
2012-10-24 21:01 ` [BUGFIX 1/2] PCI/PM: Fix deadlock when unbind device if its parent in D3cold Rafael J. Wysocki
@ 2012-11-02 16:50 ` Bjorn Helgaas
2012-11-02 20:27 ` Rafael J. Wysocki
2012-11-03 5:06 ` Huang Ying
2 siblings, 2 replies; 15+ messages in thread
From: Bjorn Helgaas @ 2012-11-02 16:50 UTC (permalink / raw)
To: Huang Ying
Cc: linux-kernel, linux-pci, linux-pm, Rafael J. Wysocki, Zhang Yanmin
On Wed, Oct 24, 2012 at 12:54 AM, Huang Ying <ying.huang@intel.com> wrote:
> If a PCI device and its parents are put into D3cold, unbinding the
> device will trigger deadlock as follow:
>
> - driver_unbind
> - device_release_driver
> - device_lock(dev) <--- previous lock here
> - __device_release_driver
> - pm_runtime_get_sync
> ...
> - rpm_resume(dev)
> - rpm_resume(dev->parent)
> ...
> - pci_pm_runtime_resume
> ...
> - pci_set_power_state
> - __pci_start_power_transition
> - pci_wakeup_bus(dev->parent->subordinate)
> - pci_walk_bus
> - device_lock(dev) <--- dead lock here
>
>
> If we do not do device_lock in pci_walk_bus, we can avoid dead lock.
> Device_lock in pci_walk_bus is introduced in commit:
> d71374dafbba7ec3f67371d3b7e9f6310a588808, corresponding email thread
> is: https://lkml.org/lkml/2006/5/26/38. The patch author Zhang Yanmin
> said device_lock is added to pci_walk_bus because:
>
> Some error handling functions call pci_walk_bus. For example, PCIe
> aer. Here we lock the device, so the driver wouldn't detach from the
> device, as the cb might call driver's callback function.
>
> So I fixed the dead lock as follow:
>
> - remove device_lock from pci_walk_bus
> - add device_lock into callback if callback will call driver's callback
>
> I checked pci_walk_bus users one by one, and found only PCIe aer needs
> device lock.
Is there a problem report or bugzilla for this issue?
> Signed-off-by: Huang Ying <ying.huang@intel.com>
> Cc: Zhang Yanmin <yanmin.zhang@intel.com>
Should this go to stable as well? The D3cold support appeared in
v3.6, so my guess is that this fix could go to v3.6.x.
> ---
> drivers/pci/bus.c | 3 ---
> drivers/pci/pcie/aer/aerdrv_core.c | 20 ++++++++++++++++----
> 2 files changed, 16 insertions(+), 7 deletions(-)
>
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -320,10 +320,7 @@ void pci_walk_bus(struct pci_bus *top, i
> } else
> next = dev->bus_list.next;
>
> - /* Run device routines with the device locked */
> - device_lock(&dev->dev);
> retval = cb(dev, userdata);
> - device_unlock(&dev->dev);
> if (retval)
> break;
> }
> --- a/drivers/pci/pcie/aer/aerdrv_core.c
> +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> @@ -213,6 +213,7 @@ static int report_error_detected(struct
> struct aer_broadcast_data *result_data;
> result_data = (struct aer_broadcast_data *) data;
>
> + device_lock(&dev->dev);
> dev->error_state = result_data->state;
>
> if (!dev->driver ||
> @@ -231,12 +232,14 @@ static int report_error_detected(struct
> dev->driver ?
> "no AER-aware driver" : "no driver");
> }
> - return 0;
> + goto out;
> }
>
> err_handler = dev->driver->err_handler;
> vote = err_handler->error_detected(dev, result_data->state);
> result_data->result = merge_result(result_data->result, vote);
> +out:
> + device_unlock(&dev->dev);
> return 0;
> }
>
> @@ -247,14 +250,17 @@ static int report_mmio_enabled(struct pc
> struct aer_broadcast_data *result_data;
> result_data = (struct aer_broadcast_data *) data;
>
> + device_lock(&dev->dev);
> if (!dev->driver ||
> !dev->driver->err_handler ||
> !dev->driver->err_handler->mmio_enabled)
> - return 0;
> + goto out;
>
> err_handler = dev->driver->err_handler;
> vote = err_handler->mmio_enabled(dev);
> result_data->result = merge_result(result_data->result, vote);
> +out:
> + device_unlock(&dev->dev);
> return 0;
> }
>
> @@ -265,14 +271,17 @@ static int report_slot_reset(struct pci_
> struct aer_broadcast_data *result_data;
> result_data = (struct aer_broadcast_data *) data;
>
> + device_lock(&dev->dev);
> if (!dev->driver ||
> !dev->driver->err_handler ||
> !dev->driver->err_handler->slot_reset)
> - return 0;
> + goto out;
>
> err_handler = dev->driver->err_handler;
> vote = err_handler->slot_reset(dev);
> result_data->result = merge_result(result_data->result, vote);
> +out:
> + device_unlock(&dev->dev);
> return 0;
> }
>
> @@ -280,15 +289,18 @@ static int report_resume(struct pci_dev
> {
> const struct pci_error_handlers *err_handler;
>
> + device_lock(&dev->dev);
> dev->error_state = pci_channel_io_normal;
>
> if (!dev->driver ||
> !dev->driver->err_handler ||
> !dev->driver->err_handler->resume)
> - return 0;
> + goto out;
>
> err_handler = dev->driver->err_handler;
> err_handler->resume(dev);
> +out:
> + device_unlock(&dev->dev);
> return 0;
> }
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [BUGFIX 1/2] PCI/PM: Fix deadlock when unbind device if its parent in D3cold
2012-11-02 16:50 ` Bjorn Helgaas
@ 2012-11-02 20:27 ` Rafael J. Wysocki
2012-11-03 5:06 ` Huang Ying
1 sibling, 0 replies; 15+ messages in thread
From: Rafael J. Wysocki @ 2012-11-02 20:27 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Huang Ying, linux-kernel, linux-pci, linux-pm, Zhang Yanmin
On Friday, November 02, 2012 10:50:50 AM Bjorn Helgaas wrote:
> On Wed, Oct 24, 2012 at 12:54 AM, Huang Ying <ying.huang@intel.com> wrote:
> > If a PCI device and its parents are put into D3cold, unbinding the
> > device will trigger deadlock as follow:
> >
> > - driver_unbind
> > - device_release_driver
> > - device_lock(dev) <--- previous lock here
> > - __device_release_driver
> > - pm_runtime_get_sync
> > ...
> > - rpm_resume(dev)
> > - rpm_resume(dev->parent)
> > ...
> > - pci_pm_runtime_resume
> > ...
> > - pci_set_power_state
> > - __pci_start_power_transition
> > - pci_wakeup_bus(dev->parent->subordinate)
> > - pci_walk_bus
> > - device_lock(dev) <--- dead lock here
> >
> >
> > If we do not do device_lock in pci_walk_bus, we can avoid dead lock.
> > Device_lock in pci_walk_bus is introduced in commit:
> > d71374dafbba7ec3f67371d3b7e9f6310a588808, corresponding email thread
> > is: https://lkml.org/lkml/2006/5/26/38. The patch author Zhang Yanmin
> > said device_lock is added to pci_walk_bus because:
> >
> > Some error handling functions call pci_walk_bus. For example, PCIe
> > aer. Here we lock the device, so the driver wouldn't detach from the
> > device, as the cb might call driver's callback function.
> >
> > So I fixed the dead lock as follow:
> >
> > - remove device_lock from pci_walk_bus
> > - add device_lock into callback if callback will call driver's callback
> >
> > I checked pci_walk_bus users one by one, and found only PCIe aer needs
> > device lock.
>
> Is there a problem report or bugzilla for this issue?
>
> > Signed-off-by: Huang Ying <ying.huang@intel.com>
> > Cc: Zhang Yanmin <yanmin.zhang@intel.com>
>
> Should this go to stable as well? The D3cold support appeared in
> v3.6, so my guess is that this fix could go to v3.6.x.
That's correct.
Thanks,
Rafael
> > ---
> > drivers/pci/bus.c | 3 ---
> > drivers/pci/pcie/aer/aerdrv_core.c | 20 ++++++++++++++++----
> > 2 files changed, 16 insertions(+), 7 deletions(-)
> >
> > --- a/drivers/pci/bus.c
> > +++ b/drivers/pci/bus.c
> > @@ -320,10 +320,7 @@ void pci_walk_bus(struct pci_bus *top, i
> > } else
> > next = dev->bus_list.next;
> >
> > - /* Run device routines with the device locked */
> > - device_lock(&dev->dev);
> > retval = cb(dev, userdata);
> > - device_unlock(&dev->dev);
> > if (retval)
> > break;
> > }
> > --- a/drivers/pci/pcie/aer/aerdrv_core.c
> > +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> > @@ -213,6 +213,7 @@ static int report_error_detected(struct
> > struct aer_broadcast_data *result_data;
> > result_data = (struct aer_broadcast_data *) data;
> >
> > + device_lock(&dev->dev);
> > dev->error_state = result_data->state;
> >
> > if (!dev->driver ||
> > @@ -231,12 +232,14 @@ static int report_error_detected(struct
> > dev->driver ?
> > "no AER-aware driver" : "no driver");
> > }
> > - return 0;
> > + goto out;
> > }
> >
> > err_handler = dev->driver->err_handler;
> > vote = err_handler->error_detected(dev, result_data->state);
> > result_data->result = merge_result(result_data->result, vote);
> > +out:
> > + device_unlock(&dev->dev);
> > return 0;
> > }
> >
> > @@ -247,14 +250,17 @@ static int report_mmio_enabled(struct pc
> > struct aer_broadcast_data *result_data;
> > result_data = (struct aer_broadcast_data *) data;
> >
> > + device_lock(&dev->dev);
> > if (!dev->driver ||
> > !dev->driver->err_handler ||
> > !dev->driver->err_handler->mmio_enabled)
> > - return 0;
> > + goto out;
> >
> > err_handler = dev->driver->err_handler;
> > vote = err_handler->mmio_enabled(dev);
> > result_data->result = merge_result(result_data->result, vote);
> > +out:
> > + device_unlock(&dev->dev);
> > return 0;
> > }
> >
> > @@ -265,14 +271,17 @@ static int report_slot_reset(struct pci_
> > struct aer_broadcast_data *result_data;
> > result_data = (struct aer_broadcast_data *) data;
> >
> > + device_lock(&dev->dev);
> > if (!dev->driver ||
> > !dev->driver->err_handler ||
> > !dev->driver->err_handler->slot_reset)
> > - return 0;
> > + goto out;
> >
> > err_handler = dev->driver->err_handler;
> > vote = err_handler->slot_reset(dev);
> > result_data->result = merge_result(result_data->result, vote);
> > +out:
> > + device_unlock(&dev->dev);
> > return 0;
> > }
> >
> > @@ -280,15 +289,18 @@ static int report_resume(struct pci_dev
> > {
> > const struct pci_error_handlers *err_handler;
> >
> > + device_lock(&dev->dev);
> > dev->error_state = pci_channel_io_normal;
> >
> > if (!dev->driver ||
> > !dev->driver->err_handler ||
> > !dev->driver->err_handler->resume)
> > - return 0;
> > + goto out;
> >
> > err_handler = dev->driver->err_handler;
> > err_handler->resume(dev);
> > +out:
> > + device_unlock(&dev->dev);
> > return 0;
> > }
> >
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [BUGFIX 1/2] PCI/PM: Fix deadlock when unbind device if its parent in D3cold
2012-11-02 16:50 ` Bjorn Helgaas
2012-11-02 20:27 ` Rafael J. Wysocki
@ 2012-11-03 5:06 ` Huang Ying
2012-11-03 17:22 ` Bjorn Helgaas
1 sibling, 1 reply; 15+ messages in thread
From: Huang Ying @ 2012-11-03 5:06 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, linux-pci, linux-pm, Rafael J. Wysocki, Zhang Yanmin
On Fri, 2012-11-02 at 10:50 -0600, Bjorn Helgaas wrote:
> On Wed, Oct 24, 2012 at 12:54 AM, Huang Ying <ying.huang@intel.com> wrote:
> > If a PCI device and its parents are put into D3cold, unbinding the
> > device will trigger deadlock as follow:
> >
> > - driver_unbind
> > - device_release_driver
> > - device_lock(dev) <--- previous lock here
> > - __device_release_driver
> > - pm_runtime_get_sync
> > ...
> > - rpm_resume(dev)
> > - rpm_resume(dev->parent)
> > ...
> > - pci_pm_runtime_resume
> > ...
> > - pci_set_power_state
> > - __pci_start_power_transition
> > - pci_wakeup_bus(dev->parent->subordinate)
> > - pci_walk_bus
> > - device_lock(dev) <--- dead lock here
> >
> >
> > If we do not do device_lock in pci_walk_bus, we can avoid dead lock.
> > Device_lock in pci_walk_bus is introduced in commit:
> > d71374dafbba7ec3f67371d3b7e9f6310a588808, corresponding email thread
> > is: https://lkml.org/lkml/2006/5/26/38. The patch author Zhang Yanmin
> > said device_lock is added to pci_walk_bus because:
> >
> > Some error handling functions call pci_walk_bus. For example, PCIe
> > aer. Here we lock the device, so the driver wouldn't detach from the
> > device, as the cb might call driver's callback function.
> >
> > So I fixed the dead lock as follow:
> >
> > - remove device_lock from pci_walk_bus
> > - add device_lock into callback if callback will call driver's callback
> >
> > I checked pci_walk_bus users one by one, and found only PCIe aer needs
> > device lock.
>
> Is there a problem report or bugzilla for this issue?
No. I found this issue when I developed kexec fixing.
Best Regards,
Huang Ying
> > Signed-off-by: Huang Ying <ying.huang@intel.com>
> > Cc: Zhang Yanmin <yanmin.zhang@intel.com>
>
> Should this go to stable as well? The D3cold support appeared in
> v3.6, so my guess is that this fix could go to v3.6.x.
>
> > ---
> > drivers/pci/bus.c | 3 ---
> > drivers/pci/pcie/aer/aerdrv_core.c | 20 ++++++++++++++++----
> > 2 files changed, 16 insertions(+), 7 deletions(-)
> >
> > --- a/drivers/pci/bus.c
> > +++ b/drivers/pci/bus.c
> > @@ -320,10 +320,7 @@ void pci_walk_bus(struct pci_bus *top, i
> > } else
> > next = dev->bus_list.next;
> >
> > - /* Run device routines with the device locked */
> > - device_lock(&dev->dev);
> > retval = cb(dev, userdata);
> > - device_unlock(&dev->dev);
> > if (retval)
> > break;
> > }
> > --- a/drivers/pci/pcie/aer/aerdrv_core.c
> > +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> > @@ -213,6 +213,7 @@ static int report_error_detected(struct
> > struct aer_broadcast_data *result_data;
> > result_data = (struct aer_broadcast_data *) data;
> >
> > + device_lock(&dev->dev);
> > dev->error_state = result_data->state;
> >
> > if (!dev->driver ||
> > @@ -231,12 +232,14 @@ static int report_error_detected(struct
> > dev->driver ?
> > "no AER-aware driver" : "no driver");
> > }
> > - return 0;
> > + goto out;
> > }
> >
> > err_handler = dev->driver->err_handler;
> > vote = err_handler->error_detected(dev, result_data->state);
> > result_data->result = merge_result(result_data->result, vote);
> > +out:
> > + device_unlock(&dev->dev);
> > return 0;
> > }
> >
> > @@ -247,14 +250,17 @@ static int report_mmio_enabled(struct pc
> > struct aer_broadcast_data *result_data;
> > result_data = (struct aer_broadcast_data *) data;
> >
> > + device_lock(&dev->dev);
> > if (!dev->driver ||
> > !dev->driver->err_handler ||
> > !dev->driver->err_handler->mmio_enabled)
> > - return 0;
> > + goto out;
> >
> > err_handler = dev->driver->err_handler;
> > vote = err_handler->mmio_enabled(dev);
> > result_data->result = merge_result(result_data->result, vote);
> > +out:
> > + device_unlock(&dev->dev);
> > return 0;
> > }
> >
> > @@ -265,14 +271,17 @@ static int report_slot_reset(struct pci_
> > struct aer_broadcast_data *result_data;
> > result_data = (struct aer_broadcast_data *) data;
> >
> > + device_lock(&dev->dev);
> > if (!dev->driver ||
> > !dev->driver->err_handler ||
> > !dev->driver->err_handler->slot_reset)
> > - return 0;
> > + goto out;
> >
> > err_handler = dev->driver->err_handler;
> > vote = err_handler->slot_reset(dev);
> > result_data->result = merge_result(result_data->result, vote);
> > +out:
> > + device_unlock(&dev->dev);
> > return 0;
> > }
> >
> > @@ -280,15 +289,18 @@ static int report_resume(struct pci_dev
> > {
> > const struct pci_error_handlers *err_handler;
> >
> > + device_lock(&dev->dev);
> > dev->error_state = pci_channel_io_normal;
> >
> > if (!dev->driver ||
> > !dev->driver->err_handler ||
> > !dev->driver->err_handler->resume)
> > - return 0;
> > + goto out;
> >
> > err_handler = dev->driver->err_handler;
> > err_handler->resume(dev);
> > +out:
> > + device_unlock(&dev->dev);
> > return 0;
> > }
> >
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [BUGFIX 1/2] PCI/PM: Fix deadlock when unbind device if its parent in D3cold
2012-11-03 5:06 ` Huang Ying
@ 2012-11-03 17:22 ` Bjorn Helgaas
2012-11-04 3:38 ` Huang Ying
0 siblings, 1 reply; 15+ messages in thread
From: Bjorn Helgaas @ 2012-11-03 17:22 UTC (permalink / raw)
To: Huang Ying
Cc: linux-kernel, linux-pci, linux-pm, Rafael J. Wysocki, Zhang Yanmin
On Fri, Nov 2, 2012 at 11:06 PM, Huang Ying <ying.huang@intel.com> wrote:
> On Fri, 2012-11-02 at 10:50 -0600, Bjorn Helgaas wrote:
>> On Wed, Oct 24, 2012 at 12:54 AM, Huang Ying <ying.huang@intel.com> wrote:
>> > If a PCI device and its parents are put into D3cold, unbinding the
>> > device will trigger deadlock as follow:
>> >
>> > - driver_unbind
>> > - device_release_driver
>> > - device_lock(dev) <--- previous lock here
>> > - __device_release_driver
>> > - pm_runtime_get_sync
>> > ...
>> > - rpm_resume(dev)
>> > - rpm_resume(dev->parent)
>> > ...
>> > - pci_pm_runtime_resume
>> > ...
>> > - pci_set_power_state
>> > - __pci_start_power_transition
>> > - pci_wakeup_bus(dev->parent->subordinate)
>> > - pci_walk_bus
>> > - device_lock(dev) <--- dead lock here
>> >
>> >
>> > If we do not do device_lock in pci_walk_bus, we can avoid dead lock.
>> > Device_lock in pci_walk_bus is introduced in commit:
>> > d71374dafbba7ec3f67371d3b7e9f6310a588808, corresponding email thread
>> > is: https://lkml.org/lkml/2006/5/26/38. The patch author Zhang Yanmin
>> > said device_lock is added to pci_walk_bus because:
>> >
>> > Some error handling functions call pci_walk_bus. For example, PCIe
>> > aer. Here we lock the device, so the driver wouldn't detach from the
>> > device, as the cb might call driver's callback function.
>> >
>> > So I fixed the dead lock as follow:
>> >
>> > - remove device_lock from pci_walk_bus
>> > - add device_lock into callback if callback will call driver's callback
>> >
>> > I checked pci_walk_bus users one by one, and found only PCIe aer needs
>> > device lock.
>>
>> Is there a problem report or bugzilla for this issue?
>
> No. I found this issue when I developed kexec fixing.
>
> Best Regards,
> Huang Ying
>
>> > Signed-off-by: Huang Ying <ying.huang@intel.com>
>> > Cc: Zhang Yanmin <yanmin.zhang@intel.com>
>>
>> Should this go to stable as well? The D3cold support appeared in
>> v3.6, so my guess is that this fix could go to v3.6.x.
What about the stable tree?
>> > ---
>> > drivers/pci/bus.c | 3 ---
>> > drivers/pci/pcie/aer/aerdrv_core.c | 20 ++++++++++++++++----
>> > 2 files changed, 16 insertions(+), 7 deletions(-)
>> >
>> > --- a/drivers/pci/bus.c
>> > +++ b/drivers/pci/bus.c
>> > @@ -320,10 +320,7 @@ void pci_walk_bus(struct pci_bus *top, i
>> > } else
>> > next = dev->bus_list.next;
>> >
>> > - /* Run device routines with the device locked */
>> > - device_lock(&dev->dev);
>> > retval = cb(dev, userdata);
>> > - device_unlock(&dev->dev);
>> > if (retval)
>> > break;
>> > }
>> > --- a/drivers/pci/pcie/aer/aerdrv_core.c
>> > +++ b/drivers/pci/pcie/aer/aerdrv_core.c
>> > @@ -213,6 +213,7 @@ static int report_error_detected(struct
>> > struct aer_broadcast_data *result_data;
>> > result_data = (struct aer_broadcast_data *) data;
>> >
>> > + device_lock(&dev->dev);
>> > dev->error_state = result_data->state;
>> >
>> > if (!dev->driver ||
>> > @@ -231,12 +232,14 @@ static int report_error_detected(struct
>> > dev->driver ?
>> > "no AER-aware driver" : "no driver");
>> > }
>> > - return 0;
>> > + goto out;
>> > }
>> >
>> > err_handler = dev->driver->err_handler;
>> > vote = err_handler->error_detected(dev, result_data->state);
>> > result_data->result = merge_result(result_data->result, vote);
>> > +out:
>> > + device_unlock(&dev->dev);
>> > return 0;
>> > }
>> >
>> > @@ -247,14 +250,17 @@ static int report_mmio_enabled(struct pc
>> > struct aer_broadcast_data *result_data;
>> > result_data = (struct aer_broadcast_data *) data;
>> >
>> > + device_lock(&dev->dev);
>> > if (!dev->driver ||
>> > !dev->driver->err_handler ||
>> > !dev->driver->err_handler->mmio_enabled)
>> > - return 0;
>> > + goto out;
>> >
>> > err_handler = dev->driver->err_handler;
>> > vote = err_handler->mmio_enabled(dev);
>> > result_data->result = merge_result(result_data->result, vote);
>> > +out:
>> > + device_unlock(&dev->dev);
>> > return 0;
>> > }
>> >
>> > @@ -265,14 +271,17 @@ static int report_slot_reset(struct pci_
>> > struct aer_broadcast_data *result_data;
>> > result_data = (struct aer_broadcast_data *) data;
>> >
>> > + device_lock(&dev->dev);
>> > if (!dev->driver ||
>> > !dev->driver->err_handler ||
>> > !dev->driver->err_handler->slot_reset)
>> > - return 0;
>> > + goto out;
>> >
>> > err_handler = dev->driver->err_handler;
>> > vote = err_handler->slot_reset(dev);
>> > result_data->result = merge_result(result_data->result, vote);
>> > +out:
>> > + device_unlock(&dev->dev);
>> > return 0;
>> > }
>> >
>> > @@ -280,15 +289,18 @@ static int report_resume(struct pci_dev
>> > {
>> > const struct pci_error_handlers *err_handler;
>> >
>> > + device_lock(&dev->dev);
>> > dev->error_state = pci_channel_io_normal;
>> >
>> > if (!dev->driver ||
>> > !dev->driver->err_handler ||
>> > !dev->driver->err_handler->resume)
>> > - return 0;
>> > + goto out;
>> >
>> > err_handler = dev->driver->err_handler;
>> > err_handler->resume(dev);
>> > +out:
>> > + device_unlock(&dev->dev);
>> > return 0;
>> > }
>> >
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [BUGFIX 1/2] PCI/PM: Fix deadlock when unbind device if its parent in D3cold
2012-11-03 17:22 ` Bjorn Helgaas
@ 2012-11-04 3:38 ` Huang Ying
2012-11-05 22:11 ` Bjorn Helgaas
0 siblings, 1 reply; 15+ messages in thread
From: Huang Ying @ 2012-11-04 3:38 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, linux-pci, linux-pm, Rafael J. Wysocki, Zhang Yanmin
On Sat, 2012-11-03 at 11:22 -0600, Bjorn Helgaas wrote:
> On Fri, Nov 2, 2012 at 11:06 PM, Huang Ying <ying.huang@intel.com> wrote:
> > On Fri, 2012-11-02 at 10:50 -0600, Bjorn Helgaas wrote:
> >> On Wed, Oct 24, 2012 at 12:54 AM, Huang Ying <ying.huang@intel.com> wrote:
> >> > If a PCI device and its parents are put into D3cold, unbinding the
> >> > device will trigger deadlock as follow:
> >> >
> >> > - driver_unbind
> >> > - device_release_driver
> >> > - device_lock(dev) <--- previous lock here
> >> > - __device_release_driver
> >> > - pm_runtime_get_sync
> >> > ...
> >> > - rpm_resume(dev)
> >> > - rpm_resume(dev->parent)
> >> > ...
> >> > - pci_pm_runtime_resume
> >> > ...
> >> > - pci_set_power_state
> >> > - __pci_start_power_transition
> >> > - pci_wakeup_bus(dev->parent->subordinate)
> >> > - pci_walk_bus
> >> > - device_lock(dev) <--- dead lock here
> >> >
> >> >
> >> > If we do not do device_lock in pci_walk_bus, we can avoid dead lock.
> >> > Device_lock in pci_walk_bus is introduced in commit:
> >> > d71374dafbba7ec3f67371d3b7e9f6310a588808, corresponding email thread
> >> > is: https://lkml.org/lkml/2006/5/26/38. The patch author Zhang Yanmin
> >> > said device_lock is added to pci_walk_bus because:
> >> >
> >> > Some error handling functions call pci_walk_bus. For example, PCIe
> >> > aer. Here we lock the device, so the driver wouldn't detach from the
> >> > device, as the cb might call driver's callback function.
> >> >
> >> > So I fixed the dead lock as follow:
> >> >
> >> > - remove device_lock from pci_walk_bus
> >> > - add device_lock into callback if callback will call driver's callback
> >> >
> >> > I checked pci_walk_bus users one by one, and found only PCIe aer needs
> >> > device lock.
> >>
> >> Is there a problem report or bugzilla for this issue?
> >
> > No. I found this issue when I developed kexec fixing.
> >
> > Best Regards,
> > Huang Ying
> >
> >> > Signed-off-by: Huang Ying <ying.huang@intel.com>
> >> > Cc: Zhang Yanmin <yanmin.zhang@intel.com>
> >>
> >> Should this go to stable as well? The D3cold support appeared in
> >> v3.6, so my guess is that this fix could go to v3.6.x.
>
> What about the stable tree?
You are right. This fix should go to v3.6.x stable tree.
Best Regards,
Huang Ying
> >> > ---
> >> > drivers/pci/bus.c | 3 ---
> >> > drivers/pci/pcie/aer/aerdrv_core.c | 20 ++++++++++++++++----
> >> > 2 files changed, 16 insertions(+), 7 deletions(-)
> >> >
> >> > --- a/drivers/pci/bus.c
> >> > +++ b/drivers/pci/bus.c
> >> > @@ -320,10 +320,7 @@ void pci_walk_bus(struct pci_bus *top, i
> >> > } else
> >> > next = dev->bus_list.next;
> >> >
> >> > - /* Run device routines with the device locked */
> >> > - device_lock(&dev->dev);
> >> > retval = cb(dev, userdata);
> >> > - device_unlock(&dev->dev);
> >> > if (retval)
> >> > break;
> >> > }
> >> > --- a/drivers/pci/pcie/aer/aerdrv_core.c
> >> > +++ b/drivers/pci/pcie/aer/aerdrv_core.c
> >> > @@ -213,6 +213,7 @@ static int report_error_detected(struct
> >> > struct aer_broadcast_data *result_data;
> >> > result_data = (struct aer_broadcast_data *) data;
> >> >
> >> > + device_lock(&dev->dev);
> >> > dev->error_state = result_data->state;
> >> >
> >> > if (!dev->driver ||
> >> > @@ -231,12 +232,14 @@ static int report_error_detected(struct
> >> > dev->driver ?
> >> > "no AER-aware driver" : "no driver");
> >> > }
> >> > - return 0;
> >> > + goto out;
> >> > }
> >> >
> >> > err_handler = dev->driver->err_handler;
> >> > vote = err_handler->error_detected(dev, result_data->state);
> >> > result_data->result = merge_result(result_data->result, vote);
> >> > +out:
> >> > + device_unlock(&dev->dev);
> >> > return 0;
> >> > }
> >> >
> >> > @@ -247,14 +250,17 @@ static int report_mmio_enabled(struct pc
> >> > struct aer_broadcast_data *result_data;
> >> > result_data = (struct aer_broadcast_data *) data;
> >> >
> >> > + device_lock(&dev->dev);
> >> > if (!dev->driver ||
> >> > !dev->driver->err_handler ||
> >> > !dev->driver->err_handler->mmio_enabled)
> >> > - return 0;
> >> > + goto out;
> >> >
> >> > err_handler = dev->driver->err_handler;
> >> > vote = err_handler->mmio_enabled(dev);
> >> > result_data->result = merge_result(result_data->result, vote);
> >> > +out:
> >> > + device_unlock(&dev->dev);
> >> > return 0;
> >> > }
> >> >
> >> > @@ -265,14 +271,17 @@ static int report_slot_reset(struct pci_
> >> > struct aer_broadcast_data *result_data;
> >> > result_data = (struct aer_broadcast_data *) data;
> >> >
> >> > + device_lock(&dev->dev);
> >> > if (!dev->driver ||
> >> > !dev->driver->err_handler ||
> >> > !dev->driver->err_handler->slot_reset)
> >> > - return 0;
> >> > + goto out;
> >> >
> >> > err_handler = dev->driver->err_handler;
> >> > vote = err_handler->slot_reset(dev);
> >> > result_data->result = merge_result(result_data->result, vote);
> >> > +out:
> >> > + device_unlock(&dev->dev);
> >> > return 0;
> >> > }
> >> >
> >> > @@ -280,15 +289,18 @@ static int report_resume(struct pci_dev
> >> > {
> >> > const struct pci_error_handlers *err_handler;
> >> >
> >> > + device_lock(&dev->dev);
> >> > dev->error_state = pci_channel_io_normal;
> >> >
> >> > if (!dev->driver ||
> >> > !dev->driver->err_handler ||
> >> > !dev->driver->err_handler->resume)
> >> > - return 0;
> >> > + goto out;
> >> >
> >> > err_handler = dev->driver->err_handler;
> >> > err_handler->resume(dev);
> >> > +out:
> >> > + device_unlock(&dev->dev);
> >> > return 0;
> >> > }
> >> >
> >
> >
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [BUGFIX 1/2] PCI/PM: Fix deadlock when unbind device if its parent in D3cold
2012-11-04 3:38 ` Huang Ying
@ 2012-11-05 22:11 ` Bjorn Helgaas
0 siblings, 0 replies; 15+ messages in thread
From: Bjorn Helgaas @ 2012-11-05 22:11 UTC (permalink / raw)
To: Huang Ying
Cc: linux-kernel, linux-pci, linux-pm, Rafael J. Wysocki, Zhang Yanmin
On Sat, Nov 3, 2012 at 9:38 PM, Huang Ying <ying.huang@intel.com> wrote:
> On Sat, 2012-11-03 at 11:22 -0600, Bjorn Helgaas wrote:
>> On Fri, Nov 2, 2012 at 11:06 PM, Huang Ying <ying.huang@intel.com> wrote:
>> > On Fri, 2012-11-02 at 10:50 -0600, Bjorn Helgaas wrote:
>> >> On Wed, Oct 24, 2012 at 12:54 AM, Huang Ying <ying.huang@intel.com> wrote:
>> >> > If a PCI device and its parents are put into D3cold, unbinding the
>> >> > device will trigger deadlock as follow:
>> >> >
>> >> > - driver_unbind
>> >> > - device_release_driver
>> >> > - device_lock(dev) <--- previous lock here
>> >> > - __device_release_driver
>> >> > - pm_runtime_get_sync
>> >> > ...
>> >> > - rpm_resume(dev)
>> >> > - rpm_resume(dev->parent)
>> >> > ...
>> >> > - pci_pm_runtime_resume
>> >> > ...
>> >> > - pci_set_power_state
>> >> > - __pci_start_power_transition
>> >> > - pci_wakeup_bus(dev->parent->subordinate)
>> >> > - pci_walk_bus
>> >> > - device_lock(dev) <--- dead lock here
>> >> >
>> >> >
>> >> > If we do not do device_lock in pci_walk_bus, we can avoid dead lock.
>> >> > Device_lock in pci_walk_bus is introduced in commit:
>> >> > d71374dafbba7ec3f67371d3b7e9f6310a588808, corresponding email thread
>> >> > is: https://lkml.org/lkml/2006/5/26/38. The patch author Zhang Yanmin
>> >> > said device_lock is added to pci_walk_bus because:
>> >> >
>> >> > Some error handling functions call pci_walk_bus. For example, PCIe
>> >> > aer. Here we lock the device, so the driver wouldn't detach from the
>> >> > device, as the cb might call driver's callback function.
>> >> >
>> >> > So I fixed the dead lock as follow:
>> >> >
>> >> > - remove device_lock from pci_walk_bus
>> >> > - add device_lock into callback if callback will call driver's callback
>> >> >
>> >> > I checked pci_walk_bus users one by one, and found only PCIe aer needs
>> >> > device lock.
>> >>
>> >> Is there a problem report or bugzilla for this issue?
>> >
>> > No. I found this issue when I developed kexec fixing.
>> >
>> > Best Regards,
>> > Huang Ying
>> >
>> >> > Signed-off-by: Huang Ying <ying.huang@intel.com>
>> >> > Cc: Zhang Yanmin <yanmin.zhang@intel.com>
>> >>
>> >> Should this go to stable as well? The D3cold support appeared in
>> >> v3.6, so my guess is that this fix could go to v3.6.x.
>>
>> What about the stable tree?
>
> You are right. This fix should go to v3.6.x stable tree.
I applied these two patches to my for-linus branch as v3.7 material. Thanks!
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2012-11-05 22:11 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-24 6:54 [BUGFIX 1/2] PCI/PM: Fix deadlock when unbind device if its parent in D3cold Huang Ying
2012-10-24 6:54 ` [BUGFIX 2/2] PCI/PM: Resume device before shutdown Huang Ying
2012-10-24 21:01 ` Rafael J. Wysocki
2012-11-02 16:52 ` Bjorn Helgaas
2012-11-02 20:26 ` Rafael J. Wysocki
2012-11-03 5:05 ` Huang Ying
2012-11-03 17:21 ` Bjorn Helgaas
2012-11-04 3:44 ` Huang Ying
2012-10-24 21:01 ` [BUGFIX 1/2] PCI/PM: Fix deadlock when unbind device if its parent in D3cold Rafael J. Wysocki
2012-11-02 16:50 ` Bjorn Helgaas
2012-11-02 20:27 ` Rafael J. Wysocki
2012-11-03 5:06 ` Huang Ying
2012-11-03 17:22 ` Bjorn Helgaas
2012-11-04 3:38 ` Huang Ying
2012-11-05 22:11 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).