On Wed, Apr 20, 2016 at 04:54:55PM +0200, Lukas Wunner wrote:
> Hi Bjorn,
>
> On Tue, Apr 19, 2016 at 06:23:29PM -0500, Bjorn Helgaas wrote:
> > On Thu, Mar 31, 2016 at 02:57:48PM +0200, Lukas Wunner wrote:
> > > Linux 4.5 introduced a behavioral change in device probing during the
> > > suspend process with commit 013c074f8642 ("PM / sleep: prohibit devices
> > > probing during suspend/hibernation"): It defers device probing during
> > > the entire suspend process, starting from the prepare phase and ending
> > > with the complete phase. A rule existed before that "we rely on sub-
> > > systems not to do any probing once a device is suspended" but it is
> > > enforced only now (Alan Stern, https://lkml.org/lkml/2015/9/15/908).
> > >
> > > This resulted in a WARN splat if a PCI device (e.g. Thunderbolt) is
> > > plugged in while the system is asleep: Upon waking up, pciehp_resume()
> > > discovers new devices in the resume phase and immediately tries to bind
> > > them to a driver. Since probing is now deferred, device_attach() returns
> > > -EPROBE_DEFER, which provoked a WARN in pci_bus_add_device().
> > >
> > > Linux 4.6-rc1 aggravates the situation with commit ab1a187bba5c ("PCI:
> > > Check device_attach() return value always"): pci_bus_add_device() no
> > > longer sets dev->is_added = 1 if device_attach() returned a negative
> > > value. This results in a BUG lockup in pci_bus_add_devices().
What is the connection here? 013c074f8642 causes device_attach() to
return -EPROBE_DEFER, and your patch makes pci_bus_add_device() treat
that as success, so it continues on and sets dev->is_added.
Since we will now set dev->is_added even for -EPROBE_DEFER, why do we
need to change the "BUG_ON(!dev->is_added)" in pci_bus_add_devices()?
I agree that device_attach() might fail for reasons other than
-EPROBE_DEFER, and it might be nice to avoid the BUG_ON then. It just
seems like those scenarios are different from the -EPROBE_DEFER one.
Sorry I'm being so dense here. I'm not very familiar with
suspend/resume, so I'm having a hard time following everything.
> > > @@ -294,7 +294,7 @@ void pci_bus_add_device(struct pci_dev *dev)
> > >
> > > dev->match_driver = true;
> > > retval = device_attach(&dev->dev);
> > > - if (retval < 0) {
> > > + if (retval < 0 && retval != -EPROBE_DEFER) {
> > > dev_warn(&dev->dev, "device attach failed (%d)\n", retval);
> >
> > I would prefer if the dev_warn() made a distinction between
> > -EPROBE_DEFER and other failures. It sounds like the -EPROBE_DEFER
> > case will happen in normal operation, and we probably shouldn't treat
> > it as a warning.
>
> Both v1 and v2 of my fix do not dev_warn() at all on -EPROBE_DEFER since,
> as you've correctly stated above, it happens in normal operation.
>
> If you want I could add a dev_info() specifically for the -EPROBE_DEFER
> case but personally I don't think it's necessary, it'll probably just
> irritate users.
You're right; sorry, I just wasn't reading your code clearly. I don't
think we need an extra message. We should already have messages for
detecting the new device and its BARs, and those should be enough.
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
