Hi All, Sorry for reply..I didn't fully read about this patch history. So i will resend the my opinion after reading the mail-history Best Regards, Jaehoon Chung On 12/21/2012 06:56 PM, Ulf Hansson wrote: > On 21 December 2012 09:35, Maya Erez <merez@xxxxxxxxxxxxxx> wrote: >> Hi Ulf, >> >> Thanks for the info. We will consider our controller behavior in suspend. >> Would it be acceptable by you to keep the polling for BKOPS completion under >> a special CAPS2 flag? > > Not sure what you propose here. I guess some host cap would be needed > to be able to cope with those drivers which uses runtime PM for normal > suspend. > > But, the "polling method" as such shall not be done just because of > preventing those drivers entering runtime susend state. > pm_runtime_get* must be used here, I think. > Then of course you need to poll for the BKOPS status. > >> >> Thanks, >> Maya >> >> -----Original Message----- >> From: linux-mmc-owner@xxxxxxxxxxxxxxx >> [mailto:linux-mmc-owner@xxxxxxxxxxxxxxx] On Behalf Of Ulf Hansson >> Sent: Thursday, December 13, 2012 12:18 PM >> To: merez@xxxxxxxxxxxxxx >> Cc: Jaehoon Chung; linux-mmc@xxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; >> open list >> Subject: Re: [PATCH v3] mmc: core: Add support for idle time BKOPS >> >> On 12 December 2012 13:32, <merez@xxxxxxxxxxxxxx> wrote: >>> Hi Ulf, >>> >>> Sorry for the late response. >>> See my reply below. >>> >>> Thanks, >>> Maya >>> >>> On Thu, December 6, 2012 2:18 am, Ulf Hansson wrote: >>>> Hi Maya, >>>> >>>> On 4 December 2012 22:17, <merez@xxxxxxxxxxxxxx> wrote: >>>>> Hi Ulf, >>>>> >>>>> Let me try to better explain: >>>>> The idea behind the periodic BKOPS is to check the card's need for >>>>> BKOPS periodically in order to prevent an urgent BKOPS need by the card. >>>>> In order to actually manage to prevent the urgent BKOPS need, the >>>>> host should give the card enough time to perform the BKOPS (when it >>>>> recognizes BKOPS need), otherwise there is no point in having the >>>>> periodic BKOPS. >>>>> The above results in the following: >>>>> 1. After starting non-urgent BKOPS we "delay" getting into suspend >>>>> by polling on the card's status (explanation below), in order to >>>>> give the card time to perform the BKOPS. This has no effect on the >>>>> power consumption since the same BKOPS operations that were >>>>> performed after the foregound operation just moved to another >>>>> location, meaning before going into suspend. >>>> >>>> I am not sure what you are talking about here, runtime suspend or >>>> system suspend? Polling the card's status will not prevent any of >>>> this. So you have got this wrong. >>> >>> I am referring to the runtime suspend. >>> Our controller implements the runtime suspend and is not using the >>> default implementation of core/bus.c. >> >> This is not the "default runtime suspend" for a host device, but for the >> card device. Do not mix this up with runtime pm for a host device. >> >> Right now runtime pm for the card _device_ is only enabled for SDIO. >> Thus SDIO drivers can use runtime pm to actually trigger an SDIO card to be >> fully suspended when it is not needed and thus save a lot of power. For >> example when a WLAN interface goes up/down. >> >>> This is the reason why in our implementation polling the card status >>> "delays" the runtime suspend while it is not the case when using the >>> default runtime suspend implementation. >>> I can try to explain here what our controller is doing but since it is >>> specific to us then I guess it is not relevant to the discussion. >>> Our controller is calling mmc_suspend_host in runtime suspend, which >>> issues an HPI to stop the BKOPS. >> >> So, doing mmc_suspend_host in you runtime_suspend callback, is that really >> what you want to do? >> >> 1. >> You will introduce significant latencies (I have seen SD-cards which needs >> more than 1 s to initialize, eMMC is better but we are anyway talking >> several 100 ms) once new requests arrives after the host as entered the >> runtime suspend state. >> >> 2. >> SD cards has no "power off" notification, so you will actually stress test >> the SD cards internal FTL to be crash safe by cutting the power to it more >> often. >> >> 3. >> You will prevent SD-cards from doing it's back ground operations, which is >> done automatically and not like in a controlled manner as for eMMC. >> >> So of course, you save some power, but is the consequences worth it? :-) >> >>> Now that I understand that this code is not needed for all the host >>> drivers I will add a flag to decide if polling is required when doing >>> an unblocking BKOPS. >> >> You must not poll to prevent this! >> >> Instead you need to prevent the host from going into runtime suspend state, >> which is simply done by pm_runtime_get_sync for the host device. >> Although, it _must_ not be done for drivers not doing mmc_suspend_host in >> their runtime suspend callbacks. Since then it will prevent these from doing >> runtime power save actions, which is not ok. >> >>> Other host drivers that actually suspend on runtime suspend can enable >>> this flag and allow BKOPS to be active for a longer period. >>> I will prepare a new patch and send it for review. >>> >>>> >>>>> 2. Using PM_SUSPEND_PREPARE instead of the workqueue would not be >>>>> efficient since we don't want to wait until the host is ready to get >>>>> into suspend and then prevent him from suspending by doing BKOPS >>>>> operations that can take a long time. It is better to start the >>>>> BKOPS earlier. >>>> >>>> I did not suggest to use PM_SUSPEND_PREPARE, but to use runtime PM >>>> for the card device. It can be an option to implement this feature on >>>> top of a workqueue. At least worth to consider. >>>> >>> >>> We consider to call mmc_start_bkops every time MMC becomes idle, to >>> check the need for BKOPS. I will test it and include it in the next patch. >>> >>>>> >>>>> I am not too familiar with the controllers code and also my >>>>> understanding in runtime suspend is very basic, so feel free to >>>>> correct me if I'm wrong here or the behavior in other controllers is >>>>> different from msm_sdcc. >>>>> mmc_claim_host calls host->ops->enable. This API is implemented per >>>>> controller but as far as I understand it, this API must prevent >>>>> suspend, otherwise we might go into suspend while there is bus >>>>> activity. By doing get_card_status we call mmc_claim_host and this >>>>> call is the one that "delays" getting into suspend. >>>> >>>> host->ops->enable is the old way of implementing runtime power save >>>> for host drivers. Nowadays most drivers is using runtime PM instead. >>>> >>>> When you say that mmc_claim_host will prevent suspend, I suppose you >>>> mean that host->ops->disable wont be called? That is definitely not >>>> the same as preventing a system suspend, and moreover it should not. >>>> If you think that the host must be prevented from entering runtime >>>> power save (runtime_supend or host->ops->disable), you must elaborate >>>> more on this, because I don't understand why this is needed. >>>> >>> Again, this is specific to our controller, so you can just ignore that. >>> >>>>> If this is not the case in other controllers than the BKOPS will not >>>>> prevent the suspend and BKOPS will be interrupted. >>>>> As for the effect on the battery consumption, this is probably >>>>> something specific to our controller, so sorry if I created a confusion. >>>>> >>>>> Additional comments inline. >>>>> >>>>> Thanks, >>>>> Maya >>>>> >>>>> On Tue, December 4, 2012 1:52 am, Ulf Hansson wrote: >>>>>> On 3 December 2012 10:49, <merez@xxxxxxxxxxxxxx> wrote: >>>>>>> Hi Jaehoon, >>>>>>> >>>>>>> With this patch we don't expect to see any degradation. Thanks for >>>>>>> verifying that. >>>>>>> The test plan would be to run the lmdd and iozone benchmarks with >>>>>>> this patch and verify that the performance is not degraded. >>>>>>> I verified it with the msm_sdcc controller. >>>>>>> >>>>>>> Chris - We do expect it to influence the battery consumption, >>>>>>> since we now delay getting into suspend (since mmc_start_bkops >>>>>>> which is called after the delayed work is executed claims the >>>>>>> host). >>>>>>> The solution for that should be done by the controller which can >>>>>>> shorten the timeout given to pm_schedule_suspend by the periodic >>>>>>> BKOPS idle time. >>>>>>> Does it make sense to you? >>>>>>> >>>>>>> Thanks, >>>>>>> Maya >>>>>>> On Thu, November 29, 2012 4:40 am, Jaehoon Chung wrote: >>>>>>>> Hi Maya, >>>>>>>> >>>>>>>> Thank you a lot for working idle time BKOPS. >>>>>>>> >>>>>>>> I tested with this patch. It's working fine.(Suspend/resume is >>>>>>>> also working well.) Test controller is sdhci controller. >>>>>>>> When i tested the performance with iozone, i didn't find that >>>>>>>> performance is decreased. >>>>>>>> Well, as Chris is mentioned, do you have any test plan? >>>>>>>> So I will test more with this patch, because i want to test with >>>>>>>> dw-mmc controller, too. >>>>>>>> >>>>>>>> On 11/25/2012 08:56 PM, Maya Erez wrote: >>>>>>>>> Devices have various maintenance operations need to perform >>>>>>>>> internally. >>>>>>>>> In order to reduce latencies during time critical operations >>>>>>>>> like read and write, it is better to execute maintenance >>>>>>>>> operations in other times - when the host is not being serviced. >>>>>>>>> Such operations are called Background operations (BKOPS). >>>>>>>>> The device notifies the status of the BKOPS need by updating >>>>>>>>> BKOPS_STATUS (EXT_CSD byte [246]). >>>>>>>>> >>>>>>>>> According to the standard a host that supports BKOPS shall check >>>>>>>>> the status periodically and start background operations as >>>>>>>>> needed, so that the device has enough time for its maintenance >>>>>>>>> operations. >>>>>>>>> >>>>>>>>> This patch adds support for this periodic check of the BKOPS status. >>>>>>>>> Since foreground operations are of higher priority than >>>>>>>>> background operations the host will check the need for BKOPS >>>>>>>>> when it is idle, and in case of an incoming request the BKOPS >>>>>>>>> operation will be interrupted. >>>>>>>>> >>>>>>>>> When the mmcqd thread is idle, a delayed work is created to >>>>>>>>> check the need for BKOPS. The time to start the delayed work is >>>>>>>>> calculated based on the host controller suspend timeout, in case >>>>>>>>> it was set. If not, a default time is used. >>>>>> >>>>>> What host controller suspend timeout are you referring to here? >>>>>> >>>>>> If you are thinking of the runtime PM autosuspend timeout used in >>>>>> many host driver, then you might have missunderstand how runtime PM >>>>>> is used in host drivers. >>>>>> This has nothing to do with BKOPS as such, unless you think that >>>>>> the card must be kept clocked during BKOPS operations, but then >>>>>> this needs to be stated somewhere in this patch and that is not the >> case. >>>>>> >>>>>> Moreover, I could not find any new timeout added for the _host_ >>>>>> struct in this patch. >>>>> Yes, I was referring to the runtime PM autosuspend timeout. Since we >>>>> want to give the BKOPS time to be performed before going into >>>>> suspend, we need to take this timeout into account. >>>> >>>> Not sure why need to consider this timeout. Anyway, if so you must >>>> instead use the runtime PM API to prevent the host from being runtime >>>> suspended, like pm_runtime_get_sync. >>>> >>>>>> >>>>>>>>> If BKOPS are required in level 1, which is non-blocking, there >>>>>>>>> will be polling of the card status to wait for the BKOPS >>>>>>>>> completion and prevent suspend that will interrupt the BKOPS. >>>>>> >>>>>> Not sure of what suspend you are talking about here. But for sure >>>>>> BKOPS must _never_ prevent a system suspend. >>>>>> >>>>>> You might want to prevent a host from being runtime suspended >>>>>> though, but that is not accomplished in this patch. >>>>> This is explained in my general comment. Let me know if it is still >>>>> not clear. >>>>>> >>>>>>>>> If the card raised an exception, the need for urgent BKOPS >>>>>>>>> (level >>>>>>>>> 2/3) >>>>>>>>> will be checked immediately and if needed, the BKOPS will be >>>>>>>>> performed without waiting for the next idle time. >>>>>>>>> >>>>>>>>> Signed-off-by: Maya Erez <merez@xxxxxxxxxxxxxx> >>>>>>>>> >>>>>>>>> --- >>>>>>>>> This patch is based on the periodic BKOPS implementation in >>>>>>>>> version >>>>>>>>> 8 >>>>>>>>> of >>>>>>>>> "support BKOPS feature for eMMC" patch. >>>>>>>>> The patch was modified to answer the following issues: >>>>>>>>> - In order to prevent a race condition between going into >>>>>>>>> suspend and starting BKOPS, >>>>>>>>> the suspend timeout of the host controller is taking into >>>>>>>>> accound in determination of the start time >>>>>>>>> of the delayed work >>>>>>>>> - Since mmc_start_bkops is called from two contexts now, >>>>>>>>> mmc_claim_host was moved to the beginning of the function >>>>>>>>> - Also, the check of doing_bkops should be protected when >>>>>>>>> determing if an HPI is needed due to the same reason. >>>>>>>>> >>>>>>>>> Changes in v3: >>>>>>>>> - Move the call to stop_bkops to block.c. >>>>>>>>> This allows us to remove the mmc_claim_host from inside >>>>>>>>> the function and doesn't cause additional degradation >>>>>>>>> due to un-neccessary calim host operation >>>>>>>>> >>>>>>>>> Changes in v2: >>>>>>>>> - Check the number of written / discarded sectors as the >>>>>>>>> trigger for checking the BKOPS need. >>>>>>>>> - Code review fixes >>>>>>>>> >>>>>>>>> --- >>>>>>>>> drivers/mmc/card/block.c | 8 ++- >>>>>>>>> drivers/mmc/card/queue.c | 2 + >>>>>>>>> drivers/mmc/core/core.c | 178 >>>>>>>>> +++++++++++++++++++++++++++++++++++++++++++--- >>>>>>>>> drivers/mmc/core/mmc.c | 23 ++++++ >>>>>>>>> include/linux/mmc/card.h | 35 +++++++++ >>>>>>>>> include/linux/mmc/core.h | 3 + >>>>>>>>> 6 files changed, 237 insertions(+), 12 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c >>>>>>>>> index 172a768..40b4ae3 100644 >>>>>>>>> --- a/drivers/mmc/card/block.c >>>>>>>>> +++ b/drivers/mmc/card/block.c >>>>>>>>> @@ -1394,9 +1394,15 @@ static int mmc_blk_issue_rq(struct >>>>>>>>> mmc_queue *mq, struct request *req) >>>>>>>>> struct mmc_blk_data *md = mq->data; >>>>>>>>> struct mmc_card *card = md->queue.card; >>>>>>>>> >>>>>>>>> - if (req && !mq->mqrq_prev->req) >>>>>>>>> + if (req && !mq->mqrq_prev->req) { >>>>>>>>> /* claim host only for the first request */ >>>>>>>>> mmc_claim_host(card->host); >>>>>>>>> + if (card->ext_csd.bkops_en && >>>>>>>>> + card->bkops_info.started_delayed_bkops) { >>>>>>>>> + >>>>>>>>> + card->bkops_info.started_delayed_bkops >>>>>>>>> = >>>>>>>>> false; >>>>>>>>> + mmc_stop_bkops(card); >>>>>>>> We didn't need to check whether mmc_stop_bkops is success or not? >>>>>>>> If mmc_stop_bkops() is failed, then bkops is continuously running. >>>>>>>> >>>>>>>> Best Regards, >>>>>>>> Jaehoon Chung >>>>>>>> >>>>>>>>> + } >>>>>>>>> + } >>>>>>>>> >>>>>>>>> ret = mmc_blk_part_switch(card, md); >>>>>>>>> if (ret) { >>>>>>>>> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c >>>>>>>>> index fadf52e..9d0c96a 100644 >>>>>>>>> --- a/drivers/mmc/card/queue.c >>>>>>>>> +++ b/drivers/mmc/card/queue.c >>>>>>>>> @@ -51,6 +51,7 @@ static int mmc_queue_thread(void *d) { >>>>>>>>> struct mmc_queue *mq = d; >>>>>>>>> struct request_queue *q = mq->queue; >>>>>>>>> + struct mmc_card *card = mq->card; >>>>>>>>> >>>>>>>>> current->flags |= PF_MEMALLOC; >>>>>>>>> >>>>>>>>> @@ -83,6 +84,7 @@ static int mmc_queue_thread(void *d) >>>>>>>>> set_current_state(TASK_RUNNING); >>>>>>>>> break; >>>>>>>>> } >>>>>>>>> + mmc_start_delayed_bkops(card); >>>>>>>>> up(&mq->thread_sem); >>>>>>>>> schedule(); >>>>>>>>> down(&mq->thread_sem); >>>>>>>>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >>>>>>>>> index 06c42cf..72ae15b 100644 >>>>>>>>> --- a/drivers/mmc/core/core.c >>>>>>>>> +++ b/drivers/mmc/core/core.c >>>>>>>>> @@ -253,9 +253,36 @@ mmc_start_request(struct mmc_host *host, struct >>>>>>>>> mmc_request *mrq) >>>>>>>>> } >>>>>>>>> >>>>>>>>> /** >>>>>>>>> + * mmc_start_delayed_bkops() - Start a delayed work to check for >>>>>>>>> + * the need of non urgent BKOPS >>>>>>>>> + * >>>>>>>>> + * @card: MMC card to start BKOPS on >>>>>>>>> + */ >>>>>>>>> +void mmc_start_delayed_bkops(struct mmc_card *card) >>>>>>>>> +{ >>>>>>>>> + if (!card || !card->ext_csd.bkops_en || >>>>>>>>> mmc_card_doing_bkops(card)) >>>>>>>>> + return; >>>>>>>>> + >>>>>>>>> + pr_debug("%s: %s: queueing delayed_bkops_work\n", >>>>>>>>> + mmc_hostname(card->host), __func__); >>>>>>>>> + >>>>>>>>> + /* >>>>>>>>> + * cancel_delayed_bkops_work will prevent a race condition >>>>>>>>> between >>>>>>>>> + * fetching a request by the mmcqd and the delayed work, in >>>>>>>>> case >>>>>>>>> + * it was removed from the queue work but not started yet >>>>>>>>> + */ >>>>>>>>> + card->bkops_info.cancel_delayed_work = false; >>>>>>>>> + card->bkops_info.started_delayed_bkops = true; >>>>>>>>> + queue_delayed_work(system_nrt_wq, &card->bkops_info.dw, >>>>>>>>> + msecs_to_jiffies( >>>>>>>>> + card->bkops_info.delay_ms)); >>>>>>>>> +} >>>>>>>>> +EXPORT_SYMBOL(mmc_start_delayed_bkops); >>>>>>>>> + >>>>>>>>> +/** >>>>>>>>> * mmc_start_bkops - start BKOPS for supported cards >>>>>>>>> * @card: MMC card to start BKOPS >>>>>>>>> - * @form_exception: A flag to indicate if this function was >>>>>>>>> + * @from_exception: A flag to indicate if this function was >>>>>>>>> * called due to an exception raised by the card >>>>>>>>> * >>>>>>>>> * Start background operations whenever requested. >>>>>>>>> @@ -269,25 +296,47 @@ void mmc_start_bkops(struct mmc_card *card, >>>>>>>>> bool >>>>>>>>> from_exception) >>>>>>>>> bool use_busy_signal; >>>>>>>>> >>>>>>>>> BUG_ON(!card); >>>>>>>>> - >>>>>>>>> - if (!card->ext_csd.bkops_en || mmc_card_doing_bkops(card)) >>>>>>>>> + if (!card->ext_csd.bkops_en) >>>>>>>>> return; >>>>>>>>> >>>>>>>>> + mmc_claim_host(card->host); >>>>>>>>> + >>>>>>>>> + if ((card->bkops_info.cancel_delayed_work) && !from_exception) >>>>>>>>> { >>>>>>>>> + pr_debug("%s: %s: cancel_delayed_work was set, exit\n", >>>>>>>>> + mmc_hostname(card->host), __func__); >>>>>>>>> + card->bkops_info.cancel_delayed_work = false; >>>>>>>>> + goto out; >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> + if (mmc_card_doing_bkops(card)) { >>>>>>>>> + pr_debug("%s: %s: already doing bkops, exit\n", >>>>>>>>> + mmc_hostname(card->host), __func__); >>>>>>>>> + goto out; >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> err = mmc_read_bkops_status(card); >>>>>>>>> if (err) { >>>>>>>>> pr_err("%s: Failed to read bkops status: %d\n", >>>>>>>>> mmc_hostname(card->host), err); >>>>>>>>> - return; >>>>>>>>> + goto out; >>>>>>>>> } >>>>>>>>> >>>>>>>>> if (!card->ext_csd.raw_bkops_status) >>>>>>>>> - return; >>>>>>>>> + goto out; >>>>>>>>> >>>>>>>>> + pr_info("%s: %s: card->ext_csd.raw_bkops_status = 0x%x\n", >>>>>>>>> + mmc_hostname(card->host), __func__, >>>>>>>>> + card->ext_csd.raw_bkops_status); >>>>>>>>> + >>>>>>>>> + /* >>>>>>>>> + * If the function was called due to exception but there is no >>>>>>>>> need >>>>>>>>> + * for urgent BKOPS, BKOPs will be performed by the delayed >>>>>>>>> BKOPs >>>>>>>>> + * work, before going to suspend >>>>>>>>> + */ >>>>>>>>> if (card->ext_csd.raw_bkops_status < EXT_CSD_BKOPS_LEVEL_2 && >>>>>>>>> from_exception) >>>>>>>>> - return; >>>>>>>>> + goto out; >>>>>>>>> >>>>>>>>> - mmc_claim_host(card->host); >>>>>>>>> if (card->ext_csd.raw_bkops_status >= EXT_CSD_BKOPS_LEVEL_2) { >>>>>>>>> timeout = MMC_BKOPS_MAX_TIMEOUT; >>>>>>>>> use_busy_signal = true; >>>>>>>>> @@ -309,13 +358,108 @@ void mmc_start_bkops(struct mmc_card *card, >>>>>>>>> bool >>>>>>>>> from_exception) >>>>>>>>> * bkops executed synchronously, otherwise >>>>>>>>> * the operation is in progress >>>>>>>>> */ >>>>>>>>> - if (!use_busy_signal) >>>>>>>>> + if (!use_busy_signal) { >>>>>>>>> mmc_card_set_doing_bkops(card); >>>>>>>>> + pr_debug("%s: %s: starting the polling thread\n", >>>>>>>>> + mmc_hostname(card->host), __func__); >>>>>>>>> + queue_work(system_nrt_wq, >>>>>>>>> + &card->bkops_info.poll_for_completion); >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> out: >>>>>>>>> mmc_release_host(card->host); >>>>>>>>> } >>>>>>>>> EXPORT_SYMBOL(mmc_start_bkops); >>>>>>>>> >>>>>>>>> +/** >>>>>>>>> + * mmc_bkops_completion_polling() - Poll on the card status to >>>>>>>>> + * wait for the non-blocking BKOPS completion >>>>>>>>> + * @work: The completion polling work >>>>>>>>> + * >>>>>>>>> + * The on-going reading of the card status will prevent the card >>>>>>>>> + * from getting into suspend while it is in the middle of >>>>>>>>> + * performing BKOPS. >>>>>> >>>>>> Not true! Suspend will not be prevented by doing a mmc_send_status. >>>>>> Moreover, I would be interested to understand more about why you need >>>>>> to prevent "suspend". Please elaborate why you think this is needed. >>>>> This is explained in my general comment. Let me know if it is still not >>>>> clear. >>>>>> >>>>>>>>> + * Since the non blocking BKOPS can be interrupted by a fetched >>>>>>>>> + * request we also check IF mmc_card_doing_bkops in each >>>>>>>>> + * iteration. >>>>>>>>> + */ >>>>>>>>> +void mmc_bkops_completion_polling(struct work_struct *work) >>>>>>>>> +{ >>>>>>>>> + struct mmc_card *card = container_of(work, struct mmc_card, >>>>>>>>> + bkops_info.poll_for_completion); >>>>>>>>> + unsigned long timeout_jiffies = jiffies + >>>>>>>>> + msecs_to_jiffies(BKOPS_COMPLETION_POLLING_TIMEOUT_MS); >>>>>>>>> + u32 status; >>>>>>>>> + int err; >>>>>>>>> + >>>>>>>>> + /* >>>>>>>>> + * Wait for the BKOPs to complete. Keep reading the status to >>>>>>>>> prevent >>>>>>>>> + * the host from getting into suspend >>>>>>>>> + */ >>>>>>>>> + do { >>>>>>>>> + mmc_claim_host(card->host); >>>>>>>>> + >>>>>>>>> + if (!mmc_card_doing_bkops(card)) >>>>>>>>> + goto out; >>>>>>>>> + >>>>>>>>> + err = mmc_send_status(card, &status); >>>>>>>>> + if (err) { >>>>>>>>> + pr_err("%s: error %d requesting status\n", >>>>>>>>> + mmc_hostname(card->host), err); >>>>>>>>> + goto out; >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> + /* >>>>>>>>> + * Some cards mishandle the status bits, so make sure >>>>>>>>> to >>>>>>>>> check >>>>>>>>> + * both the busy indication and the card state. >>>>>>>>> + */ >>>>>>>>> + if ((status & R1_READY_FOR_DATA) && >>>>>>>>> + (R1_CURRENT_STATE(status) != R1_STATE_PRG)) { >>>>>>>>> + pr_debug("%s: %s: completed BKOPs, exit >>>>>>>>> polling\n", >>>>>>>>> + mmc_hostname(card->host), __func__); >>>>>>>>> + mmc_card_clr_doing_bkops(card); >>>>>>>>> + card->bkops_info.started_delayed_bkops = false; >>>>>>>>> + goto out; >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> + mmc_release_host(card->host); >>>>>>>>> + >>>>>>>>> + /* >>>>>>>>> + * Sleep before checking the card status again to allow >>>>>>>>> the >>>>>>>>> + * card to complete the BKOPs operation >>>>>>>>> + */ >>>>>>>>> + msleep(BKOPS_COMPLETION_POLLING_INTERVAL_MS); >>>>>>>>> + } while (time_before(jiffies, timeout_jiffies)); >>>>>>>>> + >>>>>>>>> + pr_err("%s: %s: exit polling due to timeout\n", >>>>>>>>> + mmc_hostname(card->host), __func__); >>>>>>>>> + >>>>>>>>> + return; >>>>>>>>> +out: >>>>>>>>> + mmc_release_host(card->host); >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> +/** >>>>>>>>> + * mmc_start_idle_time_bkops() - check if a non urgent BKOPS is >>>>>>>>> + * needed >>>>>>>>> + * @work: The idle time BKOPS work >>>>>>>>> + */ >>>>>>>>> +void mmc_start_idle_time_bkops(struct work_struct *work) >>>>>>>>> +{ >>>>>>>>> + struct mmc_card *card = container_of(work, struct mmc_card, >>>>>>>>> + bkops_info.dw.work); >>>>>>>>> + >>>>>>>>> + /* >>>>>>>>> + * Prevent a race condition between mmc_stop_bkops and the >>>>>>>>> delayed >>>>>>>>> + * BKOPS work in case the delayed work is executed on another >>>>>>>>> CPU >>>>>>>>> + */ >>>>>>>>> + if (card->bkops_info.cancel_delayed_work) >>>>>>>>> + return; >>>>>>>>> + >>>>>>>>> + mmc_start_bkops(card, false); >>>>>>>>> +} >>>>>>>>> +EXPORT_SYMBOL(mmc_start_idle_time_bkops); >>>>>>>>> + >>>>>>>>> static void mmc_wait_done(struct mmc_request *mrq) >>>>>>>>> { >>>>>>>>> complete(&mrq->completion); >>>>>>>>> @@ -582,6 +726,17 @@ int mmc_stop_bkops(struct mmc_card *card) >>>>>>>>> int err = 0; >>>>>>>>> >>>>>>>>> BUG_ON(!card); >>>>>>>>> + >>>>>>>>> + /* >>>>>>>>> + * Notify the delayed work to be cancelled, in case it was >>>>>>>>> already >>>>>>>>> + * removed from the queue, but was not started yet >>>>>>>>> + */ >>>>>>>>> + card->bkops_info.cancel_delayed_work = true; >>>>>>>>> + if (delayed_work_pending(&card->bkops_info.dw)) >>>>>>>>> + cancel_delayed_work_sync(&card->bkops_info.dw); >>>>>>>>> + if (!mmc_card_doing_bkops(card)) >>>>>>>>> + goto out; >>>>>>>>> + >>>>>>>>> err = mmc_interrupt_hpi(card); >>>>>>>>> >>>>>>>>> /* >>>>>>>>> @@ -593,6 +748,7 @@ int mmc_stop_bkops(struct mmc_card *card) >>>>>>>>> err = 0; >>>>>>>>> } >>>>>>>>> >>>>>>>>> +out: >>>>>>>>> return err; >>>>>>>>> } >>>>>>>>> EXPORT_SYMBOL(mmc_stop_bkops); >>>>>>>>> @@ -2506,15 +2662,15 @@ int mmc_pm_notify(struct notifier_block >>>>>>>>> *notify_block, >>>>>>>>> switch (mode) { >>>>>>>>> case PM_HIBERNATION_PREPARE: >>>>>>>>> case PM_SUSPEND_PREPARE: >>>>>>>>> - if (host->card && mmc_card_mmc(host->card) && >>>>>>>>> - mmc_card_doing_bkops(host->card)) { >>>>>>>>> + if (host->card && mmc_card_mmc(host->card)) { >>>>>>>>> + mmc_claim_host(host); >>>>>>>>> err = mmc_stop_bkops(host->card); >>>>>>>>> + mmc_release_host(host); >>>>>> >>>>>> This code seems a bit strange. You will check for mmc_card_mmc, but >>>>>> not all (e)MMC will support bkops. How about acually just checking if >>>>>> bkops is "on" or "off" somehow. >>>>>> >>>>>> Additionally, so this piece of code shall stop an ongoing bkops before >>>>>> going to suspend, so that make sense. But would it not be more >>>>>> meaningfull to take care of that in mmc_suspend_host? I mean why do yo >>>>>> need to do it in the PM_SUSPEND_PREPARE notification sequence >>>>>> especially? >>>>> This code was not added by me. I just added the >>>>> mmc_claim_host(host)/mmc_release_host calls. Maybe Jaehoon, who added >>>>> this >>>>> code in the BKOPS patch can respond to your comment. >>>>>> >>>>>>>>> if (err) { >>>>>>>>> pr_err("%s: didn't stop bkops\n", >>>>>>>>> mmc_hostname(host)); >>>>>>>>> return err; >>>>>>>>> } >>>>>>>>> - mmc_card_clr_doing_bkops(host->card); >>>>>>>>> } >>>>>>>>> >>>>>>>>> spin_lock_irqsave(&host->lock, flags); >>>>>>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c >>>>>>>>> index 7cc4638..dba76e3 100644 >>>>>>>>> --- a/drivers/mmc/core/mmc.c >>>>>>>>> +++ b/drivers/mmc/core/mmc.c >>>>>>>>> @@ -1258,6 +1258,29 @@ static int mmc_init_card(struct mmc_host >>>>>>>>> *host, >>>>>>>>> u32 ocr, >>>>>>>>> } >>>>>>>>> } >>>>>>>>> >>>>>>>>> + if (!oldcard) { >>>>>>>>> + if (card->ext_csd.bkops_en) { >>>>>>>>> + INIT_DELAYED_WORK(&card->bkops_info.dw, >>>>>>>>> + mmc_start_idle_time_bkops); >>>>>>>>> + >>>>>>>>> INIT_WORK(&card->bkops_info.poll_for_completion, >>>>>>>>> + mmc_bkops_completion_polling); >>>>>> >>>>>> I guess you don't have "removable" eMMC with BKOPS support so this >>>>>> code will in practice only be executed once for an eMMC, so we are >>>>>> safe. But still I am not fond of putting this code for workqueue here. >>>>>> Either that should be done as a part of when the card is >>>>>> created/deleted or when then host is created/deleted. >>>>> I will check if there could be a better place for this. >>>>>> >>>>>>>>> + >>>>>>>>> + /* >>>>>>>>> + * Calculate the time to start the BKOPs >>>>>>>>> checking. >>>>>>>>> + * The idle time of the host controller should >>>>>>>>> be >>>>>>>>> taken >>>>>>>>> + * into account in order to prevent a race >>>>>>>>> condition >>>>>>>>> + * before starting BKOPs and going into >>>>>>>>> suspend. >>>>>>>>> + * If the host controller didn't set its idle >>>>>>>>> time, >>>>>>>>> + * a default value is used. >>>>>>>>> + */ >>>>>>>>> + card->bkops_info.delay_ms = >>>>>>>>> MMC_IDLE_BKOPS_TIME_MS; >>>>>>>>> + if (card->bkops_info.host_suspend_tout_ms) >>>>>>>>> + card->bkops_info.delay_ms = min( >>>>>>>>> + card->bkops_info.delay_ms, >>>>>>>>> + >>>>>>>>> card->bkops_info.host_suspend_tout_ms/2); >>>>>>>>> + } >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> if (!oldcard) >>>>>>>>> host->card = card; >>>>>>>>> >>>>>>>>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h >>>>>>>>> index 943550d..224e2a5 100644 >>>>>>>>> --- a/include/linux/mmc/card.h >>>>>>>>> +++ b/include/linux/mmc/card.h >>>>>>>>> @@ -208,6 +208,39 @@ struct mmc_part { >>>>>>>>> #define MMC_BLK_DATA_AREA_GP (1<<2) >>>>>>>>> }; >>>>>>>>> >>>>>>>>> +/** >>>>>>>>> + * struct mmc_bkops_info - BKOPS data >>>>>>>>> + * @dw: Idle time bkops delayed work >>>>>>>>> + * @host_suspend_tout_ms: The host controller idle time, >>>>>>>>> + * before getting into suspend >>>>>>>>> + * @delay_ms: The time to start the BKOPS >>>>>>>>> + * delayed work once MMC thread is idle >>>>>>>>> + * @poll_for_completion: Poll on BKOPS completion >>>>>>>>> + * @cancel_delayed_work: A flag to indicate if the delayed work >>>>>>>>> + * should be cancelled >>>>>>>>> + * @started_delayed_bkops: A flag to indicate if the delayed >>>>>>>>> + * work was scheduled >>>>>>>>> + * @sectors_changed: number of sectors written or >>>>>>>>> + * discard since the last idle BKOPS were scheduled >>>>>>>>> + */ >>>>>>>>> +struct mmc_bkops_info { >>>>>>>>> + struct delayed_work dw; >>>>>>>>> + unsigned int host_suspend_tout_ms; >>>>>> >>>>>> As stated earlier, what is this timeout you are referring to? What >>>>>> suspend? >>>>> Explained above. >>>>>> >>>>>>>>> + unsigned int delay_ms; >>>>>>>>> +/* >>>>>>>>> + * A default time for checking the need for non urgent BKOPS once >>>>>>>>> mmcqd >>>>>>>>> + * is idle. >>>>>>>>> + */ >>>>>>>>> +#define MMC_IDLE_BKOPS_TIME_MS 2000 >>>>>>>>> + struct work_struct poll_for_completion; >>>>>>>>> +/* Polling timeout and interval for waiting on non-blocking BKOPs >>>>>>>>> completion */ >>>>>>>>> +#define BKOPS_COMPLETION_POLLING_TIMEOUT_MS 10000 /* in ms */ >>>>>>>>> +#define BKOPS_COMPLETION_POLLING_INTERVAL_MS 1000 /* in ms */ >>>>>>>>> + bool cancel_delayed_work; >>>>>>>>> + bool started_delayed_bkops; >>>>>>>>> + unsigned int sectors_changed; >>>>>> >>>>>> Could not find "sectors_changed" being used. Maybe you forgot to >>>>>> remove this from previous version of the patch. >>>>> Yes, this should be removed. >>>>>> >>>>>>>>> +}; >>>>>>>>> + >>>>>>>>> /* >>>>>>>>> * MMC device >>>>>>>>> */ >>>>>>>>> @@ -276,6 +309,8 @@ struct mmc_card { >>>>>>>>> struct dentry *debugfs_root; >>>>>>>>> struct mmc_part part[MMC_NUM_PHY_PARTITION]; /* physical >>>>>>>>> partitions */ >>>>>>>>> unsigned int nr_parts; >>>>>>>>> + >>>>>>>>> + struct mmc_bkops_info bkops_info; >>>>>>>>> }; >>>>>>>>> >>>>>>>>> /* >>>>>>>>> diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h >>>>>>>>> index 9b9cdaf..665d345 100644 >>>>>>>>> --- a/include/linux/mmc/core.h >>>>>>>>> +++ b/include/linux/mmc/core.h >>>>>>>>> @@ -145,6 +145,9 @@ extern int mmc_app_cmd(struct mmc_host *, struct >>>>>>>>> mmc_card *); >>>>>>>>> extern int mmc_wait_for_app_cmd(struct mmc_host *, struct mmc_card >>>>>>>>> *, >>>>>>>>> struct mmc_command *, int); >>>>>>>>> extern void mmc_start_bkops(struct mmc_card *card, bool >>>>>>>>> from_exception); >>>>>>>>> +extern void mmc_start_delayed_bkops(struct mmc_card *card); >>>>>>>>> +extern void mmc_start_idle_time_bkops(struct work_struct *work); >>>>>>>>> +extern void mmc_bkops_completion_polling(struct work_struct *work); >>>>>>>>> extern int __mmc_switch(struct mmc_card *, u8, u8, u8, unsigned >>>>>>>>> int, >>>>>>>>> bool); >>>>>>>>> extern int mmc_switch(struct mmc_card *, u8, u8, u8, unsigned int); >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a >>>>>>> member >>>>>>> of Code Aurora Forum, hosted by The Linux Foundation >>>>>>> >>>>>>> -- >>>>>>> To unsubscribe from this list: send the line "unsubscribe >>>>>>> linux-kernel" >>>>>>> in >>>>>>> the body of a message to majordomo@xxxxxxxxxxxxxxx >>>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>>>>> Please read the FAQ at http://www.tux.org/lkml/ >>>>>> >>>>>> Finally some overall thoughts. What I would like to understand is how >>>>>> we decide that the card has become "idle". I belive two values should >>>>>> be considered, but are they? >>>>>> 1. The card need BKOPS to be performed for some status level. >>>>>> 2. Request inactivity for a certain timeout has occured. >>>>>> >>>>>> Have you considered to use runtime PM for the card device instead of >>>>>> the workqueue? >>>>>> >>>>>> Kind regards >>>>>> Ulf Hansson >>>>>> -- >>>>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in >>>>>> the body of a message to majordomo@xxxxxxxxxxxxxxx >>>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>>>> >>>>> >>>>> >>>>> -- >>>>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a >>>>> member >>>>> of Code Aurora Forum, hosted by The Linux Foundation >>>>> >>>>> -- >>>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" >>>>> in >>>>> the body of a message to majordomo@xxxxxxxxxxxxxxx >>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>>> Please read the FAQ at http://www.tux.org/lkml/ >>>> >>>> Kind regards >>>> Ulf Hansson >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in >>>> the body of a message to majordomo@xxxxxxxxxxxxxxx >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>> >>> >>> >>> -- >>> QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member >>> of Code Aurora Forum, hosted by The Linux Foundation >>> >> >> Kind regards >> Ulf Hansson >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in >> the body of a message to majordomo@xxxxxxxxxxxxxxx >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > Kind regards > Ulf Hansson > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
