* [PATCH 0/5 v4] Keep rtsx_usb suspended when there's no card @ 2018-07-31 6:17 Kai-Heng Feng 2018-07-31 6:17 ` [PATCH 1/5] misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection Kai-Heng Feng ` (4 more replies) 0 siblings, 5 replies; 18+ messages in thread From: Kai-Heng Feng @ 2018-07-31 6:17 UTC (permalink / raw) To: arnd, gregkh Cc: ulf.hansson, stern, bauer.chen, ricky_wu, linux-kernel, linux-usb, Kai-Heng Feng Hi, This is based on Ulf's work [1] [2]. This patch series can keep rtsx_usb suspended, to save ~0.5W on Intel platforms and ~1.5W on AMD platforms. [1] https://patchwork.kernel.org/patch/10440583/ [2] https://patchwork.kernel.org/patch/10445725/ Kai-Heng Feng (5): misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection memstick: Prevent memstick host from getting runtime suspended during card detection memstick: rtsx_usb_ms: Use ms_dev() helper memstick: rtsx_usb_ms: Support runtime power management misc: rtsx_usb: / memstick: rtsx_usb_ms: Avoid long delay before system suspend v4: Use pm_runtime_put() in memstick_check(). v3: Skip parent device check in rtsx_usb_resume_child(). Remove dev_dbg() if it only prints function name. Use ms_dev() helper at more places. Remove const qualifier for UNIVERSAL_DEV_PM_OPS. v2: Resend to linux-usb and LKML. --- Kai-Heng Feng (5): misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection memstick: Prevent memstick host from getting runtime suspended during card detection memstick: rtsx_usb_ms: Use ms_dev() helper memstick: rtsx_usb_ms: Support runtime power management misc: rtsx_usb: / memstick: rtsx_usb_ms: Avoid long delay before system suspend drivers/memstick/core/memstick.c | 4 + drivers/memstick/host/rtsx_usb_ms.c | 148 +++++++++++++++------------- drivers/misc/cardreader/rtsx_usb.c | 9 ++ 3 files changed, 91 insertions(+), 70 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/5] misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection 2018-07-31 6:17 [PATCH 0/5 v4] Keep rtsx_usb suspended when there's no card Kai-Heng Feng @ 2018-07-31 6:17 ` Kai-Heng Feng 2018-09-13 10:31 ` Kai-Heng Feng 2018-07-31 6:17 ` [PATCH 2/5] memstick: Prevent memstick host from getting runtime suspended during card detection Kai-Heng Feng ` (3 subsequent siblings) 4 siblings, 1 reply; 18+ messages in thread From: Kai-Heng Feng @ 2018-07-31 6:17 UTC (permalink / raw) To: arnd, gregkh Cc: ulf.hansson, stern, bauer.chen, ricky_wu, linux-kernel, linux-usb, Kai-Heng Feng Although rtsx_usb doesn't support card removal detection, card insertion will resume rtsx_usb by USB remote wakeup signaling. When rtsx_usb gets resumed, also resumes its child devices, rtsx_usb_sdmmc and rtsx_usb_ms, to notify them there's a card in its slot. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/misc/cardreader/rtsx_usb.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/misc/cardreader/rtsx_usb.c b/drivers/misc/cardreader/rtsx_usb.c index b97903ff1a72..f7a66f614085 100644 --- a/drivers/misc/cardreader/rtsx_usb.c +++ b/drivers/misc/cardreader/rtsx_usb.c @@ -723,8 +723,15 @@ static int rtsx_usb_suspend(struct usb_interface *intf, pm_message_t message) return 0; } +static int rtsx_usb_resume_child(struct device *dev, void *data) +{ + pm_request_resume(dev); + return 0; +} + static int rtsx_usb_resume(struct usb_interface *intf) { + device_for_each_child(&intf->dev, NULL, rtsx_usb_resume_child); return 0; } @@ -734,6 +741,7 @@ static int rtsx_usb_reset_resume(struct usb_interface *intf) (struct rtsx_ucr *)usb_get_intfdata(intf); rtsx_usb_reset_chip(ucr); + device_for_each_child(&intf->dev, NULL, rtsx_usb_resume_child); return 0; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/5] misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection 2018-07-31 6:17 ` [PATCH 1/5] misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection Kai-Heng Feng @ 2018-09-13 10:31 ` Kai-Heng Feng 2018-09-13 13:41 ` Alan Stern 0 siblings, 1 reply; 18+ messages in thread From: Kai-Heng Feng @ 2018-09-13 10:31 UTC (permalink / raw) To: Alan Stern, Ulf Hansson; +Cc: Linux Kernel Mailing List, Linux USB Mailing List Hi Alan, Ulf, at 14:17, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > Although rtsx_usb doesn't support card removal detection, card insertion > will resume rtsx_usb by USB remote wakeup signaling. > > When rtsx_usb gets resumed, also resumes its child devices, > rtsx_usb_sdmmc and rtsx_usb_ms, to notify them there's a card in its > slot. > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/misc/cardreader/rtsx_usb.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/misc/cardreader/rtsx_usb.c > b/drivers/misc/cardreader/rtsx_usb.c > index b97903ff1a72..f7a66f614085 100644 > --- a/drivers/misc/cardreader/rtsx_usb.c > +++ b/drivers/misc/cardreader/rtsx_usb.c > @@ -723,8 +723,15 @@ static int rtsx_usb_suspend(struct usb_interface > *intf, pm_message_t message) > return 0; > } > > +static int rtsx_usb_resume_child(struct device *dev, void *data) > +{ > + pm_request_resume(dev); > + return 0; > +} > + > static int rtsx_usb_resume(struct usb_interface *intf) > { > + device_for_each_child(&intf->dev, NULL, rtsx_usb_resume_child); > return 0; > } > > @@ -734,6 +741,7 @@ static int rtsx_usb_reset_resume(struct usb_interface > *intf) > (struct rtsx_ucr *)usb_get_intfdata(intf); > > rtsx_usb_reset_chip(ucr); > + device_for_each_child(&intf->dev, NULL, rtsx_usb_resume_child); > return 0; > } I am working on the next version of this series, and the last missing puzzle is to differentiate system-wide resume from runtime resume in usb_driver's resume() and reset_resume() callback. The parent device, rtsx_usb, has two child devices, rtsx_usb_ms and rtsx_usb_sdmmc. pm_request_resume() is used in rtsx_usb's resume() to facilitate USB remote wakeup signaling, so we don't need to poll the card slot status. But this has a side effect: during system resume the rtsx_usb calls pm_request_resume() in its resume(), so child devices calls their runtime_resume() instead of resume() callback. So, is it reasonable to pass pm_message_t to resume() and reset_resume() callbacks and use PMSG_IS_AUTO() to differentiate them? Kai-Heng > > -- > 2.17.1 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/5] misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection 2018-09-13 10:31 ` Kai-Heng Feng @ 2018-09-13 13:41 ` Alan Stern 2018-09-17 8:07 ` Kai-Heng Feng 0 siblings, 1 reply; 18+ messages in thread From: Alan Stern @ 2018-09-13 13:41 UTC (permalink / raw) To: Kai-Heng Feng Cc: Ulf Hansson, Linux Kernel Mailing List, Linux USB Mailing List On Thu, 13 Sep 2018, Kai-Heng Feng wrote: > I am working on the next version of this series, and the last missing > puzzle is to differentiate system-wide resume from runtime resume in > usb_driver's resume() and reset_resume() callback. > > The parent device, rtsx_usb, has two child devices, rtsx_usb_ms and > rtsx_usb_sdmmc. > pm_request_resume() is used in rtsx_usb's resume() to facilitate USB remote > wakeup signaling, so we don't need to poll the card slot status. > But this has a side effect: during system resume the rtsx_usb calls > pm_request_resume() in its resume(), so child devices calls their > runtime_resume() instead of resume() callback. Have you actually observed this? It shouldn't happen. pm_request_resume() schedules a runtime resume on the PM work queue, but this work queue is frozen during system sleep. It doesn't unfreeze until after all the devices have been restored to full power. Thus the child device's resume() callback should be invoked before runtime_resume(). > So, is it reasonable to pass pm_message_t to resume() and reset_resume() > callbacks and use PMSG_IS_AUTO() to differentiate them? It should not be necessary to do this. Alan Stern ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/5] misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection 2018-09-13 13:41 ` Alan Stern @ 2018-09-17 8:07 ` Kai-Heng Feng 0 siblings, 0 replies; 18+ messages in thread From: Kai-Heng Feng @ 2018-09-17 8:07 UTC (permalink / raw) To: Alan Stern; +Cc: Ulf Hansson, Linux Kernel Mailing List, Linux USB Mailing List at 21:41, Alan Stern <stern@rowland.harvard.edu> wrote: > On Thu, 13 Sep 2018, Kai-Heng Feng wrote: > >> I am working on the next version of this series, and the last missing >> puzzle is to differentiate system-wide resume from runtime resume in >> usb_driver's resume() and reset_resume() callback. >> >> The parent device, rtsx_usb, has two child devices, rtsx_usb_ms and >> rtsx_usb_sdmmc. >> pm_request_resume() is used in rtsx_usb's resume() to facilitate USB >> remote >> wakeup signaling, so we don't need to poll the card slot status. >> But this has a side effect: during system resume the rtsx_usb calls >> pm_request_resume() in its resume(), so child devices calls their >> runtime_resume() instead of resume() callback. > > Have you actually observed this? It shouldn't happen. > pm_request_resume() schedules a runtime resume on the PM work queue, > but this work queue is frozen during system sleep. It doesn't unfreeze > until after all the devices have been restored to full power. Thus the > child device's resume() callback should be invoked before > runtime_resume(). You are right, I read the trace wrong. It happens right before system-wide suspension, when the PM core resumes it to fully functional state. > >> So, is it reasonable to pass pm_message_t to resume() and reset_resume() >> callbacks and use PMSG_IS_AUTO() to differentiate them? > > It should not be necessary to do this. Understood, thanks. Kai-Heng > > Alan Stern ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/5] memstick: Prevent memstick host from getting runtime suspended during card detection 2018-07-31 6:17 [PATCH 0/5 v4] Keep rtsx_usb suspended when there's no card Kai-Heng Feng 2018-07-31 6:17 ` [PATCH 1/5] misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection Kai-Heng Feng @ 2018-07-31 6:17 ` Kai-Heng Feng 2018-07-31 6:17 ` [PATCH 3/5] memstick: rtsx_usb_ms: Use ms_dev() helper Kai-Heng Feng ` (2 subsequent siblings) 4 siblings, 0 replies; 18+ messages in thread From: Kai-Heng Feng @ 2018-07-31 6:17 UTC (permalink / raw) To: arnd, gregkh Cc: ulf.hansson, stern, bauer.chen, ricky_wu, linux-kernel, linux-usb, Kai-Heng Feng We can use MEMSTICK_POWER_{ON,OFF} along with pm_runtime_{get,put} helpers to let memstick host support runtime pm. There's a small window between memstick_detect_change() and its queued work, memstick_check(). In this window the rpm count may go down to zero before the memstick host powers on, so the host can be inadvertently suspended. Increment rpm count before calling memstick_check(), and decrement rpm count afterward, as now we are sure the memstick host should be suspended or not. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/memstick/core/memstick.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c index 76382c858c35..5f16a8826401 100644 --- a/drivers/memstick/core/memstick.c +++ b/drivers/memstick/core/memstick.c @@ -18,6 +18,7 @@ #include <linux/delay.h> #include <linux/slab.h> #include <linux/module.h> +#include <linux/pm_runtime.h> #define DRIVER_NAME "memstick" @@ -209,6 +210,7 @@ static int memstick_dummy_check(struct memstick_dev *card) */ void memstick_detect_change(struct memstick_host *host) { + pm_runtime_get_noresume(host->dev.parent); queue_work(workqueue, &host->media_checker); } EXPORT_SYMBOL(memstick_detect_change); @@ -479,6 +481,8 @@ static void memstick_check(struct work_struct *work) host->set_param(host, MEMSTICK_POWER, MEMSTICK_POWER_OFF); mutex_unlock(&host->lock); + + pm_runtime_put(host->dev.parent); dev_dbg(&host->dev, "memstick_check finished\n"); } -- 2.17.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/5] memstick: rtsx_usb_ms: Use ms_dev() helper 2018-07-31 6:17 [PATCH 0/5 v4] Keep rtsx_usb suspended when there's no card Kai-Heng Feng 2018-07-31 6:17 ` [PATCH 1/5] misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection Kai-Heng Feng 2018-07-31 6:17 ` [PATCH 2/5] memstick: Prevent memstick host from getting runtime suspended during card detection Kai-Heng Feng @ 2018-07-31 6:17 ` Kai-Heng Feng 2018-07-31 6:17 ` [PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power management Kai-Heng Feng 2018-07-31 6:17 ` [PATCH 5/5] misc: rtsx_usb: / memstick: rtsx_usb_ms: Avoid long delay before system suspend Kai-Heng Feng 4 siblings, 0 replies; 18+ messages in thread From: Kai-Heng Feng @ 2018-07-31 6:17 UTC (permalink / raw) To: arnd, gregkh Cc: ulf.hansson, stern, bauer.chen, ricky_wu, linux-kernel, linux-usb, Kai-Heng Feng Use ms_dev() helper for consistency. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/memstick/host/rtsx_usb_ms.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/memstick/host/rtsx_usb_ms.c b/drivers/memstick/host/rtsx_usb_ms.c index 4f64563df7de..cd12f3d1c088 100644 --- a/drivers/memstick/host/rtsx_usb_ms.c +++ b/drivers/memstick/host/rtsx_usb_ms.c @@ -785,7 +785,7 @@ static int rtsx_usb_ms_drv_remove(struct platform_device *pdev) mutex_lock(&host->host_mutex); if (host->req) { - dev_dbg(&(pdev->dev), + dev_dbg(ms_dev(host), "%s: Controller removed during transfer\n", dev_name(&msh->dev)); host->req->error = -ENOMEDIUM; @@ -807,10 +807,10 @@ static int rtsx_usb_ms_drv_remove(struct platform_device *pdev) if (pm_runtime_active(ms_dev(host))) pm_runtime_put(ms_dev(host)); - pm_runtime_disable(&pdev->dev); + pm_runtime_disable(ms_dev(host)); platform_set_drvdata(pdev, NULL); - dev_dbg(&(pdev->dev), + dev_dbg(ms_dev(host), ": Realtek USB Memstick controller has been removed\n"); return 0; -- 2.17.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power management 2018-07-31 6:17 [PATCH 0/5 v4] Keep rtsx_usb suspended when there's no card Kai-Heng Feng ` (2 preceding siblings ...) 2018-07-31 6:17 ` [PATCH 3/5] memstick: rtsx_usb_ms: Use ms_dev() helper Kai-Heng Feng @ 2018-07-31 6:17 ` Kai-Heng Feng 2018-08-01 13:14 ` Ulf Hansson 2018-08-01 13:29 ` Ulf Hansson 2018-07-31 6:17 ` [PATCH 5/5] misc: rtsx_usb: / memstick: rtsx_usb_ms: Avoid long delay before system suspend Kai-Heng Feng 4 siblings, 2 replies; 18+ messages in thread From: Kai-Heng Feng @ 2018-07-31 6:17 UTC (permalink / raw) To: arnd, gregkh Cc: ulf.hansson, stern, bauer.chen, ricky_wu, linux-kernel, linux-usb, Kai-Heng Feng 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)); 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)); } } @@ -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)); 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)); 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)); } 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)); /* 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) + 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power management 2018-07-31 6:17 ` [PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power management Kai-Heng Feng @ 2018-08-01 13:14 ` Ulf Hansson 2018-08-23 8:03 ` Kai-Heng Feng 2018-08-01 13:29 ` Ulf Hansson 1 sibling, 1 reply; 18+ messages in thread From: Ulf Hansson @ 2018-08-01 13:14 UTC (permalink / raw) To: Kai-Heng Feng Cc: Arnd Bergmann, Greg Kroah-Hartman, Alan Stern, Bauer.Chen, ricky_wu, Linux Kernel Mailing List, Linux USB List 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? > 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. > } > > } > @@ -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. I also think you be able to implement this without a ->runtime_idle() callback, as it just makes this a bit unnecessary complicated. > + 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power management 2018-08-01 13:14 ` Ulf Hansson @ 2018-08-23 8:03 ` Kai-Heng Feng 2018-08-24 6:28 ` Ulf Hansson 0 siblings, 1 reply; 18+ messages in thread From: Kai-Heng Feng @ 2018-08-23 8:03 UTC (permalink / raw) To: Ulf Hansson Cc: Arnd Bergmann, Greg Kroah-Hartman, Alan Stern, Bauer.Chen, ricky_wu, Linux Kernel Mailing List, Linux USB List 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power management 2018-08-23 8:03 ` Kai-Heng Feng @ 2018-08-24 6:28 ` Ulf Hansson 2018-08-30 8:36 ` Kai-Heng Feng 0 siblings, 1 reply; 18+ messages in thread From: Ulf Hansson @ 2018-08-24 6:28 UTC (permalink / raw) To: Kai-Heng Feng Cc: Arnd Bergmann, Greg Kroah-Hartman, Alan Stern, Bauer.Chen, ricky_wu, Linux Kernel Mailing List, Linux USB List On 23 August 2018 at 10:03, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > 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 Why does a pm_runtime_put_sync() triggers a call to rtsx_usb_ms_set_param()? It should not. Ahh, that's because you have implemented the ->runtime_suspend() callback to wrongly call memstick_suspend_host(). Drop that change, then you should be able to call pm_runtime_put_sync() here and at other places correctly. > > So maybe add runtime PM support to memstick core is the only way to support > it properly? > It shouldn't be needed, and I wonder about if the effort is worth it, especially since it seems that the only memstick driver that using runtime PM is rtsx_usb_ms. BTW, are you testing this change by using a memstick card, since I haven't seen them for ages. :-) [...] Kind regards Uffe ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power management 2018-08-24 6:28 ` Ulf Hansson @ 2018-08-30 8:36 ` Kai-Heng Feng 0 siblings, 0 replies; 18+ messages in thread From: Kai-Heng Feng @ 2018-08-30 8:36 UTC (permalink / raw) To: Ulf Hansson Cc: Arnd Bergmann, Greg Kroah-Hartman, Alan Stern, Bauer.Chen, ricky_wu, Linux Kernel Mailing List, Linux USB List at 14:28, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 23 August 2018 at 10:03, Kai-Heng Feng <kai.heng.feng@canonical.com> > wrote: >> 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 > > Why does a pm_runtime_put_sync() triggers a call to > rtsx_usb_ms_set_param()? It should not. > > Ahh, that's because you have implemented the ->runtime_suspend() > callback to wrongly call memstick_suspend_host(). Drop that change, > then you should be able to call pm_runtime_put_sync() here and at > other places correctly. Thanks for the catch! I'll address that. > >> So maybe add runtime PM support to memstick core is the only way to >> support >> it properly? > > It shouldn't be needed, and I wonder about if the effort is worth it, > especially since it seems that the only memstick driver that using > runtime PM is rtsx_usb_ms. > > BTW, are you testing this change by using a memstick card, since I > haven't seen them for ages. :-) Yes, Realtek borrowed me a MMC/MS combo device to let me work on this. Kai-Heng > > [...] > > Kind regards > Uffe ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power management 2018-07-31 6:17 ` [PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power management Kai-Heng Feng 2018-08-01 13:14 ` Ulf Hansson @ 2018-08-01 13:29 ` Ulf Hansson 2018-08-23 8:12 ` Kai-Heng Feng 1 sibling, 1 reply; 18+ messages in thread From: Ulf Hansson @ 2018-08-01 13:29 UTC (permalink / raw) To: Kai-Heng Feng Cc: Arnd Bergmann, Greg Kroah-Hartman, Alan Stern, Bauer.Chen, ricky_wu, Linux Kernel Mailing List, Linux USB List [...] > -#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); I missed this one. Does this really work? To me, this looks like doing things upside-down. To suspend the host, you first need to runtime resume it, because mmc_suspend_host() calls into one of the host ops and my touch the device, right? If you want to suspend the host (actually the naming is wrong, as it's about suspending/power-iff the memstick card), that should be done via when the memstick core finds that the card is removed or during system wide suspend. > + > 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); According to the above, this seems not correct to me. > + host->suspend = false; > + schedule_delayed_work(&host->poll_card, 100); > + > return 0; > } > -#endif /* CONFIG_PM_SLEEP */ > [...] Kind regards Uffe ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power management 2018-08-01 13:29 ` Ulf Hansson @ 2018-08-23 8:12 ` Kai-Heng Feng 2018-08-24 6:30 ` Ulf Hansson 0 siblings, 1 reply; 18+ messages in thread From: Kai-Heng Feng @ 2018-08-23 8:12 UTC (permalink / raw) To: Ulf Hansson Cc: Arnd Bergmann, Greg Kroah-Hartman, Alan Stern, Bauer.Chen, ricky_wu, Linux Kernel Mailing List, Linux USB List at 21:29, Ulf Hansson <ulf.hansson@linaro.org> wrote: > [...] > >> -#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); > > I missed this one. Does this really work? To me, this looks like doing > things upside-down. > > To suspend the host, you first need to runtime resume it, because > mmc_suspend_host() calls into one of the host ops and my touch the > device, right? > > If you want to suspend the host (actually the naming is wrong, as it's > about suspending/power-iff the memstick card), that should be done via > when the memstick core finds that the card is removed or during system > wide suspend. Do you mean the logic was wrong even before my modification? Kai-Heng > >> + >> 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); > > According to the above, this seems not correct to me. > >> + host->suspend = false; >> + schedule_delayed_work(&host->poll_card, 100); >> + >> return 0; >> } >> -#endif /* CONFIG_PM_SLEEP */ > > [...] > > Kind regards > Uffe ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power management 2018-08-23 8:12 ` Kai-Heng Feng @ 2018-08-24 6:30 ` Ulf Hansson 0 siblings, 0 replies; 18+ messages in thread From: Ulf Hansson @ 2018-08-24 6:30 UTC (permalink / raw) To: Kai-Heng Feng Cc: Arnd Bergmann, Greg Kroah-Hartman, Alan Stern, Bauer.Chen, ricky_wu, Linux Kernel Mailing List, Linux USB List On 23 August 2018 at 10:12, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > at 21:29, Ulf Hansson <ulf.hansson@linaro.org> wrote: > >> [...] >> >>> -#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); >> >> >> I missed this one. Does this really work? To me, this looks like doing >> things upside-down. >> >> To suspend the host, you first need to runtime resume it, because >> mmc_suspend_host() calls into one of the host ops and my touch the >> device, right? >> >> If you want to suspend the host (actually the naming is wrong, as it's >> about suspending/power-iff the memstick card), that should be done via >> when the memstick core finds that the card is removed or during system >> wide suspend. > > > Do you mean the logic was wrong even before my modification? No. I think you should keep the existing callbacks for system sleep, they seems to do what they should. Then add new ones for runtime PM. [...] Kind regards Uffe ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 5/5] misc: rtsx_usb: / memstick: rtsx_usb_ms: Avoid long delay before system suspend 2018-07-31 6:17 [PATCH 0/5 v4] Keep rtsx_usb suspended when there's no card Kai-Heng Feng ` (3 preceding siblings ...) 2018-07-31 6:17 ` [PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power management Kai-Heng Feng @ 2018-07-31 6:17 ` Kai-Heng Feng 4 siblings, 0 replies; 18+ messages in thread From: Kai-Heng Feng @ 2018-07-31 6:17 UTC (permalink / raw) To: arnd, gregkh Cc: ulf.hansson, stern, bauer.chen, ricky_wu, linux-kernel, linux-usb, Kai-Heng Feng There's a long power-on delay at the end of rtsx_usb_ms_set_param(). This delay is noticeable right before system suspend. To prevent already suspended memstick host from getting powered on by PM core, use DPM_FLAG_SMART_SUSPEND to avoid the situation. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/memstick/host/rtsx_usb_ms.c | 4 ++++ drivers/misc/cardreader/rtsx_usb.c | 1 + 2 files changed, 5 insertions(+) diff --git a/drivers/memstick/host/rtsx_usb_ms.c b/drivers/memstick/host/rtsx_usb_ms.c index 126eb310980a..e3b635d1220f 100644 --- a/drivers/memstick/host/rtsx_usb_ms.c +++ b/drivers/memstick/host/rtsx_usb_ms.c @@ -763,6 +763,10 @@ static int rtsx_usb_ms_drv_probe(struct platform_device *pdev) msh->set_param = rtsx_usb_ms_set_param; msh->caps = MEMSTICK_CAP_PAR4; + /* DPM_FLAG_LEAVE_SUSPENDED is not needed, the parent device will wake + * up memstick host. + */ + dev_pm_set_driver_flags(ms_dev(host), DPM_FLAG_SMART_SUSPEND); pm_runtime_set_active(ms_dev(host)); pm_runtime_enable(ms_dev(host)); diff --git a/drivers/misc/cardreader/rtsx_usb.c b/drivers/misc/cardreader/rtsx_usb.c index f7a66f614085..98bb878a6ade 100644 --- a/drivers/misc/cardreader/rtsx_usb.c +++ b/drivers/misc/cardreader/rtsx_usb.c @@ -671,6 +671,7 @@ static int rtsx_usb_probe(struct usb_interface *intf, goto out_init_fail; #ifdef CONFIG_PM + dev_pm_set_driver_flags(&intf->dev, DPM_FLAG_SMART_SUSPEND | DPM_FLAG_LEAVE_SUSPENDED); intf->needs_remote_wakeup = 1; usb_enable_autosuspend(usb_dev); #endif -- 2.17.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 0/5 v3] Keep rtsx_usb suspended when there's no card @ 2018-07-25 15:25 Kai-Heng Feng 2018-07-25 15:25 ` [PATCH 3/5] memstick: rtsx_usb_ms: Use ms_dev() helper Kai-Heng Feng 0 siblings, 1 reply; 18+ messages in thread From: Kai-Heng Feng @ 2018-07-25 15:25 UTC (permalink / raw) To: arnd, gregkh Cc: ulf.hansson, stern, bauer.chen, ricky_wu, linux-kernel, linux-usb, Kai-Heng Feng Hi, This is based on Ulf's work [1] [2]. This patch series can keep rtsx_usb suspended, to save ~0.5W on Intel platforms and ~1.5W on AMD platforms. [1] https://patchwork.kernel.org/patch/10440583/ [2] https://patchwork.kernel.org/patch/10445725/ Kai-Heng Feng (5): misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection memstick: Prevent memstick host from getting runtime suspended during card detection memstick: rtsx_usb_ms: Use ms_dev() helper memstick: rtsx_usb_ms: Support runtime power management misc: rtsx_usb: / memstick: rtsx_usb_ms: Avoid long delay before system suspend -- v3: Skip parent device check in rtsx_usb_resume_child() . Remove dev_dbg() if it only prints function name. Use ms_dev() helper at more places. Remove const qualifier for UNIVERSAL_DEV_PM_OPS. v2: Resend to linux-usb and LKML. drivers/memstick/core/memstick.c | 4 + drivers/memstick/host/rtsx_usb_ms.c | 148 +++++++++++++++------------- drivers/misc/cardreader/rtsx_usb.c | 9 ++ 3 files changed, 91 insertions(+), 70 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/5] memstick: rtsx_usb_ms: Use ms_dev() helper 2018-07-25 15:25 [PATCH 0/5 v3] Keep rtsx_usb suspended when there's no card Kai-Heng Feng @ 2018-07-25 15:25 ` Kai-Heng Feng 0 siblings, 0 replies; 18+ messages in thread From: Kai-Heng Feng @ 2018-07-25 15:25 UTC (permalink / raw) To: arnd, gregkh Cc: ulf.hansson, stern, bauer.chen, ricky_wu, linux-kernel, linux-usb, Kai-Heng Feng Use ms_dev() helper for consistency. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/memstick/host/rtsx_usb_ms.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/memstick/host/rtsx_usb_ms.c b/drivers/memstick/host/rtsx_usb_ms.c index 4f64563df7de..cd12f3d1c088 100644 --- a/drivers/memstick/host/rtsx_usb_ms.c +++ b/drivers/memstick/host/rtsx_usb_ms.c @@ -785,7 +785,7 @@ static int rtsx_usb_ms_drv_remove(struct platform_device *pdev) mutex_lock(&host->host_mutex); if (host->req) { - dev_dbg(&(pdev->dev), + dev_dbg(ms_dev(host), "%s: Controller removed during transfer\n", dev_name(&msh->dev)); host->req->error = -ENOMEDIUM; @@ -807,10 +807,10 @@ static int rtsx_usb_ms_drv_remove(struct platform_device *pdev) if (pm_runtime_active(ms_dev(host))) pm_runtime_put(ms_dev(host)); - pm_runtime_disable(&pdev->dev); + pm_runtime_disable(ms_dev(host)); platform_set_drvdata(pdev, NULL); - dev_dbg(&(pdev->dev), + dev_dbg(ms_dev(host), ": Realtek USB Memstick controller has been removed\n"); return 0; -- 2.17.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 0/5 v2] Keep rtsx_usb suspended when there's no card @ 2018-07-23 10:27 Kai-Heng Feng 2018-07-23 10:27 ` [PATCH 3/5] memstick: rtsx_usb_ms: Use ms_dev() helper Kai-Heng Feng 0 siblings, 1 reply; 18+ messages in thread From: Kai-Heng Feng @ 2018-07-23 10:27 UTC (permalink / raw) To: arnd, gregkh Cc: ulf.hansson, stern, bauer.chen, ricky_wu, linux-kernel, linux-usb, Kai-Heng Feng Hi, This is based on Ulf's work [1] [2]. This patch series can keep rtsx_usb suspended, to save ~0.5W on Intel platforms and ~1.5W on AMD platforms. [1] https://patchwork.kernel.org/patch/10440583/ [2] https://patchwork.kernel.org/patch/10445725/ Kai-Heng Feng (5): misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection memstick: Prevent memstick host from getting runtime suspended during card detection memstick: rtsx_usb_ms: Use ms_dev() helper memstick: rtsx_usb_ms: Support runtime power management misc: rtsx_usb: / memstick: rtsx_usb_ms: Avoid long delay before system suspend v2: Sends to linux-usb and LKML. drivers/memstick/core/memstick.c | 4 + drivers/memstick/host/rtsx_usb_ms.c | 145 ++++++++++++++++------------ drivers/misc/cardreader/rtsx_usb.c | 14 +++ 3 files changed, 99 insertions(+), 64 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/5] memstick: rtsx_usb_ms: Use ms_dev() helper 2018-07-23 10:27 [PATCH 0/5 v2] Keep rtsx_usb suspended when there's no card Kai-Heng Feng @ 2018-07-23 10:27 ` Kai-Heng Feng 0 siblings, 0 replies; 18+ messages in thread From: Kai-Heng Feng @ 2018-07-23 10:27 UTC (permalink / raw) To: arnd, gregkh Cc: ulf.hansson, stern, bauer.chen, ricky_wu, linux-kernel, linux-usb, Kai-Heng Feng Use ms_dev() helper for consistency. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/memstick/host/rtsx_usb_ms.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/memstick/host/rtsx_usb_ms.c b/drivers/memstick/host/rtsx_usb_ms.c index 4f64563df7de..d56f0863653f 100644 --- a/drivers/memstick/host/rtsx_usb_ms.c +++ b/drivers/memstick/host/rtsx_usb_ms.c @@ -807,7 +807,7 @@ static int rtsx_usb_ms_drv_remove(struct platform_device *pdev) if (pm_runtime_active(ms_dev(host))) pm_runtime_put(ms_dev(host)); - pm_runtime_disable(&pdev->dev); + pm_runtime_disable(ms_dev(host)); platform_set_drvdata(pdev, NULL); dev_dbg(&(pdev->dev), -- 2.17.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-09-17 8:07 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-31 6:17 [PATCH 0/5 v4] Keep rtsx_usb suspended when there's no card Kai-Heng Feng 2018-07-31 6:17 ` [PATCH 1/5] misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection Kai-Heng Feng 2018-09-13 10:31 ` Kai-Heng Feng 2018-09-13 13:41 ` Alan Stern 2018-09-17 8:07 ` Kai-Heng Feng 2018-07-31 6:17 ` [PATCH 2/5] memstick: Prevent memstick host from getting runtime suspended during card detection Kai-Heng Feng 2018-07-31 6:17 ` [PATCH 3/5] memstick: rtsx_usb_ms: Use ms_dev() helper Kai-Heng Feng 2018-07-31 6:17 ` [PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power management Kai-Heng Feng 2018-08-01 13:14 ` Ulf Hansson 2018-08-23 8:03 ` Kai-Heng Feng 2018-08-24 6:28 ` Ulf Hansson 2018-08-30 8:36 ` Kai-Heng Feng 2018-08-01 13:29 ` Ulf Hansson 2018-08-23 8:12 ` Kai-Heng Feng 2018-08-24 6:30 ` Ulf Hansson 2018-07-31 6:17 ` [PATCH 5/5] misc: rtsx_usb: / memstick: rtsx_usb_ms: Avoid long delay before system suspend Kai-Heng Feng -- strict thread matches above, loose matches on Subject: below -- 2018-07-25 15:25 [PATCH 0/5 v3] Keep rtsx_usb suspended when there's no card Kai-Heng Feng 2018-07-25 15:25 ` [PATCH 3/5] memstick: rtsx_usb_ms: Use ms_dev() helper Kai-Heng Feng 2018-07-23 10:27 [PATCH 0/5 v2] Keep rtsx_usb suspended when there's no card Kai-Heng Feng 2018-07-23 10:27 ` [PATCH 3/5] memstick: rtsx_usb_ms: Use ms_dev() helper Kai-Heng Feng
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).