[PATCH 0/4 v5] Keep rtsx_usb suspended when there's no card

[PATCH 0/4 v5] Keep rtsx_usb suspended when there's no card

[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

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    |   4 +
 drivers/memstick/host/rtsx_usb_ms.c | 173 +++++++++++++++++-----------
 drivers/misc/cardreader/rtsx_usb.c  |   8 ++
 3 files changed, 117 insertions(+), 68 deletions(-)

-- 
2.17.1

[PATCH 1/4 v5] misc: rtsx_usb: Use USB remote wakeup signaling for card insertion detection

[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

[PATCH 2/4 v5] memstick: Prevent memstick host from getting runtime suspended during card detection

[Kai-Heng Feng]

We can use MEMSTICK_POWER_{ON,OFF} along with pm_runtime_{get,put}
helpers to let memstick host support runtime pm.

There's a small window between memstick_detect_change() and its queued
work, memstick_check(). In this window the rpm count may go down to zero
before the memstick host powers on, so the host can be inadvertently
suspended.

Increment rpm count before calling memstick_check(), and decrement rpm
count afterward, as now we are sure the memstick host should be
suspended or not.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/memstick/core/memstick.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
index 76382c858c35..5f16a8826401 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"
 
@@ -209,6 +210,7 @@ static int memstick_dummy_check(struct memstick_dev *card)
  */
 void memstick_detect_change(struct memstick_host *host)
 {
+	pm_runtime_get_noresume(host->dev.parent);
 	queue_work(workqueue, &host->media_checker);
 }
 EXPORT_SYMBOL(memstick_detect_change);
@@ -479,6 +481,8 @@ 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

Re: [PATCH 2/4 v5] memstick: Prevent memstick host from getting runtime suspended during card detection

[Ulf Hansson]

On 24 October 2018 at 10:49, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
> We can use MEMSTICK_POWER_{ON,OFF} along with pm_runtime_{get,put}
> helpers to let memstick host support runtime pm.
>
> There's a small window between memstick_detect_change() and its queued
> work, memstick_check(). In this window the rpm count may go down to zero
> before the memstick host powers on, so the host can be inadvertently
> suspended.
>
> Increment rpm count before calling memstick_check(), and decrement rpm
> count afterward, as now we are sure the memstick host should be
> suspended or not.
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/memstick/core/memstick.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
> index 76382c858c35..5f16a8826401 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"
>
> @@ -209,6 +210,7 @@ static int memstick_dummy_check(struct memstick_dev *card)
>   */
>  void memstick_detect_change(struct memstick_host *host)
>  {
> +       pm_runtime_get_noresume(host->dev.parent);
>         queue_work(workqueue, &host->media_checker);
>  }
>  EXPORT_SYMBOL(memstick_detect_change);
> @@ -479,6 +481,8 @@ 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");
>  }
>

I am not sure this works, sorry.

More precisely, I don't think there is a guarantee that the calls to
pm_runtime_get|put*() becomes properly balanced. In principle
memstick_detect_change() could be called, without actually causing a
new work to be scheduled if there is already such a work in the queue
(depends on the workqueue configuration). Isn't it so?

Kind regards
Uffe

Re: [PATCH 2/4 v5] memstick: Prevent memstick host from getting runtime suspended during card detection

[Kai Heng Feng]

> On Oct 29, 2018, at 20:25, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> 
> On 24 October 2018 at 10:49, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
>> We can use MEMSTICK_POWER_{ON,OFF} along with pm_runtime_{get,put}
>> helpers to let memstick host support runtime pm.
>> 
>> There's a small window between memstick_detect_change() and its queued
>> work, memstick_check(). In this window the rpm count may go down to zero
>> before the memstick host powers on, so the host can be inadvertently
>> suspended.
>> 
>> Increment rpm count before calling memstick_check(), and decrement rpm
>> count afterward, as now we are sure the memstick host should be
>> suspended or not.
>> 
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> ---
>> drivers/memstick/core/memstick.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>> 
>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
>> index 76382c858c35..5f16a8826401 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"
>> 
>> @@ -209,6 +210,7 @@ static int memstick_dummy_check(struct memstick_dev *card)
>>  */
>> void memstick_detect_change(struct memstick_host *host)
>> {
>> +       pm_runtime_get_noresume(host->dev.parent);
>>        queue_work(workqueue, &host->media_checker);
>> }
>> EXPORT_SYMBOL(memstick_detect_change);
>> @@ -479,6 +481,8 @@ 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");
>> }
>> 
> 
> I am not sure this works, sorry.
> 
> More precisely, I don't think there is a guarantee that the calls to
> pm_runtime_get|put*() becomes properly balanced. In principle
> memstick_detect_change() could be called, without actually causing a
> new work to be scheduled if there is already such a work in the queue
> (depends on the workqueue configuration). Isn't it so?

