Hi Ulf, Sorry for the late reply. at 21:14, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
On 31 July 2018 at 08:17, Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> --- 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.cindex 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.1Kind regards Uffe
