Re: [PATCH 1/2] PCI: Add pci_ignore_hotplug() to ignore hotplug events for a device
From: Rajat Jain
Date: Fri Sep 12 2014 - 12:40:48 EST
Hi,
On Thu, Sep 11, 2014 at 3:24 PM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote:
> Powering off a hot-pluggable device, e.g., with pci_set_power_state(D3cold),
> normally generates a hot-remove event that unbinds the driver.
>
> Some drivers expect to remain bound to a device even while they power it
> off and back on again. This can be dangerous, because if the device is
> removed or replaced while it is powered off, the driver doesn't know that
> anything changed. But some drivers accept that risk.
>
> Add pci_ignore_hotplug() for use by drivers that know their device cannot
> be removed. Using pci_ignore_hotplug() tells the PCI core that hot-plug
> events for the device should be ignored.
>
> The radeon and nouveau drivers use this to switch between a low-power,
> integrated GPU and a higher-power, higher-performance discrete GPU. They
> power off the unused GPU, but they want to remain bound to it.
>
> This is a reimplementation of f244d8b623da ("ACPIPHP / radeon / nouveau:
> Fix VGA switcheroo problem related to hotplug") but extends it to work with
> both acpiphp and pciehp.
>
> This fixes a problem where systems with dual GPUs using the radeon drivers
> become unusable, freezing every few seconds (see bugzillas below). The
> resume of the radeon device may also fail, e.g.,
>
> This fixes problems on dual GPU systems where the radeon driver becomes
> unusable because of problems while suspending the device, as in bug 79701:
>
> [drm] radeon: finishing device.
> radeon 0000:01:00.0: Userspace still has active objects !
> radeon 0000:01:00.0: ffff8800cb4ec288 ffff8800cb4ec000 16384 4294967297 force free
> ...
> WARNING: CPU: 0 PID: 67 at /home/apw/COD/linux/drivers/gpu/drm/radeon/radeon_gart.c:234 radeon_gart_unbind+0xd2/0xe0 [radeon]()
> trying to unbind memory from uninitialized GART !
>
> or while resuming it, as in bug 77261:
>
> radeon 0000:01:00.0: ring 0 stalled for more than 10158msec
> radeon 0000:01:00.0: GPU lockup ...
> radeon 0000:01:00.0: GPU pci config reset
> pciehp 0000:00:01.0:pcie04: Card not present on Slot(1-1)
> radeon 0000:01:00.0: GPU reset succeeded, trying to resume
> *ERROR* radeon: dpm resume failed
> radeon 0000:01:00.0: Wait for MC idle timedout !
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=77261
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=79701
> Reported-by: Shawn Starr <shawn.starr@xxxxxxxxxx>
> Reported-by: Jose P. <lbdkmjdf@xxxxxxxxxxxxxxx>
> Tested-by: SpacemanSpiff <a818958@xxxxxxxxx>
> Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> CC: stable@xxxxxxxxxxxxxxx # v3.15+
Acked-by: Rajat Jain <rajatxjain@xxxxxxxxx>
Just some additional notes (not related to the current problem). I
think there are other places where we could make use of the same newly
introduced pci_ignore_hotplug():
https://lkml.org/lkml/2013/12/15/151
I think that at least there are 2 places where this can potentially be utilized:
1) While handling AER, we may reset the link while trying to do a
recovery. This will result in a hot-remove followed by a hot-add. We
may utilize this call in that situation (will also need a
pci_resume_hotplug() call to cleat the ignore_hotplug flag)
2) Some drivers [mpt3sas_base.c: _base_diag_reset()] may reset the
device during a firmware upgrade or diagnostics.
Thanks,
Rajat
> ---
> drivers/gpu/drm/nouveau/nouveau_drm.c | 1 +
> drivers/gpu/drm/radeon/radeon_drv.c | 1 +
> drivers/pci/hotplug/acpiphp_glue.c | 16 ++++++----------
> drivers/pci/hotplug/pciehp_hpc.c | 12 ++++++++++++
> include/linux/pci.h | 6 ++++++
> 5 files changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 250a5e88c751..9c3af96a7153 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -627,6 +627,7 @@ int nouveau_pmops_suspend(struct device *dev)
>
> pci_save_state(pdev);
> pci_disable_device(pdev);
> + pci_ignore_hotplug(pdev);
> pci_set_power_state(pdev, PCI_D3hot);
> return 0;
> }
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
> index 8df888908833..abbd87adfd75 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -440,6 +440,7 @@ static int radeon_pmops_runtime_suspend(struct device *dev)
> ret = radeon_suspend_kms(drm_dev, false, false);
> pci_save_state(pdev);
> pci_disable_device(pdev);
> + pci_ignore_hotplug(pdev);
> pci_set_power_state(pdev, PCI_D3cold);
> drm_dev->switch_power_state = DRM_SWITCH_POWER_DYNAMIC_OFF;
>
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 70741c8c46a0..6cd5160fc057 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -560,19 +560,15 @@ static void disable_slot(struct acpiphp_slot *slot)
> slot->flags &= (~SLOT_ENABLED);
> }
>
> -static bool acpiphp_no_hotplug(struct acpi_device *adev)
> -{
> - return adev && adev->flags.no_hotplug;
> -}
> -
> static bool slot_no_hotplug(struct acpiphp_slot *slot)
> {
> - struct acpiphp_func *func;
> + struct pci_bus *bus = slot->bus;
> + struct pci_dev *dev;
>
> - list_for_each_entry(func, &slot->funcs, sibling)
> - if (acpiphp_no_hotplug(func_to_acpi_device(func)))
> + list_for_each_entry(dev, &bus->devices, bus_list) {
> + if (PCI_SLOT(dev->devfn) == slot->device && dev->ignore_hotplug)
> return true;
> -
> + }
> return false;
> }
>
> @@ -645,7 +641,7 @@ static void trim_stale_devices(struct pci_dev *dev)
>
> status = acpi_evaluate_integer(adev->handle, "_STA", NULL, &sta);
> alive = (ACPI_SUCCESS(status) && device_status_valid(sta))
> - || acpiphp_no_hotplug(adev);
> + || dev->ignore_hotplug;
> }
> if (!alive)
> alive = pci_device_is_present(dev);
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 9da84b8b27d8..5e01ae39ec46 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -506,6 +506,8 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
> {
> struct controller *ctrl = (struct controller *)dev_id;
> struct pci_dev *pdev = ctrl_dev(ctrl);
> + struct pci_bus *subordinate = pdev->subordinate;
> + struct pci_dev *dev;
> struct slot *slot = ctrl->slot;
> u16 detected, intr_loc;
>
> @@ -539,6 +541,16 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
> wake_up(&ctrl->queue);
> }
>
> + if (subordinate) {
> + list_for_each_entry(dev, &subordinate->devices, bus_list) {
> + if (dev->ignore_hotplug) {
> + ctrl_dbg(ctrl, "ignoring hotplug event %#06x (%s requested no hotplug)\n",
> + intr_loc, pci_name(dev));
> + return IRQ_HANDLED;
> + }
> + }
> + }
> +
> if (!(intr_loc & ~PCI_EXP_SLTSTA_CC))
> return IRQ_HANDLED;
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 61978a460841..96453f9bc8ba 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -303,6 +303,7 @@ struct pci_dev {
> D3cold, not set for devices
> powered on/off by the
> corresponding bridge */
> + unsigned int ignore_hotplug:1; /* Ignore hotplug events */
> unsigned int d3_delay; /* D3->D0 transition time in ms */
> unsigned int d3cold_delay; /* D3cold->D0 transition time in ms */
>
> @@ -1021,6 +1022,11 @@ bool pci_dev_run_wake(struct pci_dev *dev);
> bool pci_check_pme_status(struct pci_dev *dev);
> void pci_pme_wakeup_bus(struct pci_bus *bus);
>
> +static inline void pci_ignore_hotplug(struct pci_dev *dev)
> +{
> + dev->ignore_hotplug = 1;
> +}
> +
> static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state,
> bool enable)
> {
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/