Re: [PATCH v3] driver core: Cancel scheduled pm_runtime_idle() on device removal

[Rafael J. Wysocki]

On Mon, Mar 4, 2024 at 4:51 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> On Mon, Mar 04, 2024 at 03:38:38PM +0100, Rafael J. Wysocki wrote:
> > On Thu, Feb 29, 2024 at 7:23 AM Kai-Heng Feng
> > <kai.heng.feng@xxxxxxxxxxxxx> wrote:
> > >
> > > When inserting an SD7.0 card to Realtek card reader, the card reader
> > > unplugs itself and morph into a NVMe device. The slot Link down on hot
> > > unplugged can cause the following error:
> > >
> > > pcieport 0000:00:1c.0: pciehp: Slot(8): Link Down
> > > BUG: unable to handle page fault for address: ffffb24d403e5010
> > > PGD 100000067 P4D 100000067 PUD 1001fe067 PMD 100d97067 PTE 0
> > > Oops: 0000 [#1] PREEMPT SMP PTI
> > > CPU: 3 PID: 534 Comm: kworker/3:10 Not tainted 6.4.0 #6
> > > Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./H370M Pro4, BIOS P3.40 10/25/2018
> > > Workqueue: pm pm_runtime_work
> > > RIP: 0010:ioread32+0x2e/0x70
> > ...
> > > Call Trace:
> > >  <TASK>
> > >  ? show_regs+0x68/0x70
> > >  ? __die_body+0x20/0x70
> > >  ? __die+0x2b/0x40
> > >  ? page_fault_oops+0x160/0x480
> > >  ? search_bpf_extables+0x63/0x90
> > >  ? ioread32+0x2e/0x70
> > >  ? search_exception_tables+0x5f/0x70
> > >  ? kernelmode_fixup_or_oops+0xa2/0x120
> > >  ? __bad_area_nosemaphore+0x179/0x230
> > >  ? bad_area_nosemaphore+0x16/0x20
> > >  ? do_kern_addr_fault+0x8b/0xa0
> > >  ? exc_page_fault+0xe5/0x180
> > >  ? asm_exc_page_fault+0x27/0x30
> > >  ? ioread32+0x2e/0x70
> > >  ? rtsx_pci_write_register+0x5b/0x90 [rtsx_pci]
> > >  rtsx_set_l1off_sub+0x1c/0x30 [rtsx_pci]
> > >  rts5261_set_l1off_cfg_sub_d0+0x36/0x40 [rtsx_pci]
> > >  rtsx_pci_runtime_idle+0xc7/0x160 [rtsx_pci]
> > >  ? __pfx_pci_pm_runtime_idle+0x10/0x10
> > >  pci_pm_runtime_idle+0x34/0x70
> > >  rpm_idle+0xc4/0x2b0
> > >  pm_runtime_work+0x93/0xc0
> > >  process_one_work+0x21a/0x430
> > >  worker_thread+0x4a/0x3c0
> > ...
>
> > > This happens because scheduled pm_runtime_idle() is not cancelled.
> >
> > But rpm_resume() changes dev->power.request to RPM_REQ_NONE and if
> > pm_runtime_work() sees this, it will not run rpm_idle().
> >
> > However, rpm_resume() doesn't deactivate the autosuspend timer if it
> > is running (see the comment in rpm_resume() regarding this), so it may
> > queue up a runtime PM work later.
> >
> > If this is not desirable, you need to stop the autosuspend timer
> > explicitly in addition to calling pm_runtime_get_sync().
>
> I don't quite follow all this.  I think the race is between
> rtsx_pci_remove() (not resume) and rtsx_pci_runtime_idle().

I think so too and the latter is not expected to run.

>   rtsx_pci_remove()
>   {
>     pm_runtime_get_sync()
>     pm_runtime_forbid()
>     ...
>
> If this is an rtsx bug, what exactly should be added to
> rtsx_pci_remove()?
>
> Is there ever a case where we want any runtime PM work to happen
> during or after a driver .remove()?  If not, maybe the driver core
> should prevent that, which I think is basically what this patch does.

No, it is not, because it doesn't actually prevent the race from
occurring, it just narrows the window quite a bit.

It would be better to call pm_runtime_dont_use_autosuspend() instead
of pm_runtime_barrier().

> If this is an rtsx driver bug, I'm concerned there may be many other
> drivers with a similar issue.  rtsx exercises this path more than most
> because the device switches between card reader and NVMe SSD using
> hotplug add/remove based on whether an SD card is inserted (see [1]).

This is a valid concern, so it is mostly a matter of where to disable
autosuspend.

It may be the driver core in principle, but note that it calls
->remove() after invoking pm_runtime_put_sync(), so why would it
disable autosuspend when it allows runtime PM to race with device
removal in general?

Another way might be to add a pm_runtime_dont_use_autosuspend() call
at the beginning of pci_device_remove().

Or just remove the optimization in question from rpm_resume() which is
quite confusing and causes people to make assumptions that lead to
incorrect behavior in this particular case.

So this (modulo GMail-induced whitespace breakage):

---
 drivers/base/power/runtime.c |    9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -782,15 +782,8 @@ static int rpm_resume(struct device *dev
     if (retval)
         goto out;

-    /*
-     * Other scheduled or pending requests need to be canceled.  Small
-     * optimization: If an autosuspend timer is running, leave it running
-     * rather than cancelling it now only to restart it again in the near
-     * future.
-     */
     dev->power.request = RPM_REQ_NONE;
-    if (!dev->power.timer_autosuspends)
-        pm_runtime_deactivate_timer(dev);
+    pm_runtime_deactivate_timer(dev);

     if (dev->power.runtime_status == RPM_ACTIVE) {
         retval = 1;