Re: Crashes in 5.15-git in i2c code

Re: Crashes in 5.15-git in i2c code

[Bjorn Helgaas]

[+cc Uwe, Rafael, linux-pm, linux-pci, linux-kernel, beginning of
thread: https://lore.kernel.org/linux-i2c/CAP145pgdrdiMAT7=-iB1DMgA7t_bMqTcJL4N0=6u8kNY3EU0dw@mail.gmail.com/T/#t]

On Mon, Nov 08, 2021 at 05:34:14PM +0100, Robert Święcki wrote:
> > I'm daily-driving the linux from Linus' git (recompiling every day or
> > two), and yesterday it stopped booting. Below is the dmesg from
> > pstore.
> > ...
> 
> This introduced the bug: 0c5c62ddf88c34bc83b66e4ac9beb2bb0e1887d4
> https://github.com/torvalds/linux/commit/0c5c62ddf88c34bc83b66e4ac9beb2bb0e1887d4

Thank you very much for the debugging and this report!  This report is
for i2c, but the problem will affect many drivers.

> > <1>[    1.431369][  T447] BUG: kernel NULL pointer dereference,
> > address: 0000000000000540
> > <1>[    1.431371][  T447] #PF: supervisor read access in kernel mode
> > <1>[    1.431375][  T447] #PF: error_code(0x0000) - not-present page
> > <6>[    1.431378][  T447] PGD 0 P4D 0
> > <4>[    1.431384][  T447] Oops: 0000 [#1] PREEMPT SMP NOPTI
> > <4>[    1.431388][  T447] CPU: 12 PID: 447 Comm: systemd-udevd
> > Tainted: G            E     5.15.0+ #91
> > <4>[    1.431391][  T447] Hardware name: ASUS System Product Name/ROG
> > CROSSHAIR VIII FORMULA, BIOS 3801 07/30/2021
> > <4>[    1.431392][  T447] RIP: 0010:i2c_dw_pci_resume+0x8/0x40
> > [i2c_designware_pci]
> > <4>[    1.431399][  T447] Code: 00 00 00 00 66 66 2e 0f 1f 84 00 00 00
> > 00 00 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 84 00 00 00 00 00 53 48
> > 8b 5f 78 48 89 df <ff> 93 40 05 00 00 c6 83 c0 05 00 00 00 5b c3 66 66
> > 2e 0f 1f 84 00
> > <4>[    1.431401][  T447] RSP: 0018:ffffb3e740a13ba8 EFLAGS: 00010286
> > <4>[    1.431403][  T447] RAX: 0000000000000000 RBX: 0000000000000000
> > RCX: 0000000000000000

  $ ./scripts/decodecode < oops
    22:       53                      push   %rbx
    23:       48 8b 5f 78             mov    0x78(%rdi),%rbx
    27:       48 89 df                mov    %rbx,%rdi
    2a:*      ff 93 40 05 00 00       callq  *0x540(%rbx)             <-- trapping instruction
    30:       c6 83 c0 05 00 00 00    movb   $0x0,0x5c0(%rbx)
    37:       5b                      pop    %rbx
    38:       c3                      retq

  static int i2c_dw_pci_resume(struct device *dev)
  {
    struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
    int ret;

    ret = i_dev->init(i_dev);
    i_dev->suspended = false;

    return ret;

So I think we're trying to call i_dev->init(), which is a NULL
pointer.

> > <4>[    1.431422][  T447]  pci_pm_runtime_resume+0xaa/0x100
> > <4>[    1.431434][  T447]  __rpm_callback+0x3c/0x100
> > <4>[    1.431442][  T447]  rpm_callback+0x54/0x80
> > <4>[    1.431445][  T447]  rpm_resume+0x410/0x700
> > <4>[    1.431455][  T447]  __pm_runtime_resume+0x45/0x80
> > <4>[    1.431457][  T447]  pci_device_probe+0xa2/0x140
> > <4>[    1.431459][  T447]  really_probe+0x1e4/0x400
> > <4>[    1.431464][  T447]  __driver_probe_device+0xf9/0x180
> > <4>[    1.431466][  T447]  driver_probe_device+0x19/0xc0

I think the problem here is that:

  - really_probe() sets dev->driver

  - local_pci_probe() calls pm_runtime_get_sync(), which leads to:

  - pci_pm_runtime_resume(), which previously skipped the driver's
    .runtime_resume() method when "pci_dev->driver" as NULL

  - after 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of
    pci_dev->driver") [1], it checks "dev->driver" instead of
    "pci_dev->driver"

  - dev->driver is non-NULL (set by really_probe() above), but at this
    point pci_dev->driver used to be NULL because local_pci_probe()
    didn't set it until after after calling pm_runtime_get_sync() (see
    b5f9c644eb1b ("PCI: Remove struct pci_dev->driver") [2])

  - because dev->driver is non-NULL, we call i2c_dw_pci_resume()
    before i2c_dw_pci_probe(), so the driver init hasn't been done

Here's the call tree:

    really_probe
      dev->driver = drv;                       # <--
      call_driver_probe
        dev->bus->probe
          pci_device_probe
            __pci_device_probe
              pci_call_probe
                local_pci_probe
                  pm_runtime_get_sync
                    ...
                    pci_pm_runtime_resume
  -                   if (!pci_dev->driver)    # 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of pci_dev->driver")
  +                   if (!to_pci_driver(dev->driver))
                        return 0
                      pm->runtime_resume
                        i2c_dw_pci_resume
                          i_dev->init()        # <-- NULL ptr deref
  -                 pci_dev->driver = pci_drv  # b5f9c644eb1b ("PCI: Remove struct pci_dev->driver")
                  pci_drv->probe
                    i2c_dw_pci_probe

Note that we used to call pm_runtime_get_sync() (and the driver's
runtime_resume() method) in the window where dev->driver had been set
but pci_dev->driver had not.  Since we got rid of pci_dev->driver
altogether, that window no longer exists, so we call the driver's
runtime_resume() when it isn't prepared for it.

[1] https://git.kernel.org/linus/2a4d9408c9e8
[2] https://git.kernel.org/linus/b5f9c644eb1b

Re: Crashes in 5.15-git in i2c code

[Bjorn Helgaas]

[+cc Linus -- FYI since I'm sure you'll see more reports of this; we
call driver resume methods before they're prepared for it.  We'll fix
or revert this ASAP.]

On Mon, Nov 08, 2021 at 12:58:25PM -0600, Bjorn Helgaas wrote:
> [+cc Uwe, Rafael, linux-pm, linux-pci, linux-kernel, beginning of
> thread: https://lore.kernel.org/linux-i2c/CAP145pgdrdiMAT7=-iB1DMgA7t_bMqTcJL4N0=6u8kNY3EU0dw@mail.gmail.com/T/#t]
> 
> On Mon, Nov 08, 2021 at 05:34:14PM +0100, Robert Święcki wrote:
> > > I'm daily-driving the linux from Linus' git (recompiling every day or
> > > two), and yesterday it stopped booting. Below is the dmesg from
> > > pstore.
> > > ...
> > 
> > This introduced the bug: 0c5c62ddf88c34bc83b66e4ac9beb2bb0e1887d4
> > https://github.com/torvalds/linux/commit/0c5c62ddf88c34bc83b66e4ac9beb2bb0e1887d4
> 
> Thank you very much for the debugging and this report!  This report is
> for i2c, but the problem will affect many drivers.
> 
> > > <1>[    1.431369][  T447] BUG: kernel NULL pointer dereference,
> > > address: 0000000000000540
> > > <1>[    1.431371][  T447] #PF: supervisor read access in kernel mode
> > > <1>[    1.431375][  T447] #PF: error_code(0x0000) - not-present page
> > > <6>[    1.431378][  T447] PGD 0 P4D 0
> > > <4>[    1.431384][  T447] Oops: 0000 [#1] PREEMPT SMP NOPTI
> > > <4>[    1.431388][  T447] CPU: 12 PID: 447 Comm: systemd-udevd
> > > Tainted: G            E     5.15.0+ #91
> > > <4>[    1.431391][  T447] Hardware name: ASUS System Product Name/ROG
> > > CROSSHAIR VIII FORMULA, BIOS 3801 07/30/2021
> > > <4>[    1.431392][  T447] RIP: 0010:i2c_dw_pci_resume+0x8/0x40
> > > [i2c_designware_pci]
> > > <4>[    1.431399][  T447] Code: 00 00 00 00 66 66 2e 0f 1f 84 00 00 00
> > > 00 00 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 84 00 00 00 00 00 53 48
> > > 8b 5f 78 48 89 df <ff> 93 40 05 00 00 c6 83 c0 05 00 00 00 5b c3 66 66
> > > 2e 0f 1f 84 00
> > > <4>[    1.431401][  T447] RSP: 0018:ffffb3e740a13ba8 EFLAGS: 00010286
> > > <4>[    1.431403][  T447] RAX: 0000000000000000 RBX: 0000000000000000
> > > RCX: 0000000000000000
> 
>   $ ./scripts/decodecode < oops
>     22:       53                      push   %rbx
>     23:       48 8b 5f 78             mov    0x78(%rdi),%rbx
>     27:       48 89 df                mov    %rbx,%rdi
>     2a:*      ff 93 40 05 00 00       callq  *0x540(%rbx)             <-- trapping instruction
>     30:       c6 83 c0 05 00 00 00    movb   $0x0,0x5c0(%rbx)
>     37:       5b                      pop    %rbx
>     38:       c3                      retq
> 
>   static int i2c_dw_pci_resume(struct device *dev)
>   {
>     struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
>     int ret;
> 
>     ret = i_dev->init(i_dev);
>     i_dev->suspended = false;
> 
>     return ret;
> 
> So I think we're trying to call i_dev->init(), which is a NULL
> pointer.
> 
> > > <4>[    1.431422][  T447]  pci_pm_runtime_resume+0xaa/0x100
> > > <4>[    1.431434][  T447]  __rpm_callback+0x3c/0x100
> > > <4>[    1.431442][  T447]  rpm_callback+0x54/0x80
> > > <4>[    1.431445][  T447]  rpm_resume+0x410/0x700
> > > <4>[    1.431455][  T447]  __pm_runtime_resume+0x45/0x80
> > > <4>[    1.431457][  T447]  pci_device_probe+0xa2/0x140
> > > <4>[    1.431459][  T447]  really_probe+0x1e4/0x400
> > > <4>[    1.431464][  T447]  __driver_probe_device+0xf9/0x180
> > > <4>[    1.431466][  T447]  driver_probe_device+0x19/0xc0
> 
> I think the problem here is that:
> 
>   - really_probe() sets dev->driver
> 
>   - local_pci_probe() calls pm_runtime_get_sync(), which leads to:
> 
>   - pci_pm_runtime_resume(), which previously skipped the driver's
>     .runtime_resume() method when "pci_dev->driver" as NULL
> 
>   - after 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of
>     pci_dev->driver") [1], it checks "dev->driver" instead of
>     "pci_dev->driver"
> 
>   - dev->driver is non-NULL (set by really_probe() above), but at this
>     point pci_dev->driver used to be NULL because local_pci_probe()
>     didn't set it until after after calling pm_runtime_get_sync() (see
>     b5f9c644eb1b ("PCI: Remove struct pci_dev->driver") [2])
> 
>   - because dev->driver is non-NULL, we call i2c_dw_pci_resume()
>     before i2c_dw_pci_probe(), so the driver init hasn't been done
> 
> Here's the call tree:
> 
>     really_probe
>       dev->driver = drv;                       # <--
>       call_driver_probe
>         dev->bus->probe
>           pci_device_probe
>             __pci_device_probe
>               pci_call_probe
>                 local_pci_probe
>                   pm_runtime_get_sync
>                     ...
>                     pci_pm_runtime_resume
>   -                   if (!pci_dev->driver)    # 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of pci_dev->driver")
>   +                   if (!to_pci_driver(dev->driver))
>                         return 0
>                       pm->runtime_resume
>                         i2c_dw_pci_resume
>                           i_dev->init()        # <-- NULL ptr deref
>   -                 pci_dev->driver = pci_drv  # b5f9c644eb1b ("PCI: Remove struct pci_dev->driver")
>                   pci_drv->probe
>                     i2c_dw_pci_probe
> 
> Note that we used to call pm_runtime_get_sync() (and the driver's
> runtime_resume() method) in the window where dev->driver had been set
> but pci_dev->driver had not.  Since we got rid of pci_dev->driver
> altogether, that window no longer exists, so we call the driver's
> runtime_resume() when it isn't prepared for it.
> 
> [1] https://git.kernel.org/linus/2a4d9408c9e8
> [2] https://git.kernel.org/linus/b5f9c644eb1b

[PATCH] pci: Don't call resume callback for nearly bound devices

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 6777 bytes --]

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.

Thanks to Bjorn Helgaas for the analysis.

Reported-by: Robert Święcki <robert@swiecki.net>
Fixes: 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of pci_dev->driver")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
On Mon, Nov 08, 2021 at 12:58:23PM -0600, Bjorn Helgaas wrote:
> [+cc Uwe, Rafael, linux-pm, linux-pci, linux-kernel, beginning of
> thread: https://lore.kernel.org/linux-i2c/CAP145pgdrdiMAT7=-iB1DMgA7t_bMqTcJL4N0=6u8kNY3EU0dw@mail.gmail.com/T/#t]
> 
> On Mon, Nov 08, 2021 at 05:34:14PM +0100, Robert Święcki wrote:
> > > I'm daily-driving the linux from Linus' git (recompiling every day or
> > > two), and yesterday it stopped booting. Below is the dmesg from
> > > pstore.
> > > ...
> > 
> > This introduced the bug: 0c5c62ddf88c34bc83b66e4ac9beb2bb0e1887d4
> > https://github.com/torvalds/linux/commit/0c5c62ddf88c34bc83b66e4ac9beb2bb0e1887d4
> 
> Thank you very much for the debugging and this report!  This report is
> for i2c, but the problem will affect many drivers.
> 
> > > <1>[    1.431369][  T447] BUG: kernel NULL pointer dereference,
> > > address: 0000000000000540
> > > <1>[    1.431371][  T447] #PF: supervisor read access in kernel mode
> > > <1>[    1.431375][  T447] #PF: error_code(0x0000) - not-present page
> > > <6>[    1.431378][  T447] PGD 0 P4D 0
> > > <4>[    1.431384][  T447] Oops: 0000 [#1] PREEMPT SMP NOPTI
> > > <4>[    1.431388][  T447] CPU: 12 PID: 447 Comm: systemd-udevd
> > > Tainted: G            E     5.15.0+ #91
> > > <4>[    1.431391][  T447] Hardware name: ASUS System Product Name/ROG
> > > CROSSHAIR VIII FORMULA, BIOS 3801 07/30/2021
> > > <4>[    1.431392][  T447] RIP: 0010:i2c_dw_pci_resume+0x8/0x40
> > > [i2c_designware_pci]
> > > <4>[    1.431399][  T447] Code: 00 00 00 00 66 66 2e 0f 1f 84 00 00 00
> > > 00 00 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 84 00 00 00 00 00 53 48
> > > 8b 5f 78 48 89 df <ff> 93 40 05 00 00 c6 83 c0 05 00 00 00 5b c3 66 66
> > > 2e 0f 1f 84 00
> > > <4>[    1.431401][  T447] RSP: 0018:ffffb3e740a13ba8 EFLAGS: 00010286
> > > <4>[    1.431403][  T447] RAX: 0000000000000000 RBX: 0000000000000000
> > > RCX: 0000000000000000
> 
>   $ ./scripts/decodecode < oops
>     22:       53                      push   %rbx
>     23:       48 8b 5f 78             mov    0x78(%rdi),%rbx
>     27:       48 89 df                mov    %rbx,%rdi
>     2a:*      ff 93 40 05 00 00       callq  *0x540(%rbx)             <-- trapping instruction
>     30:       c6 83 c0 05 00 00 00    movb   $0x0,0x5c0(%rbx)
>     37:       5b                      pop    %rbx
>     38:       c3                      retq
> 
>   static int i2c_dw_pci_resume(struct device *dev)
>   {
>     struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
>     int ret;
> 
>     ret = i_dev->init(i_dev);
>     i_dev->suspended = false;
> 
>     return ret;
> 
> So I think we're trying to call i_dev->init(), which is a NULL
> pointer.
> 
> > > <4>[    1.431422][  T447]  pci_pm_runtime_resume+0xaa/0x100
> > > <4>[    1.431434][  T447]  __rpm_callback+0x3c/0x100
> > > <4>[    1.431442][  T447]  rpm_callback+0x54/0x80
> > > <4>[    1.431445][  T447]  rpm_resume+0x410/0x700
> > > <4>[    1.431455][  T447]  __pm_runtime_resume+0x45/0x80
> > > <4>[    1.431457][  T447]  pci_device_probe+0xa2/0x140
> > > <4>[    1.431459][  T447]  really_probe+0x1e4/0x400
> > > <4>[    1.431464][  T447]  __driver_probe_device+0xf9/0x180
> > > <4>[    1.431466][  T447]  driver_probe_device+0x19/0xc0
> 
> I think the problem here is that:
> 
>   - really_probe() sets dev->driver
> 
>   - local_pci_probe() calls pm_runtime_get_sync(), which leads to:
> 
>   - pci_pm_runtime_resume(), which previously skipped the driver's
>     .runtime_resume() method when "pci_dev->driver" as NULL
> 
>   - after 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of
>     pci_dev->driver") [1], it checks "dev->driver" instead of
>     "pci_dev->driver"
> 
>   - dev->driver is non-NULL (set by really_probe() above), but at this
>     point pci_dev->driver used to be NULL because local_pci_probe()
>     didn't set it until after after calling pm_runtime_get_sync() (see
>     b5f9c644eb1b ("PCI: Remove struct pci_dev->driver") [2])
> 
>   - because dev->driver is non-NULL, we call i2c_dw_pci_resume()
>     before i2c_dw_pci_probe(), so the driver init hasn't been done
> 
> Here's the call tree:
> 
>     really_probe
>       dev->driver = drv;                       # <--
>       call_driver_probe
>         dev->bus->probe
>           pci_device_probe
>             __pci_device_probe
>               pci_call_probe
>                 local_pci_probe
>                   pm_runtime_get_sync
>                     ...
>                     pci_pm_runtime_resume
>   -                   if (!pci_dev->driver)    # 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of pci_dev->driver")
>   +                   if (!to_pci_driver(dev->driver))
>                         return 0
>                       pm->runtime_resume
>                         i2c_dw_pci_resume
>                           i_dev->init()        # <-- NULL ptr deref
>   -                 pci_dev->driver = pci_drv  # b5f9c644eb1b ("PCI: Remove struct pci_dev->driver")
>                   pci_drv->probe
>                     i2c_dw_pci_probe

I think this analysis is right.

I didn't test this patch, @Robert, maybe you can do this?

Best regards
Uwe

 drivers/pci/pci-driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 1d98c974381c..202533654012 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1299,7 +1299,7 @@ static int pci_pm_runtime_resume(struct device *dev)
 	 */
 	pci_restore_standard_config(pci_dev);
 
-	if (!to_pci_driver(dev->driver))
+	if (!device_is_bound(dev))
 		return 0;
 
 	pci_fixup_device(pci_fixup_resume_early, pci_dev);
-- 
2.30.2


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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Robert Święcki]

Hi all,

> > Here's the call tree:
> >
> >     really_probe
> >       dev->driver = drv;                       # <--
> >       call_driver_probe
> >         dev->bus->probe
> >           pci_device_probe
> >             __pci_device_probe
> >               pci_call_probe
> >                 local_pci_probe
> >                   pm_runtime_get_sync
> >                     ...
> >                     pci_pm_runtime_resume
> >   -                   if (!pci_dev->driver)    # 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of pci_dev->driver")
> >   +                   if (!to_pci_driver(dev->driver))
> >                         return 0
> >                       pm->runtime_resume
> >                         i2c_dw_pci_resume
> >                           i_dev->init()        # <-- NULL ptr deref
> >   -                 pci_dev->driver = pci_drv  # b5f9c644eb1b ("PCI: Remove struct pci_dev->driver")
> >                   pci_drv->probe
> >                     i2c_dw_pci_probe
>
> I think this analysis is right.
>
> I didn't test this patch, @Robert, maybe you can do this?
>
> Best regards
> Uwe
>
>  drivers/pci/pci-driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 1d98c974381c..202533654012 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1299,7 +1299,7 @@ static int pci_pm_runtime_resume(struct device *dev)
>          */
>         pci_restore_standard_config(pci_dev);
>
> -       if (!to_pci_driver(dev->driver))
> +       if (!device_is_bound(dev))
>                 return 0;
>
>         pci_fixup_device(pci_fixup_resume_early, pci_dev);

Yes, that fixes it. Thanks for the patch.

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Krzysztof Wilczyński]

Hello!

[...]
> > I didn't test this patch, @Robert, maybe you can do this?
> >
> > Best regards
> > Uwe
> >
> >  drivers/pci/pci-driver.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index 1d98c974381c..202533654012 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -1299,7 +1299,7 @@ static int pci_pm_runtime_resume(struct device *dev)
> >          */
> >         pci_restore_standard_config(pci_dev);
> >
> > -       if (!to_pci_driver(dev->driver))
> > +       if (!device_is_bound(dev))
> >                 return 0;
> >
> >         pci_fixup_device(pci_fixup_resume_early, pci_dev);
> 
> Yes, that fixes it. Thanks for the patch.

Thank you Uwe for the patch and Robert for testing!

	Krzysztof

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Bjorn Helgaas]

[+cc Greg: new device_is_bound() use]

On Mon, Nov 08, 2021 at 10:22:26PM +0100, Uwe Kleine-König wrote:
> 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().

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 we just need a comment there about why it looks different than
the other PM interfaces?

I also notice that the only other uses of device_is_bound()
outside the driver core are in iommu_group_store_type() and
regulator_resolve_supply().  This patch seems like a reasonable use,
but I always look twice when we do something unique.

> Reported-by: Robert Święcki <robert@swiecki.net>
> Fixes: 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of pci_dev->driver")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> On Mon, Nov 08, 2021 at 12:58:23PM -0600, Bjorn Helgaas wrote:
> > [+cc Uwe, Rafael, linux-pm, linux-pci, linux-kernel, beginning of
> > thread: https://lore.kernel.org/linux-i2c/CAP145pgdrdiMAT7=-iB1DMgA7t_bMqTcJL4N0=6u8kNY3EU0dw@mail.gmail.com/T/#t]
> > 
> > On Mon, Nov 08, 2021 at 05:34:14PM +0100, Robert Święcki wrote:
> > > > I'm daily-driving the linux from Linus' git (recompiling every day or
> > > > two), and yesterday it stopped booting. Below is the dmesg from
> > > > pstore.
> > > > ...
> > > 
> > > This introduced the bug: 0c5c62ddf88c34bc83b66e4ac9beb2bb0e1887d4
> > > https://github.com/torvalds/linux/commit/0c5c62ddf88c34bc83b66e4ac9beb2bb0e1887d4
> > 
> > Thank you very much for the debugging and this report!  This report is
> > for i2c, but the problem will affect many drivers.
> > 
> > > > <1>[    1.431369][  T447] BUG: kernel NULL pointer dereference,
> > > > address: 0000000000000540
> > > > <1>[    1.431371][  T447] #PF: supervisor read access in kernel mode
> > > > <1>[    1.431375][  T447] #PF: error_code(0x0000) - not-present page
> > > > <6>[    1.431378][  T447] PGD 0 P4D 0
> > > > <4>[    1.431384][  T447] Oops: 0000 [#1] PREEMPT SMP NOPTI
> > > > <4>[    1.431388][  T447] CPU: 12 PID: 447 Comm: systemd-udevd
> > > > Tainted: G            E     5.15.0+ #91
> > > > <4>[    1.431391][  T447] Hardware name: ASUS System Product Name/ROG
> > > > CROSSHAIR VIII FORMULA, BIOS 3801 07/30/2021
> > > > <4>[    1.431392][  T447] RIP: 0010:i2c_dw_pci_resume+0x8/0x40
> > > > [i2c_designware_pci]
> > > > <4>[    1.431399][  T447] Code: 00 00 00 00 66 66 2e 0f 1f 84 00 00 00
> > > > 00 00 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 84 00 00 00 00 00 53 48
> > > > 8b 5f 78 48 89 df <ff> 93 40 05 00 00 c6 83 c0 05 00 00 00 5b c3 66 66
> > > > 2e 0f 1f 84 00
> > > > <4>[    1.431401][  T447] RSP: 0018:ffffb3e740a13ba8 EFLAGS: 00010286
> > > > <4>[    1.431403][  T447] RAX: 0000000000000000 RBX: 0000000000000000
> > > > RCX: 0000000000000000
> > 
> >   $ ./scripts/decodecode < oops
> >     22:       53                      push   %rbx
> >     23:       48 8b 5f 78             mov    0x78(%rdi),%rbx
> >     27:       48 89 df                mov    %rbx,%rdi
> >     2a:*      ff 93 40 05 00 00       callq  *0x540(%rbx)             <-- trapping instruction
> >     30:       c6 83 c0 05 00 00 00    movb   $0x0,0x5c0(%rbx)
> >     37:       5b                      pop    %rbx
> >     38:       c3                      retq
> > 
> >   static int i2c_dw_pci_resume(struct device *dev)
> >   {
> >     struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
> >     int ret;
> > 
> >     ret = i_dev->init(i_dev);
> >     i_dev->suspended = false;
> > 
> >     return ret;
> > 
> > So I think we're trying to call i_dev->init(), which is a NULL
> > pointer.
> > 
> > > > <4>[    1.431422][  T447]  pci_pm_runtime_resume+0xaa/0x100
> > > > <4>[    1.431434][  T447]  __rpm_callback+0x3c/0x100
> > > > <4>[    1.431442][  T447]  rpm_callback+0x54/0x80
> > > > <4>[    1.431445][  T447]  rpm_resume+0x410/0x700
> > > > <4>[    1.431455][  T447]  __pm_runtime_resume+0x45/0x80
> > > > <4>[    1.431457][  T447]  pci_device_probe+0xa2/0x140
> > > > <4>[    1.431459][  T447]  really_probe+0x1e4/0x400
> > > > <4>[    1.431464][  T447]  __driver_probe_device+0xf9/0x180
> > > > <4>[    1.431466][  T447]  driver_probe_device+0x19/0xc0
> > 
> > I think the problem here is that:
> > 
> >   - really_probe() sets dev->driver
> > 
> >   - local_pci_probe() calls pm_runtime_get_sync(), which leads to:
> > 
> >   - pci_pm_runtime_resume(), which previously skipped the driver's
> >     .runtime_resume() method when "pci_dev->driver" as NULL
> > 
> >   - after 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of
> >     pci_dev->driver") [1], it checks "dev->driver" instead of
> >     "pci_dev->driver"
> > 
> >   - dev->driver is non-NULL (set by really_probe() above), but at this
> >     point pci_dev->driver used to be NULL because local_pci_probe()
> >     didn't set it until after after calling pm_runtime_get_sync() (see
> >     b5f9c644eb1b ("PCI: Remove struct pci_dev->driver") [2])
> > 
> >   - because dev->driver is non-NULL, we call i2c_dw_pci_resume()
> >     before i2c_dw_pci_probe(), so the driver init hasn't been done
> > 
> > Here's the call tree:
> > 
> >     really_probe
> >       dev->driver = drv;                       # <--
> >       call_driver_probe
> >         dev->bus->probe
> >           pci_device_probe
> >             __pci_device_probe
> >               pci_call_probe
> >                 local_pci_probe
> >                   pm_runtime_get_sync
> >                     ...
> >                     pci_pm_runtime_resume
> >   -                   if (!pci_dev->driver)    # 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of pci_dev->driver")
> >   +                   if (!to_pci_driver(dev->driver))
> >                         return 0
> >                       pm->runtime_resume
> >                         i2c_dw_pci_resume
> >                           i_dev->init()        # <-- NULL ptr deref
> >   -                 pci_dev->driver = pci_drv  # b5f9c644eb1b ("PCI: Remove struct pci_dev->driver")
> >                   pci_drv->probe
> >                     i2c_dw_pci_probe
> 
> I think this analysis is right.
> 
> I didn't test this patch, @Robert, maybe you can do this?
> 
> Best regards
> Uwe
> 
>  drivers/pci/pci-driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 1d98c974381c..202533654012 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1299,7 +1299,7 @@ static int pci_pm_runtime_resume(struct device *dev)
>  	 */
>  	pci_restore_standard_config(pci_dev);
>  
> -	if (!to_pci_driver(dev->driver))
> +	if (!device_is_bound(dev))
>  		return 0;
>  
>  	pci_fixup_device(pci_fixup_resume_early, pci_dev);
> -- 
> 2.30.2
> 
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Greg Kroah-Hartman]

On Mon, Nov 08, 2021 at 08:56:19PM -0600, Bjorn Helgaas wrote:
> [+cc Greg: new device_is_bound() use]
> 
> On Mon, Nov 08, 2021 at 10:22:26PM +0100, Uwe Kleine-König wrote:
> > 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().
> 
> 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 we just need a comment there about why it looks different than
> the other PM interfaces?
> 
> I also notice that the only other uses of device_is_bound()
> outside the driver core are in iommu_group_store_type() and
> regulator_resolve_supply().  This patch seems like a reasonable use,
> but I always look twice when we do something unique.

I agree that this looks really odd.  No one should care outside of the
driver core to call device_is_bound(), as if a driver is being called,
implicitly you know that the device is bound to that driver.

Why does the PCI core care if a device is bound to a pci driver at this
point in time?

But, this does feel like an odd use of to_pci_driver() here, what needs
to be known here, if a pci driver is in control of a device here or not?

thanks,

greg k-h

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2454 bytes --]

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:
> > 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/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Robert Święcki]

Also, this might be unrelated, but the following happened around this
patch (ie https://github.com/torvalds/linux/commit/0c5c62ddf88c34bc83b66e4ac9beb2bb0e1887d4)

I sometimes run Win11 under qemu/kvm/vfio (gfx card), and it stopped
booting (even with the latest patch for pci). Another Linux distro
boots fine under the same qemu/kvm/vfio, but Win11 stops at the boot
screen.

It worked well with 5.15.0 and a few PRs later - with the git tip it's
not booting - and it works well with 5.15.0-rc7

Maybe related to pci/vfio changes, maybe not, just leaving it here for
track record. I'll try to debug it a bit.

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Bjorn Helgaas]

On Tue, Nov 09, 2021 at 01:42:29PM +0100, Robert Święcki wrote:
> Also, this might be unrelated, but the following happened around this
> patch (ie https://github.com/torvalds/linux/commit/0c5c62ddf88c34bc83b66e4ac9beb2bb0e1887d4)
> 
> I sometimes run Win11 under qemu/kvm/vfio (gfx card), and it stopped
> booting (even with the latest patch for pci). Another Linux distro
> boots fine under the same qemu/kvm/vfio, but Win11 stops at the boot
> screen.
> 
> It worked well with 5.15.0 and a few PRs later - with the git tip it's
> not booting - and it works well with 5.15.0-rc7
> 
> Maybe related to pci/vfio changes, maybe not, just leaving it here for
> track record. I'll try to debug it a bit.

Hmm.  I have no idea how to debug a Win11 boot issue.  Unless there
are clues in the dmesg log or some qemu or kvm logs (if you find any
such logs, please put the complete logs somewhere), the only thing I
can think to do would be bisecting it.  "git bisect" between v5.15 and
0c5c62ddf88c looks like about 13 steps.

Bjorn

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Robert Święcki]

Hi,

śr., 10 lis 2021 o 22:26 Bjorn Helgaas <helgaas@kernel.org> napisał(a):
>
> On Tue, Nov 09, 2021 at 01:42:29PM +0100, Robert Święcki wrote:
> > Also, this might be unrelated, but the following happened around this
> > patch (ie https://github.com/torvalds/linux/commit/0c5c62ddf88c34bc83b66e4ac9beb2bb0e1887d4)
> >
> > I sometimes run Win11 under qemu/kvm/vfio (gfx card), and it stopped
> > booting (even with the latest patch for pci). Another Linux distro
> > boots fine under the same qemu/kvm/vfio, but Win11 stops at the boot
> > screen.
> >
> > It worked well with 5.15.0 and a few PRs later - with the git tip it's
> > not booting - and it works well with 5.15.0-rc7
> >
> > Maybe related to pci/vfio changes, maybe not, just leaving it here for
> > track record. I'll try to debug it a bit.
>
> Hmm.  I have no idea how to debug a Win11 boot issue.  Unless there
> are clues in the dmesg log or some qemu or kvm logs (if you find any
> such logs, please put the complete logs somewhere), the only thing I
> can think to do would be bisecting it.  "git bisect" between v5.15 and
> 0c5c62ddf88c looks like about 13 steps.

I think I wouldn't worry about it just yet. It's a problem with many
possible culprits (kvm, qemu, vfio, Win11 itself). Once the pci code
is stable, I'll take another look and do some, at least basic,
debugging. Though the problem is somewhat intermittent (mostly the
system doesn't boot, but sometimes it does).

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Rafael J. Wysocki]

On Tue, Nov 9, 2021 at 7:59 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> 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:
> > > 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()?

Yes, in principle it might be replaced with pm_runtime_get_noresume().

In theory, that may be problematic if a device is put into a low-power
state on remove and then the driver is bound again to it.

> 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.

If it is called after ->remove() and before clearing the device's
driver pointer, then yes.

If this is turned into pm_runtime_put_noidle(), all should work.

> > 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.

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Bjorn Helgaas]

On Tue, Nov 09, 2021 at 06:18:18PM +0100, Rafael J. Wysocki wrote:
> On Tue, Nov 9, 2021 at 7:59 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > 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:
> > > > 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()?
> 
> Yes, in principle it might be replaced with pm_runtime_get_noresume().
> 
> In theory, that may be problematic if a device is put into a low-power
> state on remove and then the driver is bound again to it.
> 
> > 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.
> 
> If it is called after ->remove() and before clearing the device's
> driver pointer, then yes.

Yes, that is the case:

  pci_device_remove
    if (drv->remove) {
      pm_runtime_get_sync
      drv->remove()                # <-- driver ->remove() method
      pm_runtime_put_noidle
    }
    ...
    pm_runtime_put_sync            # <-- after ->remove()

So pm_runtime_put_sync() is called after drv->remove(), and it may
call drv->pm->runtime_idle().  I think the driver may not expect this.

> If this is turned into pm_runtime_put_noidle(), all should work.

pci_device_remove() already calls pm_runtime_put_noidle() immediately
after calling the driver ->remove() method.

Are you saying we should do this, which means pci_device_remove()
would call pm_runtime_put_noidle() twice?

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 1d98c974381c..79c1a920fdc8 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -318,7 +318,7 @@ static long local_pci_probe(void *_ddi)
 	 * count, in its probe routine and pm_runtime_get_noresume() in
 	 * its remove routine.
 	 */
-	pm_runtime_get_sync(dev);
+	pm_runtime_get_noresume(dev);
 	rc = pci_drv->probe(pci_dev, ddi->id);
 	if (!rc)
 		return rc;
@@ -465,7 +465,7 @@ static void pci_device_remove(struct device *dev)
 	pci_iov_remove(pci_dev);
 
 	/* Undo the runtime PM settings in local_pci_probe() */
-	pm_runtime_put_sync(dev);
+	pm_runtime_put_noidle(dev);
 
 	/*
 	 * If the device is still on, set the power state as "unknown",

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Rafael J. Wysocki]

On Tue, Nov 9, 2021 at 7:12 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Tue, Nov 09, 2021 at 06:18:18PM +0100, Rafael J. Wysocki wrote:
> > On Tue, Nov 9, 2021 at 7:59 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > 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:
> > > > > 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()?
> >
> > Yes, in principle it might be replaced with pm_runtime_get_noresume().
> >
> > In theory, that may be problematic if a device is put into a low-power
> > state on remove and then the driver is bound again to it.
> >
> > > 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.
> >
> > If it is called after ->remove() and before clearing the device's
> > driver pointer, then yes.
>
> Yes, that is the case:
>
>   pci_device_remove
>     if (drv->remove) {
>       pm_runtime_get_sync
>       drv->remove()                # <-- driver ->remove() method
>       pm_runtime_put_noidle
>     }
>     ...
>     pm_runtime_put_sync            # <-- after ->remove()
>
> So pm_runtime_put_sync() is called after drv->remove(), and it may
> call drv->pm->runtime_idle().  I think the driver may not expect this.
>
> > If this is turned into pm_runtime_put_noidle(), all should work.
>
> pci_device_remove() already calls pm_runtime_put_noidle() immediately
> after calling the driver ->remove() method.
>
> Are you saying we should do this, which means pci_device_remove()
> would call pm_runtime_put_noidle() twice?

Well, they are both needed to keep the PM-runtime reference counting in balance.

This still has an issue, though, because user space would be able to
trigger a runtime suspend via sysfs after we've dropped the last
reference to the device in pci_device_remove().

So instead, we can drop the pm_runtime_get_sync() and
pm_runtime_put_sync() from local_pci_probe() and pci_device_remove(),
respectively, and add pm_runtine_get_noresume() to pci_pm_init(),
which will prevent PM-runtime from touching the device until it has a
driver that supports PM-runtime.

We'll lose the theoretical ability to put unbound devices into D3 this
way, but we learned some time ago that this isn't safe in all cases
anyway.

> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 1d98c974381c..79c1a920fdc8 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -318,7 +318,7 @@ static long local_pci_probe(void *_ddi)
>          * count, in its probe routine and pm_runtime_get_noresume() in
>          * its remove routine.
>          */
> -       pm_runtime_get_sync(dev);
> +       pm_runtime_get_noresume(dev);
>         rc = pci_drv->probe(pci_dev, ddi->id);
>         if (!rc)
>                 return rc;
> @@ -465,7 +465,7 @@ static void pci_device_remove(struct device *dev)
>         pci_iov_remove(pci_dev);
>
>         /* Undo the runtime PM settings in local_pci_probe() */
> -       pm_runtime_put_sync(dev);
> +       pm_runtime_put_noidle(dev);
>
>         /*
>          * If the device is still on, set the power state as "unknown",

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Rafael J. Wysocki]

On Tue, Nov 9, 2021 at 7:52 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Nov 9, 2021 at 7:12 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > On Tue, Nov 09, 2021 at 06:18:18PM +0100, Rafael J. Wysocki wrote:
> > > On Tue, Nov 9, 2021 at 7:59 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > 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:
> > > > > > 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()?
> > >
> > > Yes, in principle it might be replaced with pm_runtime_get_noresume().
> > >
> > > In theory, that may be problematic if a device is put into a low-power
> > > state on remove and then the driver is bound again to it.
> > >
> > > > 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.
> > >
> > > If it is called after ->remove() and before clearing the device's
> > > driver pointer, then yes.
> >
> > Yes, that is the case:
> >
> >   pci_device_remove
> >     if (drv->remove) {
> >       pm_runtime_get_sync
> >       drv->remove()                # <-- driver ->remove() method
> >       pm_runtime_put_noidle
> >     }
> >     ...
> >     pm_runtime_put_sync            # <-- after ->remove()
> >
> > So pm_runtime_put_sync() is called after drv->remove(), and it may
> > call drv->pm->runtime_idle().  I think the driver may not expect this.
> >
> > > If this is turned into pm_runtime_put_noidle(), all should work.
> >
> > pci_device_remove() already calls pm_runtime_put_noidle() immediately
> > after calling the driver ->remove() method.
> >
> > Are you saying we should do this, which means pci_device_remove()
> > would call pm_runtime_put_noidle() twice?
>
> Well, they are both needed to keep the PM-runtime reference counting in balance.
>
> This still has an issue, though, because user space would be able to
> trigger a runtime suspend via sysfs after we've dropped the last
> reference to the device in pci_device_remove().
>
> So instead, we can drop the pm_runtime_get_sync() and
> pm_runtime_put_sync() from local_pci_probe() and pci_device_remove(),
> respectively, and add pm_runtine_get_noresume() to pci_pm_init(),
> which will prevent PM-runtime from touching the device until it has a
> driver that supports PM-runtime.
>
> We'll lose the theoretical ability to put unbound devices into D3 this
> way, but we learned some time ago that this isn't safe in all cases
> anyway.

IOW, something like this (untested and most likely white-space-damaged).

---
 drivers/pci/pci-driver.c |   13 -------------
 drivers/pci/pci.c        |    7 +++++++
 2 files changed, 7 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -309,16 +309,6 @@ static long local_pci_probe(void *_ddi)
     struct device *dev = &pci_dev->dev;
     int rc;

-    /*
-     * Unbound PCI devices are always put in D0, regardless of
-     * runtime PM status.  During probe, the device is set to
-     * active and the usage count is incremented.  If the driver
-     * supports runtime PM, it should call pm_runtime_put_noidle(),
-     * or any other runtime PM helper function decrementing the usage
-     * count, in its probe routine and pm_runtime_get_noresume() in
-     * its remove routine.
-     */
-    pm_runtime_get_sync(dev);
     pci_dev->driver = pci_drv;
     rc = pci_drv->probe(pci_dev, ddi->id);
     if (!rc)
@@ -470,9 +460,6 @@ static void pci_device_remove(struct dev
         pci_iov_remove(pci_dev);
     }

-    /* Undo the runtime PM settings in local_pci_probe() */
-    pm_runtime_put_sync(dev);
-
     /*
      * If the device is still on, set the power state as "unknown",
      * since it might change by the next time we load the driver.
Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -3048,7 +3048,14 @@ void pci_pm_init(struct pci_dev *dev)
     u16 pmc;

     pm_runtime_forbid(&dev->dev);
+    /*
+     * Unbound PCI devices are always put in D0.  If the driver supports
+     * runtime PM, it should call pm_runtime_put_noidle(), or any other
+     * runtime PM helper function decrementing the usage count, in its
+     * probe routine and pm_runtime_get_noresume() in its remove routine.
+     */
     pm_runtime_set_active(&dev->dev);
+    pm_runtime_get_noresume(&dev->dev);
     pm_runtime_enable(&dev->dev);
     device_enable_async_suspend(&dev->dev);
     dev->wakeup_prepared = false;

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Bjorn Helgaas]

