All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
@ 2016-09-17 10:14 Ulf Hansson
       [not found] ` <1474107278-3271-1-git-send-email-ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
  0 siblings, 1 reply; 16+ messages in thread
From: Ulf Hansson @ 2016-09-17 10:14 UTC (permalink / raw)
  To: linux-mmc, Ulf Hansson
  Cc: Ritesh Raj Sarraf, Alan Stern, USB list, Micky Ching,
	Roger Tseng, Wei WANG

Each access of the parent device (usb device) needs to be done in runtime
resumed state. Currently this isn't case while changing the leds, so let's
add pm_runtime_get_sync() and pm_runtime_put() around these calls.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

While discussing an issue[1] related to runtime PM, I found out that this
minor change at least improves the behavior that has been observed.

[1]
http://www.spinics.net/lists/linux-usb/msg144634.html

---
 drivers/mmc/host/rtsx_usb_sdmmc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c
index 6c71fc9..a59c7fa 100644
--- a/drivers/mmc/host/rtsx_usb_sdmmc.c
+++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
@@ -1314,6 +1314,7 @@ static void rtsx_usb_update_led(struct work_struct *work)
 		container_of(work, struct rtsx_usb_sdmmc, led_work);
 	struct rtsx_ucr *ucr = host->ucr;
 
+	pm_runtime_get_sync(sdmmc_dev(host));
 	mutex_lock(&ucr->dev_mutex);
 
 	if (host->led.brightness == LED_OFF)
@@ -1322,6 +1323,7 @@ static void rtsx_usb_update_led(struct work_struct *work)
 		rtsx_usb_turn_on_led(ucr);
 
 	mutex_unlock(&ucr->dev_mutex);
+	pm_runtime_put(sdmmc_dev(host));
 }
 #endif
 
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
       [not found] ` <1474107278-3271-1-git-send-email-ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
@ 2016-09-18  2:30   ` Alan Stern
  2016-09-19  9:24     ` Ulf Hansson
  0 siblings, 1 reply; 16+ messages in thread
From: Alan Stern @ 2016-09-18  2:30 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: linux-mmc-u79uwXL29TY76Z2rM5mHXA, Ritesh Raj Sarraf, USB list,
	Micky Ching, Roger Tseng, Wei WANG

On Sat, 17 Sep 2016, Ulf Hansson wrote:

> Each access of the parent device (usb device) needs to be done in runtime
> resumed state. Currently this isn't case while changing the leds, so let's
> add pm_runtime_get_sync() and pm_runtime_put() around these calls.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
> 
> While discussing an issue[1] related to runtime PM, I found out that this
> minor change at least improves the behavior that has been observed.
> 
> [1]
> http://www.spinics.net/lists/linux-usb/msg144634.html
> 
> ---
>  drivers/mmc/host/rtsx_usb_sdmmc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c
> index 6c71fc9..a59c7fa 100644
> --- a/drivers/mmc/host/rtsx_usb_sdmmc.c
> +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
> @@ -1314,6 +1314,7 @@ static void rtsx_usb_update_led(struct work_struct *work)
>  		container_of(work, struct rtsx_usb_sdmmc, led_work);
>  	struct rtsx_ucr *ucr = host->ucr;
>  
> +	pm_runtime_get_sync(sdmmc_dev(host));
>  	mutex_lock(&ucr->dev_mutex);
>  
>  	if (host->led.brightness == LED_OFF)
> @@ -1322,6 +1323,7 @@ static void rtsx_usb_update_led(struct work_struct *work)
>  		rtsx_usb_turn_on_led(ucr);
>  
>  	mutex_unlock(&ucr->dev_mutex);
> +	pm_runtime_put(sdmmc_dev(host));
>  }
>  #endif

The missing aspect here is that this won't stop the parent USB device
from going into autosuspend every 2 seconds and then resuming shortly 
afterward.  There are two ways of preventing this:

	Call usb_mark_last_busy() at appropriate places.

	Enable autosuspend for the sdmmc device.

The second approach would also prevent the sdmmc device from going into
autosuspend as soon as the LED update is finished.  Maybe that's okay,
but if going into suspend is a lightweight procedure then you may want
to prevent it.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
  2016-09-18  2:30   ` Alan Stern
@ 2016-09-19  9:24     ` Ulf Hansson
       [not found]       ` <CAPDyKFpFObRkvUC5kOKznE3FAGL6H_Hufa7ZEFWpmB694AY9ow-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 16+ messages in thread
From: Ulf Hansson @ 2016-09-19  9:24 UTC (permalink / raw)
  To: Alan Stern
  Cc: linux-mmc, Ritesh Raj Sarraf, USB list, Micky Ching, Roger Tseng,
	Wei WANG

On 18 September 2016 at 04:30, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sat, 17 Sep 2016, Ulf Hansson wrote:
>
>> Each access of the parent device (usb device) needs to be done in runtime
>> resumed state. Currently this isn't case while changing the leds, so let's
>> add pm_runtime_get_sync() and pm_runtime_put() around these calls.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>
>> While discussing an issue[1] related to runtime PM, I found out that this
>> minor change at least improves the behavior that has been observed.
>>
>> [1]
>> http://www.spinics.net/lists/linux-usb/msg144634.html
>>
>> ---
>>  drivers/mmc/host/rtsx_usb_sdmmc.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c
>> index 6c71fc9..a59c7fa 100644
>> --- a/drivers/mmc/host/rtsx_usb_sdmmc.c
>> +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
>> @@ -1314,6 +1314,7 @@ static void rtsx_usb_update_led(struct work_struct *work)
>>               container_of(work, struct rtsx_usb_sdmmc, led_work);
>>       struct rtsx_ucr *ucr = host->ucr;
>>
>> +     pm_runtime_get_sync(sdmmc_dev(host));
>>       mutex_lock(&ucr->dev_mutex);
>>
>>       if (host->led.brightness == LED_OFF)
>> @@ -1322,6 +1323,7 @@ static void rtsx_usb_update_led(struct work_struct *work)
>>               rtsx_usb_turn_on_led(ucr);
>>
>>       mutex_unlock(&ucr->dev_mutex);
>> +     pm_runtime_put(sdmmc_dev(host));
>>  }
>>  #endif
>
> The missing aspect here is that this won't stop the parent USB device
> from going into autosuspend every 2 seconds and then resuming shortly
> afterward.  There are two ways of preventing this:
>
>         Call usb_mark_last_busy() at appropriate places.
>
>         Enable autosuspend for the sdmmc device.
>
> The second approach would also prevent the sdmmc device from going into
> autosuspend as soon as the LED update is finished.  Maybe that's okay,
> but if going into suspend is a lightweight procedure then you may want
> to prevent it.
>

We can for sure enable autosuspend for the sdmmc device, although as
soon as an SD card will be detected the rtsx driver will increase the
runtime PM usage count. The count is decreased when the card is
removed (or failed to be initialized), thus runtime suspend is
prevented as long as there is a functional card inserted.

I am wondering what you think would be a good autosuspend timeout in
this case? It seems to me that the only thing the rtsx driver really
care about is to tell the parent device that it needs to be runtime
resumed during a certain timeframe, more or less it would like to
inherit the parents settings.

Other mmc hosts, not being usb-mmc devices, are using an autosuspend
timeout of ~50-100ms but that doesn't seem like good value here,
right?

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
       [not found]       ` <CAPDyKFpFObRkvUC5kOKznE3FAGL6H_Hufa7ZEFWpmB694AY9ow-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-09-19 18:02         ` Alan Stern
  2016-09-20  9:34           ` Ulf Hansson
       [not found]           ` <Pine.LNX.4.44L0.1609191348440.1458-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
  0 siblings, 2 replies; 16+ messages in thread
From: Alan Stern @ 2016-09-19 18:02 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: linux-mmc, Ritesh Raj Sarraf, USB list, Micky Ching, Roger Tseng,
	Wei WANG

On Mon, 19 Sep 2016, Ulf Hansson wrote:

> On 18 September 2016 at 04:30, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> > On Sat, 17 Sep 2016, Ulf Hansson wrote:
> >
> >> Each access of the parent device (usb device) needs to be done in runtime
> >> resumed state. Currently this isn't case while changing the leds, so let's
> >> add pm_runtime_get_sync() and pm_runtime_put() around these calls.
> >>
> >> Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> ---
> >>
> >> While discussing an issue[1] related to runtime PM, I found out that this
> >> minor change at least improves the behavior that has been observed.
> >>
> >> [1]
> >> http://www.spinics.net/lists/linux-usb/msg144634.html
> >>
> >> ---
> >>  drivers/mmc/host/rtsx_usb_sdmmc.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c
> >> index 6c71fc9..a59c7fa 100644
> >> --- a/drivers/mmc/host/rtsx_usb_sdmmc.c
> >> +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
> >> @@ -1314,6 +1314,7 @@ static void rtsx_usb_update_led(struct work_struct *work)
> >>               container_of(work, struct rtsx_usb_sdmmc, led_work);
> >>       struct rtsx_ucr *ucr = host->ucr;
> >>
> >> +     pm_runtime_get_sync(sdmmc_dev(host));
> >>       mutex_lock(&ucr->dev_mutex);
> >>
> >>       if (host->led.brightness == LED_OFF)
> >> @@ -1322,6 +1323,7 @@ static void rtsx_usb_update_led(struct work_struct *work)
> >>               rtsx_usb_turn_on_led(ucr);
> >>
> >>       mutex_unlock(&ucr->dev_mutex);
> >> +     pm_runtime_put(sdmmc_dev(host));
> >>  }
> >>  #endif
> >
> > The missing aspect here is that this won't stop the parent USB device
> > from going into autosuspend every 2 seconds and then resuming shortly
> > afterward.  There are two ways of preventing this:
> >
> >         Call usb_mark_last_busy() at appropriate places.
> >
> >         Enable autosuspend for the sdmmc device.
> >
> > The second approach would also prevent the sdmmc device from going into
> > autosuspend as soon as the LED update is finished.  Maybe that's okay,
> > but if going into suspend is a lightweight procedure then you may want
> > to prevent it.
> >
> 
> We can for sure enable autosuspend for the sdmmc device, although as
> soon as an SD card will be detected the rtsx driver will increase the
> runtime PM usage count. The count is decreased when the card is
> removed (or failed to be initialized), thus runtime suspend is
> prevented as long as there is a functional card inserted.

Which means that autosuspend matters only when a card isn't present, 
and the host is polled every second or so to see whether a card has 
been inserted.

Under those circumstances you probably don't want to use autosuspend.  
That is, resuming before each poll and suspending afterward may use 
less energy than staying at full power all the time.

> I am wondering what you think would be a good autosuspend timeout in
> this case? It seems to me that the only thing the rtsx driver really
> care about is to tell the parent device that it needs to be runtime
> resumed during a certain timeframe, more or less it would like to
> inherit the parents settings.
> 
> Other mmc hosts, not being usb-mmc devices, are using an autosuspend
> timeout of ~50-100ms but that doesn't seem like good value here,
> right?

Well, if you decide to let the device go into runtime suspend between
polls then there's no reason to use autosuspend at all.  Once a poll 
has ended, you know there won't be any more activity until the next 
poll.

On the other hand, if you decide to keep the device at full power all 
the time during polling, then any autosuspend timeout larger than 1000 
ms would do what you want.

Mostly I'm concerned about how this will interact with the USB runtime
PM.  The thing is, suspending the sdmmc device doesn't save any energy,
whereas suspending the USB device does.

Let's see how well everything works with the patch to the rtsx memstick
driver.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
  2016-09-19 18:02         ` Alan Stern
@ 2016-09-20  9:34           ` Ulf Hansson
       [not found]             ` <CAPDyKFq2Y1vn1OjNJJg7hocbzFo-QUpezLSMWnF_1cSJ9Ot3NQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
       [not found]           ` <Pine.LNX.4.44L0.1609191348440.1458-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
  1 sibling, 1 reply; 16+ messages in thread
From: Ulf Hansson @ 2016-09-20  9:34 UTC (permalink / raw)
  To: Alan Stern
  Cc: linux-mmc, Ritesh Raj Sarraf, USB list, Micky Ching, Roger Tseng,
	Wei WANG

On 19 September 2016 at 20:02, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, 19 Sep 2016, Ulf Hansson wrote:
>
>> On 18 September 2016 at 04:30, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Sat, 17 Sep 2016, Ulf Hansson wrote:
>> >
>> >> Each access of the parent device (usb device) needs to be done in runtime
>> >> resumed state. Currently this isn't case while changing the leds, so let's
>> >> add pm_runtime_get_sync() and pm_runtime_put() around these calls.
>> >>
>> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> >> ---
>> >>
>> >> While discussing an issue[1] related to runtime PM, I found out that this
>> >> minor change at least improves the behavior that has been observed.
>> >>
>> >> [1]
>> >> http://www.spinics.net/lists/linux-usb/msg144634.html
>> >>
>> >> ---
>> >>  drivers/mmc/host/rtsx_usb_sdmmc.c | 2 ++
>> >>  1 file changed, 2 insertions(+)
>> >>
>> >> diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c
>> >> index 6c71fc9..a59c7fa 100644
>> >> --- a/drivers/mmc/host/rtsx_usb_sdmmc.c
>> >> +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
>> >> @@ -1314,6 +1314,7 @@ static void rtsx_usb_update_led(struct work_struct *work)
>> >>               container_of(work, struct rtsx_usb_sdmmc, led_work);
>> >>       struct rtsx_ucr *ucr = host->ucr;
>> >>
>> >> +     pm_runtime_get_sync(sdmmc_dev(host));
>> >>       mutex_lock(&ucr->dev_mutex);
>> >>
>> >>       if (host->led.brightness == LED_OFF)
>> >> @@ -1322,6 +1323,7 @@ static void rtsx_usb_update_led(struct work_struct *work)
>> >>               rtsx_usb_turn_on_led(ucr);
>> >>
>> >>       mutex_unlock(&ucr->dev_mutex);
>> >> +     pm_runtime_put(sdmmc_dev(host));
>> >>  }
>> >>  #endif
>> >
>> > The missing aspect here is that this won't stop the parent USB device
>> > from going into autosuspend every 2 seconds and then resuming shortly
>> > afterward.  There are two ways of preventing this:
>> >
>> >         Call usb_mark_last_busy() at appropriate places.
>> >
>> >         Enable autosuspend for the sdmmc device.
>> >
>> > The second approach would also prevent the sdmmc device from going into
>> > autosuspend as soon as the LED update is finished.  Maybe that's okay,
>> > but if going into suspend is a lightweight procedure then you may want
>> > to prevent it.
>> >
>>
>> We can for sure enable autosuspend for the sdmmc device, although as
>> soon as an SD card will be detected the rtsx driver will increase the
>> runtime PM usage count. The count is decreased when the card is
>> removed (or failed to be initialized), thus runtime suspend is
>> prevented as long as there is a functional card inserted.
>
> Which means that autosuspend matters only when a card isn't present,
> and the host is polled every second or so to see whether a card has
> been inserted.
>
> Under those circumstances you probably don't want to use autosuspend.
> That is, resuming before each poll and suspending afterward may use
> less energy than staying at full power all the time.
>
>> I am wondering what you think would be a good autosuspend timeout in
>> this case? It seems to me that the only thing the rtsx driver really
>> care about is to tell the parent device that it needs to be runtime
>> resumed during a certain timeframe, more or less it would like to
>> inherit the parents settings.
>>
>> Other mmc hosts, not being usb-mmc devices, are using an autosuspend
>> timeout of ~50-100ms but that doesn't seem like good value here,
>> right?
>
> Well, if you decide to let the device go into runtime suspend between
> polls then there's no reason to use autosuspend at all.  Once a poll
> has ended, you know there won't be any more activity until the next
> poll.

We could change that, as currently the approach in the mmc core isn't
that sophisticated. I even think this has been discussed earlier for
the very similar reasons regards polling card detect mode.

I guess the main reason to why we yet have changed this, is because
mmc host drivers are using an autosuspend timeout of ~50-100 ms, so in
the end it haven't been such a big deal.

>
> On the other hand, if you decide to keep the device at full power all
> the time during polling, then any autosuspend timeout larger than 1000
> ms would do what you want.
>
> Mostly I'm concerned about how this will interact with the USB runtime
> PM.  The thing is, suspending the sdmmc device doesn't save any energy,
> whereas suspending the USB device does.

Yes, I agree.

My concern is also 2s autosuspend timeout which is set for the usb
device. Somehow I feel we need to be able "share" more information
between a parent-child relationship, in this case between the sdmmc
device and the usb device.

An observation I made, is when the sdmmc device gets runtime resumed
(pm_runtime_get_sync()), the parent device (the usb device) may also
become runtime resumed (unless it's already). In this sequence, but
*only* when actually runtime resuming the usb device, the runtime PM
core decides to update the last busy mark for the usb device. Should
it really do that?

Moreover, I am curious about the 2s usb timeout. Why isn't that chosen
to something like ~100ms instead? Is there is a long latency to
runtime resume the usb device or because we fear to wear out the HW,
which may be powered on/off too frequently?

If we assume that the usb device shouldn't be used with a timeout less
than 2s, then I think we have two options:

*) As the mmc polling timeout is 1s, there is really no point in
trying to runtime suspend the usb device, it may just be left runtime
resumed all the time. Wasting power, of course!
**) Add an interface to allow dynamically changes of the mmc polling
timeout to better suit the current user.

[...]

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
       [not found]           ` <Pine.LNX.4.44L0.1609191348440.1458-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
@ 2016-09-20  9:40             ` Oliver Neukum
  2016-09-20 14:12               ` Alan Stern
  0 siblings, 1 reply; 16+ messages in thread
From: Oliver Neukum @ 2016-09-20  9:40 UTC (permalink / raw)
  To: Alan Stern
  Cc: Ulf Hansson, Micky Ching, Wei WANG, Roger Tseng,
	Ritesh Raj Sarraf, linux-mmc, USB list

On Mon, 2016-09-19 at 14:02 -0400, Alan Stern wrote:
> > We can for sure enable autosuspend for the sdmmc device, although as
> > soon as an SD card will be detected the rtsx driver will increase
> the
> > runtime PM usage count. The count is decreased when the card is
> > removed (or failed to be initialized), thus runtime suspend is
> > prevented as long as there is a functional card inserted.

Testing autosuspend with card readers on usb_storage I saw a uniform
response of reporting a medium change event upon resume.
I am afraid other kinds of readers are not better in that regard.
> 
> Which means that autosuspend matters only when a card isn't present, 
> and the host is polled every second or so to see whether a card has 
> been inserted.
> 
> Under those circumstances you probably don't want to use
> autosuspend.  
> That is, resuming before each poll and suspending afterward may use 
> less energy than staying at full power all the time.

Is that based on concrete figures about power consumption?

And it seems to me that we need a way to indicate that the heuristics
should not be used, but a device immediately suspended. The timer
is sensible only if the next wakeup is unknown.

	Regards
		Oliver



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
       [not found]             ` <CAPDyKFq2Y1vn1OjNJJg7hocbzFo-QUpezLSMWnF_1cSJ9Ot3NQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-09-20 14:09               ` Alan Stern
       [not found]                 ` <Pine.LNX.4.44L0.1609200951500.1459-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
  0 siblings, 1 reply; 16+ messages in thread
From: Alan Stern @ 2016-09-20 14:09 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: linux-mmc, Ritesh Raj Sarraf, USB list, Micky Ching, Roger Tseng,
	Wei WANG

On Tue, 20 Sep 2016, Ulf Hansson wrote:

> >> I am wondering what you think would be a good autosuspend timeout in
> >> this case? It seems to me that the only thing the rtsx driver really
> >> care about is to tell the parent device that it needs to be runtime
> >> resumed during a certain timeframe, more or less it would like to
> >> inherit the parents settings.
> >>
> >> Other mmc hosts, not being usb-mmc devices, are using an autosuspend
> >> timeout of ~50-100ms but that doesn't seem like good value here,
> >> right?
> >
> > Well, if you decide to let the device go into runtime suspend between
> > polls then there's no reason to use autosuspend at all.  Once a poll
> > has ended, you know there won't be any more activity until the next
> > poll.
> 
> We could change that, as currently the approach in the mmc core isn't
> that sophisticated. I even think this has been discussed earlier for
> the very similar reasons regards polling card detect mode.
> 
> I guess the main reason to why we yet have changed this, is because
> mmc host drivers are using an autosuspend timeout of ~50-100 ms, so in
> the end it haven't been such a big deal.

No, it isn't.  Although at a polling interval of 1 second, it means 
reducing the low-power time by as much as 10%.

> > On the other hand, if you decide to keep the device at full power all
> > the time during polling, then any autosuspend timeout larger than 1000
> > ms would do what you want.
> >
> > Mostly I'm concerned about how this will interact with the USB runtime
> > PM.  The thing is, suspending the sdmmc device doesn't save any energy,
> > whereas suspending the USB device does.
> 
> Yes, I agree.
> 
> My concern is also 2s autosuspend timeout which is set for the usb
> device. Somehow I feel we need to be able "share" more information
> between a parent-child relationship, in this case between the sdmmc
> device and the usb device.

I agree, but it's not clear how this should be done.  One easy solution 
would be to turn off USB autosuspend and do all the runtime-PM 
management in the sdmmc and memstick drivers.

> An observation I made, is when the sdmmc device gets runtime resumed
> (pm_runtime_get_sync()), the parent device (the usb device) may also
> become runtime resumed (unless it's already). In this sequence, but
> *only* when actually runtime resuming the usb device, the runtime PM
> core decides to update the last busy mark for the usb device. Should
> it really do that?

Yes, that's deliberate.  The whole idea of autosuspend is to prevent 
the device from being runtime-suspended too soon after it was used, and 
the PM core considers runtime-resume to be a form of usage.

> Moreover, I am curious about the 2s usb timeout. Why isn't that chosen
> to something like ~100ms instead? Is there is a long latency to
> runtime resume the usb device or because we fear to wear out the HW,
> which may be powered on/off too frequently?

When I first implemented runtime PM for the USB stack, I had to choose 
a autosuspend timeout.  Not having any basis for such a choice, and 
figuring that a suspend-resume cycle takes around 100 ms, I decided 
that 2 seconds would be a reasonable value.  But it's just a default; 
drivers and userspace can change it whenever they want.

> If we assume that the usb device shouldn't be used with a timeout less
> than 2s, then I think we have two options:
> 
> *) As the mmc polling timeout is 1s, there is really no point in
> trying to runtime suspend the usb device, it may just be left runtime
> resumed all the time. Wasting power, of course!

Or we can decrease the USB autosuspend delay to 100 ms.

> **) Add an interface to allow dynamically changes of the mmc polling
> timeout to better suit the current user.

Note that the block layer does its own polling for removable media, and
it already has a sysfs interface to control the polling interval (or 
disable it entirely).  But I don't know how the MMC stack interacts 
with the block layer.

One awkward point is that the sdmmc and memstick drivers each do their
own polling.  This is a waste.  You can see it in the usbmon trace;  
every second there are two query-response interactions.  Even if
there's no good way to reduce the number, we should at least try to
synchronize the polls so that the device doesn't need to be resumed
twice every second.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
  2016-09-20  9:40             ` Oliver Neukum
@ 2016-09-20 14:12               ` Alan Stern
       [not found]                 ` <Pine.LNX.4.44L0.1609201009470.1459-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
  0 siblings, 1 reply; 16+ messages in thread
From: Alan Stern @ 2016-09-20 14:12 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Ulf Hansson, Micky Ching, Wei WANG, Roger Tseng,
	Ritesh Raj Sarraf, linux-mmc, USB list

On Tue, 20 Sep 2016, Oliver Neukum wrote:

> On Mon, 2016-09-19 at 14:02 -0400, Alan Stern wrote:
> > > We can for sure enable autosuspend for the sdmmc device, although as
> > > soon as an SD card will be detected the rtsx driver will increase
> > the
> > > runtime PM usage count. The count is decreased when the card is
> > > removed (or failed to be initialized), thus runtime suspend is
> > > prevented as long as there is a functional card inserted.
> 
> Testing autosuspend with card readers on usb_storage I saw a uniform
> response of reporting a medium change event upon resume.
> I am afraid other kinds of readers are not better in that regard.

That shouldn't be an issue in this case, at least, not with the current 
code.  The sdmmc and memstick drivers block autosuspend if media is 
present.

> > Which means that autosuspend matters only when a card isn't present, 
> > and the host is polled every second or so to see whether a card has 
> > been inserted.
> > 
> > Under those circumstances you probably don't want to use
> > autosuspend.  
> > That is, resuming before each poll and suspending afterward may use 
> > less energy than staying at full power all the time.
> 
> Is that based on concrete figures about power consumption?

No.

> And it seems to me that we need a way to indicate that the heuristics
> should not be used, but a device immediately suspended. The timer
> is sensible only if the next wakeup is unknown.

The driver can always turn off autosuspend if it wants to.

Alan Stern


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
       [not found]                 ` <Pine.LNX.4.44L0.1609201009470.1459-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
@ 2016-09-21  9:50                   ` Oliver Neukum
       [not found]                     ` <1474451450.2675.13.camel-IBi9RG/b67k@public.gmane.org>
  0 siblings, 1 reply; 16+ messages in thread
From: Oliver Neukum @ 2016-09-21  9:50 UTC (permalink / raw)
  To: Alan Stern
  Cc: Ulf Hansson, Micky Ching, Wei WANG, Roger Tseng,
	Ritesh Raj Sarraf, linux-mmc, USB list

On Tue, 2016-09-20 at 10:12 -0400, Alan Stern wrote:
> On Tue, 20 Sep 2016, Oliver Neukum wrote:

> That shouldn't be an issue in this case, at least, not with the current 
> code.  The sdmmc and memstick drivers block autosuspend if media is 
> present.

Good.
> 
> > > Which means that autosuspend matters only when a card isn't present, 
> > > and the host is polled every second or so to see whether a card has 
> > > been inserted.
> > > 
> > > Under those circumstances you probably don't want to use
> > > autosuspend.  
> > > That is, resuming before each poll and suspending afterward may use 
> > > less energy than staying at full power all the time.
> > 
> > Is that based on concrete figures about power consumption?
> 
> No.

Well, I have no idea how to improve this much without hideous
overengineering.

> > And it seems to me that we need a way to indicate that the heuristics
> > should not be used, but a device immediately suspended. The timer
> > is sensible only if the next wakeup is unknown.
> 
> The driver can always turn off autosuspend if it wants to.

Yes, but this is not the point. A heuristic with a timeout makes
sense only if the uses are unpredictable. If you know with a high
degree of probability when the next activity comes, you ought to either
suspend now or not all until the next activity.

Likewise the heuristic is appropriate for leaf nodes. You get nothing
from a delay on inner nodes. Any storage (generic sense) device
is an inner node. It should suspend immediately after the block
device which is the leaf node.

	Regards
		Oliver



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
       [not found]                 ` <Pine.LNX.4.44L0.1609200951500.1459-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
@ 2016-09-21 10:39                   ` Ulf Hansson
       [not found]                     ` <CAPDyKFqgfhsOdrz5ncTh5Z_OZ6tvnMVoQ_2g7ZyM02aaVzGQWg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 16+ messages in thread
From: Ulf Hansson @ 2016-09-21 10:39 UTC (permalink / raw)
  To: Alan Stern
  Cc: linux-mmc, Ritesh Raj Sarraf, USB list, Micky Ching, Roger Tseng,
	Wei WANG

On 20 September 2016 at 16:09, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
>
> On Tue, 20 Sep 2016, Ulf Hansson wrote:
>
> > >> I am wondering what you think would be a good autosuspend timeout in
> > >> this case? It seems to me that the only thing the rtsx driver really
> > >> care about is to tell the parent device that it needs to be runtime
> > >> resumed during a certain timeframe, more or less it would like to
> > >> inherit the parents settings.
> > >>
> > >> Other mmc hosts, not being usb-mmc devices, are using an autosuspend
> > >> timeout of ~50-100ms but that doesn't seem like good value here,
> > >> right?
> > >
> > > Well, if you decide to let the device go into runtime suspend between
> > > polls then there's no reason to use autosuspend at all.  Once a poll
> > > has ended, you know there won't be any more activity until the next
> > > poll.
> >
> > We could change that, as currently the approach in the mmc core isn't
> > that sophisticated. I even think this has been discussed earlier for
> > the very similar reasons regards polling card detect mode.
> >
> > I guess the main reason to why we yet have changed this, is because
> > mmc host drivers are using an autosuspend timeout of ~50-100 ms, so in
> > the end it haven't been such a big deal.
>
> No, it isn't.  Although at a polling interval of 1 second, it means
> reducing the low-power time by as much as 10%.

Yes, I agree we shouldn't neglect its impact - especially in this usb use case.

I will put it on my TODO list for mmc PM things.

>
> > > On the other hand, if you decide to keep the device at full power all
> > > the time during polling, then any autosuspend timeout larger than 1000
> > > ms would do what you want.
> > >
> > > Mostly I'm concerned about how this will interact with the USB runtime
> > > PM.  The thing is, suspending the sdmmc device doesn't save any energy,
> > > whereas suspending the USB device does.
> >
> > Yes, I agree.
> >
> > My concern is also 2s autosuspend timeout which is set for the usb
> > device. Somehow I feel we need to be able "share" more information
> > between a parent-child relationship, in this case between the sdmmc
> > device and the usb device.
>
> I agree, but it's not clear how this should be done.  One easy solution
> would be to turn off USB autosuspend and do all the runtime-PM
> management in the sdmmc and memstick drivers.

Hmm, this could be a very good option. In the end the sdmmc/memstick
drivers knows best about which autosuspend timeout to use.

>
> > An observation I made, is when the sdmmc device gets runtime resumed
> > (pm_runtime_get_sync()), the parent device (the usb device) may also
> > become runtime resumed (unless it's already). In this sequence, but
> > *only* when actually runtime resuming the usb device, the runtime PM
> > core decides to update the last busy mark for the usb device. Should
> > it really do that?
>
> Yes, that's deliberate.  The whole idea of autosuspend is to prevent
> the device from being runtime-suspended too soon after it was used, and
> the PM core considers runtime-resume to be a form of usage.

I understand it's deliberate, but I was more question whether we
actually should have the runtime PM core to behaves like this.

I don't think its behaviour is consistent, as in such case all calls
to __pm_runtime_resume() should trigger the runtime PM core to update
the last busy mark.

Don't get me wrong, I am *not* suggesting we should do that change, as
it would mean the last busy mark would be updated way too often.
Instead, perhaps it's better to leave the responsibility of updating
the last busy mark to the runtime PM users solely.

>
> > Moreover, I am curious about the 2s usb timeout. Why isn't that chosen
> > to something like ~100ms instead? Is there is a long latency to
> > runtime resume the usb device or because we fear to wear out the HW,
> > which may be powered on/off too frequently?
>
> When I first implemented runtime PM for the USB stack, I had to choose
> a autosuspend timeout.  Not having any basis for such a choice, and
> figuring that a suspend-resume cycle takes around 100 ms, I decided
> that 2 seconds would be a reasonable value.  But it's just a default;
> drivers and userspace can change it whenever they want.

Okay, I see. Thanks for sharing that information.

>
> > If we assume that the usb device shouldn't be used with a timeout less
> > than 2s, then I think we have two options:
> >
> > *) As the mmc polling timeout is 1s, there is really no point in
> > trying to runtime suspend the usb device, it may just be left runtime
> > resumed all the time. Wasting power, of course!
>
> Or we can decrease the USB autosuspend delay to 100 ms.

Yes, something like that makes sense to me.

Unless we decide to turn off autosuspend completely for the usb host
as you suggested above. Then it would really become clear that the
sdmmc/memstick drivers gets the responsible for the autosuspend, which
certainly makes most sense.

>
> > **) Add an interface to allow dynamically changes of the mmc polling
> > timeout to better suit the current user.
>
> Note that the block layer does its own polling for removable media, and
> it already has a sysfs interface to control the polling interval (or
> disable it entirely).  But I don't know how the MMC stack interacts
> with the block layer.
>
> One awkward point is that the sdmmc and memstick drivers each do their
> own polling.  This is a waste.  You can see it in the usbmon trace;
> every second there are two query-response interactions.  Even if
> there's no good way to reduce the number, we should at least try to
> synchronize the polls so that the device doesn't need to be resumed
> twice every second.

Yes, you are right. I just haven't been able to prioritize doing that
change for MMC. Another thing added on my mmc TODO list. :-)

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
       [not found]                     ` <1474451450.2675.13.camel-IBi9RG/b67k@public.gmane.org>
@ 2016-09-21 14:35                       ` Alan Stern
  2016-09-22 13:24                         ` Oliver Neukum
  0 siblings, 1 reply; 16+ messages in thread
From: Alan Stern @ 2016-09-21 14:35 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Ulf Hansson, Micky Ching, Wei WANG, Roger Tseng,
	Ritesh Raj Sarraf, linux-mmc, USB list

On Wed, 21 Sep 2016, Oliver Neukum wrote:

> On Tue, 2016-09-20 at 10:12 -0400, Alan Stern wrote:
> > On Tue, 20 Sep 2016, Oliver Neukum wrote:
> 
> > That shouldn't be an issue in this case, at least, not with the current 
> > code.  The sdmmc and memstick drivers block autosuspend if media is 
> > present.
> 
> Good.
> > 
> > > > Which means that autosuspend matters only when a card isn't present, 
> > > > and the host is polled every second or so to see whether a card has 
> > > > been inserted.
> > > > 
> > > > Under those circumstances you probably don't want to use
> > > > autosuspend.  
> > > > That is, resuming before each poll and suspending afterward may use 
> > > > less energy than staying at full power all the time.
> > > 
> > > Is that based on concrete figures about power consumption?
> > 
> > No.
> 
> Well, I have no idea how to improve this much without hideous
> overengineering.
> 
> > > And it seems to me that we need a way to indicate that the heuristics
> > > should not be used, but a device immediately suspended. The timer
> > > is sensible only if the next wakeup is unknown.
> > 
> > The driver can always turn off autosuspend if it wants to.
> 
> Yes, but this is not the point. A heuristic with a timeout makes
> sense only if the uses are unpredictable. If you know with a high
> degree of probability when the next activity comes, you ought to either
> suspend now or not all until the next activity.
> 
> Likewise the heuristic is appropriate for leaf nodes. You get nothing
> from a delay on inner nodes.

Almost true, but not quite.  When an inner node has more than one leaf
beneath it, enabling an autosuspend delay for the inner node can make 
sense -- particularly if the leaf activities are uncorrelated.

> Any storage (generic sense) device
> is an inner node. It should suspend immediately after the block
> device which is the leaf node.

Yes.  In this case, however, the USB device has two platform devices 
beneath it: one for SDMMC and one for MemoryStick cards.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
       [not found]                     ` <CAPDyKFqgfhsOdrz5ncTh5Z_OZ6tvnMVoQ_2g7ZyM02aaVzGQWg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-09-21 14:45                       ` Alan Stern
       [not found]                         ` <Pine.LNX.4.44L0.1609211038270.1996-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
  0 siblings, 1 reply; 16+ messages in thread
From: Alan Stern @ 2016-09-21 14:45 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: linux-mmc, Ritesh Raj Sarraf, USB list, Micky Ching, Roger Tseng,
	Wei WANG

On Wed, 21 Sep 2016, Ulf Hansson wrote:

> > > My concern is also 2s autosuspend timeout which is set for the usb
> > > device. Somehow I feel we need to be able "share" more information
> > > between a parent-child relationship, in this case between the sdmmc
> > > device and the usb device.
> >
> > I agree, but it's not clear how this should be done.  One easy solution
> > would be to turn off USB autosuspend and do all the runtime-PM
> > management in the sdmmc and memstick drivers.
> 
> Hmm, this could be a very good option. In the end the sdmmc/memstick
> drivers knows best about which autosuspend timeout to use.

I tend to agree, and so does Oliver.

> > > An observation I made, is when the sdmmc device gets runtime resumed
> > > (pm_runtime_get_sync()), the parent device (the usb device) may also
> > > become runtime resumed (unless it's already). In this sequence, but
> > > *only* when actually runtime resuming the usb device, the runtime PM
> > > core decides to update the last busy mark for the usb device. Should
> > > it really do that?
> >
> > Yes, that's deliberate.  The whole idea of autosuspend is to prevent
> > the device from being runtime-suspended too soon after it was used, and
> > the PM core considers runtime-resume to be a form of usage.
> 
> I understand it's deliberate, but I was more question whether we
> actually should have the runtime PM core to behaves like this.
> 
> I don't think its behaviour is consistent, as in such case all calls
> to __pm_runtime_resume() should trigger the runtime PM core to update
> the last busy mark.

Not a bad idea...

> Don't get me wrong, I am *not* suggesting we should do that change, as
> it would mean the last busy mark would be updated way too often.

The updates aren't very expensive.  Just one atomic write.  It probably
takes less time than acquiring the runtime-PM spinlock.

> Instead, perhaps it's better to leave the responsibility of updating
> the last busy mark to the runtime PM users solely.

Maybe, but I think doing it once in the core, like this, can remove the
need for a lot of function calls in drivers.

> > > If we assume that the usb device shouldn't be used with a timeout less
> > > than 2s, then I think we have two options:
> > >
> > > *) As the mmc polling timeout is 1s, there is really no point in
> > > trying to runtime suspend the usb device, it may just be left runtime
> > > resumed all the time. Wasting power, of course!
> >
> > Or we can decrease the USB autosuspend delay to 100 ms.
> 
> Yes, something like that makes sense to me.
> 
> Unless we decide to turn off autosuspend completely for the usb host
> as you suggested above. Then it would really become clear that the
> sdmmc/memstick drivers gets the responsible for the autosuspend, which
> certainly makes most sense.

Yes.

> > > **) Add an interface to allow dynamically changes of the mmc polling
> > > timeout to better suit the current user.
> >
> > Note that the block layer does its own polling for removable media, and
> > it already has a sysfs interface to control the polling interval (or
> > disable it entirely).  But I don't know how the MMC stack interacts
> > with the block layer.
> >
> > One awkward point is that the sdmmc and memstick drivers each do their
> > own polling.  This is a waste.  You can see it in the usbmon trace;
> > every second there are two query-response interactions.  Even if
> > there's no good way to reduce the number, we should at least try to
> > synchronize the polls so that the device doesn't need to be resumed
> > twice every second.
> 
> Yes, you are right. I just haven't been able to prioritize doing that
> change for MMC. Another thing added on my mmc TODO list. :-)

To tell the truth, I'm not sure how you would synchronize the polling
activities in the sdmmc and memstick drivers.  Move most of it into the
upper MFD driver?

One point worth mentioning is that if you already know an SDMMC card is
present then there's no reason to poll for a MemoryStick card, and vice
versa.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
       [not found]                         ` <Pine.LNX.4.44L0.1609211038270.1996-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
@ 2016-09-22 10:13                           ` Ulf Hansson
       [not found]                             ` <CAPDyKFozTL9h3HXoimHc4X3jeWQtJaedrfExVq1A5g7-JzcNLg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 16+ messages in thread
From: Ulf Hansson @ 2016-09-22 10:13 UTC (permalink / raw)
  To: Alan Stern
  Cc: linux-mmc, Ritesh Raj Sarraf, USB list, Micky Ching, Roger Tseng,
	Wei WANG

On 21 September 2016 at 16:45, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Wed, 21 Sep 2016, Ulf Hansson wrote:
>
>> > > My concern is also 2s autosuspend timeout which is set for the usb
>> > > device. Somehow I feel we need to be able "share" more information
>> > > between a parent-child relationship, in this case between the sdmmc
>> > > device and the usb device.
>> >
>> > I agree, but it's not clear how this should be done.  One easy solution
>> > would be to turn off USB autosuspend and do all the runtime-PM
>> > management in the sdmmc and memstick drivers.
>>
>> Hmm, this could be a very good option. In the end the sdmmc/memstick
>> drivers knows best about which autosuspend timeout to use.
>
> I tend to agree, and so does Oliver.

Okay.

>
>> > > An observation I made, is when the sdmmc device gets runtime resumed
>> > > (pm_runtime_get_sync()), the parent device (the usb device) may also
>> > > become runtime resumed (unless it's already). In this sequence, but
>> > > *only* when actually runtime resuming the usb device, the runtime PM
>> > > core decides to update the last busy mark for the usb device. Should
>> > > it really do that?
>> >
>> > Yes, that's deliberate.  The whole idea of autosuspend is to prevent
>> > the device from being runtime-suspended too soon after it was used, and
>> > the PM core considers runtime-resume to be a form of usage.
>>
>> I understand it's deliberate, but I was more question whether we
>> actually should have the runtime PM core to behaves like this.
>>
>> I don't think its behaviour is consistent, as in such case all calls
>> to __pm_runtime_resume() should trigger the runtime PM core to update
>> the last busy mark.
>
> Not a bad idea...

Yes, it is. :-)

Although, I am still concerned about he inconsistent behaviour.

>
>> Don't get me wrong, I am *not* suggesting we should do that change, as
>> it would mean the last busy mark would be updated way too often.
>
> The updates aren't very expensive.  Just one atomic write.  It probably
> takes less time than acquiring the runtime-PM spinlock.
>
>> Instead, perhaps it's better to leave the responsibility of updating
>> the last busy mark to the runtime PM users solely.
>
> Maybe, but I think doing it once in the core, like this, can remove the
> need for a lot of function calls in drivers.

Unfortunate not. Most updates of the last busy mark happens when a
device is no longer required to be runtime resumed. As when a driver
has completed to serve a request and is about to call pm_runtime_put()
(or similar API).

So, I still believe doing it in the runtime PM core is just a waste.

I think it's better to leave the update to the users entirely, it
would become consistent but also more flexible, as one could easily
think of situations where you may in some cases want to update the
last busy mark and in some other not.

>
>> > > If we assume that the usb device shouldn't be used with a timeout less
>> > > than 2s, then I think we have two options:
>> > >
>> > > *) As the mmc polling timeout is 1s, there is really no point in
>> > > trying to runtime suspend the usb device, it may just be left runtime
>> > > resumed all the time. Wasting power, of course!
>> >
>> > Or we can decrease the USB autosuspend delay to 100 ms.
>>
>> Yes, something like that makes sense to me.
>>
>> Unless we decide to turn off autosuspend completely for the usb host
>> as you suggested above. Then it would really become clear that the
>> sdmmc/memstick drivers gets the responsible for the autosuspend, which
>> certainly makes most sense.
>
> Yes.

Okay.

I will submit a series with the relevant changes we have come up with
during the discussions. I will keep you posted.

>
>> > > **) Add an interface to allow dynamically changes of the mmc polling
>> > > timeout to better suit the current user.
>> >
>> > Note that the block layer does its own polling for removable media, and
>> > it already has a sysfs interface to control the polling interval (or
>> > disable it entirely).  But I don't know how the MMC stack interacts
>> > with the block layer.
>> >
>> > One awkward point is that the sdmmc and memstick drivers each do their
>> > own polling.  This is a waste.  You can see it in the usbmon trace;
>> > every second there are two query-response interactions.  Even if
>> > there's no good way to reduce the number, we should at least try to
>> > synchronize the polls so that the device doesn't need to be resumed
>> > twice every second.
>>
>> Yes, you are right. I just haven't been able to prioritize doing that
>> change for MMC. Another thing added on my mmc TODO list. :-)
>
> To tell the truth, I'm not sure how you would synchronize the polling
> activities in the sdmmc and memstick drivers.  Move most of it into the
> upper MFD driver?

I don't know, you may be right! I will have to think about it.

>
> One point worth mentioning is that if you already know an SDMMC card is
> present then there's no reason to poll for a MemoryStick card, and vice
> versa.

Yes, indeed. Although you still need to poll, as otherwise you
wouldn't detect if it gets removed.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
  2016-09-21 14:35                       ` Alan Stern
@ 2016-09-22 13:24                         ` Oliver Neukum
       [not found]                           ` <1474550656.11364.32.camel-IBi9RG/b67k@public.gmane.org>
  0 siblings, 1 reply; 16+ messages in thread
From: Oliver Neukum @ 2016-09-22 13:24 UTC (permalink / raw)
  To: Alan Stern
  Cc: Ulf Hansson, Micky Ching, Wei WANG, Roger Tseng,
	Ritesh Raj Sarraf, linux-mmc, USB list

On Wed, 2016-09-21 at 10:35 -0400, Alan Stern wrote:
> On Wed, 21 Sep 2016, Oliver Neukum wrote:

> > Yes, but this is not the point. A heuristic with a timeout makes
> > sense only if the uses are unpredictable. If you know with a high
> > degree of probability when the next activity comes, you ought to either
> > suspend now or not all until the next activity.
> > 
> > Likewise the heuristic is appropriate for leaf nodes. You get nothing
> > from a delay on inner nodes.
> 
> Almost true, but not quite.  When an inner node has more than one leaf
> beneath it, enabling an autosuspend delay for the inner node can make 
> sense -- particularly if the leaf activities are uncorrelated.


Well, it is true that an inner node is likelier to be woken up
depending on the number of children. That is a reason to have a longer
timeout for an inner node. But it should start when the first node goes
idle. It makes no sense to start yet another timeout when the last node
goes idle.

In terms of mathematics I think we would need to multiply the timeout
with the square root of busy children and restart it whenever a child
goes to idle.
But it seems to me that this is impractical.

So I would suggest that we are missing an API for drivers to tell the
core that they become idle for a known period of time and to propagate
that immediately up if that is the last leaf to become idle.

> > Any storage (generic sense) device
> > is an inner node. It should suspend immediately after the block
> > device which is the leaf node.
> 
> Yes.  In this case, however, the USB device has two platform devices 
> beneath it: one for SDMMC and one for MemoryStick cards.

Indeed, we can hope that the power efficient work queue used will
join the polling of both devices. Ideally we could model the mutual
exclusion.

	Regards
		Oliver




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
       [not found]                             ` <CAPDyKFozTL9h3HXoimHc4X3jeWQtJaedrfExVq1A5g7-JzcNLg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2016-09-22 13:56                               ` Alan Stern
  0 siblings, 0 replies; 16+ messages in thread
From: Alan Stern @ 2016-09-22 13:56 UTC (permalink / raw)
  To: Ulf Hansson; +Cc: linux-mmc, Ritesh Raj Sarraf, USB list, Roger Tseng, Wei WANG

On Thu, 22 Sep 2016, Ulf Hansson wrote:

> >> > > An observation I made, is when the sdmmc device gets runtime resumed
> >> > > (pm_runtime_get_sync()), the parent device (the usb device) may also
> >> > > become runtime resumed (unless it's already). In this sequence, but
> >> > > *only* when actually runtime resuming the usb device, the runtime PM
> >> > > core decides to update the last busy mark for the usb device. Should
> >> > > it really do that?
> >> >
> >> > Yes, that's deliberate.  The whole idea of autosuspend is to prevent
> >> > the device from being runtime-suspended too soon after it was used, and
> >> > the PM core considers runtime-resume to be a form of usage.
> >>
> >> I understand it's deliberate, but I was more question whether we
> >> actually should have the runtime PM core to behaves like this.
> >>
> >> I don't think its behaviour is consistent, as in such case all calls
> >> to __pm_runtime_resume() should trigger the runtime PM core to update
> >> the last busy mark.
> >
> > Not a bad idea...
> 
> Yes, it is. :-)
> 
> Although, I am still concerned about he inconsistent behaviour.
> 
> >
> >> Don't get me wrong, I am *not* suggesting we should do that change, as
> >> it would mean the last busy mark would be updated way too often.
> >
> > The updates aren't very expensive.  Just one atomic write.  It probably
> > takes less time than acquiring the runtime-PM spinlock.
> >
> >> Instead, perhaps it's better to leave the responsibility of updating
> >> the last busy mark to the runtime PM users solely.
> >
> > Maybe, but I think doing it once in the core, like this, can remove the
> > need for a lot of function calls in drivers.
> 
> Unfortunate not. Most updates of the last busy mark happens when a
> device is no longer required to be runtime resumed. As when a driver
> has completed to serve a request and is about to call pm_runtime_put()
> (or similar API).
> 
> So, I still believe doing it in the runtime PM core is just a waste.
> 
> I think it's better to leave the update to the users entirely, it
> would become consistent but also more flexible, as one could easily
> think of situations where you may in some cases want to update the
> last busy mark and in some other not.

You can try making this change if you want.  I'd be afraid of 
regressions.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led
       [not found]                           ` <1474550656.11364.32.camel-IBi9RG/b67k@public.gmane.org>
@ 2016-09-22 14:00                             ` Alan Stern
  0 siblings, 0 replies; 16+ messages in thread
From: Alan Stern @ 2016-09-22 14:00 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: Ulf Hansson, Wei WANG, Roger Tseng, Ritesh Raj Sarraf, linux-mmc,
	USB list

On Thu, 22 Sep 2016, Oliver Neukum wrote:

> On Wed, 2016-09-21 at 10:35 -0400, Alan Stern wrote:
> > On Wed, 21 Sep 2016, Oliver Neukum wrote:
> 
> > > Yes, but this is not the point. A heuristic with a timeout makes
> > > sense only if the uses are unpredictable. If you know with a high
> > > degree of probability when the next activity comes, you ought to either
> > > suspend now or not all until the next activity.
> > > 
> > > Likewise the heuristic is appropriate for leaf nodes. You get nothing
> > > from a delay on inner nodes.
> > 
> > Almost true, but not quite.  When an inner node has more than one leaf
> > beneath it, enabling an autosuspend delay for the inner node can make 
> > sense -- particularly if the leaf activities are uncorrelated.
> 
> 
> Well, it is true that an inner node is likelier to be woken up
> depending on the number of children. That is a reason to have a longer
> timeout for an inner node. But it should start when the first node goes
> idle. It makes no sense to start yet another timeout when the last node
> goes idle.
> 
> In terms of mathematics I think we would need to multiply the timeout
> with the square root of busy children and restart it whenever a child
> goes to idle.
> But it seems to me that this is impractical.
> 
> So I would suggest that we are missing an API for drivers to tell the
> core that they become idle for a known period of time and to propagate
> that immediately up if that is the last leaf to become idle.

This seems like over-engineering.  In many cases the driver does not 
know how long the idle period will last.  And it may happen that the 
leaf drivers do not understand the energy tradeoffs involved in 
suspending the ancestor device.

> > > Any storage (generic sense) device
> > > is an inner node. It should suspend immediately after the block
> > > device which is the leaf node.
> > 
> > Yes.  In this case, however, the USB device has two platform devices 
> > beneath it: one for SDMMC and one for MemoryStick cards.
> 
> Indeed, we can hope that the power efficient work queue used will
> join the polling of both devices. Ideally we could model the mutual
> exclusion.

Ulf is going to work on it.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2016-09-22 14:00 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-17 10:14 [PATCH] mmc: rtsx_usb_sdmmc: Handle runtime PM while changing led Ulf Hansson
     [not found] ` <1474107278-3271-1-git-send-email-ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2016-09-18  2:30   ` Alan Stern
2016-09-19  9:24     ` Ulf Hansson
     [not found]       ` <CAPDyKFpFObRkvUC5kOKznE3FAGL6H_Hufa7ZEFWpmB694AY9ow-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-09-19 18:02         ` Alan Stern
2016-09-20  9:34           ` Ulf Hansson
     [not found]             ` <CAPDyKFq2Y1vn1OjNJJg7hocbzFo-QUpezLSMWnF_1cSJ9Ot3NQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-09-20 14:09               ` Alan Stern
     [not found]                 ` <Pine.LNX.4.44L0.1609200951500.1459-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
2016-09-21 10:39                   ` Ulf Hansson
     [not found]                     ` <CAPDyKFqgfhsOdrz5ncTh5Z_OZ6tvnMVoQ_2g7ZyM02aaVzGQWg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-09-21 14:45                       ` Alan Stern
     [not found]                         ` <Pine.LNX.4.44L0.1609211038270.1996-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
2016-09-22 10:13                           ` Ulf Hansson
     [not found]                             ` <CAPDyKFozTL9h3HXoimHc4X3jeWQtJaedrfExVq1A5g7-JzcNLg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-09-22 13:56                               ` Alan Stern
     [not found]           ` <Pine.LNX.4.44L0.1609191348440.1458-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
2016-09-20  9:40             ` Oliver Neukum
2016-09-20 14:12               ` Alan Stern
     [not found]                 ` <Pine.LNX.4.44L0.1609201009470.1459-100000-IYeN2dnnYyZXsRXLowluHWD2FQJk+8+b@public.gmane.org>
2016-09-21  9:50                   ` Oliver Neukum
     [not found]                     ` <1474451450.2675.13.camel-IBi9RG/b67k@public.gmane.org>
2016-09-21 14:35                       ` Alan Stern
2016-09-22 13:24                         ` Oliver Neukum
     [not found]                           ` <1474550656.11364.32.camel-IBi9RG/b67k@public.gmane.org>
2016-09-22 14:00                             ` Alan Stern

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.