Re: [PATCH] PCI: Fix device attach failure handling

[Lukas Wunner <lukas@wunner.de>]

Hi Bjorn,

On Wed, Apr 20, 2016 at 02:48:08PM -0500, Bjorn Helgaas wrote:
> On Wed, Apr 20, 2016 at 04:54:55PM +0200, Lukas Wunner wrote:
> > 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.

Correct. The idea is that your commit ab1a187bba5c already throws a
warning message and undoes the steps preceding device_attach(),
so there's no reason to BUG_ON at this point. If we want to BUG_ON
whenever device_attach() fails we should do that in pci_bus_add_devices()
right after the call to device_attach().

It just doesn't seem to make sense to BUG_ON, there's no bug here,
ever since ab1a187bba5c got introduced, dev->is_added == false is now
a legitimate and expected result if device_attach() failed.

The commit message above is written from a user's point of view:
In 4.5 users got a WARN, in 4.6 they got a BUG when doing the same thing.


> Sorry I'm being so dense here.  I'm not very familiar with
> suspend/resume, so I'm having a hard time following everything.

More like me writing incomprehensible / irritating stuff. Please ask
more questions if I failed to make everything clear.

Thanks,

Lukas