On Tue, Nov 09, 2021 at 07:58:47PM +0100, Rafael J. Wysocki wrote:
> On Tue, Nov 9, 2021 at 7:52 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Tue, Nov 9, 2021 at 7:12 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Tue, Nov 09, 2021 at 06:18:18PM +0100, Rafael J. Wysocki wrote:
> > > > On Tue, Nov 9, 2021 at 7:59 AM Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > 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:
> > > > > > > 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()?
> > > >
> > > > Yes, in principle it might be replaced with pm_runtime_get_noresume().
> > > >
> > > > In theory, that may be problematic if a device is put into a low-power
> > > > state on remove and then the driver is bound again to it.
> > > >
> > > > > 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.
> > > >
> > > > If it is called after ->remove() and before clearing the device's
> > > > driver pointer, then yes.
> > >
> > > Yes, that is the case:
> > >
> > >   pci_device_remove
> > >     if (drv->remove) {
> > >       pm_runtime_get_sync
> > >       drv->remove()                # <-- driver ->remove() method
> > >       pm_runtime_put_noidle
> > >     }
> > >     ...
> > >     pm_runtime_put_sync            # <-- after ->remove()
> > >
> > > So pm_runtime_put_sync() is called after drv->remove(), and it may
> > > call drv->pm->runtime_idle().  I think the driver may not expect this.
> > >
> > > > If this is turned into pm_runtime_put_noidle(), all should work.
> > >
> > > pci_device_remove() already calls pm_runtime_put_noidle() immediately
> > > after calling the driver ->remove() method.
> > >
> > > Are you saying we should do this, which means pci_device_remove()
> > > would call pm_runtime_put_noidle() twice?
> >
> > Well, they are both needed to keep the PM-runtime reference counting in balance.
> >
> > This still has an issue, though, because user space would be able to
> > trigger a runtime suspend via sysfs after we've dropped the last
> > reference to the device in pci_device_remove().
> >
> > So instead, we can drop the pm_runtime_get_sync() and
> > pm_runtime_put_sync() from local_pci_probe() and pci_device_remove(),
> > respectively, and add pm_runtine_get_noresume() to pci_pm_init(),
> > which will prevent PM-runtime from touching the device until it has a
> > driver that supports PM-runtime.
> >
> > We'll lose the theoretical ability to put unbound devices into D3 this
> > way, but we learned some time ago that this isn't safe in all cases
> > anyway.
> 
> IOW, something like this (untested and most likely white-space-damaged).