You are right.

We can use test_and_set_bit() or alike to properly balance pm_runtime
helpers, but the most straightforward solution in my mind is to merge 
memstick_detect_change() and memstick_check() as one function.

memstick_detect_change() it’s the only user of memstick_check() anyway.

Or is there a better way in your mind?

Kai-Heng

> 
> Kind regards
> Uffe

Re: [PATCH 2/4 v5] memstick: Prevent memstick host from getting runtime suspended during card detection

[Ulf Hansson]

On 29 October 2018 at 17:31, Kai Heng Feng <kai.heng.feng@canonical.com> wrote:
>
>
>> On Oct 29, 2018, at 20:25, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>
>> On 24 October 2018 at 10:49, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
>>> We can use MEMSTICK_POWER_{ON,OFF} along with pm_runtime_{get,put}
>>> helpers to let memstick host support runtime pm.
>>>
>>> There's a small window between memstick_detect_change() and its queued
>>> work, memstick_check(). In this window the rpm count may go down to zero
>>> before the memstick host powers on, so the host can be inadvertently
>>> suspended.
>>>
>>> Increment rpm count before calling memstick_check(), and decrement rpm
>>> count afterward, as now we are sure the memstick host should be
>>> suspended or not.
>>>
>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>> ---
>>> drivers/memstick/core/memstick.c | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
>>> index 76382c858c35..5f16a8826401 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"
>>>
>>> @@ -209,6 +210,7 @@ static int memstick_dummy_check(struct memstick_dev *card)
>>>  */
>>> void memstick_detect_change(struct memstick_host *host)
>>> {
>>> +       pm_runtime_get_noresume(host->dev.parent);
>>>        queue_work(workqueue, &host->media_checker);
>>> }
>>> EXPORT_SYMBOL(memstick_detect_change);
>>> @@ -479,6 +481,8 @@ 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");
>>> }
>>>
>>
>> I am not sure this works, sorry.
>>
>> More precisely, I don't think there is a guarantee that the calls to
>> pm_runtime_get|put*() becomes properly balanced. In principle
>> memstick_detect_change() could be called, without actually causing a
>> new work to be scheduled if there is already such a work in the queue
>> (depends on the workqueue configuration). Isn't it so?
>
> You are right.
>
> We can use test_and_set_bit() or alike to properly balance pm_runtime
> helpers, but the most straightforward solution in my mind is to merge
> memstick_detect_change() and memstick_check() as one function.
>
> memstick_detect_change() it’s the only user of memstick_check() anyway.

I suspect memstick_detect_change() is supposed to be called by host
drivers, when they receive some kind of notification due to a card
being inserted or removed. I guess that happen (at least
hypothetically) also from atomic (IRQ) context.

As memstick_check() is doing hole bunch of operations, I am not sure
bypassing the work-queue is a good idea, if that is what you are
proposing.

>
> Or is there a better way in your mind?

I don't know.

Well, I am not sure I understand why you need to call
pm_runtime_get_noresume() from memstick_detect_change() in the first
place. Could you explain that in more detail?

Kind regards
Uffe

Re: [PATCH 2/4 v5] memstick: Prevent memstick host from getting runtime suspended during card detection

[Kai Heng Feng]

