Messages in this thread | | | Subject | Re: [PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power management | From | Kai-Heng Feng <> | Date | Thu, 30 Aug 2018 16:36:16 +0800 |
| |
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
| |