Thanks!  I applied this manually to for-linus in hopes of making the
the next linux-next build.

Please send any testing reports and corrections to the patch and
commit log!

commit dd414877b58b ("PCI/PM: Prevent runtime PM until claimed by a driver that supports it")
Author: Bjorn Helgaas <bhelgaas@google.com>
Date:   Tue Nov 9 13:36:09 2021 -0600

    PCI/PM: Prevent runtime PM until claimed by a driver that supports it
    
    Previously we had a path that could call a driver's ->runtime_resume()
    method before calling the driver's ->probe() method, which is a problem
    because ->runtime_resume() often relies on initialization done in
    ->probe():
    
      local_pci_probe
        pm_runtime_get_sync
          ...
            pci_pm_runtime_resume
              if (!pci_dev->driver)
                return 0;                          <-- early exit
              dev->driver->pm->runtime_resume();   <-- driver ->runtime_resume()
        pci_dev->driver = pci_drv;
        pci_drv->probe()                           <-- driver ->probe()
    
    Prior to 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of
    pci_dev->driver"), we took the early exit, which avoided the problem.  But
    2a4d9408c9e8 removed pci_dev->driver (since it's redundant with
    device->driver), so we no longer take the early exit, which leads to havoc
    in ->runtime_resume().
    
    Similarly, we could call the driver's ->runtime_idle() method after its
    ->remove() method.
    
    Avoid the problem by dropping the pm_runtime_get_sync() and
    pm_runtime_put_sync() from local_pci_probe() and pci_device_remove(),
    respectively.
    
    Add pm_runtime_get_noresume(), which uses no driver PM callbacks, to the
    pci_pm_init() enumeration path.  This will prevent PM-runtime from touching
    the device until it has a driver that supports PM-runtime.
    
    Link: https://lore.kernel.org/r/CAJZ5v0impb8uscbp8LUTBMExfMoGz=cPrTWhSGh0GF_SANNKPQ@mail.gmail.com
    Fixes: 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of pci_dev->driver")
    Reported-by: Robert Święcki <robert@swiecki.net>
    Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 1d98c974381c..41cdf510214f 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -309,16 +309,6 @@ static long local_pci_probe(void *_ddi)
 	struct device *dev = &pci_dev->dev;
 	int rc;
 
