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>
>> wrote:
>>>
>>> In order to let host's parent device, rtsx_usb, to use USB remote wake
>>> up signaling to do card detection, it needs to be suspended. Hence it's
>>> necessary to add runtime PM support for the memstick host.
>>>
>>> To keep memstick host stays suspended when it's not in use, convert the
>>> card detection function from kthread to delayed_work, which can be
>>> scheduled when the host is resumed and can be canceled when the host is
>>> suspended.
>>>
>>> Use an idle function check if there's no card and the power mode is
>>> MEMSTICK_POWER_OFF. If both criteria are met, put the device to suspend.
>>>
>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
>>> ---
>>> drivers/memstick/host/rtsx_usb_ms.c | 138 ++++++++++++++--------------
>>> 1 file changed, 71 insertions(+), 67 deletions(-)
>>>
>>> diff --git a/drivers/memstick/host/rtsx_usb_ms.c
>>> b/drivers/memstick/host/rtsx_usb_ms.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
