* [PATCH 0/4 v6] Keep rtsx_usb suspended when there's no card
@ 2018-10-31 6:58 Kai-Heng Feng
2018-10-31 6:59 ` [PATCH 1/4 v6] misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection Kai-Heng Feng
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Kai-Heng Feng @ 2018-10-31 6:58 UTC (permalink / raw)
To: arnd, gregkh; +Cc: ulf.hansson, stern, linux-usb, linux-kernel, 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 (4):
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
v6: Move pm_runtime_get_noresume() to media_check(), to avoid
potentially unblanced rpm count.
v5: Corretly use system suspend/resume and runtime suspend/resume callback.
Prevent runtime callbacks get call during 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.
drivers/memstick/core/memstick.c | 3 +
drivers/memstick/host/rtsx_usb_ms.c | 173 +++++++++++++++++-----------
drivers/misc/cardreader/rtsx_usb.c | 8 ++
3 files changed, 116 insertions(+), 68 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/4 v6] misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection
2018-10-31 6:58 [PATCH 0/4 v6] Keep rtsx_usb suspended when there's no card Kai-Heng Feng
@ 2018-10-31 6:59 ` Kai-Heng Feng
2018-10-31 6:59 ` [PATCH 2/4 v6] memstick: Prevent memstick host from getting runtime suspended during card detection Kai-Heng Feng
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Kai-Heng Feng @ 2018-10-31 6:59 UTC (permalink / raw)
To: arnd, gregkh; +Cc: ulf.hansson, stern, linux-usb, linux-kernel, 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] 6+ messages in thread
* [PATCH 2/4 v6] memstick: Prevent memstick host from getting runtime suspended during card detection
2018-10-31 6:58 [PATCH 0/4 v6] Keep rtsx_usb suspended when there's no card Kai-Heng Feng
2018-10-31 6:59 ` [PATCH 1/4 v6] misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection Kai-Heng Feng
@ 2018-10-31 6:59 ` Kai-Heng Feng
2018-10-31 6:59 ` [PATCH 3/4 v6] memstick: rtsx_usb_ms: Use ms_dev() helper Kai-Heng Feng
2018-10-31 6:59 ` [PATCH 4/4 v6] memstick: rtsx_usb_ms: Support runtime power management Kai-Heng Feng
3 siblings, 0 replies; 6+ messages in thread
From: Kai-Heng Feng @ 2018-10-31 6:59 UTC (permalink / raw)
To: arnd, gregkh; +Cc: ulf.hansson, stern, linux-usb, linux-kernel, Kai-Heng Feng
We can use MEMSTICK_POWER_{ON,OFF} along with pm_runtime_{get,put}
helpers to let memstick host support runtime pm.
The rpm count may go down to zero before the memstick host powers on, so
the host can be runtime suspended.
So before doing card detection, increment the rpm count to avoid the
host gets runtime suspended. Balance the rpm count after card detection
is done.
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
drivers/memstick/core/memstick.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
index 76382c858c35..1246d69ba187 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"
@@ -436,6 +437,7 @@ static void memstick_check(struct work_struct *work)
struct memstick_dev *card;
dev_dbg(&host->dev, "memstick_check started\n");
+ pm_runtime_get_noresume(host->dev.parent);
mutex_lock(&host->lock);
if (!host->card) {
if (memstick_power_on(host))
@@ -479,6 +481,7 @@ 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] 6+ messages in thread
* [PATCH 3/4 v6] memstick: rtsx_usb_ms: Use ms_dev() helper
2018-10-31 6:58 [PATCH 0/4 v6] Keep rtsx_usb suspended when there's no card Kai-Heng Feng
2018-10-31 6:59 ` [PATCH 1/4 v6] misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection Kai-Heng Feng
2018-10-31 6:59 ` [PATCH 2/4 v6] memstick: Prevent memstick host from getting runtime suspended during card detection Kai-Heng Feng
@ 2018-10-31 6:59 ` Kai-Heng Feng
2018-10-31 6:59 ` [PATCH 4/4 v6] memstick: rtsx_usb_ms: Support runtime power management Kai-Heng Feng
3 siblings, 0 replies; 6+ messages in thread
From: Kai-Heng Feng @ 2018-10-31 6:59 UTC (permalink / raw)
To: arnd, gregkh; +Cc: ulf.hansson, stern, linux-usb, linux-kernel, 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] 6+ messages in thread
* [PATCH 4/4 v6] memstick: rtsx_usb_ms: Support runtime power management
2018-10-31 6:58 [PATCH 0/4 v6] Keep rtsx_usb suspended when there's no card Kai-Heng Feng
` (2 preceding siblings ...)
2018-10-31 6:59 ` [PATCH 3/4 v6] memstick: rtsx_usb_ms: Use ms_dev() helper Kai-Heng Feng
@ 2018-10-31 6:59 ` Kai-Heng Feng
2018-10-31 10:40 ` Ulf Hansson
3 siblings, 1 reply; 6+ messages in thread
From: Kai-Heng Feng @ 2018-10-31 6:59 UTC (permalink / raw)
To: arnd, gregkh; +Cc: ulf.hansson, stern, linux-usb, linux-kernel, 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.
Put the device to suspend when there's no card and the power mode is
MEMSTICK_POWER_OFF.
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
drivers/memstick/host/rtsx_usb_ms.c | 167 +++++++++++++++++-----------
1 file changed, 102 insertions(+), 65 deletions(-)
diff --git a/drivers/memstick/host/rtsx_usb_ms.c b/drivers/memstick/host/rtsx_usb_ms.c
index cd12f3d1c088..3800c24b084e 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 system_suspending;
};
static inline struct device *ms_dev(struct rtsx_usb_ms *host)
@@ -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_sync(ms_dev(host));
}
}
@@ -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,25 +637,44 @@ 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_sync(ms_dev(host));
/* power-on delay */
- if (param == MEMSTICK_POWER && value == MEMSTICK_POWER_ON)
+ if (param == MEMSTICK_POWER && value == MEMSTICK_POWER_ON) {
usleep_range(10000, 12000);
+ if (!host->eject)
+ schedule_delayed_work(&host->poll_card, 100);
+ }
+
dev_dbg(ms_dev(host), "%s: return = %d\n", __func__, err);
return err;
}
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM
static int rtsx_usb_ms_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__);
+ /* Since we use rtsx_usb's resume callback to runtime resume its
+ * children to implement remote wakeup signaling, this causes
+ * rtsx_usb_ms' runtime resume callback runs after its suspend
+ * callback:
+ * rtsx_usb_ms_suspend()
+ * rtsx_usb_resume()
+ * -> rtsx_usb_ms_runtime_resume()
+ * -> memstick_detect_change()
+ *
+ * rtsx_usb_suspend()
+ *
+ * To avoid this, skip runtime resume/suspend if system suspend is
+ * underway.
+ */
+ host->system_suspending = true;
memstick_suspend_host(msh);
+
return 0;
}
@@ -665,58 +683,83 @@ static int rtsx_usb_ms_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->system_suspending = false;
+
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_suspend(struct device *dev)
+{
+ struct rtsx_usb_ms *host = dev_get_drvdata(dev);
+
+ if (host->system_suspending)
+ return 0;
+
+ if (host->msh->card || host->power_mode != MEMSTICK_POWER_OFF)
+ return -EAGAIN;
+
+ return 0;
+}
+
+static int rtsx_usb_ms_runtime_resume(struct device *dev)
+{
+ struct rtsx_usb_ms *host = dev_get_drvdata(dev);
+
+
+ if (host->system_suspending)
+ return 0;
+
+ memstick_detect_change(host->msh);
+
+ return 0;
+}
+
+static const struct dev_pm_ops rtsx_usb_ms_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(rtsx_usb_ms_suspend, rtsx_usb_ms_resume)
+ SET_RUNTIME_PM_OPS(rtsx_usb_ms_runtime_suspend, rtsx_usb_ms_runtime_resume, NULL)
+};
+
+#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);
-
- /* 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 (host->eject || host->power_mode != MEMSTICK_POWER_ON)
+ return;
- /* 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_sync(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);
- 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;
+poll_again:
+ pm_runtime_put_sync(ms_dev(host));
+
+ if (!host->eject && host->power_mode == MEMSTICK_POWER_ON)
+ schedule_delayed_work(&host->poll_card, 100);
}
static int rtsx_usb_ms_drv_probe(struct platform_device *pdev)
@@ -747,39 +790,35 @@ 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));
+
+ pm_runtime_get_sync(ms_dev(host));
err = memstick_add_host(msh);
if (err)
goto err_out;
- wake_up_process(host->detect_ms);
+ pm_runtime_put(ms_dev(host));
+
return 0;
err_out:
memstick_free_host(msh);
+ pm_runtime_put_noidle(ms_dev(host));
return err;
}
static int rtsx_usb_ms_drv_remove(struct platform_device *pdev)
{
struct rtsx_usb_ms *host = platform_get_drvdata(pdev);
- struct memstick_host *msh;
+ struct memstick_host *msh = host->msh;
int err;
- msh = host->msh;
host->eject = true;
cancel_work_sync(&host->handle_req);
@@ -797,7 +836,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 +854,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 +869,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] 6+ messages in thread
* Re: [PATCH 4/4 v6] memstick: rtsx_usb_ms: Support runtime power management
2018-10-31 6:59 ` [PATCH 4/4 v6] memstick: rtsx_usb_ms: Support runtime power management Kai-Heng Feng
@ 2018-10-31 10:40 ` Ulf Hansson
0 siblings, 0 replies; 6+ messages in thread
From: Ulf Hansson @ 2018-10-31 10:40 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: Arnd Bergmann, Greg Kroah-Hartman, Alan Stern, Linux USB List,
Linux Kernel Mailing List
On 31 October 2018 at 07:59, 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.
>
> Put the device to suspend when there's no card and the power mode is
> MEMSTICK_POWER_OFF.
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> drivers/memstick/host/rtsx_usb_ms.c | 167 +++++++++++++++++-----------
> 1 file changed, 102 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/memstick/host/rtsx_usb_ms.c b/drivers/memstick/host/rtsx_usb_ms.c
> index cd12f3d1c088..3800c24b084e 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 system_suspending;
> };
>
> static inline struct device *ms_dev(struct rtsx_usb_ms *host)
> @@ -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_sync(ms_dev(host));
> }
>
> }
> @@ -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,25 +637,44 @@ 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_sync(ms_dev(host));
>
> /* power-on delay */
> - if (param == MEMSTICK_POWER && value == MEMSTICK_POWER_ON)
> + if (param == MEMSTICK_POWER && value == MEMSTICK_POWER_ON) {
> usleep_range(10000, 12000);
>
> + if (!host->eject)
> + schedule_delayed_work(&host->poll_card, 100);
> + }
> +
> dev_dbg(ms_dev(host), "%s: return = %d\n", __func__, err);
> return err;
> }
>
> -#ifdef CONFIG_PM_SLEEP
> +#ifdef CONFIG_PM
I think you should keep CONFIG_PM_SLEEP, else this triggers a warning
about unused functions, when CONFIG_PM is set but CONFIG_PM_SLEEP is
unset.
> static int rtsx_usb_ms_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__);
> + /* Since we use rtsx_usb's resume callback to runtime resume its
> + * children to implement remote wakeup signaling, this causes
> + * rtsx_usb_ms' runtime resume callback runs after its suspend
> + * callback:
> + * rtsx_usb_ms_suspend()
> + * rtsx_usb_resume()
> + * -> rtsx_usb_ms_runtime_resume()
> + * -> memstick_detect_change()
> + *
> + * rtsx_usb_suspend()
> + *
> + * To avoid this, skip runtime resume/suspend if system suspend is
> + * underway.
> + */
>
> + host->system_suspending = true;
> memstick_suspend_host(msh);
> +
> return 0;
> }
>
> @@ -665,58 +683,83 @@ static int rtsx_usb_ms_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->system_suspending = false;
> +
> 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)
Because of the above comment, then add:
#ifdef CONFIG_PM
> +static int rtsx_usb_ms_runtime_suspend(struct device *dev)
> +{
> + struct rtsx_usb_ms *host = dev_get_drvdata(dev);
> +
> + if (host->system_suspending)
> + return 0;
> +
> + if (host->msh->card || host->power_mode != MEMSTICK_POWER_OFF)
> + return -EAGAIN;
> +
> + return 0;
> +}
> +
> +static int rtsx_usb_ms_runtime_resume(struct device *dev)
> +{
> + struct rtsx_usb_ms *host = dev_get_drvdata(dev);
> +
> +
> + if (host->system_suspending)
> + return 0;
> +
> + memstick_detect_change(host->msh);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops rtsx_usb_ms_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(rtsx_usb_ms_suspend, rtsx_usb_ms_resume)
> + SET_RUNTIME_PM_OPS(rtsx_usb_ms_runtime_suspend, rtsx_usb_ms_runtime_resume, NULL)
> +};
> +
> +#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);
> -
> - /* 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 (host->eject || host->power_mode != MEMSTICK_POWER_ON)
> + return;
>
> - /* 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_sync(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);
>
> - 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;
> +poll_again:
> + pm_runtime_put_sync(ms_dev(host));
> +
> + if (!host->eject && host->power_mode == MEMSTICK_POWER_ON)
> + schedule_delayed_work(&host->poll_card, 100);
> }
>
> static int rtsx_usb_ms_drv_probe(struct platform_device *pdev)
> @@ -747,39 +790,35 @@ 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));
> +
> + pm_runtime_get_sync(ms_dev(host));
I would rather re-place/re-order this with:
pm_runtime_get_noresume()
pm_runtime_set_active()
pm_runtime_enable()
> err = memstick_add_host(msh);
> if (err)
> goto err_out;
>
> - wake_up_process(host->detect_ms);
> + pm_runtime_put(ms_dev(host));
> +
> return 0;
> err_out:
> memstick_free_host(msh);
Looks like you need a pm_runtime_disable(); here as well. However,
that seems like an existing bug, perhaps it deserves it own change.
> + pm_runtime_put_noidle(ms_dev(host));
> return err;
> }
>
[...]
Kind regards
Uffe
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-31 10:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-31 6:58 [PATCH 0/4 v6] Keep rtsx_usb suspended when there's no card Kai-Heng Feng
2018-10-31 6:59 ` [PATCH 1/4 v6] misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection Kai-Heng Feng
2018-10-31 6:59 ` [PATCH 2/4 v6] memstick: Prevent memstick host from getting runtime suspended during card detection Kai-Heng Feng
2018-10-31 6:59 ` [PATCH 3/4 v6] memstick: rtsx_usb_ms: Use ms_dev() helper Kai-Heng Feng
2018-10-31 6:59 ` [PATCH 4/4 v6] memstick: rtsx_usb_ms: Support runtime power management Kai-Heng Feng
2018-10-31 10:40 ` Ulf Hansson
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).