-	/*
-	 * Unbound PCI devices are always put in D0, regardless of
-	 * runtime PM status.  During probe, the device is set to
-	 * active and the usage count is incremented.  If the driver
-	 * supports runtime PM, it should call pm_runtime_put_noidle(),
-	 * or any other runtime PM helper function decrementing the usage
-	 * count, in its probe routine and pm_runtime_get_noresume() in
-	 * its remove routine.
-	 */
-	pm_runtime_get_sync(dev);
 	rc = pci_drv->probe(pci_dev, ddi->id);
 	if (!rc)
 		return rc;
@@ -464,9 +454,6 @@ static void pci_device_remove(struct device *dev)
 	pcibios_free_irq(pci_dev);
 	pci_iov_remove(pci_dev);
 
-	/* Undo the runtime PM settings in local_pci_probe() */
-	pm_runtime_put_sync(dev);
-
 	/*
 	 * If the device is still on, set the power state as "unknown",
 	 * since it might change by the next time we load the driver.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b88db815ee01..e9c38b994c73 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3097,7 +3097,15 @@ void pci_pm_init(struct pci_dev *dev)
 	u16 pmc;
 
 	pm_runtime_forbid(&dev->dev);
+
+	/*
+	 * Unbound PCI devices are always put in D0.  If the driver supports
+	 * runtime PM, it should call pm_runtime_put_noidle(), or any other
+	 * runtime PM helper function decrementing the usage count, in its
+	 * probe routine and pm_runtime_get_noresume() in its remove routine.
+	 */
 	pm_runtime_set_active(&dev->dev);
+	pm_runtime_get_noresume(&dev->dev);
 	pm_runtime_enable(&dev->dev);
 	device_enable_async_suspend(&dev->dev);
 	dev->wakeup_prepared = false;

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 8079 bytes --]

