Messages in this thread | | | Subject | Re: [PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power management | From | Kai-Heng Feng <> | Date | Thu, 23 Aug 2018 16:03:51 +0800 |
| |
Hi Ulf,
Sorry for the late reply.
at 21:14, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 31 July 2018 at 08:17, Kai-Heng Feng <kai.heng.feng@canonical.com> > wrote: >> In order to let host's parent device, rtsx_usb, to use USB remote wake >> up signaling to do card detection, it needs to be suspended. Hence it's >> necessary to add runtime PM support for the memstick host. >> >> To keep memstick host stays suspended when it's not in use, convert the >> card detection function from kthread to delayed_work, which can be >> scheduled when the host is resumed and can be canceled when the host is >> suspended. >> >> Use an idle function check if there's no card and the power mode is >> MEMSTICK_POWER_OFF. If both criteria are met, put the device to suspend. >> >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >> --- >> drivers/memstick/host/rtsx_usb_ms.c | 138 ++++++++++++++-------------- >> 1 file changed, 71 insertions(+), 67 deletions(-) >> >> diff --git a/drivers/memstick/host/rtsx_usb_ms.c >> b/drivers/memstick/host/rtsx_usb_ms.c >> index cd12f3d1c088..126eb310980a 100644 >> --- a/drivers/memstick/host/rtsx_usb_ms.c >> +++ b/drivers/memstick/host/rtsx_usb_ms.c >> @@ -40,15 +40,14 @@ struct rtsx_usb_ms { >> >> struct mutex host_mutex; >> struct work_struct handle_req; >> - >> - struct task_struct *detect_ms; >> - struct completion detect_ms_exit; >> + struct delayed_work poll_card; >> >> u8 ssc_depth; >> unsigned int clock; >> int power_mode; >> unsigned char ifmode; >> bool eject; >> + bool suspend; >> }; >> >> static inline struct device *ms_dev(struct rtsx_usb_ms *host) >> @@ -524,7 +523,7 @@ static void rtsx_usb_ms_handle_req(struct >> work_struct *work) >> int rc; >> >> if (!host->req) { >> - pm_runtime_get_sync(ms_dev(host)); >> + pm_runtime_get_noresume(ms_dev(host)); > > I don't think this is safe. > > The memstick core doesn't manage runtime PM, hence there are no > guarantee that the device is runtime resumed at this point, or is > there?
No guarantees there. Right now this only gets call when the host is powered on.
> >> do { >> rc = memstick_next_req(msh, &host->req); >> dev_dbg(ms_dev(host), "next req %d\n", rc); >> @@ -545,7 +544,7 @@ static void rtsx_usb_ms_handle_req(struct >> work_struct *work) >> host->req->error); >> } >> } while (!rc); >> - pm_runtime_put(ms_dev(host)); >> + pm_runtime_put_noidle(ms_dev(host)); > > According to the above, I think this should stay as is. Or perhaps you > want to use pm_runtime_put_sync() instead, as to avoid the device from > being unnecessary resumed and hence also its parent.
The tricky part is that pm_runtime_put_sync() calls rtsx_usb_ms_set_param() which calls pm_runtime_* helpers again
So maybe add runtime PM support to memstick core is the only way to support it properly?
> >> } >> >> } >> @@ -572,7 +571,7 @@ static int rtsx_usb_ms_set_param(struct >> memstick_host *msh, >> dev_dbg(ms_dev(host), "%s: param = %d, value = %d\n", >> __func__, param, value); >> >> - pm_runtime_get_sync(ms_dev(host)); >> + pm_runtime_get_noresume(ms_dev(host)); > > Ditto. > >> mutex_lock(&ucr->dev_mutex); >> >> err = rtsx_usb_card_exclusive_check(ucr, RTSX_USB_MS_CARD); >> @@ -585,14 +584,14 @@ static int rtsx_usb_ms_set_param(struct >> memstick_host *msh, >> break; >> >> if (value == MEMSTICK_POWER_ON) { >> - pm_runtime_get_sync(ms_dev(host)); >> + pm_runtime_get_noresume(ms_dev(host)); > > Ditto. > >> err = ms_power_on(host); >> + if (err) >> + pm_runtime_put_noidle(ms_dev(host)); >> } else if (value == MEMSTICK_POWER_OFF) { >> err = ms_power_off(host); >> - if (host->msh->card) >> + if (!err) >> pm_runtime_put_noidle(ms_dev(host)); >> - else >> - pm_runtime_put(ms_dev(host)); > > Ditto. > >> } else >> err = -EINVAL; >> if (!err) >> @@ -638,7 +637,7 @@ static int rtsx_usb_ms_set_param(struct >> memstick_host *msh, >> } >> out: >> mutex_unlock(&ucr->dev_mutex); >> - pm_runtime_put(ms_dev(host)); >> + pm_runtime_put_noidle(ms_dev(host)); > > Ditto. > >> /* power-on delay */ >> if (param == MEMSTICK_POWER && value == MEMSTICK_POWER_ON) >> @@ -648,75 +647,86 @@ static int rtsx_usb_ms_set_param(struct >> memstick_host *msh, >> return err; >> } >> >> -#ifdef CONFIG_PM_SLEEP >> -static int rtsx_usb_ms_suspend(struct device *dev) >> +#ifdef CONFIG_PM >> +static int rtsx_usb_ms_runtime_suspend(struct device *dev) >> { >> struct rtsx_usb_ms *host = dev_get_drvdata(dev); >> struct memstick_host *msh = host->msh; >> >> - dev_dbg(ms_dev(host), "--> %s\n", __func__); >> - >> + host->suspend = true; >> memstick_suspend_host(msh); >> + >> return 0; >> } >> >> -static int rtsx_usb_ms_resume(struct device *dev) >> +static int rtsx_usb_ms_runtime_resume(struct device *dev) >> { >> struct rtsx_usb_ms *host = dev_get_drvdata(dev); >> struct memstick_host *msh = host->msh; >> >> - dev_dbg(ms_dev(host), "--> %s\n", __func__); >> - >> memstick_resume_host(msh); >> + host->suspend = false; >> + schedule_delayed_work(&host->poll_card, 100); >> + >> return 0; >> } >> -#endif /* CONFIG_PM_SLEEP */ >> >> -/* >> - * Thread function of ms card slot detection. The thread starts right >> after >> - * successful host addition. It stops while the driver removal function >> sets >> - * host->eject true. >> - */ >> -static int rtsx_usb_detect_ms_card(void *__host) >> +static int rtsx_usb_ms_runtime_idle(struct device *dev) >> +{ >> + struct rtsx_usb_ms *host = dev_get_drvdata(dev); >> + >> + if (!host->msh->card && host->power_mode == MEMSTICK_POWER_OFF) { >> + pm_schedule_suspend(dev, 0); >> + return 0; >> + } >> + >> + return -EAGAIN; >> +} >> + >> +static UNIVERSAL_DEV_PM_OPS(rtsx_usb_ms_pm_ops, >> + rtsx_usb_ms_runtime_suspend, rtsx_usb_ms_runtime_resume, >> + rtsx_usb_ms_runtime_idle); >> +#endif /* CONFIG_PM */ >> + >> +static void rtsx_usb_ms_poll_card(struct work_struct *work) >> { >> - struct rtsx_usb_ms *host = (struct rtsx_usb_ms *)__host; >> + struct rtsx_usb_ms *host = container_of(work, struct rtsx_usb_ms, >> + poll_card.work); >> struct rtsx_ucr *ucr = host->ucr; >> - u8 val = 0; >> int err; >> + u8 val; >> >> - for (;;) { >> - pm_runtime_get_sync(ms_dev(host)); >> - mutex_lock(&ucr->dev_mutex); >> + if (host->eject || host->suspend) > > The runtime PM state of the device could potentially be changed by > user space, via sysfs, as well. > > It seems like what you really should be checking is whether > host->power_mode is MEMSTICK_POWER_OFF and then act accordingly.
Makes sense.
> > I also think you be able to implement this without a ->runtime_idle() > callback, as it just makes this a bit unnecessary complicated.
You are right. Will update it with runtime_suspend() version.
> >> + return; >> >> - /* Check pending MS card changes */ >> - err = rtsx_usb_read_register(ucr, CARD_INT_PEND, &val); >> - if (err) { >> - mutex_unlock(&ucr->dev_mutex); >> - goto poll_again; >> - } >> - >> - /* Clear the pending */ >> - rtsx_usb_write_register(ucr, CARD_INT_PEND, >> - XD_INT | MS_INT | SD_INT, >> - XD_INT | MS_INT | SD_INT); >> + pm_runtime_get_noresume(ms_dev(host)); >> + mutex_lock(&ucr->dev_mutex); >> >> + /* Check pending MS card changes */ >> + err = rtsx_usb_read_register(ucr, CARD_INT_PEND, &val); >> + if (err) { >> mutex_unlock(&ucr->dev_mutex); >> + goto poll_again; >> + } >> >> - if (val & MS_INT) { >> - dev_dbg(ms_dev(host), "MS slot change >> detected\n"); >> - memstick_detect_change(host->msh); >> - } >> + /* Clear the pending */ >> + rtsx_usb_write_register(ucr, CARD_INT_PEND, >> + XD_INT | MS_INT | SD_INT, >> + XD_INT | MS_INT | SD_INT); >> >> -poll_again: >> - pm_runtime_put(ms_dev(host)); >> - if (host->eject) >> - break; >> + mutex_unlock(&ucr->dev_mutex); >> + pm_runtime_put_noidle(ms_dev(host)); >> >> - schedule_timeout_idle(HZ); >> + if (val & MS_INT) { >> + dev_dbg(ms_dev(host), "MS slot change detected\n"); >> + memstick_detect_change(host->msh); >> } >> >> - complete(&host->detect_ms_exit); >> - return 0; >> + pm_runtime_idle(ms_dev(host)); >> + >> +poll_again: >> + if (!host->eject && !host->suspend) >> + schedule_delayed_work(&host->poll_card, 100); >> } >> >> static int rtsx_usb_ms_drv_probe(struct platform_device *pdev) >> @@ -747,26 +757,21 @@ static int rtsx_usb_ms_drv_probe(struct >> platform_device *pdev) >> mutex_init(&host->host_mutex); >> INIT_WORK(&host->handle_req, rtsx_usb_ms_handle_req); >> >> - init_completion(&host->detect_ms_exit); >> - host->detect_ms = kthread_create(rtsx_usb_detect_ms_card, host, >> - "rtsx_usb_ms_%d", pdev->id); >> - if (IS_ERR(host->detect_ms)) { >> - dev_dbg(&(pdev->dev), >> - "Unable to create polling thread.\n"); >> - err = PTR_ERR(host->detect_ms); >> - goto err_out; >> - } >> + INIT_DELAYED_WORK(&host->poll_card, rtsx_usb_ms_poll_card); >> >> msh->request = rtsx_usb_ms_request; >> msh->set_param = rtsx_usb_ms_set_param; >> msh->caps = MEMSTICK_CAP_PAR4; >> >> - pm_runtime_enable(&pdev->dev); >> + pm_runtime_set_active(ms_dev(host)); >> + pm_runtime_enable(ms_dev(host)); >> + >> err = memstick_add_host(msh); >> if (err) >> goto err_out; >> >> - wake_up_process(host->detect_ms); >> + schedule_delayed_work(&host->poll_card, 100); >> + >> return 0; >> err_out: >> memstick_free_host(msh); >> @@ -782,6 +787,7 @@ static int rtsx_usb_ms_drv_remove(struct >> platform_device *pdev) >> msh = host->msh; >> host->eject = true; >> cancel_work_sync(&host->handle_req); >> + cancel_delayed_work_sync(&host->poll_card); >> >> mutex_lock(&host->host_mutex); >> if (host->req) { >> @@ -797,7 +803,6 @@ static int rtsx_usb_ms_drv_remove(struct >> platform_device *pdev) >> } >> mutex_unlock(&host->host_mutex); >> >> - wait_for_completion(&host->detect_ms_exit); >> memstick_remove_host(msh); >> memstick_free_host(msh); >> >> @@ -816,9 +821,6 @@ static int rtsx_usb_ms_drv_remove(struct >> platform_device *pdev) >> return 0; >> } >> >> -static SIMPLE_DEV_PM_OPS(rtsx_usb_ms_pm_ops, >> - rtsx_usb_ms_suspend, rtsx_usb_ms_resume); >> - >> static struct platform_device_id rtsx_usb_ms_ids[] = { >> { >> .name = "rtsx_usb_ms", >> @@ -834,7 +836,9 @@ static struct platform_driver rtsx_usb_ms_driver = { >> .id_table = rtsx_usb_ms_ids, >> .driver = { >> .name = "rtsx_usb_ms", >> +#ifdef CONFIG_PM >> .pm = &rtsx_usb_ms_pm_ops, >> +#endif >> }, >> }; >> module_platform_driver(rtsx_usb_ms_driver); >> -- >> 2.17.1 > > Kind regards > Uffe
| |