[PATCH V2 2/2] mmc: block: Enable runtime pm for mmc blkdevice

[PATCH V2 2/2] mmc: block: Enable runtime pm for mmc blkdevice

[Ulf Hansson]

From: Ulf Hansson <ulf.hansson@linaro.org>

Once the mmc blkdevice is being probed, runtime pm will be enabled.
By using runtime autosuspend, the power save operations can be done
when request inactivity occurs for a certain time. Right now the
selected timeout value is set to 3 s.

Moreover, when the blk device is being suspended, we make sure the device
will be runtime resumed. The reason for doing this is that we want the
host suspend sequence to be unaware of any runtime power save operations,
so it can just handle the suspend as the device is fully powered from a
runtime perspective.

This patch is preparing to make it possible to move BKOPS handling into
the runtime callbacks for the mmc bus_ops. Thus IDLE BKOPS can be
accomplished.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Acked-by: Maya Erez <merez@codeaurora.org>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index e12a03c..536331a 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -34,6 +34,7 @@
 #include <linux/delay.h>
 #include <linux/capability.h>
 #include <linux/compat.h>
+#include <linux/pm_runtime.h>
 
 #include <linux/mmc/ioctl.h>
 #include <linux/mmc/card.h>
@@ -222,6 +223,7 @@ static ssize_t power_ro_lock_store(struct device *dev,
 	md = mmc_blk_get(dev_to_disk(dev));
 	card = md->queue.card;
 
+	pm_runtime_get_sync(&card->dev);
 	mmc_claim_host(card->host);
 
 	ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BOOT_WP,
@@ -234,6 +236,8 @@ static ssize_t power_ro_lock_store(struct device *dev,
 		card->ext_csd.boot_ro_lock |= EXT_CSD_BOOT_WP_B_PWR_WP_EN;
 
 	mmc_release_host(card->host);
+	pm_runtime_mark_last_busy(&card->dev);
+	pm_runtime_put_autosuspend(&card->dev);
 
 	if (!ret) {
 		pr_info("%s: Locking boot partition ro until next power on\n",
@@ -492,6 +496,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
 
 	mrq.cmd = &cmd;
 
+	pm_runtime_get_sync(&card->dev);
 	mmc_claim_host(card->host);
 
 	err = mmc_blk_part_switch(card, md);
@@ -560,6 +565,8 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
 
 cmd_rel_host:
 	mmc_release_host(card->host);
+	pm_runtime_mark_last_busy(&card->dev);
+	pm_runtime_put_autosuspend(&card->dev);
 
 cmd_done:
 	mmc_blk_put(md);
@@ -1894,9 +1901,11 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
 	struct mmc_host *host = card->host;
 	unsigned long flags;
 
-	if (req && !mq->mqrq_prev->req)
+	if (req && !mq->mqrq_prev->req) {
+		pm_runtime_get_sync(&card->dev);
 		/* claim host only for the first request */
 		mmc_claim_host(card->host);
+	}
 
 	ret = mmc_blk_part_switch(card, md);
 	if (ret) {
@@ -1933,7 +1942,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
 
 out:
 	if ((!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST)) ||
-	     (req && (req->cmd_flags & MMC_REQ_SPECIAL_MASK)))
+	     (req && (req->cmd_flags & MMC_REQ_SPECIAL_MASK))) {
 		/*
 		 * Release host when there are no more requests
 		 * and after special request(discard, flush) is done.
@@ -1941,6 +1950,10 @@ out:
 		 * the 'mmc_blk_issue_rq' with 'mqrq_prev->req'.
 		 */
 		mmc_release_host(card->host);
+		pm_runtime_mark_last_busy(&card->dev);
+		pm_runtime_put_autosuspend(&card->dev);
+	}
+
 	return ret;
 }
 
@@ -2337,6 +2350,12 @@ static int mmc_blk_probe(struct mmc_card *card)
 		if (mmc_add_disk(part_md))
 			goto out;
 	}
+
+	pm_runtime_set_active(&card->dev);
+	pm_runtime_set_autosuspend_delay(&card->dev, 3000);
+	pm_runtime_use_autosuspend(&card->dev);
+	pm_runtime_enable(&card->dev);
+
 	return 0;
 
  out:
@@ -2350,9 +2369,12 @@ static void mmc_blk_remove(struct mmc_card *card)
 	struct mmc_blk_data *md = mmc_get_drvdata(card);
 
 	mmc_blk_remove_parts(card, md);
+	pm_runtime_get_sync(&card->dev);
 	mmc_claim_host(card->host);
 	mmc_blk_part_switch(card, md);
 	mmc_release_host(card->host);
+	pm_runtime_disable(&card->dev);
+	pm_runtime_put_noidle(&card->dev);
 	mmc_blk_remove_req(md);
 	mmc_set_drvdata(card, NULL);
 }