On Tue, Nov 09, 2021 at 02:05:18PM -0600, Bjorn Helgaas wrote:
> On Tue, Nov 09, 2021 at 07:58:47PM +0100, Rafael J. Wysocki wrote:
> > On Tue, Nov 9, 2021 at 7:52 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > On Tue, Nov 9, 2021 at 7:12 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Tue, Nov 09, 2021 at 06:18:18PM +0100, Rafael J. Wysocki wrote:
> > > > > On Tue, Nov 9, 2021 at 7:59 AM Uwe Kleine-König
> > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > 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:
> > > > > > > > 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()?
> > > > >
> > > > > Yes, in principle it might be replaced with pm_runtime_get_noresume().
> > > > >
> > > > > In theory, that may be problematic if a device is put into a low-power
> > > > > state on remove and then the driver is bound again to it.
> > > > >
> > > > > > 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.
> > > > >
> > > > > If it is called after ->remove() and before clearing the device's
> > > > > driver pointer, then yes.
> > > >
> > > > Yes, that is the case:
> > > >
> > > >   pci_device_remove
> > > >     if (drv->remove) {
> > > >       pm_runtime_get_sync
> > > >       drv->remove()                # <-- driver ->remove() method
> > > >       pm_runtime_put_noidle
> > > >     }
> > > >     ...
> > > >     pm_runtime_put_sync            # <-- after ->remove()
> > > >
> > > > So pm_runtime_put_sync() is called after drv->remove(), and it may
> > > > call drv->pm->runtime_idle().  I think the driver may not expect this.
> > > >
> > > > > If this is turned into pm_runtime_put_noidle(), all should work.
> > > >
> > > > pci_device_remove() already calls pm_runtime_put_noidle() immediately
> > > > after calling the driver ->remove() method.
> > > >
> > > > Are you saying we should do this, which means pci_device_remove()
> > > > would call pm_runtime_put_noidle() twice?
> > >
> > > Well, they are both needed to keep the PM-runtime reference counting in balance.
> > >
> > > This still has an issue, though, because user space would be able to
> > > trigger a runtime suspend via sysfs after we've dropped the last
> > > reference to the device in pci_device_remove().
> > >
> > > So instead, we can drop the pm_runtime_get_sync() and
> > > pm_runtime_put_sync() from local_pci_probe() and pci_device_remove(),
> > > respectively, and add pm_runtine_get_noresume() to pci_pm_init(),
> > > which will prevent PM-runtime from touching the device until it has a
> > > driver that supports PM-runtime.
> > >
> > > We'll lose the theoretical ability to put unbound devices into D3 this
> > > way, but we learned some time ago that this isn't safe in all cases
> > > anyway.
> > 
> > IOW, something like this (untested and most likely white-space-damaged).
> 
> Thanks!  I applied this manually to for-linus in hopes of making the
> the next linux-next build.
> 
> Please send any testing reports and corrections to the patch and
> commit log!
> 
> commit dd414877b58b ("PCI/PM: Prevent runtime PM until claimed by a driver that supports it")
> Author: Bjorn Helgaas <bhelgaas@google.com>
> Date:   Tue Nov 9 13:36:09 2021 -0600
> 
>     PCI/PM: Prevent runtime PM until claimed by a driver that supports it
>     
>     Previously we had a path that could call a driver's ->runtime_resume()
>     method before calling the driver's ->probe() method, which is a problem
>     because ->runtime_resume() often relies on initialization done in
>     ->probe():
>     
>       local_pci_probe
>         pm_runtime_get_sync
>           ...
>             pci_pm_runtime_resume
>               if (!pci_dev->driver)
>                 return 0;                          <-- early exit
>               dev->driver->pm->runtime_resume();   <-- driver ->runtime_resume()
>         pci_dev->driver = pci_drv;
>         pci_drv->probe()                           <-- driver ->probe()
>     
>     Prior to 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of
>     pci_dev->driver"), we took the early exit, which avoided the problem.  But
>     2a4d9408c9e8 removed pci_dev->driver (since it's redundant with
>     device->driver), so we no longer take the early exit, which leads to havoc
>     in ->runtime_resume().
>     
>     Similarly, we could call the driver's ->runtime_idle() method after its
>     ->remove() method.
>     
>     Avoid the problem by dropping the pm_runtime_get_sync() and
>     pm_runtime_put_sync() from local_pci_probe() and pci_device_remove(),
>     respectively.
>     
>     Add pm_runtime_get_noresume(), which uses no driver PM callbacks, to the
>     pci_pm_init() enumeration path.  This will prevent PM-runtime from touching
>     the device until it has a driver that supports PM-runtime.
>     
>     Link: https://lore.kernel.org/r/CAJZ5v0impb8uscbp8LUTBMExfMoGz=cPrTWhSGh0GF_SANNKPQ@mail.gmail.com
>     Fixes: 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of pci_dev->driver")
>     Reported-by: Robert Święcki <robert@swiecki.net>
>     Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

I like this, this feels better than my initial suggestion using
device_is_bound().

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Bjorn Helgaas]

On Tue, Nov 09, 2021 at 02:05:18PM -0600, Bjorn Helgaas wrote:
> On Tue, Nov 09, 2021 at 07:58:47PM +0100, Rafael J. Wysocki wrote:
> > On Tue, Nov 9, 2021 at 7:52 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > ...

> > > So instead, we can drop the pm_runtime_get_sync() and
> > > pm_runtime_put_sync() from local_pci_probe() and pci_device_remove(),
> > > respectively, and add pm_runtine_get_noresume() to pci_pm_init(),
> > > which will prevent PM-runtime from touching the device until it has a
> > > driver that supports PM-runtime.
> > >
> > > We'll lose the theoretical ability to put unbound devices into D3 this
> > > way, but we learned some time ago that this isn't safe in all cases
> > > anyway.
> > 
> > IOW, something like this (untested and most likely white-space-damaged).
> 
> Thanks!  I applied this manually to for-linus in hopes of making the
> the next linux-next build.
> 
> Please send any testing reports and corrections to the patch and
> commit log!

Robert, I hate to ask even more of you, but if you have a chance, it
would be very helpful if you could test the patch below.  I'm pretty
sure it should fix the problem you saw, and I hope to ask Linus to
merge it today.

> commit dd414877b58b ("PCI/PM: Prevent runtime PM until claimed by a driver that supports it")
> Author: Bjorn Helgaas <bhelgaas@google.com>
> Date:   Tue Nov 9 13:36:09 2021 -0600
> 
>     PCI/PM: Prevent runtime PM until claimed by a driver that supports it
>     
>     Previously we had a path that could call a driver's ->runtime_resume()
>     method before calling the driver's ->probe() method, which is a problem
>     because ->runtime_resume() often relies on initialization done in
>     ->probe():
>     
>       local_pci_probe
>         pm_runtime_get_sync
>           ...
>             pci_pm_runtime_resume
>               if (!pci_dev->driver)
>                 return 0;                          <-- early exit
>               dev->driver->pm->runtime_resume();   <-- driver ->runtime_resume()
>         pci_dev->driver = pci_drv;
>         pci_drv->probe()                           <-- driver ->probe()
>     
>     Prior to 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of
>     pci_dev->driver"), we took the early exit, which avoided the problem.  But
>     2a4d9408c9e8 removed pci_dev->driver (since it's redundant with
>     device->driver), so we no longer take the early exit, which leads to havoc
>     in ->runtime_resume().
>     
>     Similarly, we could call the driver's ->runtime_idle() method after its
>     ->remove() method.
>     
>     Avoid the problem by dropping the pm_runtime_get_sync() and
>     pm_runtime_put_sync() from local_pci_probe() and pci_device_remove(),
>     respectively.
>     
>     Add pm_runtime_get_noresume(), which uses no driver PM callbacks, to the
>     pci_pm_init() enumeration path.  This will prevent PM-runtime from touching
>     the device until it has a driver that supports PM-runtime.
>     
>     Link: https://lore.kernel.org/r/CAJZ5v0impb8uscbp8LUTBMExfMoGz=cPrTWhSGh0GF_SANNKPQ@mail.gmail.com
>     Fixes: 2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of pci_dev->driver")
>     Reported-by: Robert Święcki <robert@swiecki.net>
>     Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 1d98c974381c..41cdf510214f 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -309,16 +309,6 @@ static long local_pci_probe(void *_ddi)
>  	struct device *dev = &pci_dev->dev;
>  	int rc;
>  
> -	/*
> -	 * Unbound PCI devices are always put in D0, regardless of
> -	 * runtime PM status.  During probe, the device is set to
> -	 * active and the usage count is incremented.  If the driver
> -	 * supports runtime PM, it should call pm_runtime_put_noidle(),
> -	 * or any other runtime PM helper function decrementing the usage
> -	 * count, in its probe routine and pm_runtime_get_noresume() in
> -	 * its remove routine.
> -	 */
> -	pm_runtime_get_sync(dev);
>  	rc = pci_drv->probe(pci_dev, ddi->id);
>  	if (!rc)
>  		return rc;
> @@ -464,9 +454,6 @@ static void pci_device_remove(struct device *dev)
>  	pcibios_free_irq(pci_dev);
>  	pci_iov_remove(pci_dev);
>  
> -	/* Undo the runtime PM settings in local_pci_probe() */
> -	pm_runtime_put_sync(dev);
> -
>  	/*
>  	 * If the device is still on, set the power state as "unknown",
>  	 * since it might change by the next time we load the driver.
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b88db815ee01..e9c38b994c73 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3097,7 +3097,15 @@ void pci_pm_init(struct pci_dev *dev)
>  	u16 pmc;
>  
>  	pm_runtime_forbid(&dev->dev);
> +
> +	/*
> +	 * Unbound PCI devices are always put in D0.  If the driver supports
> +	 * runtime PM, it should call pm_runtime_put_noidle(), or any other
> +	 * runtime PM helper function decrementing the usage count, in its
> +	 * probe routine and pm_runtime_get_noresume() in its remove routine.
> +	 */
>  	pm_runtime_set_active(&dev->dev);
> +	pm_runtime_get_noresume(&dev->dev);
>  	pm_runtime_enable(&dev->dev);
>  	device_enable_async_suspend(&dev->dev);
>  	dev->wakeup_prepared = false;

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Robert Święcki]

Hi all,

śr., 10 lis 2021 o 15:14 Bjorn Helgaas <helgaas@kernel.org> napisał(a):
> On Tue, Nov 09, 2021 at 02:05:18PM -0600, Bjorn Helgaas wrote:
> > On Tue, Nov 09, 2021 at 07:58:47PM +0100, Rafael J. Wysocki wrote:
> > > On Tue, Nov 9, 2021 at 7:52 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > ...
>
> > > > So instead, we can drop the pm_runtime_get_sync() and
> > > > pm_runtime_put_sync() from local_pci_probe() and pci_device_remove(),
> > > > respectively, and add pm_runtine_get_noresume() to pci_pm_init(),
> > > > which will prevent PM-runtime from touching the device until it has a
> > > > driver that supports PM-runtime.
> > > >
> > > > We'll lose the theoretical ability to put unbound devices into D3 this
> > > > way, but we learned some time ago that this isn't safe in all cases
> > > > anyway.
> > >
> > > IOW, something like this (untested and most likely white-space-damaged).
> >
> > Thanks!  I applied this manually to for-linus in hopes of making the
> > the next linux-next build.
> >
> > Please send any testing reports and corrections to the patch and
> > commit log!
>
> Robert, I hate to ask even more of you, but if you have a chance, it
> would be very helpful if you could test the patch below.  I'm pretty
> sure it should fix the problem you saw, and I hope to ask Linus to
> merge it today.

I think the most recent patch creates some timeouts and other problems
in pci-related code? Things I haven't seen before. But, granted, my
kernel testing approach is not with focus on details, so maybe I did
sth wrong.

code is now at: cb690f5238d71f543f4ce874aa59237cf53a877c
https://github.com/torvalds/linux/commit/cb690f5238d71f543f4ce874aa59237cf53a877c

