| Message ID | 1492769288-7474-3-git-send-email-adrian.hunter@intel.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On 21 April 2017 at 12:08, Adrian Hunter <adrian.hunter@intel.com> wrote: > The SDIO card state might be being preserved during hibernation, for > example a SDIO wifi card supporting WOWLAN. That state will be lost if an > SDIO reset is done. One way to avoid that would be to build mmc core as a > module and simply not load it until after attempting to restore the > hibernation image. However that won't work if the hibernation image is > stored on eMMC which, of course, requires mmc core. I don't follow here. Are you saying the SDIO card is kept powered in hibernation, as to be able to support WOWLAN, right? Then, it feels plain wrong the mmc_rescan() tries to re-initialize it. That should never happen, unless something is broken of course. Kind regards Uffe > > It is assumed on such systems that the platform will power cycle the SDIO > card or not as necessary so that the SDIO reset is not needed. Add a > capability flag to reflect that and use it to skip the SDIO reset at scan > time. > > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/core/core.c | 6 +++++- > include/linux/mmc/host.h | 1 + > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index 6987976252ad..178e23bf0c30 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -2639,8 +2639,12 @@ static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq) > * if the card is being re-initialized, just send it. CMD52 > * should be ignored by SD/eMMC cards. > * Skip it if we already know that we do not support SDIO commands > + * Also skip it if we know this host controller has a SDIO card that > + * needs to be able to restore from hibernation without losing the card > + * state e.g. an SDIO wifi card supporting WOWLAN. > */ > - if (!(host->caps2 & MMC_CAP2_NO_SDIO)) > + if (!(host->caps2 & MMC_CAP2_NO_SDIO) && > + !(host->caps2 & MMC_CAP2_NO_SDIO_RESET)) > sdio_reset(host); > > mmc_go_idle(host); > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h > index 78c544e296cd..187a7ba41364 100644 > --- a/include/linux/mmc/host.h > +++ b/include/linux/mmc/host.h > @@ -281,6 +281,7 @@ struct mmc_host { > u32 caps2; /* More host capabilities */ > > #define MMC_CAP2_BOOTPART_NOACC (1 << 0) /* Boot partition no access */ > +#define MMC_CAP2_NO_SDIO_RESET (1 << 1) /* Do not SDIO reset at scan */ > #define MMC_CAP2_FULL_PWR_CYCLE (1 << 2) /* Can do full power cycle */ > #define MMC_CAP2_HS200_1_8V_SDR (1 << 5) /* can support */ > #define MMC_CAP2_HS200_1_2V_SDR (1 << 6) /* can support */ > -- > 1.9.1 >
On 24/04/17 23:33, Ulf Hansson wrote: > On 21 April 2017 at 12:08, Adrian Hunter <adrian.hunter@intel.com> wrote: >> The SDIO card state might be being preserved during hibernation, for >> example a SDIO wifi card supporting WOWLAN. That state will be lost if an >> SDIO reset is done. One way to avoid that would be to build mmc core as a >> module and simply not load it until after attempting to restore the >> hibernation image. However that won't work if the hibernation image is >> stored on eMMC which, of course, requires mmc core. > > I don't follow here. Are you saying the SDIO card is kept powered in > hibernation, as to be able to support WOWLAN, right? Yes > > Then, it feels plain wrong the mmc_rescan() tries to re-initialize it. > That should never happen, unless something is broken of course. The thing to note about hibernation is that there is a regular boot in between saving the hibernation image and restoring it again. At boot time, the kernel knows almost nothing about whether there is a hibernation image and whether or not it will be restored. Consequently it becomes difficult to avoid mmc_rescan(). As mentioned above, we need mmc_rescan() to initialize the eMMC so that the hibernation image can be read. > > Kind regards > Uffe > >> >> It is assumed on such systems that the platform will power cycle the SDIO >> card or not as necessary so that the SDIO reset is not needed. Add a >> capability flag to reflect that and use it to skip the SDIO reset at scan >> time. >> >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >> --- >> drivers/mmc/core/core.c | 6 +++++- >> include/linux/mmc/host.h | 1 + >> 2 files changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >> index 6987976252ad..178e23bf0c30 100644 >> --- a/drivers/mmc/core/core.c >> +++ b/drivers/mmc/core/core.c >> @@ -2639,8 +2639,12 @@ static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq) >> * if the card is being re-initialized, just send it. CMD52 >> * should be ignored by SD/eMMC cards. >> * Skip it if we already know that we do not support SDIO commands >> + * Also skip it if we know this host controller has a SDIO card that >> + * needs to be able to restore from hibernation without losing the card >> + * state e.g. an SDIO wifi card supporting WOWLAN. >> */ >> - if (!(host->caps2 & MMC_CAP2_NO_SDIO)) >> + if (!(host->caps2 & MMC_CAP2_NO_SDIO) && >> + !(host->caps2 & MMC_CAP2_NO_SDIO_RESET)) >> sdio_reset(host); >> >> mmc_go_idle(host); >> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h >> index 78c544e296cd..187a7ba41364 100644 >> --- a/include/linux/mmc/host.h >> +++ b/include/linux/mmc/host.h >> @@ -281,6 +281,7 @@ struct mmc_host { >> u32 caps2; /* More host capabilities */ >> >> #define MMC_CAP2_BOOTPART_NOACC (1 << 0) /* Boot partition no access */ >> +#define MMC_CAP2_NO_SDIO_RESET (1 << 1) /* Do not SDIO reset at scan */ >> #define MMC_CAP2_FULL_PWR_CYCLE (1 << 2) /* Can do full power cycle */ >> #define MMC_CAP2_HS200_1_8V_SDR (1 << 5) /* can support */ >> #define MMC_CAP2_HS200_1_2V_SDR (1 << 6) /* can support */ >> -- >> 1.9.1 >> >
+Grygorii Strashko On 25 April 2017 at 08:21, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 24/04/17 23:33, Ulf Hansson wrote: >> On 21 April 2017 at 12:08, Adrian Hunter <adrian.hunter@intel.com> wrote: >>> The SDIO card state might be being preserved during hibernation, for >>> example a SDIO wifi card supporting WOWLAN. That state will be lost if an >>> SDIO reset is done. One way to avoid that would be to build mmc core as a >>> module and simply not load it until after attempting to restore the >>> hibernation image. However that won't work if the hibernation image is >>> stored on eMMC which, of course, requires mmc core. >> >> I don't follow here. Are you saying the SDIO card is kept powered in >> hibernation, as to be able to support WOWLAN, right? > > Yes Okay, makes sense! > >> >> Then, it feels plain wrong the mmc_rescan() tries to re-initialize it. >> That should never happen, unless something is broken of course. > > The thing to note about hibernation is that there is a regular boot in > between saving the hibernation image and restoring it again. At boot time, > the kernel knows almost nothing about whether there is a hibernation image > and whether or not it will be restored. Consequently it becomes difficult > to avoid mmc_rescan(). As mentioned above, we need mmc_rescan() to > initialize the eMMC so that the hibernation image can be read. What's wrong with using the hibernation callbacks in the struct dev_pm_ops? We already do this. To manage the eMMC/SD/SDIO card device, we do this in /drivers/mmc/core/bus.c: static const struct dev_pm_ops mmc_bus_pm_ops = { SET_RUNTIME_PM_OPS(mmc_runtime_suspend, mmc_runtime_resume, NULL) SET_SYSTEM_SLEEP_PM_OPS(mmc_bus_suspend, mmc_bus_resume) }; To mange the sdio func device, we do this in /drivers/mmc/core/sdio_bus.c static const struct dev_pm_ops sdio_bus_pm_ops = { SET_SYSTEM_SLEEP_PM_OPS(pm_generic_suspend, pm_generic_resume) SET_RUNTIME_PM_OPS( pm_generic_runtime_suspend, pm_generic_runtime_resume, NULL ) }; In other words, we are re-using the same callbacks for system PM suspend as for system PM hibernation. I do imagine there are something broken for SDIO in this path, mainly because I haven't heard much from people using it. However, for (e)MMC/SD card point of view, restoring from hibernation should work. I remember that Grygorii Strashko has been working on this, perhaps he can confirm some of his observations. Kind regards Uffe
On 25/04/17 10:52, Ulf Hansson wrote: > +Grygorii Strashko > > On 25 April 2017 at 08:21, Adrian Hunter <adrian.hunter@intel.com> wrote: >> On 24/04/17 23:33, Ulf Hansson wrote: >>> On 21 April 2017 at 12:08, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>> The SDIO card state might be being preserved during hibernation, for >>>> example a SDIO wifi card supporting WOWLAN. That state will be lost if an >>>> SDIO reset is done. One way to avoid that would be to build mmc core as a >>>> module and simply not load it until after attempting to restore the >>>> hibernation image. However that won't work if the hibernation image is >>>> stored on eMMC which, of course, requires mmc core. >>> >>> I don't follow here. Are you saying the SDIO card is kept powered in >>> hibernation, as to be able to support WOWLAN, right? >> >> Yes > > Okay, makes sense! > >> >>> >>> Then, it feels plain wrong the mmc_rescan() tries to re-initialize it. >>> That should never happen, unless something is broken of course. >> >> The thing to note about hibernation is that there is a regular boot in >> between saving the hibernation image and restoring it again. At boot time, >> the kernel knows almost nothing about whether there is a hibernation image >> and whether or not it will be restored. Consequently it becomes difficult >> to avoid mmc_rescan(). As mentioned above, we need mmc_rescan() to >> initialize the eMMC so that the hibernation image can be read. > > What's wrong with using the hibernation callbacks in the struct > dev_pm_ops? We already do this. Here is the scenario. The kernel has just booted. User space wants to try to restore a hibernation image, if there is one. So user space loads the mmc core because the hibernation image is on eMMC. Mmc core does an SDIO reset on the SDIO card and the state is lost. It has little to do with pm callbacks AFAICS. > > To manage the eMMC/SD/SDIO card device, we do this in /drivers/mmc/core/bus.c: > > static const struct dev_pm_ops mmc_bus_pm_ops = { > SET_RUNTIME_PM_OPS(mmc_runtime_suspend, mmc_runtime_resume, NULL) > SET_SYSTEM_SLEEP_PM_OPS(mmc_bus_suspend, mmc_bus_resume) > }; > > To mange the sdio func device, we do this in /drivers/mmc/core/sdio_bus.c > > static const struct dev_pm_ops sdio_bus_pm_ops = { > SET_SYSTEM_SLEEP_PM_OPS(pm_generic_suspend, pm_generic_resume) > SET_RUNTIME_PM_OPS( > pm_generic_runtime_suspend, > pm_generic_runtime_resume, > NULL > ) > }; > > In other words, we are re-using the same callbacks for system PM > suspend as for system PM hibernation. > > I do imagine there are something broken for SDIO in this path, mainly > because I haven't heard much from people using it. > > However, for (e)MMC/SD card point of view, restoring from hibernation > should work. I remember that Grygorii Strashko has been working on > this, perhaps he can confirm some of his observations. > > Kind regards > Uffe >
On 25 April 2017 at 09:57, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 25/04/17 10:52, Ulf Hansson wrote: >> +Grygorii Strashko >> >> On 25 April 2017 at 08:21, Adrian Hunter <adrian.hunter@intel.com> wrote: >>> On 24/04/17 23:33, Ulf Hansson wrote: >>>> On 21 April 2017 at 12:08, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>> The SDIO card state might be being preserved during hibernation, for >>>>> example a SDIO wifi card supporting WOWLAN. That state will be lost if an >>>>> SDIO reset is done. One way to avoid that would be to build mmc core as a >>>>> module and simply not load it until after attempting to restore the >>>>> hibernation image. However that won't work if the hibernation image is >>>>> stored on eMMC which, of course, requires mmc core. >>>> >>>> I don't follow here. Are you saying the SDIO card is kept powered in >>>> hibernation, as to be able to support WOWLAN, right? >>> >>> Yes >> >> Okay, makes sense! >> >>> >>>> >>>> Then, it feels plain wrong the mmc_rescan() tries to re-initialize it. >>>> That should never happen, unless something is broken of course. >>> >>> The thing to note about hibernation is that there is a regular boot in >>> between saving the hibernation image and restoring it again. At boot time, >>> the kernel knows almost nothing about whether there is a hibernation image >>> and whether or not it will be restored. Consequently it becomes difficult >>> to avoid mmc_rescan(). As mentioned above, we need mmc_rescan() to >>> initialize the eMMC so that the hibernation image can be read. >> >> What's wrong with using the hibernation callbacks in the struct >> dev_pm_ops? We already do this. > > Here is the scenario. The kernel has just booted. User space wants to try > to restore a hibernation image, if there is one. So user space loads the > mmc core because the hibernation image is on eMMC. Mmc core does an SDIO > reset on the SDIO card and the state is lost. It has little to do with pm > callbacks AFAICS. Ah, now I see what you mean. I thought the problem was during the actual restoring of the hibernation image. Alright, when a boot is triggered by WOWLAN , you want to avoid sending the reset command for the SDIO card before the re-initialization of the SDIO card starts. The problem with this approach is that you can't differentiate between a cold boot and a boot triggered by WOWLAN, right!? In other words, in some cases the reset command may be needed while in other it won't. Maybe you can elaborate more on what exactly what the problem is with sending the reset command when the boot is triggered from WOWLAN? Yes, the SDIO card loses its context, but how is that a problem? [...] Kind regards Uffe
On 25/04/17 13:46, Ulf Hansson wrote: > On 25 April 2017 at 09:57, Adrian Hunter <adrian.hunter@intel.com> wrote: >> On 25/04/17 10:52, Ulf Hansson wrote: >>> +Grygorii Strashko >>> >>> On 25 April 2017 at 08:21, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>> On 24/04/17 23:33, Ulf Hansson wrote: >>>>> On 21 April 2017 at 12:08, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>>> The SDIO card state might be being preserved during hibernation, for >>>>>> example a SDIO wifi card supporting WOWLAN. That state will be lost if an >>>>>> SDIO reset is done. One way to avoid that would be to build mmc core as a >>>>>> module and simply not load it until after attempting to restore the >>>>>> hibernation image. However that won't work if the hibernation image is >>>>>> stored on eMMC which, of course, requires mmc core. >>>>> >>>>> I don't follow here. Are you saying the SDIO card is kept powered in >>>>> hibernation, as to be able to support WOWLAN, right? >>>> >>>> Yes >>> >>> Okay, makes sense! >>> >>>> >>>>> >>>>> Then, it feels plain wrong the mmc_rescan() tries to re-initialize it. >>>>> That should never happen, unless something is broken of course. >>>> >>>> The thing to note about hibernation is that there is a regular boot in >>>> between saving the hibernation image and restoring it again. At boot time, >>>> the kernel knows almost nothing about whether there is a hibernation image >>>> and whether or not it will be restored. Consequently it becomes difficult >>>> to avoid mmc_rescan(). As mentioned above, we need mmc_rescan() to >>>> initialize the eMMC so that the hibernation image can be read. >>> >>> What's wrong with using the hibernation callbacks in the struct >>> dev_pm_ops? We already do this. >> >> Here is the scenario. The kernel has just booted. User space wants to try >> to restore a hibernation image, if there is one. So user space loads the >> mmc core because the hibernation image is on eMMC. Mmc core does an SDIO >> reset on the SDIO card and the state is lost. It has little to do with pm >> callbacks AFAICS. > > Ah, now I see what you mean. I thought the problem was during the > actual restoring of the hibernation image. > > Alright, when a boot is triggered by WOWLAN , you want to avoid > sending the reset command for the SDIO card before the > re-initialization of the SDIO card starts. > > The problem with this approach is that you can't differentiate between > a cold boot and a boot triggered by WOWLAN, right!? In other words, in > some cases the reset command may be needed while in other it won't. SDIO reset is only needed if the card has not been power-cycled. The assumption is that the platform takes care of that when needed. e.g. when rebooting instead of going to S4. > > Maybe you can elaborate more on what exactly what the problem is with > sending the reset command when the boot is triggered from WOWLAN? Yes, > the SDIO card loses its context, but how is that a problem? The wifi driver expects to find the function in an initialized state. Otherwise it would have to re-enable the function and re-do the function-specific initialization. I don't know if there are other consequences. Presumably it will have lost any information about the nature of the wake-up trigger.
On 25 April 2017 at 13:20, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 25/04/17 13:46, Ulf Hansson wrote: >> On 25 April 2017 at 09:57, Adrian Hunter <adrian.hunter@intel.com> wrote: >>> On 25/04/17 10:52, Ulf Hansson wrote: >>>> +Grygorii Strashko >>>> >>>> On 25 April 2017 at 08:21, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>> On 24/04/17 23:33, Ulf Hansson wrote: >>>>>> On 21 April 2017 at 12:08, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>>>> The SDIO card state might be being preserved during hibernation, for >>>>>>> example a SDIO wifi card supporting WOWLAN. That state will be lost if an >>>>>>> SDIO reset is done. One way to avoid that would be to build mmc core as a >>>>>>> module and simply not load it until after attempting to restore the >>>>>>> hibernation image. However that won't work if the hibernation image is >>>>>>> stored on eMMC which, of course, requires mmc core. >>>>>> >>>>>> I don't follow here. Are you saying the SDIO card is kept powered in >>>>>> hibernation, as to be able to support WOWLAN, right? >>>>> >>>>> Yes >>>> >>>> Okay, makes sense! >>>> >>>>> >>>>>> >>>>>> Then, it feels plain wrong the mmc_rescan() tries to re-initialize it. >>>>>> That should never happen, unless something is broken of course. >>>>> >>>>> The thing to note about hibernation is that there is a regular boot in >>>>> between saving the hibernation image and restoring it again. At boot time, >>>>> the kernel knows almost nothing about whether there is a hibernation image >>>>> and whether or not it will be restored. Consequently it becomes difficult >>>>> to avoid mmc_rescan(). As mentioned above, we need mmc_rescan() to >>>>> initialize the eMMC so that the hibernation image can be read. >>>> >>>> What's wrong with using the hibernation callbacks in the struct >>>> dev_pm_ops? We already do this. >>> >>> Here is the scenario. The kernel has just booted. User space wants to try >>> to restore a hibernation image, if there is one. So user space loads the >>> mmc core because the hibernation image is on eMMC. Mmc core does an SDIO >>> reset on the SDIO card and the state is lost. It has little to do with pm >>> callbacks AFAICS. >> >> Ah, now I see what you mean. I thought the problem was during the >> actual restoring of the hibernation image. >> >> Alright, when a boot is triggered by WOWLAN , you want to avoid >> sending the reset command for the SDIO card before the >> re-initialization of the SDIO card starts. >> >> The problem with this approach is that you can't differentiate between >> a cold boot and a boot triggered by WOWLAN, right!? In other words, in >> some cases the reset command may be needed while in other it won't. > > SDIO reset is only needed if the card has not been power-cycled. The > assumption is that the platform takes care of that when needed. e.g. when > rebooting instead of going to S4. > >> >> Maybe you can elaborate more on what exactly what the problem is with >> sending the reset command when the boot is triggered from WOWLAN? Yes, >> the SDIO card loses its context, but how is that a problem? > > The wifi driver expects to find the function in an initialized state. So how does the the wifi driver distinguish this a boot caused by WOWLAN - and not a cold boot? > Otherwise it would have to re-enable the function and re-do the > function-specific initialization. I don't know if there are other At boot the SDIO func driver becomes probed, when the mmc core finds and SDIO card and then registers a func device for it. Are you saying that the SDIO func driver can take shortcuts when enabling the func device, when the boot is trigger from WOWLAN? > consequences. Presumably it will have lost any information about the nature > of the wake-up trigger. How does that matter? Kind regards Uffe
On 25/04/17 15:24, Ulf Hansson wrote: > On 25 April 2017 at 13:20, Adrian Hunter <adrian.hunter@intel.com> wrote: >> On 25/04/17 13:46, Ulf Hansson wrote: >>> On 25 April 2017 at 09:57, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>> On 25/04/17 10:52, Ulf Hansson wrote: >>>>> +Grygorii Strashko >>>>> >>>>> On 25 April 2017 at 08:21, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>>> On 24/04/17 23:33, Ulf Hansson wrote: >>>>>>> On 21 April 2017 at 12:08, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>>>>> The SDIO card state might be being preserved during hibernation, for >>>>>>>> example a SDIO wifi card supporting WOWLAN. That state will be lost if an >>>>>>>> SDIO reset is done. One way to avoid that would be to build mmc core as a >>>>>>>> module and simply not load it until after attempting to restore the >>>>>>>> hibernation image. However that won't work if the hibernation image is >>>>>>>> stored on eMMC which, of course, requires mmc core. >>>>>>> >>>>>>> I don't follow here. Are you saying the SDIO card is kept powered in >>>>>>> hibernation, as to be able to support WOWLAN, right? >>>>>> >>>>>> Yes >>>>> >>>>> Okay, makes sense! >>>>> >>>>>> >>>>>>> >>>>>>> Then, it feels plain wrong the mmc_rescan() tries to re-initialize it. >>>>>>> That should never happen, unless something is broken of course. >>>>>> >>>>>> The thing to note about hibernation is that there is a regular boot in >>>>>> between saving the hibernation image and restoring it again. At boot time, >>>>>> the kernel knows almost nothing about whether there is a hibernation image >>>>>> and whether or not it will be restored. Consequently it becomes difficult >>>>>> to avoid mmc_rescan(). As mentioned above, we need mmc_rescan() to >>>>>> initialize the eMMC so that the hibernation image can be read. >>>>> >>>>> What's wrong with using the hibernation callbacks in the struct >>>>> dev_pm_ops? We already do this. >>>> >>>> Here is the scenario. The kernel has just booted. User space wants to try >>>> to restore a hibernation image, if there is one. So user space loads the >>>> mmc core because the hibernation image is on eMMC. Mmc core does an SDIO >>>> reset on the SDIO card and the state is lost. It has little to do with pm >>>> callbacks AFAICS. >>> >>> Ah, now I see what you mean. I thought the problem was during the >>> actual restoring of the hibernation image. >>> >>> Alright, when a boot is triggered by WOWLAN , you want to avoid >>> sending the reset command for the SDIO card before the >>> re-initialization of the SDIO card starts. >>> >>> The problem with this approach is that you can't differentiate between >>> a cold boot and a boot triggered by WOWLAN, right!? In other words, in >>> some cases the reset command may be needed while in other it won't. >> >> SDIO reset is only needed if the card has not been power-cycled. The >> assumption is that the platform takes care of that when needed. e.g. when >> rebooting instead of going to S4. >> >>> >>> Maybe you can elaborate more on what exactly what the problem is with >>> sending the reset command when the boot is triggered from WOWLAN? Yes, >>> the SDIO card loses its context, but how is that a problem? >> >> The wifi driver expects to find the function in an initialized state. > > So how does the the wifi driver distinguish this a boot caused by > WOWLAN - and not a cold boot? It doesn't have to. It doesn't get loaded until after the attempt to restore the hibernation image. So in the hibernation case it has its ->restore() callback. If there is no hibernation image, it gets loaded and probed. AFAICT that is the normal way to stop drivers interfering with hibernation i.e. don't load them until after the attempt is made to restore the hibernation image. We could do that with mmc core too, but for the fact that the hibernation image is on eMMC. > >> Otherwise it would have to re-enable the function and re-do the >> function-specific initialization. I don't know if there are other > > At boot the SDIO func driver becomes probed, when the mmc core finds > and SDIO card and then registers a func device for it. Are you saying > that the SDIO func driver can take shortcuts when enabling the func > device, when the boot is trigger from WOWLAN? No. > >> consequences. Presumably it will have lost any information about the nature >> of the wake-up trigger. > > How does that matter? I don't know if it matters.
On 04/25/2017 07:45 AM, Adrian Hunter wrote: > On 25/04/17 15:24, Ulf Hansson wrote: >> On 25 April 2017 at 13:20, Adrian Hunter <adrian.hunter@intel.com> wrote: >>> On 25/04/17 13:46, Ulf Hansson wrote: >>>> On 25 April 2017 at 09:57, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>> On 25/04/17 10:52, Ulf Hansson wrote: >>>>>> +Grygorii Strashko >>>>>> >>>>>> On 25 April 2017 at 08:21, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>>>> On 24/04/17 23:33, Ulf Hansson wrote: >>>>>>>> On 21 April 2017 at 12:08, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>>>>>> The SDIO card state might be being preserved during hibernation, for >>>>>>>>> example a SDIO wifi card supporting WOWLAN. That state will be lost if an >>>>>>>>> SDIO reset is done. One way to avoid that would be to build mmc core as a >>>>>>>>> module and simply not load it until after attempting to restore the >>>>>>>>> hibernation image. However that won't work if the hibernation image is >>>>>>>>> stored on eMMC which, of course, requires mmc core. >>>>>>>> >>>>>>>> I don't follow here. Are you saying the SDIO card is kept powered in >>>>>>>> hibernation, as to be able to support WOWLAN, right? >>>>>>> >>>>>>> Yes >>>>>> >>>>>> Okay, makes sense! >>>>>> >>>>>>> >>>>>>>> >>>>>>>> Then, it feels plain wrong the mmc_rescan() tries to re-initialize it. >>>>>>>> That should never happen, unless something is broken of course. >>>>>>> >>>>>>> The thing to note about hibernation is that there is a regular boot in >>>>>>> between saving the hibernation image and restoring it again. At boot time, >>>>>>> the kernel knows almost nothing about whether there is a hibernation image >>>>>>> and whether or not it will be restored. Consequently it becomes difficult >>>>>>> to avoid mmc_rescan(). As mentioned above, we need mmc_rescan() to >>>>>>> initialize the eMMC so that the hibernation image can be read. >>>>>> >>>>>> What's wrong with using the hibernation callbacks in the struct >>>>>> dev_pm_ops? We already do this. >>>>> >>>>> Here is the scenario. The kernel has just booted. User space wants to try >>>>> to restore a hibernation image, if there is one. So user space loads the >>>>> mmc core because the hibernation image is on eMMC. Mmc core does an SDIO >>>>> reset on the SDIO card and the state is lost. It has little to do with pm >>>>> callbacks AFAICS. >>>> >>>> Ah, now I see what you mean. I thought the problem was during the >>>> actual restoring of the hibernation image. >>>> >>>> Alright, when a boot is triggered by WOWLAN , you want to avoid >>>> sending the reset command for the SDIO card before the >>>> re-initialization of the SDIO card starts. >>>> >>>> The problem with this approach is that you can't differentiate between >>>> a cold boot and a boot triggered by WOWLAN, right!? In other words, in >>>> some cases the reset command may be needed while in other it won't. >>> >>> SDIO reset is only needed if the card has not been power-cycled. The >>> assumption is that the platform takes care of that when needed. e.g. when >>> rebooting instead of going to S4. >>> >>>> >>>> Maybe you can elaborate more on what exactly what the problem is with >>>> sending the reset command when the boot is triggered from WOWLAN? Yes, >>>> the SDIO card loses its context, but how is that a problem? >>> >>> The wifi driver expects to find the function in an initialized state. >> >> So how does the the wifi driver distinguish this a boot caused by >> WOWLAN - and not a cold boot? > > It doesn't have to. It doesn't get loaded until after the attempt to > restore the hibernation image. So in the hibernation case it has its > ->restore() callback. For this case is a good question of how Kernel wifi card driver state will be synchronized HW state after restore from Hibernation (first of all wireless state - scan results, connection state and parameters)? Most probably wifi driver will need to perform mostly full re-initialization on restore, so... >If there is no hibernation image, it gets loaded and > probed. But in this case, as per you patch, there are will be no SDIO reset so do you expect that wifi driver will still work? > > AFAICT that is the normal way to stop drivers interfering with hibernation > i.e. don't load them until after the attempt is made to restore the > hibernation image. We could do that with mmc core too, but for the fact > that the hibernation image is on eMMC. From my experience, it pretty hard to restore HW state which runs by FW, so as W/A we just unloaded remoteproc, wireless and graphic drivers before hibernation and reloaded them right after. As I know the same approach often used in x86 world also. > >> >>> Otherwise it would have to re-enable the function and re-do the >>> function-specific initialization. I don't know if there are other >> >> At boot the SDIO func driver becomes probed, when the mmc core finds >> and SDIO card and then registers a func device for it. Are you saying >> that the SDIO func driver can take shortcuts when enabling the func >> device, when the boot is trigger from WOWLAN? > > No. > >> >>> consequences. Presumably it will have lost any information about the nature >>> of the wake-up trigger. >> >> How does that matter? > > I don't know if it matters. >
On 04/25/2017 09:51 PM, Grygorii Strashko wrote: > > > On 04/25/2017 07:45 AM, Adrian Hunter wrote: >> On 25/04/17 15:24, Ulf Hansson wrote: >>> On 25 April 2017 at 13:20, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>> On 25/04/17 13:46, Ulf Hansson wrote: >>>>> On 25 April 2017 at 09:57, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>>> On 25/04/17 10:52, Ulf Hansson wrote: >>>>>>> +Grygorii Strashko >>>>>>> >>>>>>> On 25 April 2017 at 08:21, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>>>>> On 24/04/17 23:33, Ulf Hansson wrote: >>>>>>>>> On 21 April 2017 at 12:08, Adrian Hunter <adrian.hunter@intel.com> wrote: >>>>>>>>>> The SDIO card state might be being preserved during hibernation, for >>>>>>>>>> example a SDIO wifi card supporting WOWLAN. That state will be lost if an >>>>>>>>>> SDIO reset is done. One way to avoid that would be to build mmc core as a >>>>>>>>>> module and simply not load it until after attempting to restore the >>>>>>>>>> hibernation image. However that won't work if the hibernation image is >>>>>>>>>> stored on eMMC which, of course, requires mmc core. >>>>>>>>> >>>>>>>>> I don't follow here. Are you saying the SDIO card is kept powered in >>>>>>>>> hibernation, as to be able to support WOWLAN, right? >>>>>>>> >>>>>>>> Yes >>>>>>> >>>>>>> Okay, makes sense! >>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>> Then, it feels plain wrong the mmc_rescan() tries to re-initialize it. >>>>>>>>> That should never happen, unless something is broken of course. >>>>>>>> >>>>>>>> The thing to note about hibernation is that there is a regular boot in >>>>>>>> between saving the hibernation image and restoring it again. At boot time, >>>>>>>> the kernel knows almost nothing about whether there is a hibernation image >>>>>>>> and whether or not it will be restored. Consequently it becomes difficult >>>>>>>> to avoid mmc_rescan(). As mentioned above, we need mmc_rescan() to >>>>>>>> initialize the eMMC so that the hibernation image can be read. >>>>>>> >>>>>>> What's wrong with using the hibernation callbacks in the struct >>>>>>> dev_pm_ops? We already do this. >>>>>> >>>>>> Here is the scenario. The kernel has just booted. User space wants to try >>>>>> to restore a hibernation image, if there is one. So user space loads the >>>>>> mmc core because the hibernation image is on eMMC. Mmc core does an SDIO >>>>>> reset on the SDIO card and the state is lost. It has little to do with pm >>>>>> callbacks AFAICS. >>>>> >>>>> Ah, now I see what you mean. I thought the problem was during the >>>>> actual restoring of the hibernation image. >>>>> >>>>> Alright, when a boot is triggered by WOWLAN , you want to avoid >>>>> sending the reset command for the SDIO card before the >>>>> re-initialization of the SDIO card starts. >>>>> >>>>> The problem with this approach is that you can't differentiate between >>>>> a cold boot and a boot triggered by WOWLAN, right!? In other words, in >>>>> some cases the reset command may be needed while in other it won't. >>>> >>>> SDIO reset is only needed if the card has not been power-cycled. The >>>> assumption is that the platform takes care of that when needed. e.g. when >>>> rebooting instead of going to S4. >>>> >>>>> >>>>> Maybe you can elaborate more on what exactly what the problem is with >>>>> sending the reset command when the boot is triggered from WOWLAN? Yes, >>>>> the SDIO card loses its context, but how is that a problem? >>>> >>>> The wifi driver expects to find the function in an initialized state. >>> >>> So how does the the wifi driver distinguish this a boot caused by >>> WOWLAN - and not a cold boot? >> >> It doesn't have to. It doesn't get loaded until after the attempt to >> restore the hibernation image. So in the hibernation case it has its >> ->restore() callback. > > For this case is a good question of how Kernel wifi card driver state will be > synchronized HW state after restore from Hibernation (first of all wireless state - > scan results, connection state and parameters)? Most probably wifi driver will > need to perform mostly full re-initialization on restore, so... > >> If there is no hibernation image, it gets loaded and >> probed. > > But in this case, as per you patch, there are will be no SDIO reset so > do you expect that wifi driver will still work? It does still work yes. > >> >> AFAICT that is the normal way to stop drivers interfering with hibernation >> i.e. don't load them until after the attempt is made to restore the >> hibernation image. We could do that with mmc core too, but for the fact >> that the hibernation image is on eMMC. > >>From my experience, it pretty hard to restore HW state which runs by FW, so as > W/A we just unloaded remoteproc, wireless and graphic drivers before hibernation and > reloaded them right after. As I know the same approach often used in x86 world also. Not sure it would make sense to try and configure WOWLAN and then remove the driver? > > >> >>> >>>> Otherwise it would have to re-enable the function and re-do the >>>> function-specific initialization. I don't know if there are other >>> >>> At boot the SDIO func driver becomes probed, when the mmc core finds >>> and SDIO card and then registers a func device for it. Are you saying >>> that the SDIO func driver can take shortcuts when enabling the func >>> device, when the boot is trigger from WOWLAN? >> >> No. >> >>> >>>> consequences. Presumably it will have lost any information about the nature >>>> of the wake-up trigger. >>> >>> How does that matter? >> >> I don't know if it matters. >> >
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 6987976252ad..178e23bf0c30 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -2639,8 +2639,12 @@ static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq) * if the card is being re-initialized, just send it. CMD52 * should be ignored by SD/eMMC cards. * Skip it if we already know that we do not support SDIO commands + * Also skip it if we know this host controller has a SDIO card that + * needs to be able to restore from hibernation without losing the card + * state e.g. an SDIO wifi card supporting WOWLAN. */ - if (!(host->caps2 & MMC_CAP2_NO_SDIO)) + if (!(host->caps2 & MMC_CAP2_NO_SDIO) && + !(host->caps2 & MMC_CAP2_NO_SDIO_RESET)) sdio_reset(host); mmc_go_idle(host); diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index 78c544e296cd..187a7ba41364 100644 --- a/include/linux/mmc/host.h +++ b/include/linux/mmc/host.h @@ -281,6 +281,7 @@ struct mmc_host { u32 caps2; /* More host capabilities */ #define MMC_CAP2_BOOTPART_NOACC (1 << 0) /* Boot partition no access */ +#define MMC_CAP2_NO_SDIO_RESET (1 << 1) /* Do not SDIO reset at scan */ #define MMC_CAP2_FULL_PWR_CYCLE (1 << 2) /* Can do full power cycle */ #define MMC_CAP2_HS200_1_8V_SDR (1 << 5) /* can support */ #define MMC_CAP2_HS200_1_2V_SDR (1 << 6) /* can support */
The SDIO card state might be being preserved during hibernation, for example a SDIO wifi card supporting WOWLAN. That state will be lost if an SDIO reset is done. One way to avoid that would be to build mmc core as a module and simply not load it until after attempting to restore the hibernation image. However that won't work if the hibernation image is stored on eMMC which, of course, requires mmc core. It is assumed on such systems that the platform will power cycle the SDIO card or not as necessary so that the SDIO reset is not needed. Add a capability flag to reflect that and use it to skip the SDIO reset at scan time. Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> --- drivers/mmc/core/core.c | 6 +++++- include/linux/mmc/host.h | 1 + 2 files changed, 6 insertions(+), 1 deletion(-)