linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Lukas Wunner <lukas@wunner.de>
To: Matthew Minter <matt@masarand.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>, linux-pci@vger.kernel.org
Subject: Access to inaccessible PCI devices by 30fdfb929e82
Date: Mon, 29 Jan 2018 15:08:15 +0100	[thread overview]
Message-ID: <20180129140815.GA22041@wunner.de> (raw)

There appears to be an issue with commit 30fdfb929e82

    Author: Matthew Minter <matt@masarand.com>
    Date:   Wed Jun 28 15:14:04 2017 -0500
    PCI: Add a call to pci_assign_irq() in pci_device_probe()

wherein config space of a PCI device is accessed upon probe even though
the device may be inaccessible:

pci_pm_runtime_idle() and pci_pm_runtime_suspend() keep unbound devices
in D0, however they return 0 which results in the PM core considering
the device to have "suspended" status.  By default we prevent the device
from runtime suspending by calling pm_runtime_forbid() in pci_pm_init(),
however the user may subsequently allow runtime PM via sysfs.

If the user allows runtime PM on an unbound device and that device is
located below a root port, that root port will runtime suspend to D3hot
because its child is considered to be runtime suspended.

Even though the device is technically in D0, the bus below the root port
is now no longer in B0 and so the device is *effectively* in D3hot and
thus inaccessible.

We call pm_runtime_get_sync() in local_pci_probe() and this causes the
root port to resume to D0, making the device accessible again.

However the call to pci_assign_irq() introduced by the above-mentioned
commit happens *before* the call to pm_runtime_get_sync().  Thus, when
pci_assign_irq() accesses config space, it reads 0xff as IRQ number.

There are multiple possible solutions, including:

- Moving the invocation of pci_assign_irq() after pm_runtime_get_sync().

- Moving the invocation of pm_runtime_get_sync() before pci_assign_irq().

- Amending pci_pm_runtime_idle() and pci_pm_runtime_suspend() like this:
  if (!pci_dev->driver) {
	pm_runtime_forbid(dev);
	return -EBUSY;
  }

I don't know which solution is appropriate so I'm deferring to Rafael.

I'm not affected by this issue (my machine's struct pci_host_bridge
lacks the ->map_irq callback), it's just something that caught my eye
while perusing the code.

Thanks,

Lukas

                 reply	other threads:[~2018-01-29 14:08 UTC|newest]

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180129140815.GA22041@wunner.de \
    --to=lukas@wunner.de \
    --cc=bhelgaas@google.com \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=matt@masarand.com \
    --cc=rafael.j.wysocki@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).