Hello,

On Mon, Nov 08, 2021 at 08:56:19PM -0600, Bjorn Helgaas wrote:

[+cc Greg: new device_is_bound() use]

ack, that's what I would have suggested now, too.

On Mon, Nov 08, 2021 at 10:22:26PM +0100, Uwe Kleine-König wrote:

quoted

pci_pm_runtime_resume() exits early when the device to resume isn't
bound yet:

	if (!to_pci_driver(dev->driver))
		return 0;

This however isn't true when the device currently probes and
local_pci_probe() calls pm_runtime_get_sync() because then the driver
core already setup dev->driver. As a result the driver's resume callback
is called before the driver's probe function is called and so more often
than not required driver data isn't setup yet.

So replace the check for the device being unbound by a check that only
becomes true after .probe() succeeded.

I like the fact that this patch is short and simple.

But there are 30+ users of to_pci_driver().  This patch asserts that
*one* of them, pci_pm_runtime_resume(), is special and needs to test
device_is_bound() instead of using to_pci_driver().

Maybe for the other locations using device_is_bound(&pdev->dev) instead
of to_pci_driver(pdev) != NULL would be nice, too?

I have another doubt: device_is_bound() should (according to its
kernel-doc) be called with the device lock held. For the call stack that
is (maybe) fixed here, the lock is held (by __device_attach). We
probably should check if the lock is also held for the other calls of
pci_pm_runtime_resume().

Hmm, the device lock is a mutex, the pm functions might be called in
atomic context, right?

It's special because the current PM implementation calls it via
pm_runtime_get_sync() before the driver's .probe() method.  That
connection is a little bit obscure and fragile.  What if the PM
implementation changes?

Maybe a saver bet would be to not use pm_runtime_get_sync() in
local_pci_probe()?

I wonder if the same problem exists on remove, i.e. pci_device_remove()
calls pm_runtime_put_sync() after the driver's .remove() callback was
called.

Maybe we just need a comment there about why it looks different than
the other PM interfaces?

A comment is a good idea for sure.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |