Re: [PATCH v3] mmc: core: Add support for idle time BKOPS

[Ulf Hansson]

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/