> On Oct 30, 2018, at 21:03, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> 
> On 29 October 2018 at 17:31, Kai Heng Feng <kai.heng.feng@canonical.com> wrote:
>> 
>> 
>>> On Oct 29, 2018, at 20:25, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> 
>>> On 24 October 2018 at 10:49, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
>>>> We can use MEMSTICK_POWER_{ON,OFF} along with pm_runtime_{get,put}
>>>> helpers to let memstick host support runtime pm.
>>>> 
>>>> There's a small window between memstick_detect_change() and its queued
>>>> work, memstick_check(). In this window the rpm count may go down to zero
>>>> before the memstick host powers on, so the host can be inadvertently
>>>> suspended.
>>>> 
>>>> Increment rpm count before calling memstick_check(), and decrement rpm
>>>> count afterward, as now we are sure the memstick host should be
>>>> suspended or not.
>>>> 
>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>>> ---
>>>> drivers/memstick/core/memstick.c | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>> 
>>>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
>>>> index 76382c858c35..5f16a8826401 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"
>>>> 
>>>> @@ -209,6 +210,7 @@ static int memstick_dummy_check(struct memstick_dev *card)
>>>> */
>>>> void memstick_detect_change(struct memstick_host *host)
>>>> {
>>>> +       pm_runtime_get_noresume(host->dev.parent);
>>>>       queue_work(workqueue, &host->media_checker);
>>>> }
>>>> EXPORT_SYMBOL(memstick_detect_change);
>>>> @@ -479,6 +481,8 @@ 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");
>>>> }
>>>> 
>>> 
>>> I am not sure this works, sorry.
>>> 
>>> More precisely, I don't think there is a guarantee that the calls to
>>> pm_runtime_get|put*() becomes properly balanced. In principle
>>> memstick_detect_change() could be called, without actually causing a
>>> new work to be scheduled if there is already such a work in the queue
>>> (depends on the workqueue configuration). Isn't it so?
>> 
>> You are right.
>> 
>> We can use test_and_set_bit() or alike to properly balance pm_runtime
>> helpers, but the most straightforward solution in my mind is to merge
>> memstick_detect_change() and memstick_check() as one function.
>> 
>> memstick_detect_change() it’s the only user of memstick_check() anyway.
> 
> I suspect memstick_detect_change() is supposed to be called by host
> drivers, when they receive some kind of notification due to a card
> being inserted or removed. I guess that happen (at least
> hypothetically) also from atomic (IRQ) context.
> 
> As memstick_check() is doing hole bunch of operations, I am not sure
> bypassing the work-queue is a good idea, if that is what you are
> proposing.

Okay, it’s better to keep it that way.

> 
>> 
>> Or is there a better way in your mind?
> 
> I don't know.
> 
> Well, I am not sure I understand why you need to call
> pm_runtime_get_noresume() from memstick_detect_change() in the first
> place. Could you explain that in more detail?

I guess it didn’t explain it well enough in the log, let me add some detail:
There's a small window between memstick_detect_change() and its queued
work, memstick_check(). In this window the rpm count may go down to zero
before the memstick host powers on, where I use
pm_runtime_get_noresume() to increment the rpm count.

memstick_check() uses some functions in rtsx_usb_ms that have
pm_runtime_put*() so the rpm count may go down to zero, before the
memstick host powers on.

Kai-Heng

> 
> Kind regards
> Uffe

Re: [PATCH 2/4 v5] memstick: Prevent memstick host from getting runtime suspended during card detection

[Ulf Hansson]

