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