lis 10 17:26:41 jd kernel: Linux version 5.15.0+ (jagger@jd) (gcc
(Debian 11.2.0-10) 11.2.0, GNU ld (GNU Binutils for Debian)
2.37.50.20211102) #105 SMP PREEMPT Wed Nov 10 17:23:49 CET 2021
lis 10 17:26:41 jd kernel: Command line: BOOT_IMAGE=/vmlinuz-5.15.0+
root=UUID=8759fa14-93a4-4dc1-87e6-aa6f5cdbb2ff ro nosplash
mitigations=off no_file_caps apparmor=0 selinux=0 audit=0
amdgpu.ppfeaturemask=0xffffffff hugepagesz=1G default_hugepagesz=1G
hugepages=8 amd_iommu=on iommu=pt
vfio-pci.ids=10de:1f02,10de:10f9,10de:1ada,10de:1adb
drm.edid_firmware=DP-1:edid/sam-g9.edid isolcpus=1,2,5,6,17,18,21,22
..
lis 10 17:26:41 jd kernel: usb usb5: runtime PM trying to activate
child device usb5 but parent (0000:0c:00.2) is not active
..
lis 10 17:26:41 jd kernel: i2c-designware-pci 0000:0c:00.3: timeout in
disabling adapter
lis 10 17:26:41 jd kernel: i2c-designware-pci 0000:0c:00.3: timeout in
disabling adapter
..
lis 10 17:26:41 jd kernel: usb 1-5.3: config 1 has an invalid
interface number: 2 but max is 1
lis 10 17:26:41 jd kernel: usb 1-5.3: config 1 has no interface number 1
..
lis 10 17:26:41 jd kernel: i2c-designware-pci 0000:0c:00.3: timeout in
disabling adapter
..
lis 10 17:26:41 jd kernel: i2c-designware-pci 0000:0c:00.3: timeout in
disabling adapter
lis 10 17:26:41 jd kernel: i2c-designware-pci 0000:0c:00.3: i2c
timeout error -110
lis 10 17:26:41 jd kernel: ucsi_ccg 3-0008: i2c_transfer failed -110
lis 10 17:26:41 jd kernel: ucsi_ccg 3-0008: ucsi_ccg_init failed - -110
lis 10 17:26:41 jd kernel: ucsi_ccg: probe of 3-0008 failed with error -110
lis 10 17:26:42 jd kernel: snd_hda_intel 0000:0c:00.1:
azx_get_response timeout, switching to polling mode: last
cmd=0x000f0000
lis 10 17:26:42 jd kernel: hdaudio hdaudioC0D0: runtime PM trying to
activate child device hdaudioC0D0 but parent (0000:0c:00.1) is not a>
lis 10 17:26:42 jd kernel: snd_hda_codec_hdmi hdaudioC0D0: HDMI:
failed to get afg sub nodes
lis 10 17:26:42 jd kernel: snd_hda_codec_hdmi: probe of hdaudioC0D0
failed with error -22
lis 10 17:26:42 jd kernel: snd_hda_codec_hdmi hdaudioC0D0: HDMI:
failed to get afg sub nodes
lis 10 17:26:42 jd kernel: snd_hda_codec_hdmi: probe of hdaudioC0D0
failed with error -22
lis 10 17:26:42 jd kernel: snd_hda_codec_hdmi hdaudioC0D0: HDMI:
failed to get afg sub nodes
lis 10 17:26:42 jd kernel: snd_hda_codec_hdmi: probe of hdaudioC0D0
failed with error -22
...
lis 10 17:26:42 jd kernel: snd_hda_codec_generic: probe of hdaudioC0D0
failed with error -5
lis 10 17:26:42 jd kernel: snd_hda_intel 0000:0c:00.1: Cannot probe
codecs, giving up
lis 10 17:26:42 jd kernel: ------------[ cut here ]------------
lis 10 17:26:42 jd kernel: WARNING: CPU: 0 PID: 206 at
sound/hda/hdac_bus.c:73 snd_hdac_bus_exit+0x3b/0x80 [snd_hda_core]
lis 10 17:26:42 jd kernel: Modules linked in: snd_hda_codec_hdmi(E)
nls_iso8859_2(E) snd_hda_codec_realtek(E) nls_cp852(E) snd_hda_codec_>
lis 10 17:26:42 jd kernel:  i2c_designware_pci(E) drm(E) i2c_piix4(E)
i2c_designware_core(E) xhci_pci(E) backlight(E) ice(E) xhci_pci_ren>
lis 10 17:26:42 jd kernel: CPU: 0 PID: 206 Comm: kworker/0:2 Tainted:
G            E     5.15.0+ #105
lis 10 17:26:42 jd kernel: Hardware name: ASUS System Product Name/ROG
CROSSHAIR VIII FORMULA, BIOS 3901 09/07/2021
lis 10 17:26:42 jd kernel: Workqueue: events azx_probe_work [snd_hda_intel]
lis 10 17:26:42 jd kernel: RIP: 0010:snd_hdac_bus_exit+0x3b/0x80 [snd_hda_core]
lis 10 17:26:42 jd kernel: Code: 75 19 48 8b 57 58 48 8d 47 58 48 39
c2 75 1b 48 81 c7 f8 02 00 00 e9 94 61 10 d8 0f 0b 48 8b 57 58 48 8d>
lis 10 17:26:42 jd kernel: RSP: 0018:ffffa3e1008d3e40 EFLAGS: 00010283
lis 10 17:26:42 jd kernel: RAX: ffff9728c5f38080 RBX: ffff9728c5f38028
RCX: 000000008020001f
lis 10 17:26:42 jd kernel: RDX: ffff9728c1f40b00 RSI: ffffffff9a30ddc9
RDI: ffff9728c5f38028
lis 10 17:26:42 jd kernel: RBP: ffff9728c5f384d0 R08: 0000000000000000
R09: ffff9728c14a4e00
lis 10 17:26:42 jd kernel: R10: 0000000000000000 R11: 0000000000000005
R12: 00000000ffffffed
lis 10 17:26:42 jd kernel: R13: 0000000000000000 R14: 0000000000000000
R15: ffff972fcea26405
lis 10 17:26:42 jd kernel: FS:  0000000000000000(0000)
GS:ffff972fcea00000(0000) knlGS:0000000000000000
lis 10 17:26:42 jd kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
lis 10 17:26:42 jd kernel: CR2: 00007fa43a298cb0 CR3: 000000010e58e000
CR4: 0000000000750ef0
lis 10 17:26:42 jd kernel: PKRU: 55555554
lis 10 17:26:42 jd kernel: Call Trace:
lis 10 17:26:42 jd kernel:  <TASK>
lis 10 17:26:42 jd kernel:  azx_free+0xe5/0x1c0 [snd_hda_intel]
lis 10 17:26:42 jd kernel:  azx_probe_continue+0x1a8/0x300 [snd_hda_intel]
lis 10 17:26:42 jd kernel:  process_one_work+0x1eb/0x380
lis 10 17:26:42 jd kernel:  worker_thread+0x48/0x400
lis 10 17:26:42 jd kernel:  ? rescuer_thread+0x3c0/0x3c0
lis 10 17:26:42 jd kernel:  kthread+0x151/0x180
lis 10 17:26:42 jd kernel:  ? set_kthread_struct+0x40/0x40
lis 10 17:26:42 jd kernel:  ret_from_fork+0x1f/0x30
lis 10 17:26:42 jd kernel:  </TASK>
lis 10 17:26:42 jd kernel: ---[ end trace f522829b4b020462 ]---
lis 10 17:26:42 jd kernel: i2c-designware-pci 0000:0c:00.3: timeout in
disabling adapter

> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index 1d98c974381c..41cdf510214f 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -309,16 +309,6 @@ static long local_pci_probe(void *_ddi)
> >       struct device *dev = &pci_dev->dev;
> >       int rc;
> >
> > -     /*
> > -      * Unbound PCI devices are always put in D0, regardless of
> > -      * runtime PM status.  During probe, the device is set to
> > -      * active and the usage count is incremented.  If the driver
> > -      * supports runtime PM, it should call pm_runtime_put_noidle(),
> > -      * or any other runtime PM helper function decrementing the usage
> > -      * count, in its probe routine and pm_runtime_get_noresume() in
> > -      * its remove routine.
> > -      */
> > -     pm_runtime_get_sync(dev);
> >       rc = pci_drv->probe(pci_dev, ddi->id);
> >       if (!rc)
> >               return rc;
> > @@ -464,9 +454,6 @@ static void pci_device_remove(struct device *dev)
> >       pcibios_free_irq(pci_dev);
> >       pci_iov_remove(pci_dev);
> >
> > -     /* Undo the runtime PM settings in local_pci_probe() */
> > -     pm_runtime_put_sync(dev);
> > -
> >       /*
> >        * If the device is still on, set the power state as "unknown",
> >        * since it might change by the next time we load the driver.
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index b88db815ee01..e9c38b994c73 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -3097,7 +3097,15 @@ void pci_pm_init(struct pci_dev *dev)
> >       u16 pmc;
> >
> >       pm_runtime_forbid(&dev->dev);
> > +
> > +     /*
> > +      * Unbound PCI devices are always put in D0.  If the driver supports
> > +      * runtime PM, it should call pm_runtime_put_noidle(), or any other
> > +      * runtime PM helper function decrementing the usage count, in its
> > +      * probe routine and pm_runtime_get_noresume() in its remove routine.
> > +      */
> >       pm_runtime_set_active(&dev->dev);
> > +     pm_runtime_get_noresume(&dev->dev);
> >       pm_runtime_enable(&dev->dev);
> >       device_enable_async_suspend(&dev->dev);
> >       dev->wakeup_prepared = false;

-- 
Robert Święcki

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Rafael J. Wysocki]

On Wed, Nov 10, 2021 at 5:33 PM Robert Święcki <robert@swiecki.net> wrote:
>
> Hi all,
>
> śr., 10 lis 2021 o 15:14 Bjorn Helgaas <helgaas@kernel.org> napisał(a):
> > On Tue, Nov 09, 2021 at 02:05:18PM -0600, Bjorn Helgaas wrote:
> > > On Tue, Nov 09, 2021 at 07:58:47PM +0100, Rafael J. Wysocki wrote:
> > > > On Tue, Nov 9, 2021 at 7:52 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > ...
> >
> > > > > So instead, we can drop the pm_runtime_get_sync() and
> > > > > pm_runtime_put_sync() from local_pci_probe() and pci_device_remove(),
> > > > > respectively, and add pm_runtine_get_noresume() to pci_pm_init(),
> > > > > which will prevent PM-runtime from touching the device until it has a
> > > > > driver that supports PM-runtime.
> > > > >
> > > > > We'll lose the theoretical ability to put unbound devices into D3 this
> > > > > way, but we learned some time ago that this isn't safe in all cases
> > > > > anyway.
> > > >
> > > > IOW, something like this (untested and most likely white-space-damaged).
> > >
> > > Thanks!  I applied this manually to for-linus in hopes of making the
> > > the next linux-next build.
> > >
> > > Please send any testing reports and corrections to the patch and
> > > commit log!
> >
> > Robert, I hate to ask even more of you, but if you have a chance, it
> > would be very helpful if you could test the patch below.  I'm pretty
> > sure it should fix the problem you saw, and I hope to ask Linus to
> > merge it today.
>
> I think the most recent patch creates some timeouts and other problems
> in pci-related code? Things I haven't seen before.

So I honestly think that commit 2a4d9408c9e8 needs to be reverted,
because it clearly does more than it is supposed to and we need to go
back to the drawing board and do this again, but correctly.

> But, granted, my kernel testing approach is not with focus on details, so maybe I did
> sth wrong.
>
> code is now at: cb690f5238d71f543f4ce874aa59237cf53a877c
> https://github.com/torvalds/linux/commit/cb690f5238d71f543f4ce874aa59237cf53a877c
>
> lis 10 17:26:41 jd kernel: Linux version 5.15.0+ (jagger@jd) (gcc
> (Debian 11.2.0-10) 11.2.0, GNU ld (GNU Binutils for Debian)
> 2.37.50.20211102) #105 SMP PREEMPT Wed Nov 10 17:23:49 CET 2021
> lis 10 17:26:41 jd kernel: Command line: BOOT_IMAGE=/vmlinuz-5.15.0+
> root=UUID=8759fa14-93a4-4dc1-87e6-aa6f5cdbb2ff ro nosplash
> mitigations=off no_file_caps apparmor=0 selinux=0 audit=0
> amdgpu.ppfeaturemask=0xffffffff hugepagesz=1G default_hugepagesz=1G
> hugepages=8 amd_iommu=on iommu=pt
> vfio-pci.ids=10de:1f02,10de:10f9,10de:1ada,10de:1adb
> drm.edid_firmware=DP-1:edid/sam-g9.edid isolcpus=1,2,5,6,17,18,21,22
> ..
> lis 10 17:26:41 jd kernel: usb usb5: runtime PM trying to activate
> child device usb5 but parent (0000:0c:00.2) is not active
> ..
> lis 10 17:26:41 jd kernel: i2c-designware-pci 0000:0c:00.3: timeout in
> disabling adapter
> lis 10 17:26:41 jd kernel: i2c-designware-pci 0000:0c:00.3: timeout in
> disabling adapter
> ..
> lis 10 17:26:41 jd kernel: usb 1-5.3: config 1 has an invalid
> interface number: 2 but max is 1
> lis 10 17:26:41 jd kernel: usb 1-5.3: config 1 has no interface number 1
> ..
> lis 10 17:26:41 jd kernel: i2c-designware-pci 0000:0c:00.3: timeout in
> disabling adapter
> ..
> lis 10 17:26:41 jd kernel: i2c-designware-pci 0000:0c:00.3: timeout in
> disabling adapter
> lis 10 17:26:41 jd kernel: i2c-designware-pci 0000:0c:00.3: i2c
> timeout error -110
> lis 10 17:26:41 jd kernel: ucsi_ccg 3-0008: i2c_transfer failed -110
> lis 10 17:26:41 jd kernel: ucsi_ccg 3-0008: ucsi_ccg_init failed - -110
> lis 10 17:26:41 jd kernel: ucsi_ccg: probe of 3-0008 failed with error -110
> lis 10 17:26:42 jd kernel: snd_hda_intel 0000:0c:00.1:
> azx_get_response timeout, switching to polling mode: last
> cmd=0x000f0000
> lis 10 17:26:42 jd kernel: hdaudio hdaudioC0D0: runtime PM trying to
> activate child device hdaudioC0D0 but parent (0000:0c:00.1) is not a>
> lis 10 17:26:42 jd kernel: snd_hda_codec_hdmi hdaudioC0D0: HDMI:
> failed to get afg sub nodes
> lis 10 17:26:42 jd kernel: snd_hda_codec_hdmi: probe of hdaudioC0D0
> failed with error -22
> lis 10 17:26:42 jd kernel: snd_hda_codec_hdmi hdaudioC0D0: HDMI:
> failed to get afg sub nodes
> lis 10 17:26:42 jd kernel: snd_hda_codec_hdmi: probe of hdaudioC0D0
> failed with error -22
> lis 10 17:26:42 jd kernel: snd_hda_codec_hdmi hdaudioC0D0: HDMI:
> failed to get afg sub nodes
> lis 10 17:26:42 jd kernel: snd_hda_codec_hdmi: probe of hdaudioC0D0
> failed with error -22
> ...
> lis 10 17:26:42 jd kernel: snd_hda_codec_generic: probe of hdaudioC0D0
> failed with error -5
> lis 10 17:26:42 jd kernel: snd_hda_intel 0000:0c:00.1: Cannot probe
> codecs, giving up
> lis 10 17:26:42 jd kernel: ------------[ cut here ]------------
> lis 10 17:26:42 jd kernel: WARNING: CPU: 0 PID: 206 at
> sound/hda/hdac_bus.c:73 snd_hdac_bus_exit+0x3b/0x80 [snd_hda_core]
> lis 10 17:26:42 jd kernel: Modules linked in: snd_hda_codec_hdmi(E)
> nls_iso8859_2(E) snd_hda_codec_realtek(E) nls_cp852(E) snd_hda_codec_>
> lis 10 17:26:42 jd kernel:  i2c_designware_pci(E) drm(E) i2c_piix4(E)
> i2c_designware_core(E) xhci_pci(E) backlight(E) ice(E) xhci_pci_ren>
> lis 10 17:26:42 jd kernel: CPU: 0 PID: 206 Comm: kworker/0:2 Tainted:
> G            E     5.15.0+ #105
> lis 10 17:26:42 jd kernel: Hardware name: ASUS System Product Name/ROG
> CROSSHAIR VIII FORMULA, BIOS 3901 09/07/2021
> lis 10 17:26:42 jd kernel: Workqueue: events azx_probe_work [snd_hda_intel]
> lis 10 17:26:42 jd kernel: RIP: 0010:snd_hdac_bus_exit+0x3b/0x80 [snd_hda_core]
> lis 10 17:26:42 jd kernel: Code: 75 19 48 8b 57 58 48 8d 47 58 48 39
> c2 75 1b 48 81 c7 f8 02 00 00 e9 94 61 10 d8 0f 0b 48 8b 57 58 48 8d>
> lis 10 17:26:42 jd kernel: RSP: 0018:ffffa3e1008d3e40 EFLAGS: 00010283
> lis 10 17:26:42 jd kernel: RAX: ffff9728c5f38080 RBX: ffff9728c5f38028
> RCX: 000000008020001f
> lis 10 17:26:42 jd kernel: RDX: ffff9728c1f40b00 RSI: ffffffff9a30ddc9
> RDI: ffff9728c5f38028
> lis 10 17:26:42 jd kernel: RBP: ffff9728c5f384d0 R08: 0000000000000000
> R09: ffff9728c14a4e00
> lis 10 17:26:42 jd kernel: R10: 0000000000000000 R11: 0000000000000005
> R12: 00000000ffffffed
> lis 10 17:26:42 jd kernel: R13: 0000000000000000 R14: 0000000000000000
> R15: ffff972fcea26405
> lis 10 17:26:42 jd kernel: FS:  0000000000000000(0000)
> GS:ffff972fcea00000(0000) knlGS:0000000000000000
> lis 10 17:26:42 jd kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> lis 10 17:26:42 jd kernel: CR2: 00007fa43a298cb0 CR3: 000000010e58e000
> CR4: 0000000000750ef0
> lis 10 17:26:42 jd kernel: PKRU: 55555554
> lis 10 17:26:42 jd kernel: Call Trace:
> lis 10 17:26:42 jd kernel:  <TASK>
> lis 10 17:26:42 jd kernel:  azx_free+0xe5/0x1c0 [snd_hda_intel]
> lis 10 17:26:42 jd kernel:  azx_probe_continue+0x1a8/0x300 [snd_hda_intel]
> lis 10 17:26:42 jd kernel:  process_one_work+0x1eb/0x380
> lis 10 17:26:42 jd kernel:  worker_thread+0x48/0x400
> lis 10 17:26:42 jd kernel:  ? rescuer_thread+0x3c0/0x3c0
> lis 10 17:26:42 jd kernel:  kthread+0x151/0x180
> lis 10 17:26:42 jd kernel:  ? set_kthread_struct+0x40/0x40
> lis 10 17:26:42 jd kernel:  ret_from_fork+0x1f/0x30
> lis 10 17:26:42 jd kernel:  </TASK>
> lis 10 17:26:42 jd kernel: ---[ end trace f522829b4b020462 ]---
> lis 10 17:26:42 jd kernel: i2c-designware-pci 0000:0c:00.3: timeout in
> disabling adapter
>
> > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > > index 1d98c974381c..41cdf510214f 100644
> > > --- a/drivers/pci/pci-driver.c
> > > +++ b/drivers/pci/pci-driver.c
> > > @@ -309,16 +309,6 @@ static long local_pci_probe(void *_ddi)
> > >       struct device *dev = &pci_dev->dev;
> > >       int rc;
> > >
> > > -     /*
> > > -      * Unbound PCI devices are always put in D0, regardless of
> > > -      * runtime PM status.  During probe, the device is set to
> > > -      * active and the usage count is incremented.  If the driver
> > > -      * supports runtime PM, it should call pm_runtime_put_noidle(),
> > > -      * or any other runtime PM helper function decrementing the usage
> > > -      * count, in its probe routine and pm_runtime_get_noresume() in
> > > -      * its remove routine.
> > > -      */
> > > -     pm_runtime_get_sync(dev);
> > >       rc = pci_drv->probe(pci_dev, ddi->id);
> > >       if (!rc)
> > >               return rc;
> > > @@ -464,9 +454,6 @@ static void pci_device_remove(struct device *dev)
> > >       pcibios_free_irq(pci_dev);
> > >       pci_iov_remove(pci_dev);
> > >
> > > -     /* Undo the runtime PM settings in local_pci_probe() */
> > > -     pm_runtime_put_sync(dev);
> > > -
> > >       /*
> > >        * If the device is still on, set the power state as "unknown",
> > >        * since it might change by the next time we load the driver.
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index b88db815ee01..e9c38b994c73 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -3097,7 +3097,15 @@ void pci_pm_init(struct pci_dev *dev)
> > >       u16 pmc;
> > >
> > >       pm_runtime_forbid(&dev->dev);
> > > +
> > > +     /*
> > > +      * Unbound PCI devices are always put in D0.  If the driver supports
> > > +      * runtime PM, it should call pm_runtime_put_noidle(), or any other
> > > +      * runtime PM helper function decrementing the usage count, in its
> > > +      * probe routine and pm_runtime_get_noresume() in its remove routine.
> > > +      */
> > >       pm_runtime_set_active(&dev->dev);
> > > +     pm_runtime_get_noresume(&dev->dev);
> > >       pm_runtime_enable(&dev->dev);
> > >       device_enable_async_suspend(&dev->dev);
> > >       dev->wakeup_prepared = false;
>
> --
> Robert Święcki

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Bjorn Helgaas]

On Wed, Nov 10, 2021 at 05:48:52PM +0100, Rafael J. Wysocki wrote:
> On Wed, Nov 10, 2021 at 5:33 PM Robert Święcki <robert@swiecki.net> wrote:
> > śr., 10 lis 2021 o 15:14 Bjorn Helgaas <helgaas@kernel.org> napisał(a):
> > > On Tue, Nov 09, 2021 at 02:05:18PM -0600, Bjorn Helgaas wrote:
> > > > On Tue, Nov 09, 2021 at 07:58:47PM +0100, Rafael J. Wysocki wrote:
> > > > > On Tue, Nov 9, 2021 at 7:52 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > > ...
> > >
> > > > > > So instead, we can drop the pm_runtime_get_sync() and
> > > > > > pm_runtime_put_sync() from local_pci_probe() and pci_device_remove(),
> > > > > > respectively, and add pm_runtine_get_noresume() to pci_pm_init(),
> > > > > > which will prevent PM-runtime from touching the device until it has a
> > > > > > driver that supports PM-runtime.
> > > > > >
> > > > > > We'll lose the theoretical ability to put unbound devices into D3 this
> > > > > > way, but we learned some time ago that this isn't safe in all cases
> > > > > > anyway.
> > > > >
> > > > > IOW, something like this (untested and most likely white-space-damaged).
> > > >
> > > > Thanks!  I applied this manually to for-linus in hopes of making the
> > > > the next linux-next build.
> > > >
> > > > Please send any testing reports and corrections to the patch and
> > > > commit log!
> > >
> > > Robert, I hate to ask even more of you, but if you have a chance, it
> > > would be very helpful if you could test the patch below.  I'm pretty
> > > sure it should fix the problem you saw, and I hope to ask Linus to
> > > merge it today.
> >
> > I think the most recent patch creates some timeouts and other problems
> > in pci-related code? Things I haven't seen before.
> 
> So I honestly think that commit 2a4d9408c9e8 needs to be reverted,
> because it clearly does more than it is supposed to and we need to go
> back to the drawing board and do this again, but correctly.

Yep, I agree, I'll work on that today.

Bjorn

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Bjorn Helgaas]

On Wed, Nov 10, 2021 at 05:33:11PM +0100, Robert Święcki wrote:
> śr., 10 lis 2021 o 15:14 Bjorn Helgaas <helgaas@kernel.org> napisał(a):
> > On Tue, Nov 09, 2021 at 02:05:18PM -0600, Bjorn Helgaas wrote:
> > > On Tue, Nov 09, 2021 at 07:58:47PM +0100, Rafael J. Wysocki wrote:
> > > > On Tue, Nov 9, 2021 at 7:52 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > ...
> >
> > > > > So instead, we can drop the pm_runtime_get_sync() and
> > > > > pm_runtime_put_sync() from local_pci_probe() and pci_device_remove(),
> > > > > respectively, and add pm_runtine_get_noresume() to pci_pm_init(),
> > > > > which will prevent PM-runtime from touching the device until it has a
> > > > > driver that supports PM-runtime.
> > > > >
> > > > > We'll lose the theoretical ability to put unbound devices into D3 this
> > > > > way, but we learned some time ago that this isn't safe in all cases
> > > > > anyway.
> > > >
> > > > IOW, something like this (untested and most likely white-space-damaged).
> > >
> > > Thanks!  I applied this manually to for-linus in hopes of making the
> > > the next linux-next build.
> > >
> > > Please send any testing reports and corrections to the patch and
> > > commit log!
> >
> > Robert, I hate to ask even more of you, but if you have a chance, it
> > would be very helpful if you could test the patch below.  I'm pretty
> > sure it should fix the problem you saw, and I hope to ask Linus to
> > merge it today.
> 
> I think the most recent patch creates some timeouts and other problems
> in pci-related code? Things I haven't seen before. But, granted, my
> kernel testing approach is not with focus on details, so maybe I did
> sth wrong.

Thank you very much for testing this.  The patch changed the way we
use runtime PM, and the dmesg snippets below look like they could be
related to runtime PM issues.

I think the conclusion is that we need to revert these commits:

  b5f9c644eb1b ("PCI: Remove struct pci_dev->driver")
  2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of pci_dev->driver")

from Linus' tree.  I queued up those reverts on
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=for-linus

That branch also includes an unrelated revert of 041284181226
("of/irq: Allow matching of an interrupt-map local to an interrupt
controller") that shouldn't affect you unless you're on an Apple M1
machine.

> code is now at: cb690f5238d71f543f4ce874aa59237cf53a877c
> https://github.com/torvalds/linux/commit/cb690f5238d71f543f4ce874aa59237cf53a877c
> 
> lis 10 17:26:41 jd kernel: Linux version 5.15.0+ (jagger@jd) (gcc
> (Debian 11.2.0-10) 11.2.0, GNU ld (GNU Binutils for Debian)
> 2.37.50.20211102) #105 SMP PREEMPT Wed Nov 10 17:23:49 CET 2021
> lis 10 17:26:41 jd kernel: Command line: BOOT_IMAGE=/vmlinuz-5.15.0+
> root=UUID=8759fa14-93a4-4dc1-87e6-aa6f5cdbb2ff ro nosplash
> mitigations=off no_file_caps apparmor=0 selinux=0 audit=0
> amdgpu.ppfeaturemask=0xffffffff hugepagesz=1G default_hugepagesz=1G
> hugepages=8 amd_iommu=on iommu=pt
> vfio-pci.ids=10de:1f02,10de:10f9,10de:1ada,10de:1adb
> drm.edid_firmware=DP-1:edid/sam-g9.edid isolcpus=1,2,5,6,17,18,21,22
> ..
> lis 10 17:26:41 jd kernel: usb usb5: runtime PM trying to activate
> child device usb5 but parent (0000:0c:00.2) is not active
> ..
> lis 10 17:26:41 jd kernel: i2c-designware-pci 0000:0c:00.3: timeout in
> disabling adapter
> lis 10 17:26:41 jd kernel: i2c-designware-pci 0000:0c:00.3: timeout in
> disabling adapter
> ..
> lis 10 17:26:41 jd kernel: usb 1-5.3: config 1 has an invalid
> interface number: 2 but max is 1
> lis 10 17:26:41 jd kernel: usb 1-5.3: config 1 has no interface number 1
> ..
> lis 10 17:26:41 jd kernel: i2c-designware-pci 0000:0c:00.3: timeout in
> disabling adapter
> ..
> lis 10 17:26:41 jd kernel: i2c-designware-pci 0000:0c:00.3: timeout in
> disabling adapter
> lis 10 17:26:41 jd kernel: i2c-designware-pci 0000:0c:00.3: i2c
> timeout error -110
> lis 10 17:26:41 jd kernel: ucsi_ccg 3-0008: i2c_transfer failed -110
> lis 10 17:26:41 jd kernel: ucsi_ccg 3-0008: ucsi_ccg_init failed - -110
> lis 10 17:26:41 jd kernel: ucsi_ccg: probe of 3-0008 failed with error -110
> lis 10 17:26:42 jd kernel: snd_hda_intel 0000:0c:00.1:
> azx_get_response timeout, switching to polling mode: last
> cmd=0x000f0000
> lis 10 17:26:42 jd kernel: hdaudio hdaudioC0D0: runtime PM trying to
> activate child device hdaudioC0D0 but parent (0000:0c:00.1) is not a>
> lis 10 17:26:42 jd kernel: snd_hda_codec_hdmi hdaudioC0D0: HDMI:
> failed to get afg sub nodes
> lis 10 17:26:42 jd kernel: snd_hda_codec_hdmi: probe of hdaudioC0D0
> failed with error -22
> lis 10 17:26:42 jd kernel: snd_hda_codec_hdmi hdaudioC0D0: HDMI:
> failed to get afg sub nodes
> lis 10 17:26:42 jd kernel: snd_hda_codec_hdmi: probe of hdaudioC0D0
> failed with error -22
> lis 10 17:26:42 jd kernel: snd_hda_codec_hdmi hdaudioC0D0: HDMI:
> failed to get afg sub nodes
> lis 10 17:26:42 jd kernel: snd_hda_codec_hdmi: probe of hdaudioC0D0
> failed with error -22
> ...
> lis 10 17:26:42 jd kernel: snd_hda_codec_generic: probe of hdaudioC0D0
> failed with error -5
> lis 10 17:26:42 jd kernel: snd_hda_intel 0000:0c:00.1: Cannot probe
> codecs, giving up
> lis 10 17:26:42 jd kernel: ------------[ cut here ]------------
> lis 10 17:26:42 jd kernel: WARNING: CPU: 0 PID: 206 at
> sound/hda/hdac_bus.c:73 snd_hdac_bus_exit+0x3b/0x80 [snd_hda_core]
> lis 10 17:26:42 jd kernel: Modules linked in: snd_hda_codec_hdmi(E)
> nls_iso8859_2(E) snd_hda_codec_realtek(E) nls_cp852(E) snd_hda_codec_>
> lis 10 17:26:42 jd kernel:  i2c_designware_pci(E) drm(E) i2c_piix4(E)
> i2c_designware_core(E) xhci_pci(E) backlight(E) ice(E) xhci_pci_ren>
> lis 10 17:26:42 jd kernel: CPU: 0 PID: 206 Comm: kworker/0:2 Tainted:
> G            E     5.15.0+ #105
> lis 10 17:26:42 jd kernel: Hardware name: ASUS System Product Name/ROG
> CROSSHAIR VIII FORMULA, BIOS 3901 09/07/2021
> lis 10 17:26:42 jd kernel: Workqueue: events azx_probe_work [snd_hda_intel]
> lis 10 17:26:42 jd kernel: RIP: 0010:snd_hdac_bus_exit+0x3b/0x80 [snd_hda_core]
> lis 10 17:26:42 jd kernel: Code: 75 19 48 8b 57 58 48 8d 47 58 48 39
> c2 75 1b 48 81 c7 f8 02 00 00 e9 94 61 10 d8 0f 0b 48 8b 57 58 48 8d>
> lis 10 17:26:42 jd kernel: RSP: 0018:ffffa3e1008d3e40 EFLAGS: 00010283
> lis 10 17:26:42 jd kernel: RAX: ffff9728c5f38080 RBX: ffff9728c5f38028
> RCX: 000000008020001f
> lis 10 17:26:42 jd kernel: RDX: ffff9728c1f40b00 RSI: ffffffff9a30ddc9
> RDI: ffff9728c5f38028
> lis 10 17:26:42 jd kernel: RBP: ffff9728c5f384d0 R08: 0000000000000000
> R09: ffff9728c14a4e00
> lis 10 17:26:42 jd kernel: R10: 0000000000000000 R11: 0000000000000005
> R12: 00000000ffffffed
> lis 10 17:26:42 jd kernel: R13: 0000000000000000 R14: 0000000000000000
> R15: ffff972fcea26405
> lis 10 17:26:42 jd kernel: FS:  0000000000000000(0000)
> GS:ffff972fcea00000(0000) knlGS:0000000000000000
> lis 10 17:26:42 jd kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> lis 10 17:26:42 jd kernel: CR2: 00007fa43a298cb0 CR3: 000000010e58e000
> CR4: 0000000000750ef0
> lis 10 17:26:42 jd kernel: PKRU: 55555554
> lis 10 17:26:42 jd kernel: Call Trace:
> lis 10 17:26:42 jd kernel:  <TASK>
> lis 10 17:26:42 jd kernel:  azx_free+0xe5/0x1c0 [snd_hda_intel]
> lis 10 17:26:42 jd kernel:  azx_probe_continue+0x1a8/0x300 [snd_hda_intel]
> lis 10 17:26:42 jd kernel:  process_one_work+0x1eb/0x380
> lis 10 17:26:42 jd kernel:  worker_thread+0x48/0x400
> lis 10 17:26:42 jd kernel:  ? rescuer_thread+0x3c0/0x3c0
> lis 10 17:26:42 jd kernel:  kthread+0x151/0x180
> lis 10 17:26:42 jd kernel:  ? set_kthread_struct+0x40/0x40
> lis 10 17:26:42 jd kernel:  ret_from_fork+0x1f/0x30
> lis 10 17:26:42 jd kernel:  </TASK>
> lis 10 17:26:42 jd kernel: ---[ end trace f522829b4b020462 ]---
> lis 10 17:26:42 jd kernel: i2c-designware-pci 0000:0c:00.3: timeout in
> disabling adapter
> 
> > > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > > index 1d98c974381c..41cdf510214f 100644
> > > --- a/drivers/pci/pci-driver.c
> > > +++ b/drivers/pci/pci-driver.c
> > > @@ -309,16 +309,6 @@ static long local_pci_probe(void *_ddi)
> > >       struct device *dev = &pci_dev->dev;
> > >       int rc;
> > >
> > > -     /*
> > > -      * Unbound PCI devices are always put in D0, regardless of
> > > -      * runtime PM status.  During probe, the device is set to
> > > -      * active and the usage count is incremented.  If the driver
> > > -      * supports runtime PM, it should call pm_runtime_put_noidle(),
> > > -      * or any other runtime PM helper function decrementing the usage
> > > -      * count, in its probe routine and pm_runtime_get_noresume() in
> > > -      * its remove routine.
> > > -      */
> > > -     pm_runtime_get_sync(dev);
> > >       rc = pci_drv->probe(pci_dev, ddi->id);
> > >       if (!rc)
> > >               return rc;
> > > @@ -464,9 +454,6 @@ static void pci_device_remove(struct device *dev)
> > >       pcibios_free_irq(pci_dev);
> > >       pci_iov_remove(pci_dev);
> > >
> > > -     /* Undo the runtime PM settings in local_pci_probe() */
> > > -     pm_runtime_put_sync(dev);
> > > -
> > >       /*
> > >        * If the device is still on, set the power state as "unknown",
> > >        * since it might change by the next time we load the driver.
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index b88db815ee01..e9c38b994c73 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -3097,7 +3097,15 @@ void pci_pm_init(struct pci_dev *dev)
> > >       u16 pmc;
> > >
> > >       pm_runtime_forbid(&dev->dev);
> > > +
> > > +     /*
> > > +      * Unbound PCI devices are always put in D0.  If the driver supports
> > > +      * runtime PM, it should call pm_runtime_put_noidle(), or any other
> > > +      * runtime PM helper function decrementing the usage count, in its
> > > +      * probe routine and pm_runtime_get_noresume() in its remove routine.
> > > +      */
> > >       pm_runtime_set_active(&dev->dev);
> > > +     pm_runtime_get_noresume(&dev->dev);
> > >       pm_runtime_enable(&dev->dev);
> > >       device_enable_async_suspend(&dev->dev);
> > >       dev->wakeup_prepared = false;
> 
> -- 
> Robert Święcki

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Bjorn Helgaas]

On Wed, Nov 10, 2021 at 03:19:05PM -0600, Bjorn Helgaas wrote:
> On Wed, Nov 10, 2021 at 05:33:11PM +0100, Robert Święcki wrote:
> > śr., 10 lis 2021 o 15:14 Bjorn Helgaas <helgaas@kernel.org> napisał(a):
> > > On Tue, Nov 09, 2021 at 02:05:18PM -0600, Bjorn Helgaas wrote:
> > > > On Tue, Nov 09, 2021 at 07:58:47PM +0100, Rafael J. Wysocki wrote:
> > > > > On Tue, Nov 9, 2021 at 7:52 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > > ...
> > >
> > > > > > So instead, we can drop the pm_runtime_get_sync() and
> > > > > > pm_runtime_put_sync() from local_pci_probe() and pci_device_remove(),
> > > > > > respectively, and add pm_runtine_get_noresume() to pci_pm_init(),
> > > > > > which will prevent PM-runtime from touching the device until it has a
> > > > > > driver that supports PM-runtime.
> > > > > >
> > > > > > We'll lose the theoretical ability to put unbound devices into D3 this
> > > > > > way, but we learned some time ago that this isn't safe in all cases
> > > > > > anyway.
> > > > >
> > > > > IOW, something like this (untested and most likely white-space-damaged).
> > > >
> > > > Thanks!  I applied this manually to for-linus in hopes of making the
> > > > the next linux-next build.
> > > >
> > > > Please send any testing reports and corrections to the patch and
> > > > commit log!
> > >
> > > Robert, I hate to ask even more of you, but if you have a chance, it
> > > would be very helpful if you could test the patch below.  I'm pretty
> > > sure it should fix the problem you saw, and I hope to ask Linus to
> > > merge it today.
> > 
> > I think the most recent patch creates some timeouts and other problems
> > in pci-related code? Things I haven't seen before. But, granted, my
> > kernel testing approach is not with focus on details, so maybe I did
> > sth wrong.
> 
> Thank you very much for testing this.  The patch changed the way we
> use runtime PM, and the dmesg snippets below look like they could be
> related to runtime PM issues.
> 
> I think the conclusion is that we need to revert these commits:
> 
>   b5f9c644eb1b ("PCI: Remove struct pci_dev->driver")
>   2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of pci_dev->driver")
> 
> from Linus' tree.  I queued up those reverts on
> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=for-linus

These reverts appeared in the Nov 11 linux-next tree.  Any chance you
could verify that they solve the i2c_dw_pci_resume() issue?  If it's
easier, you can apply them from:
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=for-linus
instead.

Bjorn

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Robert Święcki]

Hi,

> > Thank you very much for testing this.  The patch changed the way we
> > use runtime PM, and the dmesg snippets below look like they could be
> > related to runtime PM issues.
> >
> > I think the conclusion is that we need to revert these commits:
> >
> >   b5f9c644eb1b ("PCI: Remove struct pci_dev->driver")
> >   2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of pci_dev->driver")
> >
> > from Linus' tree.  I queued up those reverts on
> > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=for-linus
>
> These reverts appeared in the Nov 11 linux-next tree.  Any chance you
> could verify that they solve the i2c_dw_pci_resume() issue?  If it's
> easier, you can apply them from:
> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=for-linus
> instead.

Looks good with the most recent 3 commits from for-linus applied on
the top of the current linus' tree.

No problematic dmesg entries, my Win11/vfio/kvm/qemu boots fine.



-- 
Robert Święcki

Re: [PATCH] pci: Don't call resume callback for nearly bound devices

[Bjorn Helgaas]

On Thu, Nov 11, 2021 at 06:32:28PM +0100, Robert Święcki wrote:
> Hi,
> 
> > > Thank you very much for testing this.  The patch changed the way we
> > > use runtime PM, and the dmesg snippets below look like they could be
> > > related to runtime PM issues.
> > >
> > > I think the conclusion is that we need to revert these commits:
> > >
> > >   b5f9c644eb1b ("PCI: Remove struct pci_dev->driver")
> > >   2a4d9408c9e8 ("PCI: Use to_pci_driver() instead of pci_dev->driver")
> > >
> > > from Linus' tree.  I queued up those reverts on
> > > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=for-linus
> >
> > These reverts appeared in the Nov 11 linux-next tree.  Any chance you
> > could verify that they solve the i2c_dw_pci_resume() issue?  If it's
> > easier, you can apply them from:
> > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=for-linus
> > instead.
> 
> Looks good with the most recent 3 commits from for-linus applied on
> the top of the current linus' tree.
> 
> No problematic dmesg entries, my Win11/vfio/kvm/qemu boots fine.

Thank you so much for testing this!  I'll ask Linus to pull the two
reverts related to the i2c_dw_pci_resume() issue.  Marc and Christian
are still hoping for a fix instead of the third revert.

Bjorn