| Message ID | 20250108-mhi_recovery_fix-v1-2-a0a00a17da46@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | bus: mhi: host: pci_generic: Couple of recovery fixes | expand |
On Wed, 8 Jan 2025 at 14:39, Manivannan Sadhasivam via B4 Relay <devnull+manivannan.sadhasivam.linaro.org@kernel.org> wrote: > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > Currently, in mhi_pci_runtime_resume(), if the resume fails, recovery_work > is started asynchronously and success is returned. But this doesn't align > with what PM core expects as documented in > Documentation/power/runtime_pm.rst: > > "Once the subsystem-level resume callback (or the driver resume callback, > if invoked directly) has completed successfully, the PM core regards the > device as fully operational, which means that the device _must_ be able to > complete I/O operations as needed. The runtime PM status of the device is > then 'active'." > > So the PM core ends up marking the runtime PM status of the device as > 'active', even though the device is not able to handle the I/O operations. > This same condition more or less applies to system resume as well. > > So to avoid this ambiguity, try to recover the device synchronously from > mhi_pci_runtime_resume() and return the actual error code in the case of > recovery failure. > > For doing so, move the recovery code to __mhi_pci_recovery_work() helper > and call that from both mhi_pci_recovery_work() and > mhi_pci_runtime_resume(). Former still ignores the return value, while the > latter passes it to PM core. > > Cc: stable@vger.kernel.org # 5.13 > Reported-by: Johan Hovold <johan@kernel.org> > Closes: https://lore.kernel.org/mhi/Z2PbEPYpqFfrLSJi@hovoldconsulting.com > Fixes: d3800c1dce24 ("bus: mhi: pci_generic: Add support for runtime PM") > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Note that it will noticeably impact the user experience on system-wide resume (mhi_pci_resume), because MHI devices usually take a while (a few seconds) to cold boot and reach a ready state (or time out in the worst case). So we may have people complaining about delayed resume regression on their laptop even if they are not using the MHI device/modem function. Are we ok with that? Regards, Loic
On Wed, Jan 08, 2025 at 04:19:06PM +0100, Loic Poulain wrote: > On Wed, 8 Jan 2025 at 14:39, Manivannan Sadhasivam via B4 Relay > <devnull+manivannan.sadhasivam.linaro.org@kernel.org> wrote: > > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > Currently, in mhi_pci_runtime_resume(), if the resume fails, recovery_work > > is started asynchronously and success is returned. But this doesn't align > > with what PM core expects as documented in > > Documentation/power/runtime_pm.rst: > > > > "Once the subsystem-level resume callback (or the driver resume callback, > > if invoked directly) has completed successfully, the PM core regards the > > device as fully operational, which means that the device _must_ be able to > > complete I/O operations as needed. The runtime PM status of the device is > > then 'active'." > > > > So the PM core ends up marking the runtime PM status of the device as > > 'active', even though the device is not able to handle the I/O operations. > > This same condition more or less applies to system resume as well. > > > > So to avoid this ambiguity, try to recover the device synchronously from > > mhi_pci_runtime_resume() and return the actual error code in the case of > > recovery failure. > > > > For doing so, move the recovery code to __mhi_pci_recovery_work() helper > > and call that from both mhi_pci_recovery_work() and > > mhi_pci_runtime_resume(). Former still ignores the return value, while the > > latter passes it to PM core. > > > > Cc: stable@vger.kernel.org # 5.13 > > Reported-by: Johan Hovold <johan@kernel.org> > > Closes: https://lore.kernel.org/mhi/Z2PbEPYpqFfrLSJi@hovoldconsulting.com > > Fixes: d3800c1dce24 ("bus: mhi: pci_generic: Add support for runtime PM") > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > Note that it will noticeably impact the user experience on system-wide > resume (mhi_pci_resume), because MHI devices usually take a while (a > few seconds) to cold boot and reach a ready state (or time out in the > worst case). So we may have people complaining about delayed resume > regression on their laptop even if they are not using the MHI > device/modem function. Are we ok with that? > Are you saying that the modem will enter D3Cold all the time during system suspend? I think you are referring to x86 host machines here. If that is the case, we should not be using mhi_pci_runtime_*() calls in mhi_pci_suspend/resume(). Rather the MHI stack should be powered down during suspend and powered ON during resume. - Mani
diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c index e92df380c785..f6de407e077e 100644 --- a/drivers/bus/mhi/host/pci_generic.c +++ b/drivers/bus/mhi/host/pci_generic.c @@ -997,10 +997,8 @@ static void mhi_pci_runtime_put(struct mhi_controller *mhi_cntrl) pm_runtime_put(mhi_cntrl->cntrl_dev); } -static void mhi_pci_recovery_work(struct work_struct *work) +static int __mhi_pci_recovery_work(struct mhi_pci_device *mhi_pdev) { - struct mhi_pci_device *mhi_pdev = container_of(work, struct mhi_pci_device, - recovery_work); struct mhi_controller *mhi_cntrl = &mhi_pdev->mhi_cntrl; struct pci_dev *pdev = to_pci_dev(mhi_cntrl->cntrl_dev); int err; @@ -1035,13 +1033,25 @@ static void mhi_pci_recovery_work(struct work_struct *work) set_bit(MHI_PCI_DEV_STARTED, &mhi_pdev->status); mod_timer(&mhi_pdev->health_check_timer, jiffies + HEALTH_CHECK_PERIOD); - return; + + return 0; err_unprepare: mhi_unprepare_after_power_down(mhi_cntrl); err_try_reset: - if (pci_try_reset_function(pdev)) + err = pci_try_reset_function(pdev); + if (err) dev_err(&pdev->dev, "Recovery failed\n"); + + return err; +} + +static void mhi_pci_recovery_work(struct work_struct *work) +{ + struct mhi_pci_device *mhi_pdev = container_of(work, struct mhi_pci_device, + recovery_work); + + __mhi_pci_recovery_work(mhi_pdev); } static void health_check(struct timer_list *t) @@ -1400,15 +1410,10 @@ static int __maybe_unused mhi_pci_runtime_resume(struct device *dev) return 0; err_recovery: - /* Do not fail to not mess up our PCI device state, the device likely - * lost power (d3cold) and we simply need to reset it from the recovery - * procedure, trigger the recovery asynchronously to prevent system - * suspend exit delaying. - */ - queue_work(system_long_wq, &mhi_pdev->recovery_work); + err = __mhi_pci_recovery_work(mhi_pdev); pm_runtime_mark_last_busy(dev); - return 0; + return err; } static int __maybe_unused mhi_pci_suspend(struct device *dev)