| Message ID | 20240221030026.10553-3-quic_bqiang@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: ath11k: hibernation support | expand |
On 2/20/2024 7:00 PM, Baochen Qiang wrote: > MHI devices may not be destroyed during suspend/hibernation, so need > to unprepare/prepare MHI channels throughout the transition. > > The RFC version adds new API to MHI stack with which an MHI controller > driver can do unprepare/prepare directly by itself, see > > https://patchwork.kernel.org/project/linux-wireless/patch/20231127162022.518834-3-kvalo@kernel.org/ > > Although it works well Mani pointed out that the design is not good > because MHI channels are managed by MHI client driver thus should not > be touched by others. See the discussion > > https://lore.kernel.org/mhi/20231127162022.518834-1-kvalo@kernel.org/ > > This version changes to add suspend/resume callbacks to achieve the > same purpose. 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 | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c > index 9ced13c0627a..b54a6c2113e9 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 qcom_mhi_qrtr_pm_suspend_late(struct device *dev) Don't your new functions also need to be annotated as __maybe_unused? > +{ > + struct mhi_device *mhi_dev = container_of(dev, struct mhi_device, dev); > + > + mhi_unprepare_from_transfer(mhi_dev); > + > + return 0; > +} > + > +static int 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 __maybe_unused qcom_mhi_qrtr_pm_ops = { this does not need to be __maybe_unused, see below > + 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,9 @@ static struct mhi_driver qcom_mhi_qrtr_driver = { > .id_table = qcom_mhi_qrtr_id_table, > .driver = { > .name = "qcom_mhi_qrtr", > +#ifdef CONFIG_PM conditional compilation isn't necessary here since the 'pm' member is always present > + .pm = &qcom_mhi_qrtr_pm_ops, > +#endif > }, > }; >
On 2/22/2024 2:24 AM, Jeff Johnson wrote: > On 2/20/2024 7:00 PM, Baochen Qiang wrote: >> MHI devices may not be destroyed during suspend/hibernation, so need >> to unprepare/prepare MHI channels throughout the transition. >> >> The RFC version adds new API to MHI stack with which an MHI controller >> driver can do unprepare/prepare directly by itself, see >> >> https://patchwork.kernel.org/project/linux-wireless/patch/20231127162022.518834-3-kvalo@kernel.org/ >> >> Although it works well Mani pointed out that the design is not good >> because MHI channels are managed by MHI client driver thus should not >> be touched by others. See the discussion >> >> https://lore.kernel.org/mhi/20231127162022.518834-1-kvalo@kernel.org/ >> >> This version changes to add suspend/resume callbacks to achieve the >> same purpose. 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 | 29 +++++++++++++++++++++++++++++ >> 1 file changed, 29 insertions(+) >> >> diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c >> index 9ced13c0627a..b54a6c2113e9 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 qcom_mhi_qrtr_pm_suspend_late(struct device *dev) > > Don't your new functions also need to be annotated as __maybe_unused? OK, will add __maybe_unused in next version. > >> +{ >> + struct mhi_device *mhi_dev = container_of(dev, struct mhi_device, dev); >> + >> + mhi_unprepare_from_transfer(mhi_dev); >> + >> + return 0; >> +} >> + >> +static int 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 __maybe_unused qcom_mhi_qrtr_pm_ops = { > > this does not need to be __maybe_unused, see below > >> + 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,9 @@ static struct mhi_driver qcom_mhi_qrtr_driver = { >> .id_table = qcom_mhi_qrtr_id_table, >> .driver = { >> .name = "qcom_mhi_qrtr", >> +#ifdef CONFIG_PM > > conditional compilation isn't necessary here since the 'pm' member is > always present Sure, will remove that in next version. > >> + .pm = &qcom_mhi_qrtr_pm_ops, >> +#endif >> }, >> }; >> >
On Wed, Feb 21, 2024 at 11:00:25AM +0800, Baochen Qiang wrote: > MHI devices may not be destroyed during suspend/hibernation, so need > to unprepare/prepare MHI channels throughout the transition. > Again, below content should go to the comment section, not in the commit message. Please add only relevant content in the commit message. - Mani > The RFC version adds new API to MHI stack with which an MHI controller > driver can do unprepare/prepare directly by itself, see > > https://patchwork.kernel.org/project/linux-wireless/patch/20231127162022.518834-3-kvalo@kernel.org/ > > Although it works well Mani pointed out that the design is not good > because MHI channels are managed by MHI client driver thus should not > be touched by others. See the discussion > > https://lore.kernel.org/mhi/20231127162022.518834-1-kvalo@kernel.org/ > > This version changes to add suspend/resume callbacks to achieve the > same purpose. 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 | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c > index 9ced13c0627a..b54a6c2113e9 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 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 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 __maybe_unused 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,9 @@ static struct mhi_driver qcom_mhi_qrtr_driver = { > .id_table = qcom_mhi_qrtr_id_table, > .driver = { > .name = "qcom_mhi_qrtr", > +#ifdef CONFIG_PM > + .pm = &qcom_mhi_qrtr_pm_ops, > +#endif > }, > }; > > -- > 2.25.1 > >
diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c index 9ced13c0627a..b54a6c2113e9 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 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 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 __maybe_unused 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,9 @@ static struct mhi_driver qcom_mhi_qrtr_driver = { .id_table = qcom_mhi_qrtr_id_table, .driver = { .name = "qcom_mhi_qrtr", +#ifdef CONFIG_PM + .pm = &qcom_mhi_qrtr_pm_ops, +#endif }, };
MHI devices may not be destroyed during suspend/hibernation, so need to unprepare/prepare MHI channels throughout the transition. The RFC version adds new API to MHI stack with which an MHI controller driver can do unprepare/prepare directly by itself, see https://patchwork.kernel.org/project/linux-wireless/patch/20231127162022.518834-3-kvalo@kernel.org/ Although it works well Mani pointed out that the design is not good because MHI channels are managed by MHI client driver thus should not be touched by others. See the discussion https://lore.kernel.org/mhi/20231127162022.518834-1-kvalo@kernel.org/ This version changes to add suspend/resume callbacks to achieve the same purpose. 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 | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)