| Message ID | 1375362389-26096-7-git-send-email-wangyijing@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Hi Bjorn, I still leave this code in pciehp modeule, because I do not find a better place to move. Only in pciehp module, we can know the device whether support hotplug, if we can not power off device, although we find the device change, but we can not remove the device for safety. What do you think? Thanks! Yijing. On 2013/8/1 21:06, Yijing Wang wrote: > If device was removed from slot and reinsert a new device during > suspend, pciehp can not identify the physical device change now. > So the old driver .resume() method will be called for the new device, > this is bad. If device support device serial number capability, > we can identify this by DSN. So the reasonable way is first remove > the old device, then enable the new device. > > Signed-off-by: Yijing Wang <wangyijing@huawei.com> > Cc: Paul Bolle <pebolle@tiscali.nl> > Cc: "Rafael J. Wysocki" <rjw@sisk.pl> > Cc: Oliver Neukum <oneukum@suse.de> > Cc: Gu Zheng <guz.fnst@cn.fujitsu.com> > Cc: linux-pci@vger.kernel.org > --- > drivers/pci/hotplug/pciehp_core.c | 34 +++++++++++++++++++++++++++++++--- > 1 files changed, 31 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c > index 7fe9dbd..db5f597 100644 > --- a/drivers/pci/hotplug/pciehp_core.c > +++ b/drivers/pci/hotplug/pciehp_core.c > @@ -301,6 +301,8 @@ static int pciehp_resume (struct pcie_device *dev) > struct controller *ctrl; > struct slot *slot; > struct pci_bus *pbus = dev->port->subordinate; > + struct pci_dev *pdev; > + int ret = 0; > u8 status; > > ctrl = get_service_data(dev); > @@ -315,10 +317,36 @@ static int pciehp_resume (struct pcie_device *dev) > if (status) { > if (list_empty(&pbus->devices)) > pciehp_enable_slot(slot); > - } else if (!list_empty(&pbus->devices)) { > - pciehp_disable_slot(slot); > + > + pdev = pci_get_slot(pbus, PCI_DEVFN(0, 0)); > + if (!pdev) > + return 0; > + ret = pci_serial_number_changed(pdev); > + pci_dev_put(pdev); > + if (ret) { > + /* > + * first power off slot, avoid the old driver > + * .remove() method touch the new hardware > + */ > + if (POWER_CTRL(ctrl)) { > + ret = pciehp_power_off_slot(slot); > + if (ret) { > + ctrl_err(ctrl, > + "Issue of Slot Disable command failed\n"); > + return 0; > + } > + msleep(1000); > + pciehp_unconfigure_device(slot); > + /* call child devices driver->suspend() for symmetry */ > + list_for_each_entry(pdev, &pbus->devices, bus_list) > + if (pdev->driver) > + pdev->driver->driver.pm->suspend(&pdev->dev); > + pciehp_enable_slot(slot); > + } > + } else if (!list_empty(&pbus->devices)) { > + pciehp_disable_slot(slot); > + } > } > - > return 0; > } > #endif /* PM */ >
Yijing, On Thu, 2013-08-01 at 21:06 +0800, Yijing Wang wrote: > diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c > index 7fe9dbd..db5f597 100644 > --- a/drivers/pci/hotplug/pciehp_core.c > +++ b/drivers/pci/hotplug/pciehp_core.c > [...] > @@ -315,10 +317,36 @@ static int pciehp_resume (struct pcie_device *dev) > if (status) { > if (list_empty(&pbus->devices)) > pciehp_enable_slot(slot); > - } else if (!list_empty(&pbus->devices)) { > - pciehp_disable_slot(slot); > + > + pdev = pci_get_slot(pbus, PCI_DEVFN(0, 0)); > + if (!pdev) > + return 0; > + ret = pci_serial_number_changed(pdev); > + pci_dev_put(pdev); > + if (ret) { > + /* > + * first power off slot, avoid the old driver > + * .remove() method touch the new hardware > + */ > + if (POWER_CTRL(ctrl)) { > + ret = pciehp_power_off_slot(slot); > + if (ret) { > + ctrl_err(ctrl, > + "Issue of Slot Disable command failed\n"); > + return 0; > + } > + msleep(1000); > + pciehp_unconfigure_device(slot); > + /* call child devices driver->suspend() for symmetry */ > + list_for_each_entry(pdev, &pbus->devices, bus_list) > + if (pdev->driver) > + pdev->driver->driver.pm->suspend(&pdev->dev); > + pciehp_enable_slot(slot); > + } > + } else if (!list_empty(&pbus->devices)) { > + pciehp_disable_slot(slot); > + } > } > - I ran into a problem with this change. This line, removing a line consisting of only a single tab, appears to conflict with a patch earlier in this series. I had to manually remove just the change that removed that line to get this patch to apply. Any idea what's going on? > return 0; > } > #endif /* PM */ Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> I ran into a problem with this change. This line, removing a line > consisting of only a single tab, appears to conflict with a patch > earlier in this series. I had to manually remove just the change that > removed that line to get this patch to apply. Any idea what's going on? > >> return 0; >> } >> #endif /* PM */ Hi Paul, Thanks for your review and test very much! Because this series was based Bjorn's pci-next branch, so maybe this is the cause, although I don't find any difference between Bjorn's pci-next tree and Linus master tree. I will refresh this patch. Thanks! Yijing.
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c index 7fe9dbd..db5f597 100644 --- a/drivers/pci/hotplug/pciehp_core.c +++ b/drivers/pci/hotplug/pciehp_core.c @@ -301,6 +301,8 @@ static int pciehp_resume (struct pcie_device *dev) struct controller *ctrl; struct slot *slot; struct pci_bus *pbus = dev->port->subordinate; + struct pci_dev *pdev; + int ret = 0; u8 status; ctrl = get_service_data(dev); @@ -315,10 +317,36 @@ static int pciehp_resume (struct pcie_device *dev) if (status) { if (list_empty(&pbus->devices)) pciehp_enable_slot(slot); - } else if (!list_empty(&pbus->devices)) { - pciehp_disable_slot(slot); + + pdev = pci_get_slot(pbus, PCI_DEVFN(0, 0)); + if (!pdev) + return 0; + ret = pci_serial_number_changed(pdev); + pci_dev_put(pdev); + if (ret) { + /* + * first power off slot, avoid the old driver + * .remove() method touch the new hardware + */ + if (POWER_CTRL(ctrl)) { + ret = pciehp_power_off_slot(slot); + if (ret) { + ctrl_err(ctrl, + "Issue of Slot Disable command failed\n"); + return 0; + } + msleep(1000); + pciehp_unconfigure_device(slot); + /* call child devices driver->suspend() for symmetry */ + list_for_each_entry(pdev, &pbus->devices, bus_list) + if (pdev->driver) + pdev->driver->driver.pm->suspend(&pdev->dev); + pciehp_enable_slot(slot); + } + } else if (!list_empty(&pbus->devices)) { + pciehp_disable_slot(slot); + } } - return 0; } #endif /* PM */
If device was removed from slot and reinsert a new device during suspend, pciehp can not identify the physical device change now. So the old driver .resume() method will be called for the new device, this is bad. If device support device serial number capability, we can identify this by DSN. So the reasonable way is first remove the old device, then enable the new device. Signed-off-by: Yijing Wang <wangyijing@huawei.com> Cc: Paul Bolle <pebolle@tiscali.nl> Cc: "Rafael J. Wysocki" <rjw@sisk.pl> Cc: Oliver Neukum <oneukum@suse.de> Cc: Gu Zheng <guz.fnst@cn.fujitsu.com> Cc: linux-pci@vger.kernel.org --- drivers/pci/hotplug/pciehp_core.c | 34 +++++++++++++++++++++++++++++++--- 1 files changed, 31 insertions(+), 3 deletions(-)