@@ -2364,6 +2386,7 @@ static int mmc_blk_suspend(struct mmc_card *card)
 	struct mmc_blk_data *md = mmc_get_drvdata(card);
 
 	if (md) {
+		pm_runtime_get_sync(&card->dev);
 		mmc_queue_suspend(&md->queue);
 		list_for_each_entry(part_md, &md->part, part) {
 			mmc_queue_suspend(&part_md->queue);
@@ -2387,6 +2410,7 @@ static int mmc_blk_resume(struct mmc_card *card)
 		list_for_each_entry(part_md, &md->part, part) {
 			mmc_queue_resume(&part_md->queue);
 		}
+		pm_runtime_put(&card->dev);
 	}
 	return 0;
 }
-- 
1.7.10

Re: [PATCH V2 2/2] mmc: block: Enable runtime pm for mmc blkdevice

[Adrian Hunter]

On 08/04/13 14:44, Ulf Hansson wrote:
> From: Ulf Hansson <ulf.hansson@linaro.org>
> 
> Once the mmc blkdevice is being probed, runtime pm will be enabled.
> By using runtime autosuspend, the power save operations can be done
> when request inactivity occurs for a certain time. Right now the
> selected timeout value is set to 3 s.
> 
> Moreover, when the blk device is being suspended, we make sure the device
> will be runtime resumed. The reason for doing this is that we want the
> host suspend sequence to be unaware of any runtime power save operations,
> so it can just handle the suspend as the device is fully powered from a
> runtime perspective.
> 
> This patch is preparing to make it possible to move BKOPS handling into
> the runtime callbacks for the mmc bus_ops. Thus IDLE BKOPS can be
> accomplished.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Acked-by: Maya Erez <merez@codeaurora.org>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Acked-by: Kevin Liu <kliu5@marvell.com>
> ---
>  drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 

There are debugfs uses of the card also (e.g.mmc_dbg_card_status_get)
that will need get/put added.

There might be others.  Please check.

It might be worth adding helpers e.g. mmc_claim_card()/mmc_release_card
so that it is easy to see the places that the host is claimed but runtime pm
is not used.

void mmc_claim_card(card)
{
	pm_runtime_get_sync(&card->dev);
	mmc_claim_host(card->host);
}

void mmc_release_card(card)
{
	mmc_release_host(card->host);
	pm_runtime_mark_last_busy(&card->dev);
	pm_runtime_put_autosuspend(&card->dev);
}

Re: [PATCH V2 2/2] mmc: block: Enable runtime pm for mmc blkdevice

[Ulf Hansson]

On 11 April 2013 10:31, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 08/04/13 14:44, Ulf Hansson wrote:
>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> Once the mmc blkdevice is being probed, runtime pm will be enabled.
>> By using runtime autosuspend, the power save operations can be done
>> when request inactivity occurs for a certain time. Right now the
>> selected timeout value is set to 3 s.
>>
>> Moreover, when the blk device is being suspended, we make sure the device
>> will be runtime resumed. The reason for doing this is that we want the
>> host suspend sequence to be unaware of any runtime power save operations,
>> so it can just handle the suspend as the device is fully powered from a
>> runtime perspective.
>>
>> This patch is preparing to make it possible to move BKOPS handling into
>> the runtime callbacks for the mmc bus_ops. Thus IDLE BKOPS can be
>> accomplished.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> Acked-by: Maya Erez <merez@codeaurora.org>
>> Acked-by: Arnd Bergmann <arnd@arndb.de>
>> Acked-by: Kevin Liu <kliu5@marvell.com>
>> ---
>>  drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
>>  1 file changed, 26 insertions(+), 2 deletions(-)
>>
>
> There are debugfs uses of the card also (e.g.mmc_dbg_card_status_get)
> that will need get/put added.

In the end it all depends on what kind of operations you decide to do
in the runtime_supend|resume callbacks.
Since the callbacks is not yet implemented for sd and mmc, this is not
required as of now.

Nevertheless a most valid point that needs to be considered, while
implementing the callbacks. Thanks for pointing this out.

>
> There might be others.  Please check.

mmc_rescan needs to be considered at card removal and at resume. But,
again this does not need to be handled as of now.

>
> It might be worth adding helpers e.g. mmc_claim_card()/mmc_release_card
> so that it is easy to see the places that the host is claimed but runtime pm
> is not used.

I am not sure we gain more visibility adding helpers at this initial
step. We already have three scenarios for get/claim.
1. mmc_claim_host and pm_runtime_get is done in conjuction.
2. only mmc_claim_host.
3. only pm_runtime_get.

For put, we have a similar situation, and we don't even use the same
runtime put API for all cases.

I see your point, it could very well be wanted to add these helpers if
we see that the callbacks force the pm_runtime API to be used from
several more places.

>
> void mmc_claim_card(card)
> {
>         pm_runtime_get_sync(&card->dev);
>         mmc_claim_host(card->host);
> }
>
> void mmc_release_card(card)
> {
>         mmc_release_host(card->host);
>         pm_runtime_mark_last_busy(&card->dev);
>         pm_runtime_put_autosuspend(&card->dev);
> }
>
>
>

Kind regards
Ulf Hansson

Re: [PATCH V2 2/2] mmc: block: Enable runtime pm for mmc blkdevice

[Adrian Hunter]

On 11/04/13 12:40, Ulf Hansson wrote:
> On 11 April 2013 10:31, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 08/04/13 14:44, Ulf Hansson wrote:
>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>
>>> Once the mmc blkdevice is being probed, runtime pm will be enabled.
>>> By using runtime autosuspend, the power save operations can be done
>>> when request inactivity occurs for a certain time. Right now the
>>> selected timeout value is set to 3 s.
>>>
>>> Moreover, when the blk device is being suspended, we make sure the device
>>> will be runtime resumed. The reason for doing this is that we want the
>>> host suspend sequence to be unaware of any runtime power save operations,
>>> so it can just handle the suspend as the device is fully powered from a
>>> runtime perspective.
>>>
>>> This patch is preparing to make it possible to move BKOPS handling into
>>> the runtime callbacks for the mmc bus_ops. Thus IDLE BKOPS can be
>>> accomplished.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> Acked-by: Maya Erez <merez@codeaurora.org>
>>> Acked-by: Arnd Bergmann <arnd@arndb.de>
>>> Acked-by: Kevin Liu <kliu5@marvell.com>
>>> ---
>>>  drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
>>>  1 file changed, 26 insertions(+), 2 deletions(-)
>>>
>>
>> There are debugfs uses of the card also (e.g.mmc_dbg_card_status_get)
>> that will need get/put added.
> 
> In the end it all depends on what kind of operations you decide to do
> in the runtime_supend|resume callbacks.
> Since the callbacks is not yet implemented for sd and mmc, this is not
> required as of now.
> 
> Nevertheless a most valid point that needs to be considered, while
> implementing the callbacks. Thanks for pointing this out.
> 
>>
>> There might be others.  Please check.
> 
> mmc_rescan needs to be considered at card removal and at resume. But,
> again this does not need to be handled as of now.

I disagree.  If you are adding runtime PM for SD/MMC cards, it must not be
possible to access the card without going through runtime PM first.  That
includes *all* code paths.  We should not leave holes for others to fall in.

> 
>>
>> It might be worth adding helpers e.g. mmc_claim_card()/mmc_release_card
>> so that it is easy to see the places that the host is claimed but runtime pm
>> is not used.
> 
> I am not sure we gain more visibility adding helpers at this initial
> step. We already have three scenarios for get/claim.
> 1. mmc_claim_host and pm_runtime_get is done in conjuction.
> 2. only mmc_claim_host.
> 3. only pm_runtime_get.
> 
> For put, we have a similar situation, and we don't even use the same
> runtime put API for all cases.
> 
> I see your point, it could very well be wanted to add these helpers if
> we see that the callbacks force the pm_runtime API to be used from
> several more places.
> 
>>
>> void mmc_claim_card(card)
>> {
>>         pm_runtime_get_sync(&card->dev);
>>         mmc_claim_host(card->host);
>> }
>>
>> void mmc_release_card(card)
>> {
>>         mmc_release_host(card->host);
>>         pm_runtime_mark_last_busy(&card->dev);
>>         pm_runtime_put_autosuspend(&card->dev);
>> }
>>
>>
>>
> 
> Kind regards
> Ulf Hansson
> 
> 

Re: [PATCH V2 2/2] mmc: block: Enable runtime pm for mmc blkdevice

[Ulf Hansson]

On 11 April 2013 11:58, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 11/04/13 12:40, Ulf Hansson wrote:
>> On 11 April 2013 10:31, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 08/04/13 14:44, Ulf Hansson wrote:
>>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>>
>>>> Once the mmc blkdevice is being probed, runtime pm will be enabled.
>>>> By using runtime autosuspend, the power save operations can be done
>>>> when request inactivity occurs for a certain time. Right now the
>>>> selected timeout value is set to 3 s.
>>>>
>>>> Moreover, when the blk device is being suspended, we make sure the device
>>>> will be runtime resumed. The reason for doing this is that we want the
>>>> host suspend sequence to be unaware of any runtime power save operations,
>>>> so it can just handle the suspend as the device is fully powered from a
>>>> runtime perspective.
>>>>
>>>> This patch is preparing to make it possible to move BKOPS handling into
>>>> the runtime callbacks for the mmc bus_ops. Thus IDLE BKOPS can be
>>>> accomplished.
>>>>
>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>> Acked-by: Maya Erez <merez@codeaurora.org>
>>>> Acked-by: Arnd Bergmann <arnd@arndb.de>
>>>> Acked-by: Kevin Liu <kliu5@marvell.com>
>>>> ---
>>>>  drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
>>>>  1 file changed, 26 insertions(+), 2 deletions(-)
>>>>
>>>
>>> There are debugfs uses of the card also (e.g.mmc_dbg_card_status_get)
>>> that will need get/put added.
>>
>> In the end it all depends on what kind of operations you decide to do
>> in the runtime_supend|resume callbacks.
>> Since the callbacks is not yet implemented for sd and mmc, this is not
>> required as of now.
>>
>> Nevertheless a most valid point that needs to be considered, while
>> implementing the callbacks. Thanks for pointing this out.
>>
>>>
>>> There might be others.  Please check.
>>
>> mmc_rescan needs to be considered at card removal and at resume. But,
>> again this does not need to be handled as of now.
>
> I disagree.  If you are adding runtime PM for SD/MMC cards, it must not be
> possible to access the card without going through runtime PM first.  That
> includes *all* code paths. We should not leave holes for others to fall in.
>

This patchset shall not be considered as full blown common solution
for runtime pm for mmc/sd/sdio. It is an initial step that we can
start build upon.

I took the approach of only adding, pm_runtime_get|put from the mmc
block layer, thus it will also _not_ affect the SDIO pm_runtime
implementation, which is already being used today. Adding what you
propose will affect SDIO as well, I think it is better to handle this
in the next steps instead.

Kind regards
Ulf Hansson

Re: [PATCH V2 2/2] mmc: block: Enable runtime pm for mmc blkdevice

[Adrian Hunter]

On 11/04/13 13:14, Ulf Hansson wrote:
> On 11 April 2013 11:58, Adrian Hunter <adrian.hunter@intel.com> wrote:
>> On 11/04/13 12:40, Ulf Hansson wrote:
>>> On 11 April 2013 10:31, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>> On 08/04/13 14:44, Ulf Hansson wrote:
>>>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>>>
>>>>> Once the mmc blkdevice is being probed, runtime pm will be enabled.
>>>>> By using runtime autosuspend, the power save operations can be done
>>>>> when request inactivity occurs for a certain time. Right now the
>>>>> selected timeout value is set to 3 s.
>>>>>
>>>>> Moreover, when the blk device is being suspended, we make sure the device
>>>>> will be runtime resumed. The reason for doing this is that we want the
>>>>> host suspend sequence to be unaware of any runtime power save operations,
>>>>> so it can just handle the suspend as the device is fully powered from a
>>>>> runtime perspective.
>>>>>
>>>>> This patch is preparing to make it possible to move BKOPS handling into
>>>>> the runtime callbacks for the mmc bus_ops. Thus IDLE BKOPS can be
>>>>> accomplished.
>>>>>
>>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>> Acked-by: Maya Erez <merez@codeaurora.org>
>>>>> Acked-by: Arnd Bergmann <arnd@arndb.de>
>>>>> Acked-by: Kevin Liu <kliu5@marvell.com>
>>>>> ---
>>>>>  drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
>>>>>  1 file changed, 26 insertions(+), 2 deletions(-)
>>>>>
>>>>
>>>> There are debugfs uses of the card also (e.g.mmc_dbg_card_status_get)
>>>> that will need get/put added.
>>>
>>> In the end it all depends on what kind of operations you decide to do
>>> in the runtime_supend|resume callbacks.
>>> Since the callbacks is not yet implemented for sd and mmc, this is not
>>> required as of now.
>>>
>>> Nevertheless a most valid point that needs to be considered, while
>>> implementing the callbacks. Thanks for pointing this out.
>>>
>>>>
>>>> There might be others.  Please check.
>>>
>>> mmc_rescan needs to be considered at card removal and at resume. But,
>>> again this does not need to be handled as of now.
>>
>> I disagree.  If you are adding runtime PM for SD/MMC cards, it must not be
>> possible to access the card without going through runtime PM first.  That
>> includes *all* code paths. We should not leave holes for others to fall in.
>>
> 
> This patchset shall not be considered as full blown common solution
> for runtime pm for mmc/sd/sdio. It is an initial step that we can
> start build upon.

That does not justify leaving holes.

> I took the approach of only adding, pm_runtime_get|put from the mmc
> block layer, thus it will also _not_ affect the SDIO pm_runtime
> implementation, which is already being used today. Adding what you
> propose will affect SDIO as well, I think it is better to handle this
> in the next steps instead.

I disagree.  SDIO is easily avoided by coding for card->type.

Fix debugfs handling.

Don't enable runtime pm for removable cards.  Document why i.e. rescan of
removable cards needs special handling depending on the power-saving
strategy e.g. if the power is off there is a risk of confusing a new card
with an old card.

That covers two omissions that we know about.  Check for others -
essentially check all mmc_claim_host() / __mmc_claim_host() /
mmc_try_claim_host() calls.

SD-Combo cards also look like a problem, so don't enable runtime pm for them
either.

Document what works (i.e. non-removable SD and MMC cards) and what doesn't
(removable and SD-Combo cards) and why.

Re: [PATCH V2 2/2] mmc: block: Enable runtime pm for mmc blkdevice

[Ulf Hansson]

On 11 April 2013 14:28, Adrian Hunter <adrian.hunter@intel.com> wrote:
> On 11/04/13 13:14, Ulf Hansson wrote:
>> On 11 April 2013 11:58, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>> On 11/04/13 12:40, Ulf Hansson wrote:
>>>> On 11 April 2013 10:31, Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>> On 08/04/13 14:44, Ulf Hansson wrote:
>>>>>> From: Ulf Hansson <ulf.hansson@linaro.org>
>>>>>>
>>>>>> Once the mmc blkdevice is being probed, runtime pm will be enabled.
>>>>>> By using runtime autosuspend, the power save operations can be done
>>>>>> when request inactivity occurs for a certain time. Right now the
>>>>>> selected timeout value is set to 3 s.
>>>>>>
>>>>>> Moreover, when the blk device is being suspended, we make sure the device
>>>>>> will be runtime resumed. The reason for doing this is that we want the
>>>>>> host suspend sequence to be unaware of any runtime power save operations,
>>>>>> so it can just handle the suspend as the device is fully powered from a
>>>>>> runtime perspective.
>>>>>>
>>>>>> This patch is preparing to make it possible to move BKOPS handling into
>>>>>> the runtime callbacks for the mmc bus_ops. Thus IDLE BKOPS can be
>>>>>> accomplished.
>>>>>>
>>>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>>>>> Acked-by: Maya Erez <merez@codeaurora.org>
>>>>>> Acked-by: Arnd Bergmann <arnd@arndb.de>
>>>>>> Acked-by: Kevin Liu <kliu5@marvell.com>
>>>>>> ---
>>>>>>  drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
>>>>>>  1 file changed, 26 insertions(+), 2 deletions(-)
>>>>>>
>>>>>
>>>>> There are debugfs uses of the card also (e.g.mmc_dbg_card_status_get)
>>>>> that will need get/put added.
>>>>
>>>> In the end it all depends on what kind of operations you decide to do
>>>> in the runtime_supend|resume callbacks.
>>>> Since the callbacks is not yet implemented for sd and mmc, this is not
>>>> required as of now.
>>>>
>>>> Nevertheless a most valid point that needs to be considered, while
>>>> implementing the callbacks. Thanks for pointing this out.
>>>>
>>>>>
>>>>> There might be others.  Please check.
>>>>
>>>> mmc_rescan needs to be considered at card removal and at resume. But,
>>>> again this does not need to be handled as of now.
>>>
>>> I disagree.  If you are adding runtime PM for SD/MMC cards, it must not be
>>> possible to access the card without going through runtime PM first.  That
>>> includes *all* code paths. We should not leave holes for others to fall in.
>>>
>>
>> This patchset shall not be considered as full blown common solution
>> for runtime pm for mmc/sd/sdio. It is an initial step that we can
>> start build upon.
>
> That does not justify leaving holes.
>
>> I took the approach of only adding, pm_runtime_get|put from the mmc
>> block layer, thus it will also _not_ affect the SDIO pm_runtime
>> implementation, which is already being used today. Adding what you
>> propose will affect SDIO as well, I think it is better to handle this
>> in the next steps instead.
>
> I disagree.  SDIO is easily avoided by coding for card->type.

Yes, it can be done, but on the other hand, I believe the runtime
support for SDIO does need additional improvement.

For example, the debugfs, which you pointed out, is as of today not
handled for sdio.

My, patch is not a fixup patch, it is adding a new feature to be able
to do additional power save. I don't want to mix a new feature with
fixups.

>
> Fix debugfs handling.

Will do it in a separate patch on top of this patch set.

>
> Don't enable runtime pm for removable cards. Document why i.e. rescan of
> removable cards needs special handling depending on the power-saving
> strategy e.g. if the power is off there is a risk of confusing a new card
> with an old card.

Removable cards can be power saved. We have been discussing the
"aggressive mmc_suspend_host" option here on the mmc list; certainly
it makes sense for removable cards as well.

Regarding documentation, I will add some information in header file
for the runtime_supend|resume callbacks.

>
> That covers two omissions that we know about.  Check for others -
> essentially check all mmc_claim_host() / __mmc_claim_host() /
> mmc_try_claim_host() calls.

Will handle this is a separate patch.

>
> SD-Combo cards also look like a problem, so don't enable runtime pm for them
> either.

Unfortunate I don't have an SD-combo card, so can't test these properly.

Anyway, there are problems with SD-combo cards before my patchset
around runtime PM. Using runtime pm for sdio (MMC_CAP_POWER_OFF_CARD),
will at runtime_suspend cut the power to the card. A block request for
the SD-combo card after this point will thus not be served
successfully.

This patchset will enable runtime pm for combo cards as you state.
That should actually fix the above problem, since the block layer will
do pm_runtime_get and thus make sure the card is properly powered when
needed. On the other hand, if MMC_CAP_POWER_OFF_CARD is not set and
thus we could expect the sdio client not doing runtime pm for the card
device, it will mean the exact opposite. Once the block layer decides
to power down the card due to request inactivity, the SDIO client will
suddenly not be able to access the SDIO interface.

The the new bug this patchset invents is probably worse than what it
fix. :-). I will adapt to you suggestion somehow.

>
> Document what works (i.e. non-removable SD and MMC cards) and what doesn't
> (removable and SD-Combo cards) and why.
>
>

Kind regards
Ulf Hansson

[PATCH V2 0/2] mmc: Use runtime pm for blkdevice

[Ulf Hansson]

From: Ulf Hansson <ulf.hansson@linaro.org>

SDIO has been using runtime pm for a while to handle runtime power save
operations. This patchset is enabling the option to make the sd/mmc
blockdevices to use runtime pm as well.

The runtime pm implementation for the block device will make use of
autosuspend to defer power save operation to after request inactivty for
a certain time.

To actually perform some power save operations the corresponding bus ops
for mmc and sd shall be implemented. Typically it could make sense to do
BKOPS for eMMC in here.


Changes in v2:
        - Removed the stated patch below from this patchset. It can be handled
        separately. "mmc: core: Remove power_restore bus_ops for mmc and sd"
        - Rebased patches on latest mmc-next.
        - Added Acks.

Ulf Hansson (2):
  mmc: core: Add bus_ops for runtime pm callbacks
  mmc: block: Enable runtime pm for mmc blkdevice

 drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
 drivers/mmc/core/bus.c   |   14 ++++++++++++--
 drivers/mmc/core/core.h  |    2 ++
 drivers/mmc/core/sdio.c  |   20 ++++++++++++++++++++
 4 files changed, 60 insertions(+), 4 deletions(-)

-- 
1.7.10

[PATCH V2 2/2] mmc: block: Enable runtime pm for mmc blkdevice

[Ulf Hansson]

From: Ulf Hansson <ulf.hansson@linaro.org>

Once the mmc blkdevice is being probed, runtime pm will be enabled.
By using runtime autosuspend, the power save operations can be done
when request inactivity occurs for a certain time. Right now the
selected timeout value is set to 3 s.

Moreover, when the blk device is being suspended, we make sure the device
will be runtime resumed. The reason for doing this is that we what the
host suspend sequence to be unaware of any runtime power save operations,
so it can just handle the suspend as the device is fully powerered from a
runtime perspective.

This patch is preparing to make it possible to move BKOPS handling into
the runtime callbacks for the mmc bus_ops. Thus IDLE BKOPS can be
accomplished.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Acked-by: Maya Erez <merez@codeaurora.org>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Kevin Liu <kliu5@marvell.com>
---
 drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index e12a03c..536331a 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -34,6 +34,7 @@
 #include <linux/delay.h>
 #include <linux/capability.h>
 #include <linux/compat.h>
+#include <linux/pm_runtime.h>
 
 #include <linux/mmc/ioctl.h>
 #include <linux/mmc/card.h>
@@ -222,6 +223,7 @@ static ssize_t power_ro_lock_store(struct device *dev,
 	md = mmc_blk_get(dev_to_disk(dev));
 	card = md->queue.card;
 
+	pm_runtime_get_sync(&card->dev);
 	mmc_claim_host(card->host);
 
 	ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BOOT_WP,
@@ -234,6 +236,8 @@ static ssize_t power_ro_lock_store(struct device *dev,
 		card->ext_csd.boot_ro_lock |= EXT_CSD_BOOT_WP_B_PWR_WP_EN;
 
 	mmc_release_host(card->host);
+	pm_runtime_mark_last_busy(&card->dev);
+	pm_runtime_put_autosuspend(&card->dev);
 
 	if (!ret) {
 		pr_info("%s: Locking boot partition ro until next power on\n",
@@ -492,6 +496,7 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
 
 	mrq.cmd = &cmd;
 
+	pm_runtime_get_sync(&card->dev);
 	mmc_claim_host(card->host);
 
 	err = mmc_blk_part_switch(card, md);
@@ -560,6 +565,8 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
 
 cmd_rel_host:
 	mmc_release_host(card->host);
+	pm_runtime_mark_last_busy(&card->dev);
+	pm_runtime_put_autosuspend(&card->dev);
 
 cmd_done:
 	mmc_blk_put(md);
@@ -1894,9 +1901,11 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
 	struct mmc_host *host = card->host;
 	unsigned long flags;
 
-	if (req && !mq->mqrq_prev->req)
+	if (req && !mq->mqrq_prev->req) {
+		pm_runtime_get_sync(&card->dev);
 		/* claim host only for the first request */
 		mmc_claim_host(card->host);
+	}
 
 	ret = mmc_blk_part_switch(card, md);
 	if (ret) {
@@ -1933,7 +1942,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
 
 out:
 	if ((!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST)) ||
-	     (req && (req->cmd_flags & MMC_REQ_SPECIAL_MASK)))
+	     (req && (req->cmd_flags & MMC_REQ_SPECIAL_MASK))) {
 		/*
 		 * Release host when there are no more requests
 		 * and after special request(discard, flush) is done.
@@ -1941,6 +1950,10 @@ out:
 		 * the 'mmc_blk_issue_rq' with 'mqrq_prev->req'.
 		 */
 		mmc_release_host(card->host);
+		pm_runtime_mark_last_busy(&card->dev);
+		pm_runtime_put_autosuspend(&card->dev);
+	}
+
 	return ret;
 }
 
@@ -2337,6 +2350,12 @@ static int mmc_blk_probe(struct mmc_card *card)
 		if (mmc_add_disk(part_md))
 			goto out;
 	}
+
+	pm_runtime_set_active(&card->dev);
+	pm_runtime_set_autosuspend_delay(&card->dev, 3000);
+	pm_runtime_use_autosuspend(&card->dev);
+	pm_runtime_enable(&card->dev);
+
 	return 0;
 
  out:
@@ -2350,9 +2369,12 @@ static void mmc_blk_remove(struct mmc_card *card)
 	struct mmc_blk_data *md = mmc_get_drvdata(card);
 
 	mmc_blk_remove_parts(card, md);
+	pm_runtime_get_sync(&card->dev);
 	mmc_claim_host(card->host);
 	mmc_blk_part_switch(card, md);
 	mmc_release_host(card->host);
+	pm_runtime_disable(&card->dev);
+	pm_runtime_put_noidle(&card->dev);
 	mmc_blk_remove_req(md);
 	mmc_set_drvdata(card, NULL);
 }
@@ -2364,6 +2386,7 @@ static int mmc_blk_suspend(struct mmc_card *card)
 	struct mmc_blk_data *md = mmc_get_drvdata(card);
 
 	if (md) {
+		pm_runtime_get_sync(&card->dev);
 		mmc_queue_suspend(&md->queue);
 		list_for_each_entry(part_md, &md->part, part) {
 			mmc_queue_suspend(&part_md->queue);
@@ -2387,6 +2410,7 @@ static int mmc_blk_resume(struct mmc_card *card)
 		list_for_each_entry(part_md, &md->part, part) {
 			mmc_queue_resume(&part_md->queue);
 		}
+		pm_runtime_put(&card->dev);
 	}
 	return 0;
 }
-- 
1.7.10

Re: [PATCH V2 2/2] mmc: block: Enable runtime pm for mmc blkdevice

[merez]

