| Message ID | 20240227063613.4478-3-quic_bqiang@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: ath11k: hibernation support | expand |
On Tue, Feb 27, 2024 at 02:36:12PM +0800, Baochen Qiang wrote: > MHI devices may not be destroyed during suspend/hibernation, so need > to unprepare/prepare MHI channels throughout the transition, this is > done by adding suspend/resume callbacks. > > The suspend callback is called in the late suspend stage, this means > MHI channels are still alive at suspend stage, and that makes it > possible for an MHI controller driver to communicate with others over > those channels at suspend stage. While the resume callback is called > in the early resume stage, for a similar reason. > > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30 > > Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Could you please confirm if this patch can be applied separately and won't cause regression? - Mani > --- > net/qrtr/mhi.c | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c > index 9ced13c0627a..e96b82a6742c 100644 > --- a/net/qrtr/mhi.c > +++ b/net/qrtr/mhi.c > @@ -118,6 +118,32 @@ static const struct mhi_device_id qcom_mhi_qrtr_id_table[] = { > }; > MODULE_DEVICE_TABLE(mhi, qcom_mhi_qrtr_id_table); > > +static int __maybe_unused qcom_mhi_qrtr_pm_suspend_late(struct device *dev) > +{ > + struct mhi_device *mhi_dev = container_of(dev, struct mhi_device, dev); > + > + mhi_unprepare_from_transfer(mhi_dev); > + > + return 0; > +} > + > +static int __maybe_unused qcom_mhi_qrtr_pm_resume_early(struct device *dev) > +{ > + struct mhi_device *mhi_dev = container_of(dev, struct mhi_device, dev); > + int rc; > + > + rc = mhi_prepare_for_transfer_autoqueue(mhi_dev); > + if (rc) > + dev_err(dev, "failed to prepare for autoqueue transfer %d\n", rc); > + > + return rc; > +} > + > +static const struct dev_pm_ops qcom_mhi_qrtr_pm_ops = { > + SET_LATE_SYSTEM_SLEEP_PM_OPS(qcom_mhi_qrtr_pm_suspend_late, > + qcom_mhi_qrtr_pm_resume_early) > +}; > + > static struct mhi_driver qcom_mhi_qrtr_driver = { > .probe = qcom_mhi_qrtr_probe, > .remove = qcom_mhi_qrtr_remove, > @@ -126,6 +152,7 @@ static struct mhi_driver qcom_mhi_qrtr_driver = { > .id_table = qcom_mhi_qrtr_id_table, > .driver = { > .name = "qcom_mhi_qrtr", > + .pm = &qcom_mhi_qrtr_pm_ops, > }, > }; > > -- > 2.25.1 >
On 2/27/2024 3:15 PM, Manivannan Sadhasivam wrote: > On Tue, Feb 27, 2024 at 02:36:12PM +0800, Baochen Qiang wrote: >> MHI devices may not be destroyed during suspend/hibernation, so need >> to unprepare/prepare MHI channels throughout the transition, this is >> done by adding suspend/resume callbacks. >> >> The suspend callback is called in the late suspend stage, this means >> MHI channels are still alive at suspend stage, and that makes it >> possible for an MHI controller driver to communicate with others over >> those channels at suspend stage. While the resume callback is called >> in the early resume stage, for a similar reason. >> >> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30 >> >> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > Could you please confirm if this patch can be applied separately and won't cause > regression? Just searched the kernel and found another two drivers using IPCR channels, one is pci_epf_mhi_driver in drivers/pci/endpoint/functions/pci-epf-mhi.c and the other is mhi_pci_driver in drivers/bus/mhi/host/pci_generic.c. For pci_epf_mhi_driver, this is not impacted because the devices for those channels are attached to mhi_ep_bus_type while QRTR MHI driver attached to mhi_bus_type. For mhi_pci_driver, I am afraid there would be regressions caused by this patch. The regression is because when system suspends, mhi_pci_suspend() is called and then qcom_mhi_qrtr_pm_suspend_late() called, however the latter would fail because MHI is moved to M3 state by call mhi_pm_suspend() in mhi_pci_suspend(). To address this, there might be two options: the first is to move mhi_pci_suspend() to a late suspend stage which would be called after qcom_mhi_qrtr_pm_suspend_late(). and the second is to avoid whole QRTR suspend operation in such cases. I prefer the second one, because if MHI is going to suspend, instead of power down, it is pointless to unprepare MHI channels and re-prepare them after resume back. We can achieve this purpose by adding a status_cb() to QRTR MHI driver which would be called when MHI goes to low power mode. And then QRTR MHI driver could decide not to suspend/resume if low power mode is notified. Your thoughts? > > - Mani > >> --- >> net/qrtr/mhi.c | 27 +++++++++++++++++++++++++++ >> 1 file changed, 27 insertions(+) >> >> diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c >> index 9ced13c0627a..e96b82a6742c 100644 >> --- a/net/qrtr/mhi.c >> +++ b/net/qrtr/mhi.c >> @@ -118,6 +118,32 @@ static const struct mhi_device_id qcom_mhi_qrtr_id_table[] = { >> }; >> MODULE_DEVICE_TABLE(mhi, qcom_mhi_qrtr_id_table); >> >> +static int __maybe_unused qcom_mhi_qrtr_pm_suspend_late(struct device *dev) >> +{ >> + struct mhi_device *mhi_dev = container_of(dev, struct mhi_device, dev); >> + >> + mhi_unprepare_from_transfer(mhi_dev); >> + >> + return 0; >> +} >> + >> +static int __maybe_unused qcom_mhi_qrtr_pm_resume_early(struct device *dev) >> +{ >> + struct mhi_device *mhi_dev = container_of(dev, struct mhi_device, dev); >> + int rc; >> + >> + rc = mhi_prepare_for_transfer_autoqueue(mhi_dev); >> + if (rc) >> + dev_err(dev, "failed to prepare for autoqueue transfer %d\n", rc); >> + >> + return rc; >> +} >> + >> +static const struct dev_pm_ops qcom_mhi_qrtr_pm_ops = { >> + SET_LATE_SYSTEM_SLEEP_PM_OPS(qcom_mhi_qrtr_pm_suspend_late, >> + qcom_mhi_qrtr_pm_resume_early) >> +}; >> + >> static struct mhi_driver qcom_mhi_qrtr_driver = { >> .probe = qcom_mhi_qrtr_probe, >> .remove = qcom_mhi_qrtr_remove, >> @@ -126,6 +152,7 @@ static struct mhi_driver qcom_mhi_qrtr_driver = { >> .id_table = qcom_mhi_qrtr_id_table, >> .driver = { >> .name = "qcom_mhi_qrtr", >> + .pm = &qcom_mhi_qrtr_pm_ops, >> }, >> }; >> >> -- >> 2.25.1 >> >
On Tue, Feb 27, 2024 at 04:39:41PM +0800, Baochen Qiang wrote: > > > On 2/27/2024 3:15 PM, Manivannan Sadhasivam wrote: > > On Tue, Feb 27, 2024 at 02:36:12PM +0800, Baochen Qiang wrote: > > > MHI devices may not be destroyed during suspend/hibernation, so need > > > to unprepare/prepare MHI channels throughout the transition, this is > > > done by adding suspend/resume callbacks. > > > > > > The suspend callback is called in the late suspend stage, this means > > > MHI channels are still alive at suspend stage, and that makes it > > > possible for an MHI controller driver to communicate with others over > > > those channels at suspend stage. While the resume callback is called > > > in the early resume stage, for a similar reason. > > > > > > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30 > > > > > > Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com> > > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > Could you please confirm if this patch can be applied separately and won't cause > > regression? > Just searched the kernel and found another two drivers using IPCR channels, > one is pci_epf_mhi_driver in drivers/pci/endpoint/functions/pci-epf-mhi.c > and the other is mhi_pci_driver in drivers/bus/mhi/host/pci_generic.c. > > For pci_epf_mhi_driver, this is not impacted because the devices for those > channels are attached to mhi_ep_bus_type while QRTR MHI driver attached to > mhi_bus_type. > > For mhi_pci_driver, I am afraid there would be regressions caused by this > patch. The regression is because when system suspends, mhi_pci_suspend() is > called and then qcom_mhi_qrtr_pm_suspend_late() called, however the latter > would fail because MHI is moved to M3 state by call mhi_pm_suspend() in > mhi_pci_suspend(). To address this, there might be two options: the first is > to move mhi_pci_suspend() to a late suspend stage which would be called > after qcom_mhi_qrtr_pm_suspend_late(). and the second is to avoid whole QRTR > suspend operation in such cases. I prefer the second one, because if MHI is > going to suspend, instead of power down, it is pointless to unprepare MHI > channels and re-prepare them after resume back. We can achieve this purpose > by adding a status_cb() to QRTR MHI driver which would be called when MHI > goes to low power mode. And then QRTR MHI driver could decide not to > suspend/resume if low power mode is notified. > > Your thoughts? > I'd rather query the MHI state of the device in suspend/resume callback using mhi_get_mhi_state(mhi_dev->mhi_cntrl) and if the device is found to be in M3 (suspend) state, then I'd skip prepare/unprepare part. Because this makes it clear that, if the device is in suspend state, then no need for the client drivers to unprepare/prepare the channels. - Mani
On 2/27/2024 5:56 PM, Manivannan Sadhasivam wrote: > On Tue, Feb 27, 2024 at 04:39:41PM +0800, Baochen Qiang wrote: >> >> >> On 2/27/2024 3:15 PM, Manivannan Sadhasivam wrote: >>> On Tue, Feb 27, 2024 at 02:36:12PM +0800, Baochen Qiang wrote: >>>> MHI devices may not be destroyed during suspend/hibernation, so need >>>> to unprepare/prepare MHI channels throughout the transition, this is >>>> done by adding suspend/resume callbacks. >>>> >>>> The suspend callback is called in the late suspend stage, this means >>>> MHI channels are still alive at suspend stage, and that makes it >>>> possible for an MHI controller driver to communicate with others over >>>> those channels at suspend stage. While the resume callback is called >>>> in the early resume stage, for a similar reason. >>>> >>>> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30 >>>> >>>> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com> >>> >>> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> >>> >>> Could you please confirm if this patch can be applied separately and won't cause >>> regression? >> Just searched the kernel and found another two drivers using IPCR channels, >> one is pci_epf_mhi_driver in drivers/pci/endpoint/functions/pci-epf-mhi.c >> and the other is mhi_pci_driver in drivers/bus/mhi/host/pci_generic.c. >> >> For pci_epf_mhi_driver, this is not impacted because the devices for those >> channels are attached to mhi_ep_bus_type while QRTR MHI driver attached to >> mhi_bus_type. >> >> For mhi_pci_driver, I am afraid there would be regressions caused by this >> patch. The regression is because when system suspends, mhi_pci_suspend() is >> called and then qcom_mhi_qrtr_pm_suspend_late() called, however the latter >> would fail because MHI is moved to M3 state by call mhi_pm_suspend() in >> mhi_pci_suspend(). To address this, there might be two options: the first is >> to move mhi_pci_suspend() to a late suspend stage which would be called >> after qcom_mhi_qrtr_pm_suspend_late(). and the second is to avoid whole QRTR >> suspend operation in such cases. I prefer the second one, because if MHI is >> going to suspend, instead of power down, it is pointless to unprepare MHI >> channels and re-prepare them after resume back. We can achieve this purpose >> by adding a status_cb() to QRTR MHI driver which would be called when MHI >> goes to low power mode. And then QRTR MHI driver could decide not to >> suspend/resume if low power mode is notified. >> >> Your thoughts? >> > > I'd rather query the MHI state of the device in suspend/resume callback using > mhi_get_mhi_state(mhi_dev->mhi_cntrl) and if the device is found to be in M3 > (suspend) state, then I'd skip prepare/unprepare part. > > Because this makes it clear that, if the device is in suspend state, then no > need for the client drivers to unprepare/prepare the channels. > Sounds reasonable, will do accordingly in v3. > - Mani >
diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c index 9ced13c0627a..e96b82a6742c 100644 --- a/net/qrtr/mhi.c +++ b/net/qrtr/mhi.c @@ -118,6 +118,32 @@ static const struct mhi_device_id qcom_mhi_qrtr_id_table[] = { }; MODULE_DEVICE_TABLE(mhi, qcom_mhi_qrtr_id_table); +static int __maybe_unused qcom_mhi_qrtr_pm_suspend_late(struct device *dev) +{ + struct mhi_device *mhi_dev = container_of(dev, struct mhi_device, dev); + + mhi_unprepare_from_transfer(mhi_dev); + + return 0; +} + +static int __maybe_unused qcom_mhi_qrtr_pm_resume_early(struct device *dev) +{ + struct mhi_device *mhi_dev = container_of(dev, struct mhi_device, dev); + int rc; + + rc = mhi_prepare_for_transfer_autoqueue(mhi_dev); + if (rc) + dev_err(dev, "failed to prepare for autoqueue transfer %d\n", rc); + + return rc; +} + +static const struct dev_pm_ops qcom_mhi_qrtr_pm_ops = { + SET_LATE_SYSTEM_SLEEP_PM_OPS(qcom_mhi_qrtr_pm_suspend_late, + qcom_mhi_qrtr_pm_resume_early) +}; + static struct mhi_driver qcom_mhi_qrtr_driver = { .probe = qcom_mhi_qrtr_probe, .remove = qcom_mhi_qrtr_remove, @@ -126,6 +152,7 @@ static struct mhi_driver qcom_mhi_qrtr_driver = { .id_table = qcom_mhi_qrtr_id_table, .driver = { .name = "qcom_mhi_qrtr", + .pm = &qcom_mhi_qrtr_pm_ops, }, };
MHI devices may not be destroyed during suspend/hibernation, so need to unprepare/prepare MHI channels throughout the transition, this is done by adding suspend/resume callbacks. The suspend callback is called in the late suspend stage, this means MHI channels are still alive at suspend stage, and that makes it possible for an MHI controller driver to communicate with others over those channels at suspend stage. While the resume callback is called in the early resume stage, for a similar reason. Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.30 Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com> --- net/qrtr/mhi.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)