On Mon, Feb 5, 2024 at 9:11 PM Stanislaw Gruszka
<stanislaw.gruszka@xxxxxxxxxxxxxxx> wrote:
>
> On Thu, Feb 01, 2024 at 03:10:35PM +0100, Rafael J. Wysocki wrote:
> > On Thu, Feb 1, 2024 at 10:36 AM Stanislaw Gruszka
> > <stanislaw.gruszka@xxxxxxxxxxxxxxx> wrote:
> > >
> > > On Mon, Jan 29, 2024 at 06:14:36PM -0600, Bjorn Helgaas wrote:
> > > > On Tue, Jan 23, 2024 at 10:00:16AM +0100, Stanislaw Gruszka wrote:
> > > > > PM runtime can be done simultaneously with AER error handling.
> > > > > Avoid that by using pm_runtime_get_sync() just after pci_dev_get()
> > > > > and pm_runtime_put() just before pci_dev_put() in AER recovery
> > > > > procedures.
> > > >
> > > > I guess there must be a general rule here, like "PCI core must use
> > > > pm_runtime_get_sync() whenever touching the device asynchronously,
> > > > i.e., when it's doing something unrelated to a call from the driver"?
> > >
> > > Clear rules would be nice, that's for sure.
> > >
> > > > Probably would apply to all subsystem cores, not just PCI.
> > >
> > > If they have something similar like AER.
> > >
> > > > > I'm not sure about DPC case since I do not see get/put there. It
> > > > > just call pci_do_recovery() from threaded irq dcd_handler().
> > > > > I think pm_runtime* should be added to this handler as well.
> > > >
> > > > s/dcd_handler/dpc_handler/
> > > >
> > > > I'm guessing the "threaded" part really doesn't matter; just the fact
> > > > that this is in response to an interrupt, not something directly
> > > > called by a driver?
> > >
> > > Yes. I added "threaded" just to emphasis that it's safe to add sleeping
> > > functions there. In context of possible solution, not related to
> > > the problem itself.
> > >
> > > > > pm_runtime_get_sync() will increase dev->power.usage_count counter to
> > > > > prevent any rpm actives. When there is suspending pending, it will wait
> > > > > for it and do the rpm resume. Not sure if that problem, on my testing
> > > > > I did not encounter issues with that.
> > > >
> > > > Sorry, I didn't catch your meaning here.
> > > I tired to write two things:
> > >
> > > First, pm_runtime_get_sync() after exit prevents possibility of
> > > runtime suspend, so we are sure device will not be powered off
> > >
> > > Second, if during pm_runtime_get_sync(), there is pending attempt
> > > to suspend device, it will be synchronized and device will
> > > be resumed.
> >
> > Not exactly. pm_runtime_get_sync() will resume the device
> > synchronously no matter what.
> >
> > > This can be problematic as device is in error state.
> >
> > If this is a real possibility (I mean, device in a low-power state and
> > in an error state at the same time), it would be better to call
> > __pm_runtime_disable(dev, false) instead of pm_runtime_get_sync(), as
> > that would prevent runtime PM from changing the device state.
>
> __pm_runtime_disable(dev, false) does not work as reliable in my
> test as pm_runtime_get_sync(), the
>
> igc 0000:02:00.0: Unable to change power state from D3hot to D0, device inaccessible
>
> message disappears, but sill have:
>
> igc 0000:02:00.0: not ready 65535ms after bus reset; giving up
> pcieport 0000:00:1c.2: AER: Root Port link has been reset (-25)
> pcieport 0000:00:1c.2: AER: subordinate device reset failed
> pcieport 0000:00:1c.2: AER: device recovery fail
But what exactly do you do?
(1) __pm_runtime_disable(dev, false)
(2) Check power state
(a) If D0 (and device runtime-active), proceed
(b) If > D0, remove power (if possible) and put into D0
or something else?
> > > But at least from software perspective we should end in device
> > > being in active state and then we can perform reset of it.
> >
> > I'm not sure about this. It may be better to power-cycle the device
> > in D3hot instead of attempting to put it into D0 beforehand.
>
> Me nigher, but in pci_reset_function() and similar resetting
> procedures we always call pci_dev_save_and_disable() before
> actual reset and it set device in D0, so I think it's ok to do so.
OK