On 30 October 2018 at 16:23, Kai Heng Feng <kai.heng.feng@canonical.com> wrote:
>
>
>> On Oct 30, 2018, at 21:03, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>
>> On 29 October 2018 at 17:31, Kai Heng Feng <kai.heng.feng@canonical.com> wrote:
>>>
>>>
>>>> On Oct 29, 2018, at 20:25, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>
>>>> On 24 October 2018 at 10:49, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
>>>>> We can use MEMSTICK_POWER_{ON,OFF} along with pm_runtime_{get,put}
>>>>> helpers to let memstick host support runtime pm.
>>>>>
>>>>> There's a small window between memstick_detect_change() and its queued
>>>>> work, memstick_check(). In this window the rpm count may go down to zero
>>>>> before the memstick host powers on, so the host can be inadvertently
>>>>> suspended.
>>>>>
>>>>> Increment rpm count before calling memstick_check(), and decrement rpm
>>>>> count afterward, as now we are sure the memstick host should be
>>>>> suspended or not.
>>>>>
>>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>>>> ---
>>>>> drivers/memstick/core/memstick.c | 4 ++++
>>>>> 1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
>>>>> index 76382c858c35..5f16a8826401 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"
>>>>>
>>>>> @@ -209,6 +210,7 @@ static int memstick_dummy_check(struct memstick_dev *card)
>>>>> */
>>>>> void memstick_detect_change(struct memstick_host *host)
>>>>> {
>>>>> +       pm_runtime_get_noresume(host->dev.parent);
>>>>>       queue_work(workqueue, &host->media_checker);
>>>>> }
>>>>> EXPORT_SYMBOL(memstick_detect_change);
>>>>> @@ -479,6 +481,8 @@ 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");
>>>>> }
>>>>>
>>>>
>>>> I am not sure this works, sorry.
>>>>
>>>> More precisely, I don't think there is a guarantee that the calls to
>>>> pm_runtime_get|put*() becomes properly balanced. In principle
>>>> memstick_detect_change() could be called, without actually causing a
>>>> new work to be scheduled if there is already such a work in the queue
>>>> (depends on the workqueue configuration). Isn't it so?
>>>
>>> You are right.
>>>
>>> We can use test_and_set_bit() or alike to properly balance pm_runtime
>>> helpers, but the most straightforward solution in my mind is to merge
>>> memstick_detect_change() and memstick_check() as one function.
>>>
>>> memstick_detect_change() it’s the only user of memstick_check() anyway.
>>
>> I suspect memstick_detect_change() is supposed to be called by host
>> drivers, when they receive some kind of notification due to a card
>> being inserted or removed. I guess that happen (at least
>> hypothetically) also from atomic (IRQ) context.
>>
>> As memstick_check() is doing hole bunch of operations, I am not sure
>> bypassing the work-queue is a good idea, if that is what you are
>> proposing.
>
> Okay, it’s better to keep it that way.
>
>>
>>>
>>> Or is there a better way in your mind?
>>
>> I don't know.
>>
>> Well, I am not sure I understand why you need to call
>> pm_runtime_get_noresume() from memstick_detect_change() in the first
>> place. Could you explain that in more detail?
>
> I guess it didn’t explain it well enough in the log, let me add some detail:
> There's a small window between memstick_detect_change() and its queued
> work, memstick_check(). In this window the rpm count may go down to zero
> before the memstick host powers on, where I use
> pm_runtime_get_noresume() to increment the rpm count.
>
> memstick_check() uses some functions in rtsx_usb_ms that have
> pm_runtime_put*() so the rpm count may go down to zero, before the
> memstick host powers on.

So then, why doesn't memstick_check() early on calls
pm_runtime_get_sync() and when it has finished with probing for a
card, balance that with a call pm_runtime_put()?

Kind regards
Uffe

Re: [PATCH 2/4 v5] memstick: Prevent memstick host from getting runtime suspended during card detection

[Kai Heng Feng]

> On Oct 31, 2018, at 12:04 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> 
> On 30 October 2018 at 16:23, Kai Heng Feng <kai.heng.feng@canonical.com> wrote:
>> 
>> 
>>> On Oct 30, 2018, at 21:03, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> 
>>> On 29 October 2018 at 17:31, Kai Heng Feng <kai.heng.feng@canonical.com> wrote:
>>>> 
>>>> 
>>>>> On Oct 29, 2018, at 20:25, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>> 
>>>>> On 24 October 2018 at 10:49, Kai-Heng Feng <kai.heng.feng@canonical.com> wrote:
>>>>>> We can use MEMSTICK_POWER_{ON,OFF} along with pm_runtime_{get,put}
>>>>>> helpers to let memstick host support runtime pm.
>>>>>> 
>>>>>> There's a small window between memstick_detect_change() and its queued
>>>>>> work, memstick_check(). In this window the rpm count may go down to zero
>>>>>> before the memstick host powers on, so the host can be inadvertently
>>>>>> suspended.
>>>>>> 
>>>>>> Increment rpm count before calling memstick_check(), and decrement rpm
>>>>>> count afterward, as now we are sure the memstick host should be
>>>>>> suspended or not.
>>>>>> 
>>>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>>>>> ---
>>>>>> drivers/memstick/core/memstick.c | 4 ++++
>>>>>> 1 file changed, 4 insertions(+)
>>>>>> 
>>>>>> diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
>>>>>> index 76382c858c35..5f16a8826401 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"
>>>>>> 
>>>>>> @@ -209,6 +210,7 @@ static int memstick_dummy_check(struct memstick_dev *card)
>>>>>> */
>>>>>> void memstick_detect_change(struct memstick_host *host)
>>>>>> {
>>>>>> +       pm_runtime_get_noresume(host->dev.parent);
>>>>>>      queue_work(workqueue, &host->media_checker);
>>>>>> }
>>>>>> EXPORT_SYMBOL(memstick_detect_change);
>>>>>> @@ -479,6 +481,8 @@ 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");
>>>>>> }
>>>>>> 
>>>>> 
>>>>> I am not sure this works, sorry.
>>>>> 
>>>>> More precisely, I don't think there is a guarantee that the calls to
>>>>> pm_runtime_get|put*() becomes properly balanced. In principle
>>>>> memstick_detect_change() could be called, without actually causing a
>>>>> new work to be scheduled if there is already such a work in the queue
>>>>> (depends on the workqueue configuration). Isn't it so?
>>>> 
>>>> You are right.
>>>> 
>>>> We can use test_and_set_bit() or alike to properly balance pm_runtime
>>>> helpers, but the most straightforward solution in my mind is to merge
>>>> memstick_detect_change() and memstick_check() as one function.
>>>> 
>>>> memstick_detect_change() it’s the only user of memstick_check() anyway.
>>> 
>>> I suspect memstick_detect_change() is supposed to be called by host
>>> drivers, when they receive some kind of notification due to a card
>>> being inserted or removed. I guess that happen (at least
>>> hypothetically) also from atomic (IRQ) context.
>>> 
>>> As memstick_check() is doing hole bunch of operations, I am not sure
>>> bypassing the work-queue is a good idea, if that is what you are
>>> proposing.
>> 
>> Okay, it’s better to keep it that way.
>> 
>>> 
>>>> 
>>>> Or is there a better way in your mind?
>>> 
>>> I don't know.
>>> 
>>> Well, I am not sure I understand why you need to call
>>> pm_runtime_get_noresume() from memstick_detect_change() in the first
>>> place. Could you explain that in more detail?
>> 
>> I guess it didn’t explain it well enough in the log, let me add some detail:
>> There's a small window between memstick_detect_change() and its queued
>> work, memstick_check(). In this window the rpm count may go down to zero
>> before the memstick host powers on, where I use
>> pm_runtime_get_noresume() to increment the rpm count.
>> 
>> memstick_check() uses some functions in rtsx_usb_ms that have
>> pm_runtime_put*() so the rpm count may go down to zero, before the
>> memstick host powers on.
> 
> So then, why doesn't memstick_check() early on calls
> pm_runtime_get_sync() and when it has finished with probing for a
> card, balance that with a call pm_runtime_put()?

This will do, not sure what I was thinking. Thanks for pointing out.

Kai-Heng

> 
> Kind regards
> Uffe

[PATCH 3/4 v5] memstick: rtsx_usb_ms: Use ms_dev() helper

[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

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

[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

Re: [PATCH 0/4 v5] Keep rtsx_usb suspended when there's no card

[Oleksandr Natalenko]

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
> 
> 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    |   4 +
>  drivers/memstick/host/rtsx_usb_ms.c | 173 +++++++++++++++++-----------
>  drivers/misc/cardreader/rtsx_usb.c  |   8 ++
>  3 files changed, 117 insertions(+), 68 deletions(-)
> 
> --
> 2.17.1

Feel free to add again:

Tested-by: Oleksandr Natalenko <oleksandr@natalenko.name>

since it works fine with:

Bus 001 Device 004: ID 0bda:0129 Realtek Semiconductor Corp. RTS5129 
Card Reader Controller

-- 
   Oleksandr Natalenko (post-factum)