Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=us-ascii;
        delsp=yes;
        format=flowed
Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.1\))
Subject: Re: [PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power
 management
From:   Kai-Heng Feng <kai.heng.feng@canonical.com>
In-Reply-To: <CAPDyKFqXBMOP+_1h4WVF1Ob_dhBfr1JC+MFfSYdp1uRmzZhZ-Q@mail.gmail.com>
Date:   Thu, 30 Aug 2018 16:36:16 +0800
Cc:     Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Alan Stern <stern@rowland.harvard.edu>,
        "Bauer.Chen" <bauer.chen@realtek.com>, ricky_wu@realtek.com,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux USB List <linux-usb@vger.kernel.org>
Content-Transfer-Encoding: 7bit
Message-Id: <3F123073-8A73-4E4C-B4A6-9777022D9927@canonical.com>
References: <20180731061721.15831-1-kai.heng.feng@canonical.com>
 <20180731061721.15831-5-kai.heng.feng@canonical.com>
 <CAPDyKFqv5apJp=zbyoZi-UE8+omu7JauJ5_7+ymY9B2BSS48bA@mail.gmail.com>
 <E133AFB3-27F6-4DD3-BB85-A85EDB51EE57@canonical.com>
 <CAPDyKFqXBMOP+_1h4WVF1Ob_dhBfr1JC+MFfSYdp1uRmzZhZ-Q@mail.gmail.com>
To:     Ulf Hansson <ulf.hansson@linaro.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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