Re: [PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power management

From: Kai-Heng Feng
Date: Thu Aug 30 2018 - 04:36:27 EST

at 14:28, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:

On 23 August 2018 at 10:03, Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx> wrote:
Hi Ulf,

Sorry for the late reply.

at 21:14, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:

On 31 July 2018 at 08:17, Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
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

Use an idle function check if there's no card and the power mode is
MEMSTICK_POWER_OFF. If both criteria are met, put the device to suspend.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
drivers/memstick/host/rtsx_usb_ms.c | 138 ++++++++++++++--------------
1 file changed, 71 insertions(+), 67 deletions(-)

diff --git a/drivers/memstick/host/rtsx_usb_ms.c
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
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

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
} 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.



Kind regards