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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux