| Message ID | 1365421497-4661-1-git-send-email-ulf.hansson@stericsson.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); } -- 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
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 -- 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
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 > > -- 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
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 -- 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
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. -- 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
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 -- 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
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; }