> From: Ulf Hansson <ulf.hansson@linaro.org>
>
> Once the mmc blkdevice is being probed, runtime pm will be enabled.
> By using runtime autosuspend, the power save operations can be done
> when request inactivity occurs for a certain time. Right now the
> selected timeout value is set to 3 s.
>
> Moreover, when the blk device is being suspended, we make sure the device
> will be runtime resumed. The reason for doing this is that we what the
> host suspend sequence to be unaware of any runtime power save operations,
> so it can just handle the suspend as the device is fully powerered from a
powered
> runtime perspective.
>
> This patch is preparing to make it possible to move BKOPS handling into
> the runtime callbacks for the mmc bus_ops. Thus IDLE BKOPS can be
> accomplished.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Acked-by: Maya Erez <merez@codeaurora.org>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Acked-by: Kevin Liu <kliu5@marvell.com>
> ---
>  drivers/mmc/card/block.c |   28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index e12a03c..536331a 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -34,6 +34,7 @@
>  #include <linux/delay.h>
>  #include <linux/capability.h>
>  #include <linux/compat.h>
> +#include <linux/pm_runtime.h>
>
>  #include <linux/mmc/ioctl.h>
>  #include <linux/mmc/card.h>
> @@ -222,6 +223,7 @@ static ssize_t power_ro_lock_store(struct device *dev,
>  	md = mmc_blk_get(dev_to_disk(dev));
>  	card = md->queue.card;
>
> +	pm_runtime_get_sync(&card->dev);
>  	mmc_claim_host(card->host);
>
>  	ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, EXT_CSD_BOOT_WP,
> @@ -234,6 +236,8 @@ static ssize_t power_ro_lock_store(struct device *dev,
>  		card->ext_csd.boot_ro_lock |= EXT_CSD_BOOT_WP_B_PWR_WP_EN;
>
>  	mmc_release_host(card->host);
> +	pm_runtime_mark_last_busy(&card->dev);
> +	pm_runtime_put_autosuspend(&card->dev);
>
>  	if (!ret) {
>  		pr_info("%s: Locking boot partition ro until next power on\n",
> @@ -492,6 +496,7 @@ static int mmc_blk_ioctl_cmd(struct block_device
> *bdev,
>
>  	mrq.cmd = &cmd;
>
> +	pm_runtime_get_sync(&card->dev);
>  	mmc_claim_host(card->host);
>
>  	err = mmc_blk_part_switch(card, md);
> @@ -560,6 +565,8 @@ static int mmc_blk_ioctl_cmd(struct block_device
> *bdev,
>
>  cmd_rel_host:
>  	mmc_release_host(card->host);
> +	pm_runtime_mark_last_busy(&card->dev);
> +	pm_runtime_put_autosuspend(&card->dev);
>
>  cmd_done:
>  	mmc_blk_put(md);
> @@ -1894,9 +1901,11 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
> struct request *req)
>  	struct mmc_host *host = card->host;
>  	unsigned long flags;
>
> -	if (req && !mq->mqrq_prev->req)
> +	if (req && !mq->mqrq_prev->req) {
> +		pm_runtime_get_sync(&card->dev);
>  		/* claim host only for the first request */
>  		mmc_claim_host(card->host);
> +	}
>
>  	ret = mmc_blk_part_switch(card, md);
>  	if (ret) {
> @@ -1933,7 +1942,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
> struct request *req)
>
>  out:
>  	if ((!req && !(mq->flags & MMC_QUEUE_NEW_REQUEST)) ||
> -	     (req && (req->cmd_flags & MMC_REQ_SPECIAL_MASK)))
> +	     (req && (req->cmd_flags & MMC_REQ_SPECIAL_MASK))) {
>  		/*
>  		 * Release host when there are no more requests
>  		 * and after special request(discard, flush) is done.
> @@ -1941,6 +1950,10 @@ out:
>  		 * the 'mmc_blk_issue_rq' with 'mqrq_prev->req'.
>  		 */
>  		mmc_release_host(card->host);
> +		pm_runtime_mark_last_busy(&card->dev);
> +		pm_runtime_put_autosuspend(&card->dev);
> +	}
> +
>  	return ret;
>  }
>
> @@ -2337,6 +2350,12 @@ static int mmc_blk_probe(struct mmc_card *card)
>  		if (mmc_add_disk(part_md))
>  			goto out;
>  	}
> +
> +	pm_runtime_set_active(&card->dev);
> +	pm_runtime_set_autosuspend_delay(&card->dev, 3000);
> +	pm_runtime_use_autosuspend(&card->dev);
> +	pm_runtime_enable(&card->dev);
> +
>  	return 0;
>
>   out:
> @@ -2350,9 +2369,12 @@ static void mmc_blk_remove(struct mmc_card *card)
>  	struct mmc_blk_data *md = mmc_get_drvdata(card);
>
>  	mmc_blk_remove_parts(card, md);
> +	pm_runtime_get_sync(&card->dev);
>  	mmc_claim_host(card->host);
>  	mmc_blk_part_switch(card, md);
>  	mmc_release_host(card->host);
> +	pm_runtime_disable(&card->dev);
> +	pm_runtime_put_noidle(&card->dev);
>  	mmc_blk_remove_req(md);
>  	mmc_set_drvdata(card, NULL);
>  }
> @@ -2364,6 +2386,7 @@ static int mmc_blk_suspend(struct mmc_card *card)
>  	struct mmc_blk_data *md = mmc_get_drvdata(card);
>
>  	if (md) {
> +		pm_runtime_get_sync(&card->dev);
>  		mmc_queue_suspend(&md->queue);
>  		list_for_each_entry(part_md, &md->part, part) {
>  			mmc_queue_suspend(&part_md->queue);
> @@ -2387,6 +2410,7 @@ static int mmc_blk_resume(struct mmc_card *card)
>  		list_for_each_entry(part_md, &md->part, part) {
>  			mmc_queue_resume(&part_md->queue);
>  		}
> +		pm_runtime_put(&card->dev);
>  	}
>  	return 0;
>  }
> --
> 1.7.10
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
Maya Erez
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation