mmc: core: Add support for idle time BKOPs
Subsystems (2)
Re: [PATCH v3] mmc: core: Add support for idle time BKOPS
In reply to Maya ErezQuoted 5 lines · Maya Erez
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.
Quoted 833 lines
Thanks,
Maya-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Ulf Hansson
Sent: Thursday, December 13, 2012 12:18 PM
To: [email protected]
Cc: Jaehoon Chung; [email protected]; [email protected]; open list
Subject: Re: [PATCH v3] mmc: core: Add support for idle time BKOPSQuoted 29 lines · On 12 December 2012 13:32, <[email protected]> wrote:
Hi Ulf,
Sorry for the late response.
See my reply below.Thanks,
MayaQuoted 20 lines · Ulf Hansson
Hi Maya,
Quoted 16 lines · On 4 December 2012 22:17, <[email protected]> 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.Quoted 7 lines
Quoted 3 lines
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 ofcore/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.
Quoted 7 lines
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 callingmmc_suspend_hostin runtime suspend, which issues an HPI to stop the BKOPS.So, doing
mmc_suspend_hostin youruntime_suspendcallback, 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? :-)
Quoted 3 lines
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_syncfor the host device. Although, it _must_ not be done for drivers not doingmmc_suspend_hostin their runtime suspend callbacks. Since then it will prevent these from doing runtime power save actions, which is not ok.Quoted 33 lines
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.
Quoted 11 lines
Quoted 6 lines
2. Using
PM_SUSPEND_PREPAREinstead 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_bkopsevery time MMC becomes idle, to check the need for BKOPS. I will test it and include it in the next patch.Quoted 14 lines
Quoted 10 lines
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_hostcallshost->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 doingget_card_statuswe callmmc_claim_hostand this call is the one that "delays" getting into suspend.
host->ops->enableis the old way of implementing runtime power save for host drivers. Nowadays most drivers is using runtime PM instead.Quoted 87 lines
Quoted 7 lines
When you say that
mmc_claim_hostwill prevent suspend, I suppose you mean thathost->ops->disablewont 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_supendorhost->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.
Quoted 78 lines
Quoted 78 lines
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,
MayaQuoted 67 lines · Ulf Hansson
Quoted 17 lines · On 3 December 2012 10:49, <[email protected]> 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 themsm_sdcccontroller.Chris - We do expect it to influence the battery consumption, since we now delay getting into suspend (since
mmc_start_bkopswhich 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 topm_schedule_suspendby the periodic BKOPS idle time.
Does it make sense to you?Quoted 40 lines
Thanks,
MayaQuoted 37 lines · Jaehoon Chung
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.Quoted 24 lines · Maya Erez
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 updatingBKOPS_STATUS(EXT_CSDbyte [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 thecase.
Quoted 608 lines
Quoted 599 lines
Quoted 6 lines
Quoted 3 lines
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.Quoted 581 lines
Quoted 10 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
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.
Quoted 222 lines
Quoted 222 lines
Quoted 222 lines
Quoted 64 lines
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 <[email protected]>---
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
- Sincemmc_start_bkopsis called from two contexts now,mmc_claim_hostwas moved to the beginning of the function
- Also, the check ofdoing_bkopsshould be protected when determing if an HPI is needed due to the same reason.Changes in v3:
- Move the call tostop_bkopstoblock.c.
This allows us to remove themmc_claim_hostfrom inside
the function and doesn't cause additional degradation
due to un-neccessary calim host operationChanges 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(-)drivers/mmc/card/block.c +5
-1@@ -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_bkopsQuoted 3 lines
=
false;
+mmc_stop_bkops(card);We didn't need to check whether
mmc_stop_bkopsis success or not? Ifmmc_stop_bkops()is failed, then bkops is continuously running.Best Regards,
Jaehoon ChungQuoted 149 lines
drivers/mmc/card/block.c +2
-0+ } + } ret =mmc_blk_part_switch (card, md);if (ret) {drivers/mmc/card/queue.c +2
-0@@ -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);drivers/mmc/core/core.c +15
-0@@ -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
casedrivers/mmc/core/core.c +12
-1+* 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);drivers/mmc/core/core.c +4
-2- -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){
drivers/mmc/core/core.c +20
-2+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
BKOPsdrivers/mmc/core/core.c +3
-2+* 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
*/drivers/mmc/core/core.c +7
-1-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);Quoted 12 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
+/**
+ *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.
Quoted 135 lines
Quoted 115 lines
Quoted 115 lines
Quoted 115 lines
+ * Since the non blocking BKOPS can be interrupted by a fetched
+ * request we also check IFmmc_card_doing_bkopsin each
+ * iteration.
+ */drivers/mmc/core/core.c +11
-0+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
drivers/mmc/core/core.c +17
-0+* 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 sureto
checkdrivers/mmc/core/core.c +5
-0+* 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, exitpolling\n",
drivers/mmc/core/core.c +10
-0+ 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
drivers/mmc/core/core.c +25
-0+* 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
CPUdrivers/mmc/core/core.c +11
-0+*/ +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
drivers/mmc/core/core.c +8
-0+* 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 ; }Quoted 15 lines
Quoted 7 lines
Quoted 7 lines
drivers/mmc/core/core.c +1
-0+out:return err; }EXPORT_SYMBOL (mmc_stop_bkops);@@ -2506,15 +2662,15 @@ int mmc_pm_notify(struct notifier_block *notify_block, switch (mode) {Quoted 8 lines
drivers/mmc/core/core.c int mmc_pm_notify(struct notifier_block · line 2669
Quoted 8 lines
drivers/mmc/core/core.c +3
-2case 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.Quoted 5 lines
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 thePM_SUSPEND_PREPAREnotification 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.Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
drivers/mmc/core/core.c +0
-1if (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);drivers/mmc/core/mmc.c +0
-0@@ -1258,6 +1258,29 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr, } }Quoted 13 lines
Quoted 7 lines
Quoted 7 lines
drivers/mmc/core/mmc.c static int mmc_init_card(struct mmc_host · line 1269
Quoted 7 lines
drivers/mmc/core/mmc.c +6
-0+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.
Quoted 50 lines
Quoted 50 lines
Quoted 50 lines
Quoted 50 lines
drivers/mmc/core/mmc.c +3
-0+ +/* +* 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,drivers/mmc/core/mmc.c +3
-0+* a default value is used. +*/ + card->bkops_info.delay_ms =
MMC_IDLE_BKOPS_TIME_MS;drivers/mmc/core/mmc.c +4
-0+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);
drivers/mmc/core/mmc.c +3
-0+ } + } +if (!oldcard) host->card = card;include/linux/mmc/card.h +13
-0@@ -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 Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
include/linux/mmc/card.h struct mmc_part { · line 228
Quoted 5 lines
include/linux/mmc/card.h +5
-0+* 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.
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
include/linux/mmc/card.h +3
-0+unsigned int delay_ms; +/* +* A default time for checking the need for non urgent BKOPS once mmcqd
+ * is idle.
+ */include/linux/mmc/card.h +3
-0+#define MMC_IDLE_BKOPS_TIME_MS 2000 +struct work_struct poll_for_completion; +/* Polling timeout and interval for waiting on non-blocking BKOPs completion */
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
include/linux/mmc/card.h
Quoted 5 lines
include/linux/mmc/card.h +5
-0+#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.
Quoted 62 lines
Quoted 45 lines
Quoted 28 lines
Quoted 28 lines
include/linux/mmc/card.h +2
-0+}; +/* * 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;include/linux/mmc/card.h +2
-0+ +struct mmc_bkops_info bkops_info; };/* include/linux/mmc/core.h +1
-0@@ -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);Quoted 10 lines
Quoted 8 lines
include/linux/mmc/core.h +2
-0+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 intmmc_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 FoundationQuoted 7 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel"
in
the body of a message to [email protected]
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?
Quoted 7 lines
Kind regards
Ulf Hansson
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
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 FoundationQuoted 6 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/Quoted 7 lines
Kind regards
Ulf Hansson
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.htmlQuoted 4 lines
--
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Quoted 7 lines
Kind regards
Ulf Hansson
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Kind regards
Ulf Hansson
Re: [PATCH v3] mmc: core: Add support for idle time BKOPS
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
Quoted 865 lines · Ulf Hansson
Quoted 5 lines · Maya Erez
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.Quoted 833 lines
Thanks,
Maya-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Ulf Hansson
Sent: Thursday, December 13, 2012 12:18 PM
To: [email protected]
Cc: Jaehoon Chung; [email protected]; [email protected]; open list
Subject: Re: [PATCH v3] mmc: core: Add support for idle time BKOPSQuoted 29 lines · On 12 December 2012 13:32, <[email protected]> wrote:
Hi Ulf,
Sorry for the late response.
See my reply below.Thanks,
MayaQuoted 20 lines · Ulf Hansson
Hi Maya,
Quoted 16 lines · On 4 December 2012 22:17, <[email protected]> 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.Quoted 7 lines
Quoted 3 lines
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 ofcore/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.
Quoted 7 lines
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 callingmmc_suspend_hostin runtime suspend, which issues an HPI to stop the BKOPS.So, doing
mmc_suspend_hostin youruntime_suspendcallback, 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? :-)
Quoted 3 lines
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_syncfor the host device. Although, it _must_ not be done for drivers not doingmmc_suspend_hostin their runtime suspend callbacks. Since then it will prevent these from doing runtime power save actions, which is not ok.Quoted 33 lines
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.
Quoted 11 lines
Quoted 6 lines
2. Using
PM_SUSPEND_PREPAREinstead 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_bkopsevery time MMC becomes idle, to check the need for BKOPS. I will test it and include it in the next patch.Quoted 14 lines
Quoted 10 lines
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_hostcallshost->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 doingget_card_statuswe callmmc_claim_hostand this call is the one that "delays" getting into suspend.
host->ops->enableis the old way of implementing runtime power save for host drivers. Nowadays most drivers is using runtime PM instead.Quoted 87 lines
Quoted 7 lines
When you say that
mmc_claim_hostwill prevent suspend, I suppose you mean thathost->ops->disablewont 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_supendorhost->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.
Quoted 78 lines
Quoted 78 lines
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,
MayaQuoted 67 lines · Ulf Hansson
Quoted 17 lines · On 3 December 2012 10:49, <[email protected]> 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 themsm_sdcccontroller.Chris - We do expect it to influence the battery consumption, since we now delay getting into suspend (since
mmc_start_bkopswhich 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 topm_schedule_suspendby the periodic BKOPS idle time.
Does it make sense to you?Quoted 40 lines
Thanks,
MayaQuoted 37 lines · Jaehoon Chung
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.Quoted 24 lines · Maya Erez
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 updatingBKOPS_STATUS(EXT_CSDbyte [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 thecase.
Quoted 608 lines
Quoted 599 lines
Quoted 6 lines
Quoted 3 lines
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.Quoted 581 lines
Quoted 10 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
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.
Quoted 222 lines
Quoted 222 lines
Quoted 222 lines
Quoted 64 lines
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 <[email protected]>---
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
- Sincemmc_start_bkopsis called from two contexts now,mmc_claim_hostwas moved to the beginning of the function
- Also, the check ofdoing_bkopsshould be protected when determing if an HPI is needed due to the same reason.Changes in v3:
- Move the call tostop_bkopstoblock.c.
This allows us to remove themmc_claim_hostfrom inside
the function and doesn't cause additional degradation
due to un-neccessary calim host operationChanges 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(-)drivers/mmc/card/block.c +5
-1@@ -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_bkopsQuoted 3 lines
=
false;
+mmc_stop_bkops(card);We didn't need to check whether
mmc_stop_bkopsis success or not? Ifmmc_stop_bkops()is failed, then bkops is continuously running.Best Regards,
Jaehoon ChungQuoted 149 lines
drivers/mmc/card/block.c +2
-0+ } + } ret =mmc_blk_part_switch (card, md);if (ret) {drivers/mmc/card/queue.c +2
-0@@ -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);drivers/mmc/core/core.c +15
-0@@ -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
casedrivers/mmc/core/core.c +12
-1+* 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);drivers/mmc/core/core.c +4
-2- -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){
drivers/mmc/core/core.c +20
-2+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
BKOPsdrivers/mmc/core/core.c +3
-2+* 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
*/drivers/mmc/core/core.c +7
-1-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);Quoted 12 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
+/**
+ *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.
Quoted 135 lines
Quoted 115 lines
Quoted 115 lines
Quoted 115 lines
+ * Since the non blocking BKOPS can be interrupted by a fetched
+ * request we also check IFmmc_card_doing_bkopsin each
+ * iteration.
+ */drivers/mmc/core/core.c +11
-0+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
drivers/mmc/core/core.c +17
-0+* 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 sureto
checkdrivers/mmc/core/core.c +5
-0+* 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, exitpolling\n",
drivers/mmc/core/core.c +10
-0+ 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
drivers/mmc/core/core.c +25
-0+* 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
CPUdrivers/mmc/core/core.c +11
-0+*/ +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
drivers/mmc/core/core.c +8
-0+* 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 ; }Quoted 15 lines
Quoted 7 lines
Quoted 7 lines
drivers/mmc/core/core.c +1
-0+out:return err; }EXPORT_SYMBOL (mmc_stop_bkops);@@ -2506,15 +2662,15 @@ int mmc_pm_notify(struct notifier_block *notify_block, switch (mode) {Quoted 8 lines
drivers/mmc/core/core.c int mmc_pm_notify(struct notifier_block · line 2669
Quoted 8 lines
drivers/mmc/core/core.c +3
-2case 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.Quoted 5 lines
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 thePM_SUSPEND_PREPAREnotification 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.Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
drivers/mmc/core/core.c +0
-1if (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);drivers/mmc/core/mmc.c +0
-0@@ -1258,6 +1258,29 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr, } }Quoted 13 lines
Quoted 7 lines
Quoted 7 lines
drivers/mmc/core/mmc.c static int mmc_init_card(struct mmc_host · line 1269
Quoted 7 lines
drivers/mmc/core/mmc.c +6
-0+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.
Quoted 50 lines
Quoted 50 lines
Quoted 50 lines
Quoted 50 lines
drivers/mmc/core/mmc.c +3
-0+ +/* +* 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,drivers/mmc/core/mmc.c +3
-0+* a default value is used. +*/ + card->bkops_info.delay_ms =
MMC_IDLE_BKOPS_TIME_MS;drivers/mmc/core/mmc.c +4
-0+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);
drivers/mmc/core/mmc.c +3
-0+ } + } +if (!oldcard) host->card = card;include/linux/mmc/card.h +13
-0@@ -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 Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
include/linux/mmc/card.h struct mmc_part { · line 228
Quoted 5 lines
include/linux/mmc/card.h +5
-0+* 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.
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
include/linux/mmc/card.h +3
-0+unsigned int delay_ms; +/* +* A default time for checking the need for non urgent BKOPS once mmcqd
+ * is idle.
+ */include/linux/mmc/card.h +3
-0+#define MMC_IDLE_BKOPS_TIME_MS 2000 +struct work_struct poll_for_completion; +/* Polling timeout and interval for waiting on non-blocking BKOPs completion */
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
include/linux/mmc/card.h
Quoted 5 lines
include/linux/mmc/card.h +5
-0+#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.
Quoted 62 lines
Quoted 45 lines
Quoted 28 lines
Quoted 28 lines
include/linux/mmc/card.h +2
-0+}; +/* * 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;include/linux/mmc/card.h +2
-0+ +struct mmc_bkops_info bkops_info; };/* include/linux/mmc/core.h +1
-0@@ -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);Quoted 10 lines
Quoted 8 lines
include/linux/mmc/core.h +2
-0+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 intmmc_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 FoundationQuoted 7 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel"
in
the body of a message to [email protected]
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?
Quoted 7 lines
Kind regards
Ulf Hansson
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
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 FoundationQuoted 6 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/Quoted 7 lines
Kind regards
Ulf Hansson
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.htmlQuoted 4 lines
--
QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux FoundationQuoted 7 lines
Kind regards
Ulf Hansson
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.htmlKind regards
Ulf Hansson
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
PATCH v4 1/1 mmc: fix async request mechanism for sequential read scenarios
When current request is running on the bus and if next request fetched by mmcqd is NULL, mmc context (mmcqd thread) gets blocked until the current request completes. This means if new request comes in while the mmcqd thread is blocked, this new request can not be prepared in parallel to current ongoing request. This may result in latency to start new request.
This change allows to wake up the MMC thread (which
is waiting for the current running request to complete). Now once the MMC thread is woken up, new request can be fetched and prepared in parallel to current running request which means this new request can be started immediately after the current running request completes.
With this change read throughput is improved by 16%.
---
v4: keeps new synchronization mechanism within mmc/core
layer, so mmc/core external API's are not changed.
- context_info moved into mmc_host struct
- context_info initialized in mmc_attach_mmc()
v3:
- new MMC_QUEUE_NEW_REQUEST flag to mark new request case
- lock added to update is_new_req flag
- condition for sending new req notification changed
- Moved start waiting for new req notification after fetching NULL
v2:
- Removed synchronization flags
- removed lock from done()
- flags names changed
v1:
- Initial submit
drivers/mmc/card/block.c +9 -14
@@ -113,17 +113,6 @@ struct mmc_blk_data {static DEFINE_MUTEX (open_lock); -enum mmc_blk_status { -MMC_BLK_SUCCESS =0 , -MMC_BLK_PARTIAL , -MMC_BLK_CMD_ERR , -MMC_BLK_RETRY , -MMC_BLK_ABORT , -MMC_BLK_DATA_ERR , -MMC_BLK_ECC_ERR , -MMC_BLK_NOMEDIUM , -}; -module_param (perdev_minors, int,0444 );MODULE_PARM_DESC (perdev_minors,"Minors numbers to allocate per device" );
@@ -1363,9 +1352,12 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc) areq = &mq->mqrq_cur->mmc_active; }else areq =NULL ; - areq =mmc_start_req (card->host, areq, (int *) &status); -if (!areq) + areq =mmc_start_req (card->host, areq, (int *)&status); +if (!areq) { +if (status ==MMC_BLK_NEW_REQUEST ) + mq->flags |=MMC_QUEUE_NEW_REQUEST ;return 0 ; + } mq_rq = container_of(areq, struct mmc_queue_req, mmc_active); brq = &mq_rq->brq;
@@ -1374,6 +1366,8 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)mmc_queue_bounce_post (mq_rq);switch (status) { +case MMC_BLK_NEW_REQUEST : +BUG ();/* should never get here */ case MMC_BLK_SUCCESS :case MMC_BLK_PARTIAL :/*
@@ -1486,6 +1480,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req) goto out; } + mq->flags &= ~MMC_QUEUE_NEW_REQUEST ;if (req && req->cmd_flags &REQ_DISCARD ) {/* complete ongoing async transfer before issuing discard */ if (card->host->areq)
@@ -1505,7 +1500,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req) } out: -if (!req) +if (!req && !(mq->flags &MMC_QUEUE_NEW_REQUEST ))/* release host only when there are no more requests */ mmc_release_host (card->host);return ret;
drivers/mmc/card/queue.c +25 -2
@@ -22,7 +22,6 @@#define MMC_QUEUE_BOUNCESZ 65536 -#define MMC_QUEUE_SUSPENDED (1 << 0)/* * Prepare a MMC request. This just filters out odd stuff.
@@ -63,11 +62,20 @@ static int mmc_queue_thread(void *d)set_current_state (TASK_INTERRUPTIBLE ); req =blk_fetch_request (q); mq->mqrq_cur->req = req; +if (!req && mq->mqrq_prev->req && + !(mq->mqrq_prev->req->cmd_flags &REQ_FLUSH ) && + !(mq->mqrq_prev->req->cmd_flags &REQ_DISCARD )) + mq->card->host->context_info.is_waiting_last_req = true; +spin_unlock_irq (q->queue_lock);if (req || mq->mqrq_prev->req) {set_current_state (TASK_RUNNING ); mq->issue_fn (mq, req); +if (mq->flags &MMC_QUEUE_NEW_REQUEST ) { + mq->flags &= ~MMC_QUEUE_NEW_REQUEST ; +continue ;/* fetch again */ + }/* * Current request becomes previous request
@@ -103,6 +111,8 @@ static void mmc_request_fn(struct request_queue *q) {struct mmc_queue *mq = q->queuedata;struct request *req; +unsigned long flags; +struct mmc_context_info *cntx;if (!mq) {while ((req =blk_fetch_request (q)) !=NULL ) {
@@ -112,7 +122,20 @@ static void mmc_request_fn(struct request_queue *q)return ; } -if (!mq->mqrq_cur->req && !mq->mqrq_prev->req) + cntx = &mq->card->host->context_info; +if (!mq->mqrq_cur->req && mq->mqrq_prev->req) { +/* +* New MMC request arrived when MMC thread may be +* blocked on the previous request to be complete +* with no current request fetched +*/ +spin_lock_irqsave (&cntx->lock, flags); +if (cntx->is_waiting_last_req) { + cntx->is_new_req = true; +wake_up_interruptible (&cntx->wait); + } +spin_unlock_irqrestore (&cntx->lock, flags); + }else if (!mq->mqrq_cur->req && !mq->mqrq_prev->req)wake_up_process (mq->thread); }
drivers/mmc/card/queue.h +2 -0
@@ -26,6 +26,8 @@ struct mmc_queue {struct mmc_card *card;struct task_struct *thread;struct semaphore thread_sem; +#define MMC_QUEUE_SUSPENDED (1 << 0) +#define MMC_QUEUE_NEW_REQUEST (1 << 1)unsigned int flags;int (*issue_fn)(struct mmc_queue *,struct request *);void *data;
drivers/mmc/core/core.c +103 -3
135 lines not on this page · raw message
drivers/mmc/core/mmc.c +6 -0
@@ -1497,6 +1497,12 @@ int mmc_attach_mmc(struct mmc_host *host)BUG_ON (!host);WARN_ON (!host->claimed); +spin_lock_init (&host->context_info.lock); + host->context_info.is_new_req = false; + host->context_info.is_done_rcv = false; + host->context_info.is_waiting_last_req = false; +init_waitqueue_head (&host->context_info.wait); +/* Set correct bus mode for MMC before attempting attach */ if (!mmc_host_is_spi (host))mmc_set_bus_mode (host,MMC_BUSMODE_OPENDRAIN );
include/linux/mmc/card.h +12 -0
@@ -187,6 +187,18 @@ struct sdio_func_tuple;#define SDIO_MAX_FUNCS 7 +enum mmc_blk_status { +MMC_BLK_SUCCESS =0 , +MMC_BLK_PARTIAL , +MMC_BLK_CMD_ERR , +MMC_BLK_RETRY , +MMC_BLK_ABORT , +MMC_BLK_DATA_ERR , +MMC_BLK_ECC_ERR , +MMC_BLK_NOMEDIUM , +MMC_BLK_NEW_REQUEST , +}; +/* The number of MMC physical partitions. These consist of: * boot partitions (2), general purpose partitions (4) in MMC v4.4. */
include/linux/mmc/core.h +2 -1
@@ -120,6 +120,7 @@ struct mmc_data {s32 host_cookie;/* host private data */ }; +struct mmc_host ;struct mmc_request {struct mmc_command *sbc;/* SET_BLOCK_COUNT for multiblock */ struct mmc_command *cmd;
@@ -128,9 +129,9 @@ struct mmc_request {struct completion completion;void (*done)(struct mmc_request *);/* completion function */ +struct mmc_host *host; }; -struct mmc_host ;struct mmc_card ;struct mmc_async_req ;
include/linux/mmc/host.h +16 -0
@@ -176,6 +176,21 @@ struct mmc_supply {struct regulator *vqmmc;/* Optional Vccq supply */ }; +/** +* mmc_context_info - synchronization details for mmc context +* @is_done_rcv wake up reason was done request +* @is_new_req wake up reason was new request +* @is_waiting_last_req mmc context waiting for single running request +* @wait wait queue +*/ +struct mmc_context_info { +bool is_done_rcv; +bool is_new_req; +bool is_waiting_last_req; +wait_queue_head_t wait; +spinlock_t lock; +}; +struct mmc_host {struct device *parent;struct device class_dev;
@@ -330,6 +345,7 @@ struct mmc_host {struct dentry *debugfs_root;struct mmc_async_req *areq;/* active async req */ +struct mmc_context_info context_info;/* async synchronization info */ #ifdef CONFIG_FAIL_MMC_REQUEST struct fault_attr fail_mmc_request;
Signature · 6 lines
1.7.6 -- Konstantin Dorfman, QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
RE: [PATCH v4] mmc: fix async request mechanism for sequential read scenarios
Hi,
Doesn't it consider SD path?
It have a problem with uninitialized 'context_info'. 'context_info' is moved into mmc_host struct in v4. Please, could you explain the reason in brief?
Thanks,
Seungwon Jeon
Quoted 426 lines · Konstantin Dorfman
When current request is running on the bus and if next request fetched by mmcqd is NULL, mmc context (mmcqd thread) gets blocked until the current request completes. This means if new request comes in while the mmcqd thread is blocked, this new request can not be prepared in parallel to current ongoing request. This may result in latency to start new request.
This change allows to wake up the MMC thread (which
is waiting for the current running request to complete). Now once the MMC thread is woken up, new request can be fetched and prepared in parallel to current running request which means this new request can be started immediately after the current running request completes.With this change read throughput is improved by 16%.
Signed-off-by Konstantin Dorfman <[email protected]>---
v4: keeps new synchronization mechanism within mmc/core
layer, so mmc/core external API's are not changed.
-context_infomoved intommc_hoststruct
-context_infoinitialized inmmc_attach_mmc()v3:
- newMMC_QUEUE_NEW_REQUESTflag to mark new request case
- lock added to updateis_new_reqflag
- condition for sending new req notification changed
- Moved start waiting for new req notification after fetching NULL
v2:
- Removed synchronization flags
- removed lock fromdone()
- flags names changed
v1:
- Initial submitdrivers/mmc/card/block.c +9
-14@@ -113,17 +113,6 @@ struct mmc_blk_data {static DEFINE_MUTEX (open_lock); -enum mmc_blk_status { -MMC_BLK_SUCCESS =0 , -MMC_BLK_PARTIAL , -MMC_BLK_CMD_ERR , -MMC_BLK_RETRY , -MMC_BLK_ABORT , -MMC_BLK_DATA_ERR , -MMC_BLK_ECC_ERR , -MMC_BLK_NOMEDIUM , -}; -module_param (perdev_minors, int,0444 );MODULE_PARM_DESC (perdev_minors,"Minors numbers to allocate per device" );@@ -1363,9 +1352,12 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc) areq = &mq->mqrq_cur->mmc_active; }else areq =NULL ; - areq =mmc_start_req (card->host, areq, (int *) &status); -if (!areq) + areq =mmc_start_req (card->host, areq, (int *)&status); +if (!areq) { +if (status ==MMC_BLK_NEW_REQUEST ) + mq->flags |=MMC_QUEUE_NEW_REQUEST ;return 0 ; + } mq_rq = container_of(areq, struct mmc_queue_req, mmc_active); brq = &mq_rq->brq;@@ -1374,6 +1366,8 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)mmc_queue_bounce_post (mq_rq);switch (status) { +case MMC_BLK_NEW_REQUEST : +BUG ();/* should never get here */ case MMC_BLK_SUCCESS :case MMC_BLK_PARTIAL :/* @@ -1486,6 +1480,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req) goto out; } + mq->flags &= ~MMC_QUEUE_NEW_REQUEST ;if (req && req->cmd_flags &REQ_DISCARD ) {/* complete ongoing async transfer before issuing discard */ if (card->host->areq)@@ -1505,7 +1500,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req) } out: -if (!req) +if (!req && !(mq->flags &MMC_QUEUE_NEW_REQUEST ))/* release host only when there are no more requests */ mmc_release_host (card->host);return ret;drivers/mmc/card/queue.c +25
-2@@ -22,7 +22,6 @@#define MMC_QUEUE_BOUNCESZ 65536 -#define MMC_QUEUE_SUSPENDED (1 << 0)/* * Prepare a MMC request. This just filters out odd stuff. @@ -63,11 +62,20 @@ static int mmc_queue_thread(void *d)set_current_state (TASK_INTERRUPTIBLE ); req =blk_fetch_request (q); mq->mqrq_cur->req = req; +if (!req && mq->mqrq_prev->req && + !(mq->mqrq_prev->req->cmd_flags &REQ_FLUSH ) && + !(mq->mqrq_prev->req->cmd_flags &REQ_DISCARD )) + mq->card->host->context_info.is_waiting_last_req = true; +spin_unlock_irq (q->queue_lock);if (req || mq->mqrq_prev->req) {set_current_state (TASK_RUNNING ); mq->issue_fn (mq, req); +if (mq->flags &MMC_QUEUE_NEW_REQUEST ) { + mq->flags &= ~MMC_QUEUE_NEW_REQUEST ; +continue ;/* fetch again */ + }/* * Current request becomes previous request @@ -103,6 +111,8 @@ static void mmc_request_fn(struct request_queue *q) {struct mmc_queue *mq = q->queuedata;struct request *req; +unsigned long flags; +struct mmc_context_info *cntx;if (!mq) {while ((req =blk_fetch_request (q)) !=NULL ) {@@ -112,7 +122,20 @@ static void mmc_request_fn(struct request_queue *q)return ; } -if (!mq->mqrq_cur->req && !mq->mqrq_prev->req) + cntx = &mq->card->host->context_info; +if (!mq->mqrq_cur->req && mq->mqrq_prev->req) { +/* +* New MMC request arrived when MMC thread may be +* blocked on the previous request to be complete +* with no current request fetched +*/ +spin_lock_irqsave (&cntx->lock, flags); +if (cntx->is_waiting_last_req) { + cntx->is_new_req = true; +wake_up_interruptible (&cntx->wait); + } +spin_unlock_irqrestore (&cntx->lock, flags); + }else if (!mq->mqrq_cur->req && !mq->mqrq_prev->req)wake_up_process (mq->thread); }drivers/mmc/card/queue.h +2
-0@@ -26,6 +26,8 @@ struct mmc_queue {struct mmc_card *card;struct task_struct *thread;struct semaphore thread_sem; +#define MMC_QUEUE_SUSPENDED (1 << 0) +#define MMC_QUEUE_NEW_REQUEST (1 << 1)unsigned int flags;int (*issue_fn)(struct mmc_queue *,struct request *);void *data;drivers/mmc/core/core.c +103
-3@@ -319,11 +319,44 @@ out: }EXPORT_SYMBOL (mmc_start_bkops); +/* +* mmc_wait_data_done() - done callback for data request +* @mrq: done data request +* +* Wakes up mmc context, passed as callback to host controller driver +*/ +static void mmc_wait_data_done (struct mmc_request *mrq) +{ + mrq->host->context_info.is_done_rcv = true; +wake_up_interruptible (&mrq->host->context_info.wait); +} +static void mmc_wait_done (struct mmc_request *mrq) {complete (&mrq->completion); } +/* +*__mmc_start_data_req() - starts data request +* @host: MMC host to start the request +* @mrq: data request to start +* +* Fills done callback that will be used when request are done by card. +* Starts data mmc request execution +*/ +static int __mmc_start_data_req (struct mmc_host *host,struct mmc_request *mrq) +{ + mrq->done = mmc_wait_data_done; + mrq->host = host; +if (mmc_card_removed (host->card)) { + mrq->cmd->error = -ENOMEDIUM ; +return -ENOMEDIUM ; + } +mmc_start_request (host, mrq); + +return 0 ; +} +static int __mmc_start_req (struct mmc_host *host,struct mmc_request *mrq) {init_completion (&mrq->completion);@@ -337,6 +370,60 @@ static int __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)return 0 ; } +/* +* mmc_wait_for_data_req_done() - wait for request completed or new +* request notification arrives +* @host: MMC host to prepare the command. +* @mrq: MMC request to wait for +* +* Blocks MMC context till host controller will ack end of data request +* execution or new request arrives from block layer. Handles +* command retries. +* +* Returns enum mmc_blk_status after checking errors. +*/ +static int mmc_wait_for_data_req_done (struct mmc_host *host, +struct mmc_request *mrq) +{ +struct mmc_command *cmd; +struct mmc_context_info *context_info = &host->context_info; +int err; +unsigned long flags; + +while (1 ) { +wait_event_interruptible (context_info->wait, + (context_info->is_done_rcv || + context_info->is_new_req)); +spin_lock_irqsave (&context_info->lock, flags); + context_info->is_waiting_last_req = false; +spin_unlock_irqrestore (&context_info->lock, flags); +if (context_info->is_done_rcv) { + context_info->is_done_rcv = false; + context_info->is_new_req = false; + cmd = mrq->cmd; +if (!cmd->error || !cmd->retries || +mmc_card_removed (host->card)) { + err = host->areq->err_check (host->card, + host->areq); +break ;/* return err */ + }else { +pr_info ("%s: req failed (CMD%u): %d, retrying...\n" , +mmc_hostname (host), + cmd->opcode, cmd->error); + cmd->retries--; + cmd->error =0 ; + host->ops->request (host, mrq); +continue ;/* wait for done/new event again */ + } + }else if (context_info->is_new_req) { + context_info->is_new_req = false; + err =MMC_BLK_NEW_REQUEST ; +break ;/* return err */ + } + }/* while */ +return err; +} +static void mmc_wait_for_req_done (struct mmc_host *host,struct mmc_request *mrq) {@@ -426,8 +513,21 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,mmc_pre_req (host, areq->mrq, !host->areq);if (host->areq) { -mmc_wait_for_req_done (host, host->areq->mrq); - err = host->areq->err_check (host->card, host->areq); + err =mmc_wait_for_data_req_done (host, host->areq->mrq); +if (err ==MMC_BLK_NEW_REQUEST ) { +if (areq) { +pr_err ("%s: new request while areq = %p" , +mmc_hostname (host), areq); +BUG_ON (1 ); + } +if (error) + *error = err; +/* +* The previous request was not completed, +* nothing to return +*/ +return NULL ; + }/* * Check BKOPS urgency for each R1 response */ @@ -439,7 +539,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host, }if (!err && areq) - start_err =__mmc_start_req (host, areq->mrq); + start_err =__mmc_start_data_req (host, areq->mrq);if (host->areq)mmc_post_req (host, host->areq->mrq,0 );drivers/mmc/core/mmc.c +6
-0@@ -1497,6 +1497,12 @@ int mmc_attach_mmc(struct mmc_host *host)BUG_ON (!host);WARN_ON (!host->claimed); +spin_lock_init (&host->context_info.lock); + host->context_info.is_new_req = false; + host->context_info.is_done_rcv = false; + host->context_info.is_waiting_last_req = false; +init_waitqueue_head (&host->context_info.wait); +/* Set correct bus mode for MMC before attempting attach */ if (!mmc_host_is_spi (host))mmc_set_bus_mode (host,MMC_BUSMODE_OPENDRAIN );include/linux/mmc/card.h +12
-0@@ -187,6 +187,18 @@ struct sdio_func_tuple;#define SDIO_MAX_FUNCS 7 +enum mmc_blk_status { +MMC_BLK_SUCCESS =0 , +MMC_BLK_PARTIAL , +MMC_BLK_CMD_ERR , +MMC_BLK_RETRY , +MMC_BLK_ABORT , +MMC_BLK_DATA_ERR , +MMC_BLK_ECC_ERR , +MMC_BLK_NOMEDIUM , +MMC_BLK_NEW_REQUEST , +}; +/* The number of MMC physical partitions. These consist of: * boot partitions (2), general purpose partitions (4) in MMC v4.4. */ include/linux/mmc/core.h +2
-1@@ -120,6 +120,7 @@ struct mmc_data {s32 host_cookie;/* host private data */ }; +struct mmc_host ;struct mmc_request {struct mmc_command *sbc;/* SET_BLOCK_COUNT for multiblock */ struct mmc_command *cmd;@@ -128,9 +129,9 @@ struct mmc_request {struct completion completion;void (*done)(struct mmc_request *);/* completion function */ +struct mmc_host *host; }; -struct mmc_host ;struct mmc_card ;struct mmc_async_req ;include/linux/mmc/host.h +16
-1@@ -176,6 +176,21 @@ struct mmc_supply {struct regulator *vqmmc;/* Optional Vccq supply */ }; +/** +* mmc_context_info - synchronization details for mmc context +* @is_done_rcv wake up reason was done request +* @is_new_req wake up reason was new request +* @is_waiting_last_req mmc context waiting for single running request +* @wait wait queue +*/ +struct mmc_context_info { +bool is_done_rcv; +bool is_new_req; +bool is_waiting_last_req; +wait_queue_head_t wait; +spinlock_t lock; +}; +struct mmc_host {struct device *parent;struct device class_dev;@@ -330,6 +345,7 @@ struct mmc_host {struct dentry *debugfs_root;struct mmc_async_req *areq;/* active async req */ +struct mmc_context_info context_info;/* async synchronization info */ #ifdef CONFIG_FAIL_MMC_REQUEST struct fault_attr fail_mmc_request; --1.7.6
--
Konstantin Dorfman,
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-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] mmc: fix async request mechanism for sequential read scenarios
Quoted 7 lines · Seungwon Jeon
Hi,
Doesn't it consider SD path?
It have a problem with uninitialized 'context_info'. 'context_info' is moved intommc_hoststruct in v4. Please, could you explain the reason in brief?
The problem I'm trying to resolve with the v4 change is: there should be no core/core.c API change from upper layers point of view.
There was such violation, when the synchronization mechanism of async request for data commands was changed (from "completion" to "wait_event"
+ number of flags to pass event reason).
Before v4, the 'context_info' initialized by mmc_init_queue() function and this is valid only for mmcblk. For example when running the mmc_test module, the code for card/queue.c, card/block.c layers is not run at all and the struct remains uninitialized.
In the current version the initialization was moved into mmc_attach_mmc(), but now, I understand, that this initialization should be moved even before mmc_attach_XXX() (XXX is sdio/sd/mmc...) - it may be placed in mmc_rescan_try_freq().
Will resend v4 after the testing.
'mmc_host' structure contains a host information for all sdio/sd/mmc, so 'context_info' may be accessed by any protocol willing to use NEW_REQUEST notification (when async request mechanism is used).
Signature · 4 lines
Konstantin Dorfman, QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH RESEND v4 1/1 mmc: fix async request mechanism for sequential read scenarios
When current request is running on the bus and if next request fetched by mmcqd is NULL, mmc context (mmcqd thread) gets blocked until the current request completes. This means if new request comes in while the mmcqd thread is blocked, this new request can not be prepared in parallel to current ongoing request. This may result in latency to start new request.
This change allows to wake up the MMC thread (which
is waiting for the current running request to complete). Now once the MMC thread is woken up, new request can be fetched and prepared in parallel to current running request which means this new request can be started immediately after the current running request completes.
With this change read throughput is improved by 16%.
---
v4:
keeps new synchronization mechanism within mmc/core layer, so mmc/core external API's are not changed.
- context_info moved into mmc_host struct
- context_info initialized in mmc_rescan_try_freq()
v3:
- new MMC_QUEUE_NEW_REQUEST flag to mark new request case
- lock added to update is_new_req flag
- condition for sending new req notification changed
- Moved start waiting for new req notification after fetching NULL
v2:
- Removed synchronization flags
- removed lock from done()
- flags names changed
v1:
- Initial submit
drivers/mmc/card/block.c | 25 +++++------ drivers/mmc/card/queue.c | 27 ++++++++++- drivers/mmc/card/queue.h | 2 + drivers/mmc/core/core.c | 112 ++++++++++++++++++++++++++++++++++++++++++++- include/linux/mmc/card.h | 12 +++++ include/linux/mmc/core.h | 3 +- include/linux/mmc/host.h | 16 +++++++ 7 files changed, 177 insertions(+), 20 deletions(-)
drivers/mmc/card/block.c +11 -14
@@ -113,17 +113,6 @@ struct mmc_blk_data {static DEFINE_MUTEX (open_lock); -enum mmc_blk_status { -MMC_BLK_SUCCESS =0 , -MMC_BLK_PARTIAL , -MMC_BLK_CMD_ERR , -MMC_BLK_RETRY , -MMC_BLK_ABORT , -MMC_BLK_DATA_ERR , -MMC_BLK_ECC_ERR , -MMC_BLK_NOMEDIUM , -}; -module_param (perdev_minors, int,0444 );MODULE_PARM_DESC (perdev_minors,"Minors numbers to allocate per device" );
@@ -1180,6 +1169,7 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,memset (brq,0 ,sizeof (struct mmc_blk_request )); brq->mrq.cmd = &brq->cmd; brq->mrq.data = &brq->data; + brq->mrq.host = card->host; brq->cmd.arg =blk_rq_pos (req);if (!mmc_card_blockaddr (card))
@@ -1363,9 +1353,12 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc) areq = &mq->mqrq_cur->mmc_active; }else areq =NULL ; - areq =mmc_start_req (card->host, areq, (int *) &status); -if (!areq) + areq =mmc_start_req (card->host, areq, (int *)&status); +if (!areq) { +if (status ==MMC_BLK_NEW_REQUEST ) + mq->flags |=MMC_QUEUE_NEW_REQUEST ;return 0 ; + } mq_rq = container_of(areq, struct mmc_queue_req, mmc_active); brq = &mq_rq->brq;
@@ -1374,6 +1367,8 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)mmc_queue_bounce_post (mq_rq);switch (status) { +case MMC_BLK_NEW_REQUEST : +BUG ();/* should never get here */ case MMC_BLK_SUCCESS :case MMC_BLK_PARTIAL :/*
@@ -1486,6 +1481,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req) goto out; } + mq->flags &= ~MMC_QUEUE_NEW_REQUEST ;if (req && req->cmd_flags &REQ_DISCARD ) {/* complete ongoing async transfer before issuing discard */ if (card->host->areq)
@@ -1505,9 +1501,10 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req) } out: -if (!req) +if (!req && !(mq->flags &MMC_QUEUE_NEW_REQUEST ))/* release host only when there are no more requests */ mmc_release_host (card->host); +return ret; }
drivers/mmc/card/queue.c +25 -2
@@ -22,7 +22,6 @@#define MMC_QUEUE_BOUNCESZ 65536 -#define MMC_QUEUE_SUSPENDED (1 << 0)/* * Prepare a MMC request. This just filters out odd stuff.
@@ -63,11 +62,20 @@ static int mmc_queue_thread(void *d)set_current_state (TASK_INTERRUPTIBLE ); req =blk_fetch_request (q); mq->mqrq_cur->req = req; +if (!req && mq->mqrq_prev->req && + !(mq->mqrq_prev->req->cmd_flags &REQ_FLUSH ) && + !(mq->mqrq_prev->req->cmd_flags &REQ_DISCARD )) + mq->card->host->context_info.is_waiting_last_req = true; +spin_unlock_irq (q->queue_lock);if (req || mq->mqrq_prev->req) {set_current_state (TASK_RUNNING ); mq->issue_fn (mq, req); +if (mq->flags &MMC_QUEUE_NEW_REQUEST ) { + mq->flags &= ~MMC_QUEUE_NEW_REQUEST ; +continue ;/* fetch again */ + }/* * Current request becomes previous request
@@ -103,6 +111,8 @@ static void mmc_request_fn(struct request_queue *q) {struct mmc_queue *mq = q->queuedata;struct request *req; +unsigned long flags; +struct mmc_context_info *cntx;if (!mq) {while ((req =blk_fetch_request (q)) !=NULL ) {
@@ -112,7 +122,20 @@ static void mmc_request_fn(struct request_queue *q)return ; } -if (!mq->mqrq_cur->req && !mq->mqrq_prev->req) + cntx = &mq->card->host->context_info; +if (!mq->mqrq_cur->req && mq->mqrq_prev->req) { +/* +* New MMC request arrived when MMC thread may be +* blocked on the previous request to be complete +* with no current request fetched +*/ +spin_lock_irqsave (&cntx->lock, flags); +if (cntx->is_waiting_last_req) { + cntx->is_new_req = true; +wake_up_interruptible (&cntx->wait); + } +spin_unlock_irqrestore (&cntx->lock, flags); + }else if (!mq->mqrq_cur->req && !mq->mqrq_prev->req)wake_up_process (mq->thread); }
drivers/mmc/card/queue.h +2 -0
@@ -26,6 +26,8 @@ struct mmc_queue {struct mmc_card *card;struct task_struct *thread;struct semaphore thread_sem; +#define MMC_QUEUE_SUSPENDED (1 << 0) +#define MMC_QUEUE_NEW_REQUEST (1 << 1)unsigned int flags;int (*issue_fn)(struct mmc_queue *,struct request *);void *data;
drivers/mmc/core/core.c +109 -3
153 lines not on this page · raw message
include/linux/mmc/card.h +12 -0
@@ -187,6 +187,18 @@ struct sdio_func_tuple;#define SDIO_MAX_FUNCS 7 +enum mmc_blk_status { +MMC_BLK_SUCCESS =0 , +MMC_BLK_PARTIAL , +MMC_BLK_CMD_ERR , +MMC_BLK_RETRY , +MMC_BLK_ABORT , +MMC_BLK_DATA_ERR , +MMC_BLK_ECC_ERR , +MMC_BLK_NOMEDIUM , +MMC_BLK_NEW_REQUEST , +}; +/* The number of MMC physical partitions. These consist of: * boot partitions (2), general purpose partitions (4) in MMC v4.4. */
include/linux/mmc/core.h +2 -1
@@ -120,6 +120,7 @@ struct mmc_data {s32 host_cookie;/* host private data */ }; +struct mmc_host ;struct mmc_request {struct mmc_command *sbc;/* SET_BLOCK_COUNT for multiblock */ struct mmc_command *cmd;
@@ -128,9 +129,9 @@ struct mmc_request {struct completion completion;void (*done)(struct mmc_request *);/* completion function */ +struct mmc_host *host; }; -struct mmc_host ;struct mmc_card ;struct mmc_async_req ;
include/linux/mmc/host.h +16 -0
@@ -177,6 +177,21 @@ struct mmc_supply {struct regulator *vqmmc;/* Optional Vccq supply */ }; +/** +* mmc_context_info - synchronization details for mmc context +* @is_done_rcv wake up reason was done request +* @is_new_req wake up reason was new request +* @is_waiting_last_req mmc context waiting for single running request +* @wait wait queue +*/ +struct mmc_context_info { +bool is_done_rcv; +bool is_new_req; +bool is_waiting_last_req; +wait_queue_head_t wait; +spinlock_t lock; +}; +struct mmc_host {struct device *parent;struct device class_dev;
@@ -331,6 +346,7 @@ struct mmc_host {struct dentry *debugfs_root;struct mmc_async_req *areq;/* active async req */ +struct mmc_context_info context_info;/* async synchronization info */ #ifdef CONFIG_FAIL_MMC_REQUEST struct fault_attr fail_mmc_request;
Signature · 6 lines
1.7.6 -- Konstantin Dorfman, QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
RE: [RESEND PATCH v4] mmc: fix async request mechanism for sequential read scenarios
Quoted 69 lines · Konstantin Dorfman
When current request is running on the bus and if next request fetched by mmcqd is NULL, mmc context (mmcqd thread) gets blocked until the current request completes. This means if new request comes in while the mmcqd thread is blocked, this new request can not be prepared in parallel to current ongoing request. This may result in latency to start new request.
This change allows to wake up the MMC thread (which
is waiting for the current running request to complete). Now once the MMC thread is woken up, new request can be fetched and prepared in parallel to current running request which means this new request can be started immediately after the current running request completes.With this change read throughput is improved by 16%.
Signed-off-by Konstantin Dorfman <[email protected]>---
v4:
keeps new synchronization mechanism within mmc/core layer, so mmc/core external API's are not changed.
-context_infomoved intommc_hoststruct
-context_infoinitialized inmmc_rescan_try_freq()v3:
- newMMC_QUEUE_NEW_REQUESTflag to mark new request case
- lock added to updateis_new_reqflag
- condition for sending new req notification changed
- Moved start waiting for new req notification after fetching NULL
v2:
- Removed synchronization flags
- removed lock fromdone()
- flags names changed
v1:
- Initial submitdrivers/mmc/card/block.c | 25 +++++------ drivers/mmc/card/queue.c | 27 ++++++++++- drivers/mmc/card/queue.h | 2 + drivers/mmc/core/core.c | 112 ++++++++++++++++++++++++++++++++++++++++++++- include/linux/mmc/card.h | 12 +++++ include/linux/mmc/core.h | 3 +- include/linux/mmc/host.h | 16 +++++++ 7 files changed, 177 insertions(+), 20 deletions(-)drivers/mmc/card/block.c +0
-11@@ -113,17 +113,6 @@ struct mmc_blk_data {static DEFINE_MUTEX (open_lock); -enum mmc_blk_status { -MMC_BLK_SUCCESS =0 , -MMC_BLK_PARTIAL , -MMC_BLK_CMD_ERR , -MMC_BLK_RETRY , -MMC_BLK_ABORT , -MMC_BLK_DATA_ERR , -MMC_BLK_ECC_ERR , -MMC_BLK_NOMEDIUM , -}; -module_param (perdev_minors, int,0444 );MODULE_PARM_DESC (perdev_minors,"Minors numbers to allocate per device" );
drivers/mmc/card/block.c static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq, · line 1172
Quoted 4 lines
drivers/mmc/card/block.c +1
-0@@ -1180,6 +1169,7 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,memset (brq,0 ,sizeof (struct mmc_blk_request )); brq->mrq.cmd = &brq->cmd; brq->mrq.data = &brq->data; + brq->mrq.host = card->host;
Above setting have been added at __mmc_start_data_req function in previous sending. Currently there is no setting for sdio. Considering sdio case, the former would be better.
Quoted 275 lines
drivers/mmc/card/block.c +10
-3brq->cmd.arg =blk_rq_pos (req);if (!mmc_card_blockaddr (card))@@ -1363,9 +1353,12 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc) areq = &mq->mqrq_cur->mmc_active; }else areq =NULL ; - areq =mmc_start_req (card->host, areq, (int *) &status); -if (!areq) + areq =mmc_start_req (card->host, areq, (int *)&status); +if (!areq) { +if (status ==MMC_BLK_NEW_REQUEST ) + mq->flags |=MMC_QUEUE_NEW_REQUEST ;return 0 ; + } mq_rq = container_of(areq, struct mmc_queue_req, mmc_active); brq = &mq_rq->brq;@@ -1374,6 +1367,8 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)mmc_queue_bounce_post (mq_rq);switch (status) { +case MMC_BLK_NEW_REQUEST : +BUG ();/* should never get here */ case MMC_BLK_SUCCESS :case MMC_BLK_PARTIAL :/* @@ -1486,6 +1481,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req) goto out; } + mq->flags &= ~MMC_QUEUE_NEW_REQUEST ;if (req && req->cmd_flags &REQ_DISCARD ) {/* complete ongoing async transfer before issuing discard */ if (card->host->areq)@@ -1505,9 +1501,10 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req) } out: -if (!req) +if (!req && !(mq->flags &MMC_QUEUE_NEW_REQUEST ))/* release host only when there are no more requests */ mmc_release_host (card->host); +return ret; }drivers/mmc/card/queue.c +25
-2@@ -22,7 +22,6 @@#define MMC_QUEUE_BOUNCESZ 65536 -#define MMC_QUEUE_SUSPENDED (1 << 0)/* * Prepare a MMC request. This just filters out odd stuff. @@ -63,11 +62,20 @@ static int mmc_queue_thread(void *d)set_current_state (TASK_INTERRUPTIBLE ); req =blk_fetch_request (q); mq->mqrq_cur->req = req; +if (!req && mq->mqrq_prev->req && + !(mq->mqrq_prev->req->cmd_flags &REQ_FLUSH ) && + !(mq->mqrq_prev->req->cmd_flags &REQ_DISCARD )) + mq->card->host->context_info.is_waiting_last_req = true; +spin_unlock_irq (q->queue_lock);if (req || mq->mqrq_prev->req) {set_current_state (TASK_RUNNING ); mq->issue_fn (mq, req); +if (mq->flags &MMC_QUEUE_NEW_REQUEST ) { + mq->flags &= ~MMC_QUEUE_NEW_REQUEST ; +continue ;/* fetch again */ + }/* * Current request becomes previous request @@ -103,6 +111,8 @@ static void mmc_request_fn(struct request_queue *q) {struct mmc_queue *mq = q->queuedata;struct request *req; +unsigned long flags; +struct mmc_context_info *cntx;if (!mq) {while ((req =blk_fetch_request (q)) !=NULL ) {@@ -112,7 +122,20 @@ static void mmc_request_fn(struct request_queue *q)return ; } -if (!mq->mqrq_cur->req && !mq->mqrq_prev->req) + cntx = &mq->card->host->context_info; +if (!mq->mqrq_cur->req && mq->mqrq_prev->req) { +/* +* New MMC request arrived when MMC thread may be +* blocked on the previous request to be complete +* with no current request fetched +*/ +spin_lock_irqsave (&cntx->lock, flags); +if (cntx->is_waiting_last_req) { + cntx->is_new_req = true; +wake_up_interruptible (&cntx->wait); + } +spin_unlock_irqrestore (&cntx->lock, flags); + }else if (!mq->mqrq_cur->req && !mq->mqrq_prev->req)wake_up_process (mq->thread); }drivers/mmc/card/queue.h +2
-0@@ -26,6 +26,8 @@ struct mmc_queue {struct mmc_card *card;struct task_struct *thread;struct semaphore thread_sem; +#define MMC_QUEUE_SUSPENDED (1 << 0) +#define MMC_QUEUE_NEW_REQUEST (1 << 1)unsigned int flags;int (*issue_fn)(struct mmc_queue *,struct request *);void *data;drivers/mmc/core/core.c +103
-3@@ -319,11 +319,43 @@ out: }EXPORT_SYMBOL (mmc_start_bkops); +/* +* mmc_wait_data_done() - done callback for data request +* @mrq: done data request +* +* Wakes up mmc context, passed as callback to host controller driver +*/ +static void mmc_wait_data_done (struct mmc_request *mrq) +{ + mrq->host->context_info.is_done_rcv = true; +wake_up_interruptible (&mrq->host->context_info.wait); +} +static void mmc_wait_done (struct mmc_request *mrq) {complete (&mrq->completion); } +/* +*__mmc_start_data_req() - starts data request +* @host: MMC host to start the request +* @mrq: data request to start +* +* Fills done callback that will be used when request are done by card. +* Starts data mmc request execution +*/ +static int __mmc_start_data_req (struct mmc_host *host,struct mmc_request *mrq) +{ + mrq->done = mmc_wait_data_done; +if (mmc_card_removed (host->card)) { + mrq->cmd->error = -ENOMEDIUM ; +return -ENOMEDIUM ; + } +mmc_start_request (host, mrq); + +return 0 ; +} +static int __mmc_start_req (struct mmc_host *host,struct mmc_request *mrq) {init_completion (&mrq->completion);@@ -337,6 +369,60 @@ static int __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)return 0 ; } +/* +* mmc_wait_for_data_req_done() - wait for request completed or new +* request notification arrives +* @host: MMC host to prepare the command. +* @mrq: MMC request to wait for +* +* Blocks MMC context till host controller will ack end of data request +* execution or new request arrives from block layer. Handles +* command retries. +* +* Returns enum mmc_blk_status after checking errors. +*/ +static int mmc_wait_for_data_req_done (struct mmc_host *host, +struct mmc_request *mrq) +{ +struct mmc_command *cmd; +struct mmc_context_info *context_info = &host->context_info; +int err; +unsigned long flags; + +while (1 ) { +wait_event_interruptible (context_info->wait, + (context_info->is_done_rcv || + context_info->is_new_req)); +spin_lock_irqsave (&context_info->lock, flags); + context_info->is_waiting_last_req = false; +spin_unlock_irqrestore (&context_info->lock, flags); +if (context_info->is_done_rcv) { + context_info->is_done_rcv = false; + context_info->is_new_req = false; + cmd = mrq->cmd; +if (!cmd->error || !cmd->retries || +mmc_card_removed (host->card)) { + err = host->areq->err_check (host->card, + host->areq); +break ;/* return err */ + }else { +pr_info ("%s: req failed (CMD%u): %d, retrying...\n" , +mmc_hostname (host), + cmd->opcode, cmd->error); + cmd->retries--; + cmd->error =0 ; + host->ops->request (host, mrq); +continue ;/* wait for done/new event again */ + } + }else if (context_info->is_new_req) { + context_info->is_new_req = false; + err =MMC_BLK_NEW_REQUEST ; +break ;/* return err */ + } + }/* while */ +return err; +} +static void mmc_wait_for_req_done (struct mmc_host *host,struct mmc_request *mrq) {@@ -426,8 +512,21 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,mmc_pre_req (host, areq->mrq, !host->areq);if (host->areq) { -mmc_wait_for_req_done (host, host->areq->mrq); - err = host->areq->err_check (host->card, host->areq); + err =mmc_wait_for_data_req_done (host, host->areq->mrq); +if (err ==MMC_BLK_NEW_REQUEST ) { +if (areq) { +pr_err ("%s: new request while areq = %p" , +mmc_hostname (host), areq); +BUG_ON (1 ); + } +if (error) + *error = err; +/* +* The previous request was not completed, +* nothing to return +*/ +return NULL ; + }/* * Check BKOPS urgency for each R1 response */ @@ -439,7 +538,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host, }if (!err && areq) - start_err =__mmc_start_req (host, areq->mrq); + start_err =__mmc_start_data_req (host, areq->mrq);if (host->areq)mmc_post_req (host, host->areq->mrq,0 );@@ -455,6 +554,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,if (error) *error = err; +return data; }EXPORT_SYMBOL (mmc_start_req);
drivers/mmc/core/core.c static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq) · line 2194
Quoted 9 lines
drivers/mmc/core/core.c +6
-0@@ -2086,6 +2186,12 @@ static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)mmc_send_if_cond (host, host->ocr_avail); +spin_lock_init (&host->context_info.lock); + host->context_info.is_new_req = false; + host->context_info.is_done_rcv = false; + host->context_info.is_waiting_last_req = false; +init_waitqueue_head (&host->context_info.wait); +
This path may be retired when mmc_rescan_try_freq is failed. How about putting these in mmc_add_card(mmc/core/bus.c) <Quote>
ret = device_add(&card->dev);
if (ret)
return ret;-> Here seems good point.
mmc_card_set_present(card);
</Quote>
Thanks,
Seungwon Jeon
Quoted 94 lines
drivers/mmc/core/core.c +0
-0/* Order's important: probe SDIO, then SD, then MMC */ if (!mmc_attach_sdio (host))return 0 ;include/linux/mmc/card.h +12
-0@@ -187,6 +187,18 @@ struct sdio_func_tuple;#define SDIO_MAX_FUNCS 7 +enum mmc_blk_status { +MMC_BLK_SUCCESS =0 , +MMC_BLK_PARTIAL , +MMC_BLK_CMD_ERR , +MMC_BLK_RETRY , +MMC_BLK_ABORT , +MMC_BLK_DATA_ERR , +MMC_BLK_ECC_ERR , +MMC_BLK_NOMEDIUM , +MMC_BLK_NEW_REQUEST , +}; +/* The number of MMC physical partitions. These consist of: * boot partitions (2), general purpose partitions (4) in MMC v4.4. */ include/linux/mmc/core.h +2
-1@@ -120,6 +120,7 @@ struct mmc_data {s32 host_cookie;/* host private data */ }; +struct mmc_host ;struct mmc_request {struct mmc_command *sbc;/* SET_BLOCK_COUNT for multiblock */ struct mmc_command *cmd;@@ -128,9 +129,9 @@ struct mmc_request {struct completion completion;void (*done)(struct mmc_request *);/* completion function */ +struct mmc_host *host; }; -struct mmc_host ;struct mmc_card ;struct mmc_async_req ;include/linux/mmc/host.h +16
-1@@ -177,6 +177,21 @@ struct mmc_supply {struct regulator *vqmmc;/* Optional Vccq supply */ }; +/** +* mmc_context_info - synchronization details for mmc context +* @is_done_rcv wake up reason was done request +* @is_new_req wake up reason was new request +* @is_waiting_last_req mmc context waiting for single running request +* @wait wait queue +*/ +struct mmc_context_info { +bool is_done_rcv; +bool is_new_req; +bool is_waiting_last_req; +wait_queue_head_t wait; +spinlock_t lock; +}; +struct mmc_host {struct device *parent;struct device class_dev;@@ -331,6 +346,7 @@ struct mmc_host {struct dentry *debugfs_root;struct mmc_async_req *areq;/* active async req */ +struct mmc_context_info context_info;/* async synchronization info */ #ifdef CONFIG_FAIL_MMC_REQUEST struct fault_attr fail_mmc_request; --1.7.6
--
Konstantin Dorfman,
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-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RESEND PATCH v4] mmc: fix async request mechanism for sequential read scenarios
In reply to Konstantin DorfmanHi, Konstantin.
I added comments more below.
Quoted 93 lines · Seungwon Jeon
Quoted 69 lines · Konstantin Dorfman
When current request is running on the bus and if next request fetched by mmcqd is NULL, mmc context (mmcqd thread) gets blocked until the current request completes. This means if new request comes in while the mmcqd thread is blocked, this new request can not be prepared in parallel to current ongoing request. This may result in latency to start new request.
This change allows to wake up the MMC thread (which
is waiting for the current running request to complete). Now once the MMC thread is woken up, new request can be fetched and prepared in parallel to current running request which means this new request can be started immediately after the current running request completes.With this change read throughput is improved by 16%.
Signed-off-by Konstantin Dorfman <[email protected]>---
v4:
keeps new synchronization mechanism within mmc/core layer, so mmc/core external API's are not changed.
-context_infomoved intommc_hoststruct
-context_infoinitialized inmmc_rescan_try_freq()v3:
- newMMC_QUEUE_NEW_REQUESTflag to mark new request case
- lock added to updateis_new_reqflag
- condition for sending new req notification changed
- Moved start waiting for new req notification after fetching NULL
v2:
- Removed synchronization flags
- removed lock fromdone()
- flags names changed
v1:
- Initial submitdrivers/mmc/card/block.c | 25 +++++------ drivers/mmc/card/queue.c | 27 ++++++++++- drivers/mmc/card/queue.h | 2 + drivers/mmc/core/core.c | 112 ++++++++++++++++++++++++++++++++++++++++++++- include/linux/mmc/card.h | 12 +++++ include/linux/mmc/core.h | 3 +- include/linux/mmc/host.h | 16 +++++++ 7 files changed, 177 insertions(+), 20 deletions(-)drivers/mmc/card/block.c +0
-11@@ -113,17 +113,6 @@ struct mmc_blk_data {static DEFINE_MUTEX (open_lock); -enum mmc_blk_status { -MMC_BLK_SUCCESS =0 , -MMC_BLK_PARTIAL , -MMC_BLK_CMD_ERR , -MMC_BLK_RETRY , -MMC_BLK_ABORT , -MMC_BLK_DATA_ERR , -MMC_BLK_ECC_ERR , -MMC_BLK_NOMEDIUM , -}; -module_param (perdev_minors, int,0444 );MODULE_PARM_DESC (perdev_minors,"Minors numbers to allocate per device" );drivers/mmc/card/block.c static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq, · line 1172
Quoted 4 lines
drivers/mmc/card/block.c +1
-0@@ -1180,6 +1169,7 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,memset (brq,0 ,sizeof (struct mmc_blk_request )); brq->mrq.cmd = &brq->cmd; brq->mrq.data = &brq->data; + brq->mrq.host = card->host;Above setting have been added at
__mmc_start_data_reqfunction in previous sending. Currently there is no setting for sdio. Considering sdio case, the former would be better.Quoted 16 lines
drivers/mmc/card/block.c +5
-2brq->cmd.arg =blk_rq_pos (req);if (!mmc_card_blockaddr (card))@@ -1363,9 +1353,12 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc) areq = &mq->mqrq_cur->mmc_active; }else areq =NULL ; - areq =mmc_start_req (card->host, areq, (int *) &status); -if (!areq) + areq =mmc_start_req (card->host, areq, (int *)&status); +if (!areq) { +if (status ==MMC_BLK_NEW_REQUEST ) + mq->flags |=MMC_QUEUE_NEW_REQUEST ;return 0 ; + }
Quoted 8 lines
drivers/mmc/card/block.c static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc) · line 1371
Quoted 8 lines
drivers/mmc/card/block.c +2
-0mq_rq = container_of(areq, struct mmc_queue_req, mmc_active); brq = &mq_rq->brq;@@ -1374,6 +1367,8 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)mmc_queue_bounce_post (mq_rq);switch (status) { +case MMC_BLK_NEW_REQUEST : +BUG ();/* should never get here */
Is there any case to reach here?
I didn't find it.
Quoted 33 lines
Quoted 33 lines
drivers/mmc/card/block.c +3
-1case MMC_BLK_SUCCESS :case MMC_BLK_PARTIAL :/* @@ -1486,6 +1481,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req) goto out; } + mq->flags &= ~MMC_QUEUE_NEW_REQUEST ;if (req && req->cmd_flags &REQ_DISCARD ) {/* complete ongoing async transfer before issuing discard */ if (card->host->areq)@@ -1505,9 +1501,10 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req) } out: -if (!req) +if (!req && !(mq->flags &MMC_QUEUE_NEW_REQUEST ))/* release host only when there are no more requests */ mmc_release_host (card->host); +return ret; }drivers/mmc/card/queue.c +0
-1@@ -22,7 +22,6 @@#define MMC_QUEUE_BOUNCESZ 65536 -#define MMC_QUEUE_SUSPENDED (1 << 0)
Quoted 11 lines
drivers/mmc/card/queue.c static int mmc_queue_thread(void *d) · line 69
Quoted 11 lines
drivers/mmc/card/queue.c +5
-0/* * Prepare a MMC request. This just filters out odd stuff. @@ -63,11 +62,20 @@ static int mmc_queue_thread(void *d)set_current_state (TASK_INTERRUPTIBLE ); req =blk_fetch_request (q); mq->mqrq_cur->req = req; +if (!req && mq->mqrq_prev->req && + !(mq->mqrq_prev->req->cmd_flags &REQ_FLUSH ) && + !(mq->mqrq_prev->req->cmd_flags &REQ_DISCARD )) + mq->card->host->context_info.is_waiting_last_req = true; +
Unlike normal R/W request, request for discard/flush will be finished synchronously.
That means blk_end_request is called with 1's cycle of 'issue_fn' and request will be freed in block layer. Currently 'mqrq_prev->req' is keeping these request and is used above condition.
Maybe, same area may be reallocated for normal R/W request after discard/flush is finished. But we still expect discard or flush is saved in 'mq->mqrq_prev->req'.
I wonder that 'mq->mqrq_prev->req' indicates the valid area in all case.
Thanks,
Seungwon Jeon
Quoted 323 lines
Quoted 207 lines
drivers/mmc/card/queue.c +20
-1spin_unlock_irq (q->queue_lock);if (req || mq->mqrq_prev->req) {set_current_state (TASK_RUNNING ); mq->issue_fn (mq, req); +if (mq->flags &MMC_QUEUE_NEW_REQUEST ) { + mq->flags &= ~MMC_QUEUE_NEW_REQUEST ; +continue ;/* fetch again */ + }/* * Current request becomes previous request @@ -103,6 +111,8 @@ static void mmc_request_fn(struct request_queue *q) {struct mmc_queue *mq = q->queuedata;struct request *req; +unsigned long flags; +struct mmc_context_info *cntx;if (!mq) {while ((req =blk_fetch_request (q)) !=NULL ) {@@ -112,7 +122,20 @@ static void mmc_request_fn(struct request_queue *q)return ; } -if (!mq->mqrq_cur->req && !mq->mqrq_prev->req) + cntx = &mq->card->host->context_info; +if (!mq->mqrq_cur->req && mq->mqrq_prev->req) { +/* +* New MMC request arrived when MMC thread may be +* blocked on the previous request to be complete +* with no current request fetched +*/ +spin_lock_irqsave (&cntx->lock, flags); +if (cntx->is_waiting_last_req) { + cntx->is_new_req = true; +wake_up_interruptible (&cntx->wait); + } +spin_unlock_irqrestore (&cntx->lock, flags); + }else if (!mq->mqrq_cur->req && !mq->mqrq_prev->req)wake_up_process (mq->thread); }drivers/mmc/card/queue.h +2
-0@@ -26,6 +26,8 @@ struct mmc_queue {struct mmc_card *card;struct task_struct *thread;struct semaphore thread_sem; +#define MMC_QUEUE_SUSPENDED (1 << 0) +#define MMC_QUEUE_NEW_REQUEST (1 << 1)unsigned int flags;int (*issue_fn)(struct mmc_queue *,struct request *);void *data;drivers/mmc/core/core.c +103
-3@@ -319,11 +319,43 @@ out: }EXPORT_SYMBOL (mmc_start_bkops); +/* +* mmc_wait_data_done() - done callback for data request +* @mrq: done data request +* +* Wakes up mmc context, passed as callback to host controller driver +*/ +static void mmc_wait_data_done (struct mmc_request *mrq) +{ + mrq->host->context_info.is_done_rcv = true; +wake_up_interruptible (&mrq->host->context_info.wait); +} +static void mmc_wait_done (struct mmc_request *mrq) {complete (&mrq->completion); } +/* +*__mmc_start_data_req() - starts data request +* @host: MMC host to start the request +* @mrq: data request to start +* +* Fills done callback that will be used when request are done by card. +* Starts data mmc request execution +*/ +static int __mmc_start_data_req (struct mmc_host *host,struct mmc_request *mrq) +{ + mrq->done = mmc_wait_data_done; +if (mmc_card_removed (host->card)) { + mrq->cmd->error = -ENOMEDIUM ; +return -ENOMEDIUM ; + } +mmc_start_request (host, mrq); + +return 0 ; +} +static int __mmc_start_req (struct mmc_host *host,struct mmc_request *mrq) {init_completion (&mrq->completion);@@ -337,6 +369,60 @@ static int __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)return 0 ; } +/* +* mmc_wait_for_data_req_done() - wait for request completed or new +* request notification arrives +* @host: MMC host to prepare the command. +* @mrq: MMC request to wait for +* +* Blocks MMC context till host controller will ack end of data request +* execution or new request arrives from block layer. Handles +* command retries. +* +* Returns enum mmc_blk_status after checking errors. +*/ +static int mmc_wait_for_data_req_done (struct mmc_host *host, +struct mmc_request *mrq) +{ +struct mmc_command *cmd; +struct mmc_context_info *context_info = &host->context_info; +int err; +unsigned long flags; + +while (1 ) { +wait_event_interruptible (context_info->wait, + (context_info->is_done_rcv || + context_info->is_new_req)); +spin_lock_irqsave (&context_info->lock, flags); + context_info->is_waiting_last_req = false; +spin_unlock_irqrestore (&context_info->lock, flags); +if (context_info->is_done_rcv) { + context_info->is_done_rcv = false; + context_info->is_new_req = false; + cmd = mrq->cmd; +if (!cmd->error || !cmd->retries || +mmc_card_removed (host->card)) { + err = host->areq->err_check (host->card, + host->areq); +break ;/* return err */ + }else { +pr_info ("%s: req failed (CMD%u): %d, retrying...\n" , +mmc_hostname (host), + cmd->opcode, cmd->error); + cmd->retries--; + cmd->error =0 ; + host->ops->request (host, mrq); +continue ;/* wait for done/new event again */ + } + }else if (context_info->is_new_req) { + context_info->is_new_req = false; + err =MMC_BLK_NEW_REQUEST ; +break ;/* return err */ + } + }/* while */ +return err; +} +static void mmc_wait_for_req_done (struct mmc_host *host,struct mmc_request *mrq) {@@ -426,8 +512,21 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,mmc_pre_req (host, areq->mrq, !host->areq);if (host->areq) { -mmc_wait_for_req_done (host, host->areq->mrq); - err = host->areq->err_check (host->card, host->areq); + err =mmc_wait_for_data_req_done (host, host->areq->mrq); +if (err ==MMC_BLK_NEW_REQUEST ) { +if (areq) { +pr_err ("%s: new request while areq = %p" , +mmc_hostname (host), areq); +BUG_ON (1 ); + } +if (error) + *error = err; +/* +* The previous request was not completed, +* nothing to return +*/ +return NULL ; + }/* * Check BKOPS urgency for each R1 response */ @@ -439,7 +538,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host, }if (!err && areq) - start_err =__mmc_start_req (host, areq->mrq); + start_err =__mmc_start_data_req (host, areq->mrq);if (host->areq)mmc_post_req (host, host->areq->mrq,0 );@@ -455,6 +554,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,if (error) *error = err; +return data; }EXPORT_SYMBOL (mmc_start_req);drivers/mmc/core/core.c static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq) · line 2194
Quoted 9 lines
drivers/mmc/core/core.c +6
-0@@ -2086,6 +2186,12 @@ static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)mmc_send_if_cond (host, host->ocr_avail); +spin_lock_init (&host->context_info.lock); + host->context_info.is_new_req = false; + host->context_info.is_done_rcv = false; + host->context_info.is_waiting_last_req = false; +init_waitqueue_head (&host->context_info.wait); +This path may be retired when
mmc_rescan_try_freqis failed. How about putting these inmmc_add_card(mmc/core/bus.c)<Quote>ret = device_add(&card->dev); if (ret) return ret;-> Here seems good point.
mmc_card_set_present(card);</Quote>
Thanks,
Seungwon JeonQuoted 94 lines
drivers/mmc/core/core.c +0
-0/* Order's important: probe SDIO, then SD, then MMC */ if (!mmc_attach_sdio (host))return 0 ;include/linux/mmc/card.h +12
-0@@ -187,6 +187,18 @@ struct sdio_func_tuple;#define SDIO_MAX_FUNCS 7 +enum mmc_blk_status { +MMC_BLK_SUCCESS =0 , +MMC_BLK_PARTIAL , +MMC_BLK_CMD_ERR , +MMC_BLK_RETRY , +MMC_BLK_ABORT , +MMC_BLK_DATA_ERR , +MMC_BLK_ECC_ERR , +MMC_BLK_NOMEDIUM , +MMC_BLK_NEW_REQUEST , +}; +/* The number of MMC physical partitions. These consist of: * boot partitions (2), general purpose partitions (4) in MMC v4.4. */ include/linux/mmc/core.h +2
-1@@ -120,6 +120,7 @@ struct mmc_data {s32 host_cookie;/* host private data */ }; +struct mmc_host ;struct mmc_request {struct mmc_command *sbc;/* SET_BLOCK_COUNT for multiblock */ struct mmc_command *cmd;@@ -128,9 +129,9 @@ struct mmc_request {struct completion completion;void (*done)(struct mmc_request *);/* completion function */ +struct mmc_host *host; }; -struct mmc_host ;struct mmc_card ;struct mmc_async_req ;include/linux/mmc/host.h +16
-1@@ -177,6 +177,21 @@ struct mmc_supply {struct regulator *vqmmc;/* Optional Vccq supply */ }; +/** +* mmc_context_info - synchronization details for mmc context +* @is_done_rcv wake up reason was done request +* @is_new_req wake up reason was new request +* @is_waiting_last_req mmc context waiting for single running request +* @wait wait queue +*/ +struct mmc_context_info { +bool is_done_rcv; +bool is_new_req; +bool is_waiting_last_req; +wait_queue_head_t wait; +spinlock_t lock; +}; +struct mmc_host {struct device *parent;struct device class_dev;@@ -331,6 +346,7 @@ struct mmc_host {struct dentry *debugfs_root;struct mmc_async_req *areq;/* active async req */ +struct mmc_context_info context_info;/* async synchronization info */ #ifdef CONFIG_FAIL_MMC_REQUEST struct fault_attr fail_mmc_request; --1.7.6
--
Konstantin Dorfman,
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-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH v4] mmc: fix async request mechanism for sequential read scenarios
Quoted 3 lines · Seungwon Jeon
Hi, Konstantin.
I added comments more below.
I've answered below.
Quoted 100 lines · Seungwon Jeon
Quoted 98 lines · Konstantin Dorfman
Quoted 69 lines
When current request is running on the bus and if next request fetched by mmcqd is NULL, mmc context (mmcqd thread) gets blocked until the current request completes. This means if new request comes in while the mmcqd thread is blocked, this new request can not be prepared in parallel to current ongoing request. This may result in latency to start new request.
This change allows to wake up the MMC thread (which
is waiting for the current running request to complete). Now once the MMC thread is woken up, new request can be fetched and prepared in parallel to current running request which means this new request can be started immediately after the current running request completes.With this change read throughput is improved by 16%.
Signed-off-by Konstantin Dorfman <[email protected]>---
v4:
keeps new synchronization mechanism within mmc/core layer, so mmc/core external API's are not changed.
-context_infomoved intommc_hoststruct
-context_infoinitialized inmmc_rescan_try_freq()v3:
- newMMC_QUEUE_NEW_REQUESTflag to mark new request case
- lock added to updateis_new_reqflag
- condition for sending new req notification changed
- Moved start waiting for new req notification after fetching NULL
v2:
- Removed synchronization flags
- removed lock fromdone()
- flags names changed
v1:
- Initial submitdrivers/mmc/card/block.c | 25 +++++------ drivers/mmc/card/queue.c | 27 ++++++++++- drivers/mmc/card/queue.h | 2 + drivers/mmc/core/core.c | 112 ++++++++++++++++++++++++++++++++++++++++++++- include/linux/mmc/card.h | 12 +++++ include/linux/mmc/core.h | 3 +- include/linux/mmc/host.h | 16 +++++++ 7 files changed, 177 insertions(+), 20 deletions(-)drivers/mmc/card/block.c +0
-11@@ -113,17 +113,6 @@ struct mmc_blk_data {static DEFINE_MUTEX (open_lock); -enum mmc_blk_status { -MMC_BLK_SUCCESS =0 , -MMC_BLK_PARTIAL , -MMC_BLK_CMD_ERR , -MMC_BLK_RETRY , -MMC_BLK_ABORT , -MMC_BLK_DATA_ERR , -MMC_BLK_ECC_ERR , -MMC_BLK_NOMEDIUM , -}; -module_param (perdev_minors, int,0444 );MODULE_PARM_DESC (perdev_minors,"Minors numbers to allocate per device" );drivers/mmc/card/block.c static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq, · line 1172
Quoted 4 lines
drivers/mmc/card/block.c +1
-0@@ -1180,6 +1169,7 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,memset (brq,0 ,sizeof (struct mmc_blk_request )); brq->mrq.cmd = &brq->cmd; brq->mrq.data = &brq->data; + brq->mrq.host = card->host;Above setting have been added at
__mmc_start_data_reqfunction in previous sending. Currently there is no setting for sdio. Considering sdio case, the former would be better.Quoted 21 lines
drivers/mmc/card/block.c +5
-2brq->cmd.arg =blk_rq_pos (req);if (!mmc_card_blockaddr (card))@@ -1363,9 +1353,12 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc) areq = &mq->mqrq_cur->mmc_active; }else areq =NULL ; - areq =mmc_start_req (card->host, areq, (int *) &status); -if (!areq) + areq =mmc_start_req (card->host, areq, (int *)&status); +if (!areq) { +if (status ==MMC_BLK_NEW_REQUEST ) + mq->flags |=MMC_QUEUE_NEW_REQUEST ;return 0 ; + } mq_rq = container_of(areq, struct mmc_queue_req, mmc_active); brq = &mq_rq->brq;@@ -1374,6 +1367,8 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)mmc_queue_bounce_post (mq_rq);
Quoted 6 lines
Quoted 3 lines
drivers/mmc/card/block.c static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc) · line 1371
Quoted 3 lines
drivers/mmc/card/block.c +2
-0switch (status) { +case MMC_BLK_NEW_REQUEST : +BUG ();/* should never get here */ Is there any case to reach here?
I didn't find it.
Correct. But since there is no "default:" in this switch, compiler demands all values of the 'status' variable present. So, we need this BUG() here or we need to introduce "default:".
Quoted 42 lines
Quoted 42 lines
Quoted 42 lines
drivers/mmc/card/block.c +3
-1case MMC_BLK_SUCCESS :case MMC_BLK_PARTIAL :/* @@ -1486,6 +1481,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req) goto out; } + mq->flags &= ~MMC_QUEUE_NEW_REQUEST ;if (req && req->cmd_flags &REQ_DISCARD ) {/* complete ongoing async transfer before issuing discard */ if (card->host->areq)@@ -1505,9 +1501,10 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req) } out: -if (!req) +if (!req && !(mq->flags &MMC_QUEUE_NEW_REQUEST ))/* release host only when there are no more requests */ mmc_release_host (card->host); +return ret; }drivers/mmc/card/queue.c +3
-1@@ -22,7 +22,6 @@#define MMC_QUEUE_BOUNCESZ 65536 -#define MMC_QUEUE_SUSPENDED (1 << 0)/* * Prepare a MMC request. This just filters out odd stuff. @@ -63,11 +62,20 @@ static int mmc_queue_thread(void *d)set_current_state (TASK_INTERRUPTIBLE ); req =blk_fetch_request (q); mq->mqrq_cur->req = req; +if (!req && mq->mqrq_prev->req && + !(mq->mqrq_prev->req->cmd_flags &REQ_FLUSH ) && + !(mq->mqrq_prev->req->cmd_flags &REQ_DISCARD ))
Quoted 8 lines
Quoted 2 lines
drivers/mmc/card/queue.c static int mmc_queue_thread(void *d) · line 69
Quoted 2 lines
drivers/mmc/card/queue.c +2
-0+ mq->card->host->context_info.is_waiting_last_req = true; +Unlike normal R/W request, request for discard/flush will be finished synchronously.
That meansblk_end_requestis called with 1's cycle of 'issue_fn' and request will be freed in block layer. Currently 'mqrq_prev->req' is keeping these request and is used above condition.
Maybe, same area may be reallocated for normal R/W request after discard/flush is finished. But we still expect discard or flush is saved in 'mq->mqrq_prev->req'.
I wonder that 'mq->mqrq_prev->req' indicates the valid area in all case.
It seems like a bug causing unnecessary call to issue_fn(mq, req) even when req is NULL, because 'mq->mqrq_prev->req' that holds old control request is not NULL. It is not directly related to the patch, it could be fixed by cleaning 'mq->mqrq_prev->req' (for control requests). I think this should be done in separate patch.
Also this action (context_info.is_waiting_last_req = true;) could be moved into card/block.c: mmc_blk_issue_rq() function just for the case, when request is data request. Do you think it will be cleaner?
Quoted 210 lines
Quoted 210 lines
Quoted 210 lines
drivers/mmc/card/queue.c +20
-1spin_unlock_irq (q->queue_lock);if (req || mq->mqrq_prev->req) {set_current_state (TASK_RUNNING ); mq->issue_fn (mq, req); +if (mq->flags &MMC_QUEUE_NEW_REQUEST ) { + mq->flags &= ~MMC_QUEUE_NEW_REQUEST ; +continue ;/* fetch again */ + }/* * Current request becomes previous request @@ -103,6 +111,8 @@ static void mmc_request_fn(struct request_queue *q) {struct mmc_queue *mq = q->queuedata;struct request *req; +unsigned long flags; +struct mmc_context_info *cntx;if (!mq) {while ((req =blk_fetch_request (q)) !=NULL ) {@@ -112,7 +122,20 @@ static void mmc_request_fn(struct request_queue *q)return ; } -if (!mq->mqrq_cur->req && !mq->mqrq_prev->req) + cntx = &mq->card->host->context_info; +if (!mq->mqrq_cur->req && mq->mqrq_prev->req) { +/* +* New MMC request arrived when MMC thread may be +* blocked on the previous request to be complete +* with no current request fetched +*/ +spin_lock_irqsave (&cntx->lock, flags); +if (cntx->is_waiting_last_req) { + cntx->is_new_req = true; +wake_up_interruptible (&cntx->wait); + } +spin_unlock_irqrestore (&cntx->lock, flags); + }else if (!mq->mqrq_cur->req && !mq->mqrq_prev->req)wake_up_process (mq->thread); }drivers/mmc/card/queue.h +2
-0@@ -26,6 +26,8 @@ struct mmc_queue {struct mmc_card *card;struct task_struct *thread;struct semaphore thread_sem; +#define MMC_QUEUE_SUSPENDED (1 << 0) +#define MMC_QUEUE_NEW_REQUEST (1 << 1)unsigned int flags;int (*issue_fn)(struct mmc_queue *,struct request *);void *data;drivers/mmc/core/core.c +103
-3@@ -319,11 +319,43 @@ out: }EXPORT_SYMBOL (mmc_start_bkops); +/* +* mmc_wait_data_done() - done callback for data request +* @mrq: done data request +* +* Wakes up mmc context, passed as callback to host controller driver +*/ +static void mmc_wait_data_done (struct mmc_request *mrq) +{ + mrq->host->context_info.is_done_rcv = true; +wake_up_interruptible (&mrq->host->context_info.wait); +} +static void mmc_wait_done (struct mmc_request *mrq) {complete (&mrq->completion); } +/* +*__mmc_start_data_req() - starts data request +* @host: MMC host to start the request +* @mrq: data request to start +* +* Fills done callback that will be used when request are done by card. +* Starts data mmc request execution +*/ +static int __mmc_start_data_req (struct mmc_host *host,struct mmc_request *mrq) +{ + mrq->done = mmc_wait_data_done; +if (mmc_card_removed (host->card)) { + mrq->cmd->error = -ENOMEDIUM ; +return -ENOMEDIUM ; + } +mmc_start_request (host, mrq); + +return 0 ; +} +static int __mmc_start_req (struct mmc_host *host,struct mmc_request *mrq) {init_completion (&mrq->completion);@@ -337,6 +369,60 @@ static int __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)return 0 ; } +/* +* mmc_wait_for_data_req_done() - wait for request completed or new +* request notification arrives +* @host: MMC host to prepare the command. +* @mrq: MMC request to wait for +* +* Blocks MMC context till host controller will ack end of data request +* execution or new request arrives from block layer. Handles +* command retries. +* +* Returns enum mmc_blk_status after checking errors. +*/ +static int mmc_wait_for_data_req_done (struct mmc_host *host, +struct mmc_request *mrq) +{ +struct mmc_command *cmd; +struct mmc_context_info *context_info = &host->context_info; +int err; +unsigned long flags; + +while (1 ) { +wait_event_interruptible (context_info->wait, + (context_info->is_done_rcv || + context_info->is_new_req)); +spin_lock_irqsave (&context_info->lock, flags); + context_info->is_waiting_last_req = false; +spin_unlock_irqrestore (&context_info->lock, flags); +if (context_info->is_done_rcv) { + context_info->is_done_rcv = false; + context_info->is_new_req = false; + cmd = mrq->cmd; +if (!cmd->error || !cmd->retries || +mmc_card_removed (host->card)) { + err = host->areq->err_check (host->card, + host->areq); +break ;/* return err */ + }else { +pr_info ("%s: req failed (CMD%u): %d, retrying...\n" , +mmc_hostname (host), + cmd->opcode, cmd->error); + cmd->retries--; + cmd->error =0 ; + host->ops->request (host, mrq); +continue ;/* wait for done/new event again */ + } + }else if (context_info->is_new_req) { + context_info->is_new_req = false; + err =MMC_BLK_NEW_REQUEST ; +break ;/* return err */ + } + }/* while */ +return err; +} +static void mmc_wait_for_req_done (struct mmc_host *host,struct mmc_request *mrq) {@@ -426,8 +512,21 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,mmc_pre_req (host, areq->mrq, !host->areq);if (host->areq) { -mmc_wait_for_req_done (host, host->areq->mrq); - err = host->areq->err_check (host->card, host->areq); + err =mmc_wait_for_data_req_done (host, host->areq->mrq); +if (err ==MMC_BLK_NEW_REQUEST ) { +if (areq) { +pr_err ("%s: new request while areq = %p" , +mmc_hostname (host), areq); +BUG_ON (1 ); + } +if (error) + *error = err; +/* +* The previous request was not completed, +* nothing to return +*/ +return NULL ; + }/* * Check BKOPS urgency for each R1 response */ @@ -439,7 +538,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host, }if (!err && areq) - start_err =__mmc_start_req (host, areq->mrq); + start_err =__mmc_start_data_req (host, areq->mrq);if (host->areq)mmc_post_req (host, host->areq->mrq,0 );@@ -455,6 +554,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,if (error) *error = err; +return data; }EXPORT_SYMBOL (mmc_start_req);@@ -2086,6 +2186,12 @@ static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)mmc_send_if_cond (host, host->ocr_avail);
Quoted 13 lines
Quoted 9 lines
drivers/mmc/core/core.c static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq) · line 2194
Quoted 6 lines
drivers/mmc/core/core.c +6
-0+spin_lock_init (&host->context_info.lock); + host->context_info.is_new_req = false; + host->context_info.is_done_rcv = false; + host->context_info.is_waiting_last_req = false; +init_waitqueue_head (&host->context_info.wait); +This path may be retired when
mmc_rescan_try_freqis failed. How about putting these inmmc_add_card(mmc/core/bus.c)<Quote>Quoted 4 lines
ret = device_add(&card->dev); if (ret) return ret;-> Here seems good point.
It is ok to put the initialization just before calling to "device_add", because data requests starting to arrive immediately after device_add(). I've tested this already and will post soon.
Quoted 101 lines
Quoted 100 lines
mmc_card_set_present(card);</Quote>
Thanks,
Seungwon JeonQuoted 94 lines
drivers/mmc/core/core.c +0
-0/* Order's important: probe SDIO, then SD, then MMC */ if (!mmc_attach_sdio (host))return 0 ;include/linux/mmc/card.h +12
-0@@ -187,6 +187,18 @@ struct sdio_func_tuple;#define SDIO_MAX_FUNCS 7 +enum mmc_blk_status { +MMC_BLK_SUCCESS =0 , +MMC_BLK_PARTIAL , +MMC_BLK_CMD_ERR , +MMC_BLK_RETRY , +MMC_BLK_ABORT , +MMC_BLK_DATA_ERR , +MMC_BLK_ECC_ERR , +MMC_BLK_NOMEDIUM , +MMC_BLK_NEW_REQUEST , +}; +/* The number of MMC physical partitions. These consist of: * boot partitions (2), general purpose partitions (4) in MMC v4.4. */ include/linux/mmc/core.h +2
-1@@ -120,6 +120,7 @@ struct mmc_data {s32 host_cookie;/* host private data */ }; +struct mmc_host ;struct mmc_request {struct mmc_command *sbc;/* SET_BLOCK_COUNT for multiblock */ struct mmc_command *cmd;@@ -128,9 +129,9 @@ struct mmc_request {struct completion completion;void (*done)(struct mmc_request *);/* completion function */ +struct mmc_host *host; }; -struct mmc_host ;struct mmc_card ;struct mmc_async_req ;include/linux/mmc/host.h +16
-1@@ -177,6 +177,21 @@ struct mmc_supply {struct regulator *vqmmc;/* Optional Vccq supply */ }; +/** +* mmc_context_info - synchronization details for mmc context +* @is_done_rcv wake up reason was done request +* @is_new_req wake up reason was new request +* @is_waiting_last_req mmc context waiting for single running request +* @wait wait queue +*/ +struct mmc_context_info { +bool is_done_rcv; +bool is_new_req; +bool is_waiting_last_req; +wait_queue_head_t wait; +spinlock_t lock; +}; +struct mmc_host {struct device *parent;struct device class_dev;@@ -331,6 +346,7 @@ struct mmc_host {struct dentry *debugfs_root;struct mmc_async_req *areq;/* active async req */ +struct mmc_context_info context_info;/* async synchronization info */ #ifdef CONFIG_FAIL_MMC_REQUEST struct fault_attr fail_mmc_request; --1.7.6
--
Konstantin Dorfman,
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-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 4 lines
Konstantin Dorfman, QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [RESEND PATCH v4] mmc: fix async request mechanism for sequential read scenarios
Hi,
Quoted 9 lines · Konstantin Dorfman
Quoted 9 lines
Quoted 6 lines
drivers/mmc/card/block.c static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc) · line 1371
Quoted 6 lines
drivers/mmc/card/block.c +2
-0@@ -1374,6 +1367,8 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)mmc_queue_bounce_post (mq_rq);switch (status) { +case MMC_BLK_NEW_REQUEST : +BUG ();/* should never get here */ Is there any case to reach here?
I didn't find it.
Quoted 3 lines
Correct. But since there is no "default:" in this switch, compiler demands all values of the 'status' variable present. So, we need this
BUG()here or we need to introduce "default:".
Please avoid using BUG() entirely (both here and elsewhere). It crashes the entire MMC stack, which means it crashes the machine entirely if you're booted from eMMC/SD. There is almost always something better that can be done than crashing the machine.
default: and pr_err() sound fine here.
Thanks,
- Chris.
Signature · 2 lines
Chris Ball <[email protected]> <http://printf.net/> One Laptop Per Child
RE: [RESEND PATCH v4] mmc: fix async request mechanism for sequential read scenarios
In reply to Konstantin DorfmanQuoted 27 lines · Konstantin Dorfman
Quoted 3 lines · Seungwon Jeon
Hi, Konstantin.
I added comments more below.
I've answered below.
Quoted 16 lines · Seungwon Jeon
Quoted 12 lines · Konstantin Dorfman
drivers/mmc/card/queue.c static int mmc_queue_thread(void *d) · line 69
Quoted 11 lines
/*
* Prepare a MMC request. This just filters out odd stuff.drivers/mmc/card/queue.c +5
-0@@ -63,11 +62,20 @@ static int mmc_queue_thread(void *d)set_current_state (TASK_INTERRUPTIBLE ); req =blk_fetch_request (q); mq->mqrq_cur->req = req; +if (!req && mq->mqrq_prev->req && + !(mq->mqrq_prev->req->cmd_flags &REQ_FLUSH ) && + !(mq->mqrq_prev->req->cmd_flags &REQ_DISCARD )) + mq->card->host->context_info.is_waiting_last_req = true; +Unlike normal R/W request, request for discard/flush will be finished synchronously. That means
blk_end_requestis called with 1's cycle of 'issue_fn' and request will be freed in blocklayer.
Quoted 4 lines
Currently '
mqrq_prev->req' is keeping these request and is used above condition. Maybe, same area may be reallocated for normal R/W request after discard/flush is finished. But we still expect discard or flush is saved in 'mq->mqrq_prev->req'.
I wonder that 'mq->mqrq_prev->req' indicates the valid area in all case.
Quoted 5 lines
It seems like a bug causing unnecessary call to
issue_fn(mq, req)even whenreqis NULL, because 'mq->mqrq_prev->req' that holds old control request is not NULL. It is not directly related to the patch, it could be fixed by cleaning 'mq->mqrq_prev->req' (for control requests). I think this should be done in separate patch.
Yes, it's different issue. I've already checked it.
But I'm seeing whether cmd_flag of 'mq->mqrq_prev->req' is valid or not after blk_end_request is completed. As it's mentioned above, special requests are completed at once.
Quoted 4 lines
Also this action (context_info.is_waiting_last_req = true;) could be moved into
card/block.c:mmc_blk_issue_rq()function just for the case, when request is data request. Do you think it will be cleaner?
Wherever it is moved, we need other condition to decide whether last data transfer is ongoing. 'card->host->areq' could be good condition.
Quoted 17 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/mmc/core/core.c +0
-0@@ -2086,6 +2186,12 @@ static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)mmc_send_if_cond (host, host->ocr_avail);Quoted 13 lines
Quoted 9 lines
drivers/mmc/core/core.c static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq) · line 2194
Quoted 6 lines
drivers/mmc/core/core.c +6
-0+spin_lock_init (&host->context_info.lock); + host->context_info.is_new_req = false; + host->context_info.is_done_rcv = false; + host->context_info.is_waiting_last_req = false; +init_waitqueue_head (&host->context_info.wait); +This path may be retired when
mmc_rescan_try_freqis failed. How about putting these inmmc_add_card(mmc/core/bus.c)<Quote>Quoted 4 lines
ret = device_add(&card->dev); if (ret) return ret;-> Here seems good point.
Quoted 3 lines
It is ok to put the initialization just before calling to "
device_add", because data requests starting to arrive immediately afterdevice_add(). I've tested this already and will post soon.
Yes, it's reasonable.
Thanks,
Seungwon Jeon
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
mmc_card_set_present(card);</Quote>
Thanks,
Seungwon Jeon
PATCH v5 1/1 mmc: fix async request mechanism for sequential read scenarios
When current request is running on the bus and if next request fetched by mmcqd is NULL, mmc context (mmcqd thread) gets blocked until the current request completes. This means that if new request comes in while the mmcqd thread is blocked, this new request can not be prepared in parallel to current ongoing request. This may result in delaying the new request execution and increase it's latency.
This change allows to wake up the MMC thread on new request arrival. Now once the MMC thread is woken up, a new request can be fetched and prepared in parallel to the current running request which means this new request can be started immediately after the current running request completes.
With this change read throughput is improved by 16%.
---
v5: - Removed BUG() and BUG_ON() with proper error handling
- Turn on of is_waiting_last_req flag moved from queue.c into block.c (mmc_blk_issue_req())
- mmc_init_context_info() called from mmc_add_card(). It is common point for MMC, SD, SDIO.
v4: keeps new synchronization mechanism within mmc/core
layer, so mmc/core external API's are not changed.
- context_info moved into mmc_host struct
- context_info initialized in mmc_attach_mmc()
v3:
- new MMC_QUEUE_NEW_REQUEST flag to mark new request case
- lock added to update is_new_req flag
- condition for sending new req notification changed
- Moved start waiting for new req notification after fetching NULL
v2:
- Removed synchronization flags
- removed lock from done()
- flags names changed
v1:
- Initial submit
drivers/mmc/card/block.c +17 -13
@@ -113,17 +113,6 @@ struct mmc_blk_data {static DEFINE_MUTEX (open_lock); -enum mmc_blk_status { -MMC_BLK_SUCCESS =0 , -MMC_BLK_PARTIAL , -MMC_BLK_CMD_ERR , -MMC_BLK_RETRY , -MMC_BLK_ABORT , -MMC_BLK_DATA_ERR , -MMC_BLK_ECC_ERR , -MMC_BLK_NOMEDIUM , -}; -module_param (perdev_minors, int,0444 );MODULE_PARM_DESC (perdev_minors,"Minors numbers to allocate per device" );
@@ -1364,8 +1353,11 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc) }else areq =NULL ; areq =mmc_start_req (card->host, areq, (int *) &status); -if (!areq) +if (!areq) { +if (status ==MMC_BLK_NEW_REQUEST ) + mq->flags |=MMC_QUEUE_NEW_REQUEST ;return 0 ; + } mq_rq = container_of(areq, struct mmc_queue_req, mmc_active); brq = &mq_rq->brq;
@@ -1438,6 +1430,10 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)break ;case MMC_BLK_NOMEDIUM : goto cmd_abort; +default : +pr_err ("%s: Unhandled return value (%d)" , + req->rq_disk->disk_name, status); + goto cmd_abort; }if (ret) {
@@ -1472,6 +1468,8 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)int ret;struct mmc_blk_data *md = mq->data;struct mmc_card *card = md->queue.card; +struct mmc_host *host = card->host; +unsigned long flags;if (req && !mq->mqrq_prev->req)/* claim host only for the first request */
@@ -1486,6 +1484,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req) goto out; } + mq->flags &= ~MMC_QUEUE_NEW_REQUEST ;if (req && req->cmd_flags &REQ_DISCARD ) {/* complete ongoing async transfer before issuing discard */ if (card->host->areq)
@@ -1501,11 +1500,16 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)mmc_blk_issue_rw_rq (mq,NULL ); ret =mmc_blk_issue_flush (mq, req); }else { +if (!req && host->areq) { +spin_lock_irqsave (&host->context_info.lock, flags); + host->context_info.is_waiting_last_req = true; +spin_unlock_irqrestore (&host->context_info.lock, flags); + } ret =mmc_blk_issue_rw_rq (mq, req); } out: -if (!req) +if (!req && !(mq->flags &MMC_QUEUE_NEW_REQUEST ))/* release host only when there are no more requests */ mmc_release_host (card->host);return ret;
drivers/mmc/card/queue.c +20 -2
@@ -22,7 +22,6 @@#define MMC_QUEUE_BOUNCESZ 65536 -#define MMC_QUEUE_SUSPENDED (1 << 0)/* * Prepare a MMC request. This just filters out odd stuff.
@@ -68,6 +67,10 @@ static int mmc_queue_thread(void *d)if (req || mq->mqrq_prev->req) {set_current_state (TASK_RUNNING ); mq->issue_fn (mq, req); +if (mq->flags &MMC_QUEUE_NEW_REQUEST ) { + mq->flags &= ~MMC_QUEUE_NEW_REQUEST ; +continue ;/* fetch again */ + }/* * Current request becomes previous request
@@ -103,6 +106,8 @@ static void mmc_request_fn(struct request_queue *q) {struct mmc_queue *mq = q->queuedata;struct request *req; +unsigned long flags; +struct mmc_context_info *cntx;if (!mq) {while ((req =blk_fetch_request (q)) !=NULL ) {
@@ -112,7 +117,20 @@ static void mmc_request_fn(struct request_queue *q)return ; } -if (!mq->mqrq_cur->req && !mq->mqrq_prev->req) + cntx = &mq->card->host->context_info; +if (!mq->mqrq_cur->req && mq->mqrq_prev->req) { +/* +* New MMC request arrived when MMC thread may be +* blocked on the previous request to be complete +* with no current request fetched +*/ +spin_lock_irqsave (&cntx->lock, flags); +if (cntx->is_waiting_last_req) { + cntx->is_new_req = true; +wake_up_interruptible (&cntx->wait); + } +spin_unlock_irqrestore (&cntx->lock, flags); + }else if (!mq->mqrq_cur->req && !mq->mqrq_prev->req)wake_up_process (mq->thread); }
drivers/mmc/card/queue.h +3 -0
@@ -27,6 +27,9 @@ struct mmc_queue {struct task_struct *thread;struct semaphore thread_sem;unsigned int flags; +#define MMC_QUEUE_SUSPENDED (1 << 0) +#define MMC_QUEUE_NEW_REQUEST (1 << 1) +int (*issue_fn)(struct mmc_queue *,struct request *);void *data;struct request_queue *queue;
drivers/mmc/core/bus.c +1 -0
@@ -321,6 +321,7 @@ int mmc_add_card(struct mmc_card *card)#ifdef CONFIG_DEBUG_FS mmc_add_card_debugfs (card);#endif +mmc_init_context_info (card->host); ret =device_add (&card->dev);if (ret)
drivers/mmc/core/core.c +118 -3
156 lines not on this page · raw message
drivers/mmc/core/core.h +1 -0
@@ -76,5 +76,6 @@ void mmc_remove_host_debugfs(struct mmc_host *host);void mmc_add_card_debugfs (struct mmc_card *card);void mmc_remove_card_debugfs (struct mmc_card *card); +void mmc_init_context_info (struct mmc_host *host);#endif
include/linux/mmc/card.h +12 -0
@@ -187,6 +187,18 @@ struct sdio_func_tuple;#define SDIO_MAX_FUNCS 7 +enum mmc_blk_status { +MMC_BLK_SUCCESS =0 , +MMC_BLK_PARTIAL , +MMC_BLK_CMD_ERR , +MMC_BLK_RETRY , +MMC_BLK_ABORT , +MMC_BLK_DATA_ERR , +MMC_BLK_ECC_ERR , +MMC_BLK_NOMEDIUM , +MMC_BLK_NEW_REQUEST , +}; +/* The number of MMC physical partitions. These consist of: * boot partitions (2), general purpose partitions (4) in MMC v4.4. */
include/linux/mmc/core.h +2 -1
@@ -120,6 +120,7 @@ struct mmc_data {s32 host_cookie;/* host private data */ }; +struct mmc_host ;struct mmc_request {struct mmc_command *sbc;/* SET_BLOCK_COUNT for multiblock */ struct mmc_command *cmd;
@@ -128,9 +129,9 @@ struct mmc_request {struct completion completion;void (*done)(struct mmc_request *);/* completion function */ +struct mmc_host *host; }; -struct mmc_host ;struct mmc_card ;struct mmc_async_req ;
include/linux/mmc/host.h +17 -0
@@ -170,6 +170,22 @@ struct mmc_slot {void *handler_priv; }; +/** +* mmc_context_info - synchronization details for mmc context +* @is_done_rcv wake up reason was done request +* @is_new_req wake up reason was new request +* @is_waiting_last_req mmc context waiting for single running request +* @wait wait queue +* @lock lock to protect data fields +*/ +struct mmc_context_info { +bool is_done_rcv; +bool is_new_req; +bool is_waiting_last_req; +wait_queue_head_t wait; +spinlock_t lock; +}; +struct regulator ;struct mmc_supply {
@@ -331,6 +347,7 @@ struct mmc_host {struct dentry *debugfs_root;struct mmc_async_req *areq;/* active async req */ +struct mmc_context_info context_info;/* async synchronization info */ #ifdef CONFIG_FAIL_MMC_REQUEST struct fault_attr fail_mmc_request;
Signature · 6 lines
1.7.6 -- Konstantin Dorfman, QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
RE: [PATCH v5] mmc: fix async request mechanism for sequential read scenarios
I checked the changes. Thanks for your work.
Quoted 470 lines · Konstantin Dorfman
When current request is running on the bus and if next request fetched by mmcqd is NULL, mmc context (mmcqd thread) gets blocked until the current request completes. This means that if new request comes in while the mmcqd thread is blocked, this new request can not be prepared in parallel to current ongoing request. This may result in delaying the new request execution and increase it's latency.
This change allows to wake up the MMC thread on new request arrival. Now once the MMC thread is woken up, a new request can be fetched and prepared in parallel to the current running request which means this new request can be started immediately after the current running request completes.
With this change read throughput is improved by 16%.
Signed-off-by Konstantin Dorfman <[email protected]>---
v5: - RemovedBUG()andBUG_ON()with proper error handling- Turn on of is_waiting_last_req flag moved from queue.c into block.c (mmc_blk_issue_req())-
mmc_init_context_info()called frommmc_add_card(). It is common point for MMC, SD, SDIO.v4: keeps new synchronization mechanism within mmc/core
layer, so mmc/core external API's are not changed.
-context_infomoved intommc_hoststruct
-context_infoinitialized inmmc_attach_mmc()v3:
- newMMC_QUEUE_NEW_REQUESTflag to mark new request case
- lock added to updateis_new_reqflag
- condition for sending new req notification changed
- Moved start waiting for new req notification after fetching NULL
v2:
- Removed synchronization flags
- removed lock fromdone()
- flags names changed
v1:
- Initial submitdrivers/mmc/card/block.c +17
-13@@ -113,17 +113,6 @@ struct mmc_blk_data {static DEFINE_MUTEX (open_lock); -enum mmc_blk_status { -MMC_BLK_SUCCESS =0 , -MMC_BLK_PARTIAL , -MMC_BLK_CMD_ERR , -MMC_BLK_RETRY , -MMC_BLK_ABORT , -MMC_BLK_DATA_ERR , -MMC_BLK_ECC_ERR , -MMC_BLK_NOMEDIUM , -}; -module_param (perdev_minors, int,0444 );MODULE_PARM_DESC (perdev_minors,"Minors numbers to allocate per device" );@@ -1364,8 +1353,11 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc) }else areq =NULL ; areq =mmc_start_req (card->host, areq, (int *) &status); -if (!areq) +if (!areq) { +if (status ==MMC_BLK_NEW_REQUEST ) + mq->flags |=MMC_QUEUE_NEW_REQUEST ;return 0 ; + } mq_rq = container_of(areq, struct mmc_queue_req, mmc_active); brq = &mq_rq->brq;@@ -1438,6 +1430,10 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)break ;case MMC_BLK_NOMEDIUM : goto cmd_abort; +default : +pr_err ("%s: Unhandled return value (%d)" , + req->rq_disk->disk_name, status); + goto cmd_abort; }if (ret) {@@ -1472,6 +1468,8 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)int ret;struct mmc_blk_data *md = mq->data;struct mmc_card *card = md->queue.card; +struct mmc_host *host = card->host; +unsigned long flags;if (req && !mq->mqrq_prev->req)/* claim host only for the first request */ @@ -1486,6 +1484,7 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req) goto out; } + mq->flags &= ~MMC_QUEUE_NEW_REQUEST ;if (req && req->cmd_flags &REQ_DISCARD ) {/* complete ongoing async transfer before issuing discard */ if (card->host->areq)@@ -1501,11 +1500,16 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)mmc_blk_issue_rw_rq (mq,NULL ); ret =mmc_blk_issue_flush (mq, req); }else { +if (!req && host->areq) { +spin_lock_irqsave (&host->context_info.lock, flags); + host->context_info.is_waiting_last_req = true; +spin_unlock_irqrestore (&host->context_info.lock, flags); + } ret =mmc_blk_issue_rw_rq (mq, req); } out: -if (!req) +if (!req && !(mq->flags &MMC_QUEUE_NEW_REQUEST ))/* release host only when there are no more requests */ mmc_release_host (card->host);return ret;drivers/mmc/card/queue.c +20
-2@@ -22,7 +22,6 @@#define MMC_QUEUE_BOUNCESZ 65536 -#define MMC_QUEUE_SUSPENDED (1 << 0)/* * Prepare a MMC request. This just filters out odd stuff. @@ -68,6 +67,10 @@ static int mmc_queue_thread(void *d)if (req || mq->mqrq_prev->req) {set_current_state (TASK_RUNNING ); mq->issue_fn (mq, req); +if (mq->flags &MMC_QUEUE_NEW_REQUEST ) { + mq->flags &= ~MMC_QUEUE_NEW_REQUEST ; +continue ;/* fetch again */ + }/* * Current request becomes previous request @@ -103,6 +106,8 @@ static void mmc_request_fn(struct request_queue *q) {struct mmc_queue *mq = q->queuedata;struct request *req; +unsigned long flags; +struct mmc_context_info *cntx;if (!mq) {while ((req =blk_fetch_request (q)) !=NULL ) {@@ -112,7 +117,20 @@ static void mmc_request_fn(struct request_queue *q)return ; } -if (!mq->mqrq_cur->req && !mq->mqrq_prev->req) + cntx = &mq->card->host->context_info; +if (!mq->mqrq_cur->req && mq->mqrq_prev->req) { +/* +* New MMC request arrived when MMC thread may be +* blocked on the previous request to be complete +* with no current request fetched +*/ +spin_lock_irqsave (&cntx->lock, flags); +if (cntx->is_waiting_last_req) { + cntx->is_new_req = true; +wake_up_interruptible (&cntx->wait); + } +spin_unlock_irqrestore (&cntx->lock, flags); + }else if (!mq->mqrq_cur->req && !mq->mqrq_prev->req)wake_up_process (mq->thread); }drivers/mmc/card/queue.h +3
-0@@ -27,6 +27,9 @@ struct mmc_queue {struct task_struct *thread;struct semaphore thread_sem;unsigned int flags; +#define MMC_QUEUE_SUSPENDED (1 << 0) +#define MMC_QUEUE_NEW_REQUEST (1 << 1) +int (*issue_fn)(struct mmc_queue *,struct request *);void *data;struct request_queue *queue;drivers/mmc/core/bus.c +1
-0@@ -321,6 +321,7 @@ int mmc_add_card(struct mmc_card *card)#ifdef CONFIG_DEBUG_FS mmc_add_card_debugfs (card);#endif +mmc_init_context_info (card->host); ret =device_add (&card->dev);if (ret)drivers/mmc/core/core.c +118
-3@@ -319,11 +319,44 @@ out: }EXPORT_SYMBOL (mmc_start_bkops); +/* +* mmc_wait_data_done() - done callback for data request +* @mrq: done data request +* +* Wakes up mmc context, passed as a callback to host controller driver +*/ +static void mmc_wait_data_done (struct mmc_request *mrq) +{ + mrq->host->context_info.is_done_rcv = true; +wake_up_interruptible (&mrq->host->context_info.wait); +} +static void mmc_wait_done (struct mmc_request *mrq) {complete (&mrq->completion); } +/* +*__mmc_start_data_req() - starts data request +* @host: MMC host to start the request +* @mrq: data request to start +* +* Sets the done callback to be called when request is completed by the card. +* Starts data mmc request execution +*/ +static int __mmc_start_data_req (struct mmc_host *host,struct mmc_request *mrq) +{ + mrq->done = mmc_wait_data_done; + mrq->host = host; +if (mmc_card_removed (host->card)) { + mrq->cmd->error = -ENOMEDIUM ; +return -ENOMEDIUM ; + } +mmc_start_request (host, mrq); + +return 0 ; +} +static int __mmc_start_req (struct mmc_host *host,struct mmc_request *mrq) {init_completion (&mrq->completion);@@ -337,6 +370,62 @@ static int __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)return 0 ; } +/* +* mmc_wait_for_data_req_done() - wait for request completed +* @host: MMC host to prepare the command. +* @mrq: MMC request to wait for +* +* Blocks MMC context till host controller will ack end of data request +* execution or new request notification arrives from the block layer. +* Handles command retries. +* +* Returns enum mmc_blk_status after checking errors. +*/ +static int mmc_wait_for_data_req_done (struct mmc_host *host, +struct mmc_request *mrq, +struct mmc_async_req *next_req) +{ +struct mmc_command *cmd; +struct mmc_context_info *context_info = &host->context_info; +int err; +unsigned long flags; + +while (1 ) { +wait_event_interruptible (context_info->wait, + (context_info->is_done_rcv || + context_info->is_new_req)); +spin_lock_irqsave (&context_info->lock, flags); + context_info->is_waiting_last_req = false; +spin_unlock_irqrestore (&context_info->lock, flags); +if (context_info->is_done_rcv) { + context_info->is_done_rcv = false; + context_info->is_new_req = false; + cmd = mrq->cmd; +if (!cmd->error || !cmd->retries || +mmc_card_removed (host->card)) { + err = host->areq->err_check (host->card, + host->areq); +break ;/* return err */ + }else { +pr_info ("%s: req failed (CMD%u): %d, retrying...\n" , +mmc_hostname (host), + cmd->opcode, cmd->error); + cmd->retries--; + cmd->error =0 ; + host->ops->request (host, mrq); +continue ;/* wait for done/new event again */ + } + }else if (context_info->is_new_req) { + context_info->is_new_req = false; +if (!next_req) { + err =MMC_BLK_NEW_REQUEST ; +break ;/* return err */ + } + } + }/* while */ +return err; +} +static void mmc_wait_for_req_done (struct mmc_host *host,struct mmc_request *mrq) {@@ -426,8 +515,17 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host,mmc_pre_req (host, areq->mrq, !host->areq);if (host->areq) { -mmc_wait_for_req_done (host, host->areq->mrq); - err = host->areq->err_check (host->card, host->areq); + err =mmc_wait_for_data_req_done (host, host->areq->mrq, + areq); +if (err ==MMC_BLK_NEW_REQUEST ) { +if (error) + *error = err; +/* +* The previous request was not completed, +* nothing to return +*/ +return NULL ; + }/* * Check BKOPS urgency for each R1 response */ @@ -439,7 +537,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host, }if (!err && areq) - start_err =__mmc_start_req (host, areq->mrq); + start_err =__mmc_start_data_req (host, areq->mrq);if (host->areq)mmc_post_req (host, host->areq->mrq,0 );@@ -2581,6 +2679,23 @@ int mmc_pm_notify(struct notifier_block *notify_block, }#endif +/** +* mmc_init_context_info() - init synchronization context +* @host: mmc host +* +* Init struct context_info needed to implement asynchronous +* request mechanism, used by mmc core, host driver and mmc requests +* supplier. +*/ +void mmc_init_context_info (struct mmc_host *host) +{ +spin_lock_init (&host->context_info.lock); + host->context_info.is_new_req = false; + host->context_info.is_done_rcv = false; + host->context_info.is_waiting_last_req = false; +init_waitqueue_head (&host->context_info.wait); +} +static int __init mmc_init (void ) {int ret;drivers/mmc/core/core.h +1
-0@@ -76,5 +76,6 @@ void mmc_remove_host_debugfs(struct mmc_host *host);void mmc_add_card_debugfs (struct mmc_card *card);void mmc_remove_card_debugfs (struct mmc_card *card); +void mmc_init_context_info (struct mmc_host *host);#endif include/linux/mmc/card.h +12
-0@@ -187,6 +187,18 @@ struct sdio_func_tuple;#define SDIO_MAX_FUNCS 7 +enum mmc_blk_status { +MMC_BLK_SUCCESS =0 , +MMC_BLK_PARTIAL , +MMC_BLK_CMD_ERR , +MMC_BLK_RETRY , +MMC_BLK_ABORT , +MMC_BLK_DATA_ERR , +MMC_BLK_ECC_ERR , +MMC_BLK_NOMEDIUM , +MMC_BLK_NEW_REQUEST , +}; +/* The number of MMC physical partitions. These consist of: * boot partitions (2), general purpose partitions (4) in MMC v4.4. */ include/linux/mmc/core.h +2
-1@@ -120,6 +120,7 @@ struct mmc_data {s32 host_cookie;/* host private data */ }; +struct mmc_host ;struct mmc_request {struct mmc_command *sbc;/* SET_BLOCK_COUNT for multiblock */ struct mmc_command *cmd;@@ -128,9 +129,9 @@ struct mmc_request {struct completion completion;void (*done)(struct mmc_request *);/* completion function */ +struct mmc_host *host; }; -struct mmc_host ;struct mmc_card ;struct mmc_async_req ;include/linux/mmc/host.h +17
-1@@ -170,6 +170,22 @@ struct mmc_slot {void *handler_priv; }; +/** +* mmc_context_info - synchronization details for mmc context +* @is_done_rcv wake up reason was done request +* @is_new_req wake up reason was new request +* @is_waiting_last_req mmc context waiting for single running request +* @wait wait queue +* @lock lock to protect data fields +*/ +struct mmc_context_info { +bool is_done_rcv; +bool is_new_req; +bool is_waiting_last_req; +wait_queue_head_t wait; +spinlock_t lock; +}; +struct regulator ;struct mmc_supply {@@ -331,6 +347,7 @@ struct mmc_host {struct dentry *debugfs_root;struct mmc_async_req *areq;/* active async req */ +struct mmc_context_info context_info;/* async synchronization info */ #ifdef CONFIG_FAIL_MMC_REQUEST struct fault_attr fail_mmc_request; --1.7.6
--
Konstantin Dorfman,
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-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] mmc: fix async request mechanism for sequential read scenarios
Hi Konstantin,
Quoted 27 lines · Seungwon Jeon
I checked the changes. Thanks for your work.
Reviewed-by Seungwon Jeon <[email protected]>Quoted 22 lines · Konstantin Dorfman
When current request is running on the bus and if next request fetched by mmcqd is NULL, mmc context (mmcqd thread) gets blocked until the current request completes. This means that if new request comes in while the mmcqd thread is blocked, this new request can not be prepared in parallel to current ongoing request. This may result in delaying the new request execution and increase it's latency.
This change allows to wake up the MMC thread on new request arrival. Now once the MMC thread is woken up, a new request can be fetched and prepared in parallel to the current running request which means this new request can be started immediately after the current running request completes.
With this change read throughput is improved by 16%.
Signed-off-by Konstantin Dorfman <[email protected]>---
v5: - RemovedBUG()andBUG_ON()with proper error handling- Turn on of is_waiting_last_req flag moved from queue.c into block.c (mmc_blk_issue_req())-
mmc_init_context_info()called frommmc_add_card(). It is common point for MMC, SD, SDIO.
Thanks, pushed to mmc-next for 3.9.
Per, would you like to add an ACK here?
- Chris.
Signature · 2 lines
Chris Ball <[email protected]> <http://printf.net/> One Laptop Per Child
Re: [PATCH v5] mmc: fix async request mechanism for sequential read scenarios
Quoted 31 lines · Chris Ball
Hi Konstantin,
Quoted 27 lines · Seungwon Jeon
I checked the changes. Thanks for your work.
Reviewed-by Seungwon Jeon <[email protected]>Quoted 22 lines · Konstantin Dorfman
When current request is running on the bus and if next request fetched by mmcqd is NULL, mmc context (mmcqd thread) gets blocked until the current request completes. This means that if new request comes in while the mmcqd thread is blocked, this new request can not be prepared in parallel to current ongoing request. This may result in delaying the new request execution and increase it's latency.
This change allows to wake up the MMC thread on new request arrival. Now once the MMC thread is woken up, a new request can be fetched and prepared in parallel to the current running request which means this new request can be started immediately after the current running request completes.
With this change read throughput is improved by 16%.
Signed-off-by Konstantin Dorfman <[email protected]>---
v5: - RemovedBUG()andBUG_ON()with proper error handling- Turn on of is_waiting_last_req flag moved from queue.c into block.c (mmc_blk_issue_req())-
mmc_init_context_info()called frommmc_add_card(). It is common point for MMC, SD, SDIO.
Quoted 1 line
Thanks, pushed to mmc-next for 3.9.
Great Chris,
Thanks,
Signature · 4 lines
Konstantin Dorfman, QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
PATCH 1/2 mmc: core: fix permanent sleep of mmcqd during card removal
In reply to Konstantin DorfmanThis patch is derived from 'mmc: fix async request mechanism ...'. According as async transfer, a request is handled with twice mmc_start_req. When the card is removed, the request is actually not issued in the first mmc_start_req [__mmc_start_data_req]. And then mmc_wait_for_data_req_done will come in the next mmc_start_req. But there is no event for completions. wake_up_interruptible is needed in __mmc_start_data_req for the case of removed card.
--- drivers/mmc/core/core.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/mmc/core/core.c +1 -0
@@ -350,6 +350,7 @@ static int __mmc_start_data_req(struct mmc_host *host, struct mmc_request *mrq) mrq->host = host;if (mmc_card_removed (host->card)) { mrq->cmd->error = -ENOMEDIUM ; +mmc_wait_data_done (mrq);return -ENOMEDIUM ; }mmc_start_request (host, mrq);
Signature · 1 line
1.7.0.4
Re: [PATCH 1/2] mmc: core: fix permanent sleep of mmcqd during card removal
Great..
I also found this problem and sent the similar patch at mailing. But i think this patch is more generic.
Quoted 26 lines · Seungwon Jeon
This patch is derived from 'mmc: fix async request mechanism ...'. According as async transfer, a request is handled with twice
mmc_start_req. When the card is removed, the request is actually not issued in the firstmmc_start_req[__mmc_start_data_req]. And thenmmc_wait_for_data_req_donewill come in the nextmmc_start_req. But there is no event for completions.wake_up_interruptibleis needed in__mmc_start_data_reqfor the case of removed card.Signed-off-by Seungwon Jeon <[email protected]>--- drivers/mmc/core/core.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)drivers/mmc/core/core.c +1
-0@@ -350,6 +350,7 @@ static int __mmc_start_data_req(struct mmc_host *host, struct mmc_request *mrq) mrq->host = host;if (mmc_card_removed (host->card)) { mrq->cmd->error = -ENOMEDIUM ; +mmc_wait_data_done (mrq);return -ENOMEDIUM ; }mmc_start_request (host, mrq);
RE: [PATCH 1/2] mmc: core: fix permanent sleep of mmcqd during card removal
I hope that you find another patch.
[PATCH 2/2] mmc: block: don't start new request when the card is removed I'd appreciate if you can review it.
Thanks,
Seungwon Jeon
Quoted 39 lines · Jaehoon Chung
Great..
I also found this problem and sent the similar patch at mailing. But i think this patch is more generic.
Acked-by Jaehoon Chung <[email protected]>Quoted 19 lines · Seungwon Jeon
This patch is derived from 'mmc: fix async request mechanism ...'. According as async transfer, a request is handled with twice
mmc_start_req. When the card is removed, the request is actually not issued in the firstmmc_start_req[__mmc_start_data_req]. And thenmmc_wait_for_data_req_donewill come in the nextmmc_start_req. But there is no event for completions.wake_up_interruptibleis needed in__mmc_start_data_reqfor the case of removed card.Signed-off-by Seungwon Jeon <[email protected]>--- drivers/mmc/core/core.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)drivers/mmc/core/core.c +0
-0drivers/mmc/core/core.c static int __mmc_start_data_req(struct mmc_host *host, struct mmc_request *mrq) · line 356
Quoted 7 lines
drivers/mmc/core/core.c +1
-0@@ -350,6 +350,7 @@ static int __mmc_start_data_req(struct mmc_host *host, struct mmc_request *mrq) mrq->host = host;if (mmc_card_removed (host->card)) { mrq->cmd->error = -ENOMEDIUM ; +mmc_wait_data_done (mrq);return -ENOMEDIUM ; }mmc_start_request (host, mrq);--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mmc: core: fix permanent sleep of mmcqd during card removal
In reply to Seungwon JeonWorks for me.
Quoted 26 lines · Seungwon Jeon
This patch is derived from 'mmc: fix async request mechanism ...'. According as async transfer, a request is handled with twice
mmc_start_req. When the card is removed, the request is actually not issued in the firstmmc_start_req[__mmc_start_data_req]. And thenmmc_wait_for_data_req_donewill come in the nextmmc_start_req. But there is no event for completions.wake_up_interruptibleis needed in__mmc_start_data_reqfor the case of removed card.Signed-off-by Seungwon Jeon <[email protected]>--- drivers/mmc/core/core.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)drivers/mmc/core/core.c +1
-0@@ -350,6 +350,7 @@ static int __mmc_start_data_req(struct mmc_host *host, struct mmc_request *mrq) mrq->host = host;if (mmc_card_removed (host->card)) { mrq->cmd->error = -ENOMEDIUM ; +mmc_wait_data_done (mrq);return -ENOMEDIUM ; }mmc_start_request (host, mrq);
Signature · 4 lines
Konstantin Dorfman, QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 1/2] mmc: core: fix permanent sleep of mmcqd during card removal
In reply to Seungwon JeonHi,
Quoted 18 lines · Seungwon Jeon
This patch is derived from 'mmc: fix async request mechanism ...'. According as async transfer, a request is handled with twice
mmc_start_req. When the card is removed, the request is actually not issued in the firstmmc_start_req[__mmc_start_data_req]. And thenmmc_wait_for_data_req_donewill come in the nextmmc_start_req. But there is no event for completions.wake_up_interruptibleis needed in__mmc_start_data_reqfor the case of removed card.Signed-off-by Seungwon Jeon <[email protected]>--- drivers/mmc/core/core.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)drivers/mmc/core/core.c +0
-0
drivers/mmc/core/core.c static int __mmc_start_data_req(struct mmc_host *host, struct mmc_request *mrq) · line 356
Quoted 7 lines
drivers/mmc/core/core.c +1
-0@@ -350,6 +350,7 @@ static int __mmc_start_data_req(struct mmc_host *host, struct mmc_request *mrq) mrq->host = host;if (mmc_card_removed (host->card)) { mrq->cmd->error = -ENOMEDIUM ; +mmc_wait_data_done (mrq);return -ENOMEDIUM ; }mmc_start_request (host, mrq);
Thanks, pushed to mmc-next for 3.9.
- Chris.
Signature · 2 lines
Chris Ball <[email protected]> <http://printf.net/> One Laptop Per Child
RE: [PATCH 1/2] mmc: core: fix permanent sleep of mmcqd during card removal
In reply to Konstantin DorfmanHi Konstantin.
Could you check this patch with [2/2]?
[PATCH 2/2] mmc: block: don't start new request when the card is removed
mmcqd is often sleeping with acquiring the claim(mmc_claim_host) when a card is removed. As a result, mmc_rescan can be blocked for the insertion of a card newly. It's a dead lock.
Thanks,
Seungwon Jeon
Quoted 27 lines · Seungwon Jeon
This patch is derived from 'mmc: fix async request mechanism ...'. According as async transfer, a request is handled with twice
mmc_start_req. When the card is removed, the request is actually not issued in the firstmmc_start_req[__mmc_start_data_req]. And thenmmc_wait_for_data_req_donewill come in the nextmmc_start_req. But there is no event for completions.wake_up_interruptibleis needed in__mmc_start_data_reqfor the case of removed card.Signed-off-by Seungwon Jeon <[email protected]>--- drivers/mmc/core/core.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)drivers/mmc/core/core.c +1
-1@@ -350,6 +350,7 @@ static int __mmc_start_data_req(struct mmc_host *host, struct mmc_request *mrq) mrq->host = host;if (mmc_card_removed (host->card)) { mrq->cmd->error = -ENOMEDIUM ; +mmc_wait_data_done (mrq);return -ENOMEDIUM ; }mmc_start_request (host, mrq); --1.7.0.4
Subhash Jadavani
Reviewed-byRe: [PATCH 1/2] mmc: core: fix permanent sleep of mmcqd during card removal
Looks good to me.
Quoted 44 lines · Seungwon Jeon
Hi Konstantin.
Could you check this patch with [2/2]?
[PATCH 2/2] mmc: block: don't start new request when the card is removedmmcqd is often sleeping with acquiring the
claim(mmc_claim_host)when a card is removed. As a result,mmc_rescancan be blocked for the insertion of a card newly. It's a dead lock.Thanks,
Seungwon JeonQuoted 26 lines · Seungwon Jeon
This patch is derived from 'mmc: fix async request mechanism ...'. According as async transfer, a request is handled with twice
mmc_start_req. When the card is removed, the request is actually not issued in the firstmmc_start_req[__mmc_start_data_req]. And thenmmc_wait_for_data_req_donewill come in the nextmmc_start_req. But there is no event for completions.wake_up_interruptibleis needed in__mmc_start_data_reqfor the case of removed card.Signed-off-by Seungwon Jeon <[email protected]>--- drivers/mmc/core/core.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)drivers/mmc/core/core.c +1
-1@@ -350,6 +350,7 @@ static int __mmc_start_data_req(struct mmc_host *host, struct mmc_request *mrq) mrq->host = host;if (mmc_card_removed (host->card)) { mrq->cmd->error = -ENOMEDIUM ; +mmc_wait_data_done (mrq);return -ENOMEDIUM ; }mmc_start_request (host, mrq); --Quoted 1 line
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mmc: core: fix permanent sleep of mmcqd during card removal
In reply to Seungwon JeonQuoted 5 lines · Seungwon Jeon
Hi Konstantin.
Could you check this patch with [2/2]?
[PATCH 2/2] mmc: block: don't start new request when the card is removed
Quoted 14 lines
mmcqd is often sleeping with acquiring the
claim(mmc_claim_host)when a card is removed. As a result,mmc_rescancan be blocked for the insertion of a card newly. It's a dead lock.Thanks,
Seungwon JeonQuoted 7 lines · Seungwon Jeon
This patch is derived from 'mmc: fix async request mechanism ...'. According as async transfer, a request is handled with twice
mmc_start_req. When the card is removed, the request is actually not issued in the firstmmc_start_req[__mmc_start_data_req]. And thenmmc_wait_for_data_req_donewill come in the nextmmc_start_req. But there is no event for completions.wake_up_interruptibleis needed in__mmc_start_data_reqfor the case of removed card.
Hi Seungwon,
I looked at this again and i guess there is something wrong with mmc_start_req() itself.
As per your commit text, first call to mmc_start_req() calls the __mmc_start_data_req() function and __mmc_start_data_req() returns the -ENOMEDIUM error (as card is removed) without starting the request on host controller. so now in mmc_start_req(), "start_err" should be set. But currently mmc_start_req() incorrectly marks the "host->areq" to "areq" in even if the start_err is set which i guess is wrong. what do you think about it?
So how about this fix? I guess this is better and it should fix the deadlock issue as well. Do let me know you thoughts on this. If it looks reasonable, i can post the formal patch.
drivers/mmc/core/core.c +0 -0
@@ -546,7 +546,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host
*host,
if ((err || start_err) && areq)mmc_post_req(host, areq->mrq, -EINVAL);
- if (err)
+ if (err || start_err)host->areq = NULL;
elsehost->areq = areq;
Regards,
Subhash
Quoted 25 lines
Quoted 19 lines
Signed-off-by Seungwon Jeon <[email protected]>--- drivers/mmc/core/core.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)drivers/mmc/core/core.c +1
-1@@ -350,6 +350,7 @@ static int __mmc_start_data_req(struct mmc_host *host, struct mmc_request *mrq) mrq->host = host;if (mmc_card_removed (host->card)) { mrq->cmd->error = -ENOMEDIUM ; +mmc_wait_data_done (mrq);return -ENOMEDIUM ; }mmc_start_request (host, mrq); --Quoted 1 line
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] mmc: core: fix permanent sleep of mmcqd during card removal
Quoted 33 lines · Subhash Jadavani
Quoted 5 lines · Seungwon Jeon
Hi Konstantin.
Could you check this patch with [2/2]?
[PATCH 2/2] mmc: block: don't start new request when the card is removedQuoted 14 lines
mmcqd is often sleeping with acquiring the
claim(mmc_claim_host)when a card is removed. As a result,mmc_rescancan be blocked for the insertion of a card newly. It's a dead lock.Thanks,
Seungwon JeonQuoted 7 lines · Seungwon Jeon
This patch is derived from 'mmc: fix async request mechanism ...'. According as async transfer, a request is handled with twice
mmc_start_req. When the card is removed, the request is actually not issued in the firstmmc_start_req[__mmc_start_data_req]. And thenmmc_wait_for_data_req_donewill come in the nextmmc_start_req. But there is no event for completions.wake_up_interruptibleis needed in__mmc_start_data_reqfor the case of removed card.Hi Seungwon,
I looked at this again and i guess there is something wrong with
mmc_start_req()itself.
As per your commit text, first call tommc_start_req()calls the__mmc_start_data_req()function and__mmc_start_data_req()returns the-ENOMEDIUMerror (as card is removed) without starting the request on host controller. so now inmmc_start_req(), "start_err" should be set. But currentlymmc_start_req()incorrectly marks the "host->areq" to "areq" in even if thestart_erris set which i guess is wrong. what do you think about it?
Quoted 3 lines
So how about this fix? I guess this is better and it should fix the deadlock issue as well. Do let me know you thoughts on this. If it looks reasonable, i can post the formal patch.
Hi Subhash,
I tested your fix, but there is still problem.
I didn't look into the reason.
Thanks,
Seungwon Jeon
Quoted 50 lines
drivers/mmc/core/core.c +1
-1@@ -546,7 +546,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host, if ((err || start_err) && areq)mmc_post_req (host, areq->mrq, -EINVAL ); -if (err) +if (err || start_err) host->areq =NULL ;else
host->areq= areq;Regards,
SubhashQuoted 17 lines
Quoted 17 lines
Signed-off-by Seungwon Jeon <[email protected]>--- drivers/mmc/core/core.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)drivers/mmc/core/core.c +1
-0@@ -350,6 +350,7 @@ static int __mmc_start_data_req(struct mmc_host *host, struct mmc_request *mrq) mrq->host = host;if (mmc_card_removed (host->card)) { mrq->cmd->error = -ENOMEDIUM ; +mmc_wait_data_done (mrq);return -ENOMEDIUM ; }Quoted 8 lines
Quoted 3 lines
drivers/mmc/core/core.c +0
-1mmc_start_request (host, mrq); --1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mmc: core: fix permanent sleep of mmcqd during card removal
Hi Subhash,
As Mr. Seungwon mentioned, your patch didn't solve the dead-lock issue. I'm prefered to the seungwon's patch.
Best Regards,
Jaehoon Chung
Quoted 101 lines · Seungwon Jeon
Quoted 33 lines · Subhash Jadavani
Quoted 5 lines · Seungwon Jeon
Hi Konstantin.
Could you check this patch with [2/2]?
[PATCH 2/2] mmc: block: don't start new request when the card is removedQuoted 14 lines
mmcqd is often sleeping with acquiring the
claim(mmc_claim_host)when a card is removed. As a result,mmc_rescancan be blocked for the insertion of a card newly. It's a dead lock.Thanks,
Seungwon JeonQuoted 7 lines · Seungwon Jeon
This patch is derived from 'mmc: fix async request mechanism ...'. According as async transfer, a request is handled with twice
mmc_start_req. When the card is removed, the request is actually not issued in the firstmmc_start_req[__mmc_start_data_req]. And thenmmc_wait_for_data_req_donewill come in the nextmmc_start_req. But there is no event for completions.wake_up_interruptibleis needed in__mmc_start_data_reqfor the case of removed card.Hi Seungwon,
I looked at this again and i guess there is something wrong with
mmc_start_req()itself.
As per your commit text, first call tommc_start_req()calls the__mmc_start_data_req()function and__mmc_start_data_req()returns the-ENOMEDIUMerror (as card is removed) without starting the request on host controller. so now inmmc_start_req(), "start_err" should be set. But currentlymmc_start_req()incorrectly marks the "host->areq" to "areq" in even if thestart_erris set which i guess is wrong. what do you think about it?Quoted 3 lines
So how about this fix? I guess this is better and it should fix the deadlock issue as well. Do let me know you thoughts on this. If it looks reasonable, i can post the formal patch.
Hi Subhash,
I tested your fix, but there is still problem.
I didn't look into the reason.Thanks,
Seungwon JeonQuoted 46 lines
drivers/mmc/core/core.c +1
-1@@ -546,7 +546,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host, if ((err || start_err) && areq)mmc_post_req (host, areq->mrq, -EINVAL ); -if (err) +if (err || start_err) host->areq =NULL ;else
host->areq= areq;Regards,
SubhashQuoted 17 lines
Quoted 17 lines
Signed-off-by Seungwon Jeon <[email protected]>--- drivers/mmc/core/core.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)drivers/mmc/core/core.c +1
-0@@ -350,6 +350,7 @@ static int __mmc_start_data_req(struct mmc_host *host, struct mmc_request *mrq) mrq->host = host;if (mmc_card_removed (host->card)) { mrq->cmd->error = -ENOMEDIUM ; +mmc_wait_data_done (mrq);return -ENOMEDIUM ; }Quoted 8 lines
Quoted 3 lines
drivers/mmc/core/core.c +0
-1mmc_start_request (host, mrq); --1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.htmlQuoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mmc: core: fix permanent sleep of mmcqd during card removal
Quoted 4 lines · Jaehoon Chung
Hi Subhash,
As Mr. Seungwon mentioned, your patch didn't solve the dead-lock issue. I'm prefered to the seungwon's patch.
Yes, i got the problem with the patch. I guess it's better to g ahead with Seungwon's patch.
Looks good to me. Reviewed-by: Subhash Jadavani <[email protected]>
Quoted 99 lines
Best Regards,
Jaehoon ChungQuoted 94 lines · Seungwon Jeon
Quoted 32 lines · Subhash Jadavani
Quoted 5 lines · Seungwon Jeon
Hi Konstantin.
Could you check this patch with [2/2]?
[PATCH 2/2] mmc: block: don't start new request when the card is removedQuoted 14 lines
mmcqd is often sleeping with acquiring the
claim(mmc_claim_host)when a card is removed. As a result,mmc_rescancan be blocked for the insertion of a card newly. It's a dead lock.Thanks,
Seungwon JeonQuoted 7 lines · Seungwon Jeon
This patch is derived from 'mmc: fix async request mechanism ...'. According as async transfer, a request is handled with twice
mmc_start_req. When the card is removed, the request is actually not issued in the firstmmc_start_req[__mmc_start_data_req]. And thenmmc_wait_for_data_req_donewill come in the nextmmc_start_req. But there is no event for completions.wake_up_interruptibleis needed in__mmc_start_data_reqfor the case of removed card.Hi Seungwon,
I looked at this again and i guess there is something wrong with
mmc_start_req()itself.
As per your commit text, first call tommc_start_req()calls the__mmc_start_data_req()function and__mmc_start_data_req()returns the-ENOMEDIUMerror (as card is removed) without starting the request on host controller. so now inmmc_start_req(), "start_err" should be set. But currentlymmc_start_req()incorrectly marks the "host->areq" to "areq" in even if thestart_erris set which i guess is wrong. what do you think about it?Quoted 3 lines
So how about this fix? I guess this is better and it should fix the deadlock issue as well. Do let me know you thoughts on this. If it looks reasonable, i can post the formal patch.
Hi Subhash,
I tested your fix, but there is still problem.
I didn't look into the reason.Thanks,
Seungwon JeonQuoted 42 lines
drivers/mmc/core/core.c +1
-1@@ -546,7 +546,7 @@ struct mmc_async_req *mmc_start_req(struct mmc_host *host, if ((err || start_err) && areq)mmc_post_req (host, areq->mrq, -EINVAL ); -if (err) +if (err || start_err) host->areq =NULL ;else
host->areq= areq;Regards,
SubhashQuoted 15 lines
Quoted 15 lines
Signed-off-by Seungwon Jeon <[email protected]>--- drivers/mmc/core/core.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)drivers/mmc/core/core.c +1
-0@@ -350,6 +350,7 @@ static int __mmc_start_data_req(struct mmc_host *host, struct mmc_request *mrq) mrq->host = host;if (mmc_card_removed (host->card)) { mrq->cmd->error = -ENOMEDIUM ; +mmc_wait_data_done (mrq);return -ENOMEDIUM ;Quoted 8 lines
Quoted 4 lines
drivers/mmc/core/core.c +0
-1}mmc_start_request (host, mrq); --1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.htmlQuoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mmc: core: fix permanent sleep of mmcqd during card removal
In reply to Seungwon JeonHello Jeon,
I'm working on this and will post results.
Thanks,
Quoted 46 lines · Seungwon Jeon
Hi Konstantin.
Could you check this patch with [2/2]?
[PATCH 2/2] mmc: block: don't start new request when the card is removedmmcqd is often sleeping with acquiring the
claim(mmc_claim_host)when a card is removed. As a result,mmc_rescancan be blocked for the insertion of a card newly. It's a dead lock.Thanks,
Seungwon JeonQuoted 26 lines · Seungwon Jeon
This patch is derived from 'mmc: fix async request mechanism ...'. According as async transfer, a request is handled with twice
mmc_start_req. When the card is removed, the request is actually not issued in the firstmmc_start_req[__mmc_start_data_req]. And thenmmc_wait_for_data_req_donewill come in the nextmmc_start_req. But there is no event for completions.wake_up_interruptibleis needed in__mmc_start_data_reqfor the case of removed card.Signed-off-by Seungwon Jeon <[email protected]>--- drivers/mmc/core/core.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)drivers/mmc/core/core.c +1
-1@@ -350,6 +350,7 @@ static int __mmc_start_data_req(struct mmc_host *host, struct mmc_request *mrq) mrq->host = host;if (mmc_card_removed (host->card)) { mrq->cmd->error = -ENOMEDIUM ; +mmc_wait_data_done (mrq);return -ENOMEDIUM ; }mmc_start_request (host, mrq); --Quoted 1 line
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 4 lines
Konstantin Dorfman, QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
v5 cover Add support for periodic BKOPS
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 can be set by the host controller. If this time is not set, a default time is used. If the card raised an exception with need for urgent BKOPS (level 2/3) a flag will be set to indicate MMC to start the BKOPS activity when it becomes idle.
Since running the BKOPS too often can impact the eMMC endurance, the card need for BKOPS is not checked every time MMC is idle (despite of cases of exception raised). In order to estimate when is the best time to check for BKOPS need the host will take into account the card capacity and percentages of changed sectors in the card. A future enhancement can be to check the card need for BKOPS only in case of random activity.
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:
- 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 v5:
- Do not allow BKOPS operation in all levels to be blocking
- Change the periodic check for BKOPS need to be based on percentage of changed sector
- Add BKOPS statistics
Changes in v4:
- Separate the polling for BKOPS completion to a different patch
- add a flag to indicate if polling for card completion is required
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
Maya Erez (3):
mmc: core: Add support for idle time BKOPS
mmc: allow the host controller to poll for BKOPS completion
mmc: core: Add MMC BKOPS statistics and debugfs ability to print them
Documentation/mmc/mmc-dev-attrs.txt | 9 + drivers/mmc/card/block.c | 96 ++++++++++++- drivers/mmc/card/queue.c | 2 + drivers/mmc/core/bus.c | 2 + drivers/mmc/core/core.c | 286 +++++++++++++++++++++++++++++++---- drivers/mmc/core/debugfs.c | 114 ++++++++++++++ drivers/mmc/core/mmc.c | 20 +++ include/linux/mmc/card.h | 64 ++++++++- include/linux/mmc/core.h | 5 + include/linux/mmc/host.h | 2 +- 10 files changed, 563 insertions(+), 37 deletions(-)
Signature · 4 lines
1.7.3.3 -- QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH v5 0/3] Add support for periodic BKOPS
Hi Maya,
I didn't see about "[PATCH v5 1/3]mmc: core: Add support for idle time BKOPS" at mailing. Could you check the patch set?
Best Regards,
Jaehoon Chung
Quoted 72 lines · Maya Erez
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 updatingBKOPS_STATUS(EXT_CSDbyte [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 can be set by the host controller. If this time is not set, a default time is used. If the card raised an exception with need for urgent BKOPS (level 2/3) a flag will be set to indicate MMC to start the BKOPS activity when it becomes idle.
Since running the BKOPS too often can impact the eMMC endurance, the card need for BKOPS is not checked every time MMC is idle (despite of cases of exception raised). In order to estimate when is the best time to check for BKOPS need the host will take into account the card capacity and percentages of changed sectors in the card. A future enhancement can be to check the card need for BKOPS only in case of random activity.
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:
- Sincemmc_start_bkopsis called from two contexts now,mmc_claim_hostwas moved to the beginning of the function
- Also, the check ofdoing_bkopsshould be protected when determing if an HPI is needed due to the same reason.Changes in v5:
- Do not allow BKOPS operation in all levels to be blocking
- Change the periodic check for BKOPS need to be based on percentage of changed sector
- Add BKOPS statisticsChanges in v4:
- Separate the polling for BKOPS completion to a different patch
- add a flag to indicate if polling for card completion is requiredChanges in v3:
- Move the call tostop_bkopstoblock.c.
This allows us to remove themmc_claim_hostfrom inside the function and doesn't cause additional degradation due to un-neccessary calim host operationChanges in v2:
- Check the number of written / discarded sectors as the trigger for checking the BKOPS need.
- Code review fixesMaya Erez (3):
mmc: core: Add support for idle time BKOPS
mmc: allow the host controller to poll for BKOPS completion
mmc: core: Add MMC BKOPS statistics and debugfs ability to print themDocumentation/mmc/mmc-dev-attrs.txt | 9 + drivers/mmc/card/block.c | 96 ++++++++++++- drivers/mmc/card/queue.c | 2 + drivers/mmc/core/bus.c | 2 + drivers/mmc/core/core.c | 286 +++++++++++++++++++++++++++++++---- drivers/mmc/core/debugfs.c | 114 ++++++++++++++ drivers/mmc/core/mmc.c | 20 +++ include/linux/mmc/card.h | 64 ++++++++- include/linux/mmc/core.h | 5 + include/linux/mmc/host.h | 2 +- 10 files changed, 563 insertions(+), 37 deletions(-)
cover mmc: Support FFU for eMMC v5.0
he Field Firmware Update (FFU) feature is new for eMMC 5.0 spec (Jedec:
JESD84-B50.pdf)
http://www.jedec.org/standards-documents/technology-focus-areas/flash-
memory-ssds-ufs-emmc/e-mmc
*New ioctl has been add:
* [Alex Lemberg] "MMC_FFU_INVOKE - transfer the new Firmware data from user spac[Alex Lemberg]space to the eMMC deivce and install the new image[Alex Lemberg] firmware.
* This solution allows to:
- Complete eMMC 5.0 FFU procedure as an atomic operation, without being interrupted by other IO requests
- Not limited Firmware data size. Using Multiple Write operations.
- Support of both EXT_CSD_MODE_OPERATION_CODES modes [Alex Lemberg]
* The solution is using "udev" device manager to transfer FW data from user space to eMMC driver [Alex Lemberg]
* Pre-existing functions from mmc_test were used in this solution.
Avi Shchislowski (3):
mmc: Support FFU for eMMC v5.0
mmc: Support FFU for eMMC v5.0
mmc: Support FFU for eMMC v5.0
drivers/mmc/card/Kconfig | 8 + drivers/mmc/card/block.c | 5 + drivers/mmc/card/mmc_test.c | 97 +-------- drivers/mmc/core/Makefile | 1 + drivers/mmc/core/core.c | 128 ++++++++++++ drivers/mmc/core/mmc.c | 9 + drivers/mmc/core/mmc_ffu.c | 487 +++++++++++++++++++++++++++++++++++++++++++ include/linux/mmc/card.h | 2 + include/linux/mmc/core.h | 30 +++ include/linux/mmc/mmc.h | 9 + 10 files changed, 690 insertions(+), 86 deletions(-) create mode 100644 drivers/mmc/core/mmc_ffu.c
Signature · 1 line
1.7.9.5
Re: [PATCH 0/3] mmc: Support FFU for eMMC v5.0
Hi,
Subjects of Patch[1/3~3/3] are same, it can be confused.
Best Regards,
Jaehoon Chung
Quoted 37 lines · Avi Shchislowski
he Field Firmware Update (FFU) feature is new for eMMC 5.0 spec (Jedec:
JESD84-B50.pdf)http://www.jedec.org/standards-documents/technology-focus-areas/flash-
memory-ssds-ufs-emmc/e-mmc*New ioctl has been add:
* [Alex Lemberg] "MMC_FFU_INVOKE- transfer the new Firmware data from user spac[Alex Lemberg]space to the eMMC deivce and install the new image[Alex Lemberg] firmware.
* This solution allows to:
- Complete eMMC 5.0 FFU procedure as an atomic operation, without being interrupted by other IO requests
- Not limited Firmware data size. Using Multiple Write operations.
- Support of bothEXT_CSD_MODE_OPERATION_CODESmodes [Alex Lemberg]
* The solution is using "udev" device manager to transfer FW data from user space to eMMC driver [Alex Lemberg]
* Pre-existing functions frommmc_testwere used in this solution.Signed-off-by Avi Shchislowski <[email protected]>Signed-off-by Alex Lemberg <[email protected]>Avi Shchislowski (3):
mmc: Support FFU for eMMC v5.0
mmc: Support FFU for eMMC v5.0
mmc: Support FFU for eMMC v5.0drivers/mmc/card/Kconfig | 8 + drivers/mmc/card/block.c | 5 + drivers/mmc/card/mmc_test.c | 97 +-------- drivers/mmc/core/Makefile | 1 + drivers/mmc/core/core.c | 128 ++++++++++++ drivers/mmc/core/mmc.c | 9 + drivers/mmc/core/mmc_ffu.c | 487 +++++++++++++++++++++++++++++++++++++++++++ include/linux/mmc/card.h | 2 + include/linux/mmc/core.h | 30 +++ include/linux/mmc/mmc.h | 9 + 10 files changed, 690 insertions(+), 86 deletions(-) create mode 100644 drivers/mmc/core/mmc_ffu.c
PATCH 1/3 mmc: Support FFU for eMMC v5.0
Add eMMC5.0 ffu ext_csd fields
drivers/mmc/core/mmc.c +9 -0
@@ -603,6 +603,15 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) card->ext_csd.data_sector_size =512 ; } +/* eMMC v5 or later */ +if (card->ext_csd.rev >=7 ) { + card->ext_csd.ffu_capable = + ((ext_csd[EXT_CSD_SUPPORTED_MODE ] &0x1 ) ==0x1 ) && + ((ext_csd[EXT_CSD_FW_CONFIG ] &0x1 ) ==0x0 ); + card->ext_csd.ffu_mode_op = ext_csd[EXT_CSD_FFU_FEATURES ]; + }else { + card->ext_csd.ffu_capable = false; + } out:return err; }
include/linux/mmc/card.h +2 -0
@@ -81,6 +81,8 @@ struct mmc_ext_csd {bool hpi_en;/* HPI enablebit */ bool hpi;/* HPI support bit */ unsigned int hpi_cmd;/* cmd used as HPI */ +bool ffu_capable;/* FFU support */ +bool ffu_mode_op;/* FFU mode operation */ bool bkops;/* background support bit */ bool bkops_en;/* background enable bit */ unsigned int data_sector_size;/* 512 bytes or 4KB */
include/linux/mmc/mmc.h +9 -0
@@ -272,6 +272,9 @@ struct _mmc_csd {* EXT_CSD fields */ +#define EXT_CSD_FFU_STATUS 26/* R */ +#define EXT_CSD_MODE_OPERATION_CODES 29/* W */ +#define EXT_CSD_MODE_CONFIG 30/* R/W */ #define EXT_CSD_FLUSH_CACHE 32/* W */ #define EXT_CSD_CACHE_CTRL 33/* R/W */ #define EXT_CSD_POWER_OFF_NOTIFICATION 34/* R/W */
@@ -290,6 +293,7 @@ struct _mmc_csd {#define EXT_CSD_SANITIZE_START 165/* W */ #define EXT_CSD_WR_REL_PARAM 166/* RO */ #define EXT_CSD_RPMB_MULT 168/* RO */ +#define EXT_CSD_FW_CONFIG 169/* R/W */ #define EXT_CSD_BOOT_WP 173/* R/W */ #define EXT_CSD_ERASE_GROUP_DEF 175/* R/W */ #define EXT_CSD_PART_CONFIG 179/* R/W */
@@ -326,6 +330,11 @@ struct _mmc_csd {#define EXT_CSD_GENERIC_CMD6_TIME 248/* RO */ #define EXT_CSD_CACHE_SIZE 249/* RO, 4 bytes */ #define EXT_CSD_PWR_CL_DDR_200_360 253/* RO */ +#define EXT_CSD_NUM_OF_FW_SEC_PROG 302/* RO, 4 bytes */ +#define EXT_CSD_FFU_ARG 487/* RO, 4 bytes */ +#define EXT_CSD_OPERATION_CODE_TIMEOUT 491/* RO */ +#define EXT_CSD_FFU_FEATURES 492/* RO */ +#define EXT_CSD_SUPPORTED_MODE 493/* RO */ #define EXT_CSD_TAG_UNIT_SIZE 498/* RO */ #define EXT_CSD_DATA_TAG_SUPPORT 499/* RO */ #define EXT_CSD_MAX_PACKED_WRITES 500/* RO */
Signature · 1 line
1.7.9.5
Re: [PATCH 1/3] mmc: Support FFU for eMMC v5.0
Hi,
how did you make the patch?
This patch didn't apply at Ulf's repository.
Quoted 12 lines · Avi Shchislowski
Add eMMC5.0 ffu
ext_csdfieldsSigned-off-by Avi Shchislowski <[email protected]>Signed-off-by Alex Lemberg <[email protected]>drivers/mmc/core/mmc.c +0
-0
drivers/mmc/core/mmc.c static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) · line 610
Quoted 8 lines
drivers/mmc/core/mmc.c +5
-0@@ -603,6 +603,15 @@ static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) card->ext_csd.data_sector_size =512 ; } +/* eMMC v5 or later */ +if (card->ext_csd.rev >=7 ) { + card->ext_csd.ffu_capable = + ((ext_csd[EXT_CSD_SUPPORTED_MODE ] &0x1 ) ==0x1 ) && + ((ext_csd[EXT_CSD_FW_CONFIG ] &0x1 ) ==0x0 );
if (ext_csd[EXT_CSD_SUPPORTED_MODE] & 0x1) && !(ext_csd[EXT_CSD_FW_CONFIG] & 0x1))
card->ext_csd.ffu_capable = true;
drivers/mmc/core/mmc.c static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) · line 613
Quoted 3 lines
drivers/mmc/core/mmc.c +3
-0+ card->ext_csd.ffu_mode_op = ext_csd[EXT_CSD_FFU_FEATURES ]; + }else { + card->ext_csd.ffu_capable = false;
Can be set to false by default before checking whether card is eMMC v5.0 or not.
card->ext_csd.ffu_capable = false;
if (card->ext_csd.rev >= 7) {
...
}Best Regards,
Jaehoon Chung
Quoted 52 lines
drivers/mmc/core/mmc.c +1
-0+ } out:return err; }include/linux/mmc/card.h +2
-0@@ -81,6 +81,8 @@ struct mmc_ext_csd {bool hpi_en;/* HPI enablebit */ bool hpi;/* HPI support bit */ unsigned int hpi_cmd;/* cmd used as HPI */ +bool ffu_capable;/* FFU support */ +bool ffu_mode_op;/* FFU mode operation */ bool bkops;/* background support bit */ bool bkops_en;/* background enable bit */ unsigned int data_sector_size;/* 512 bytes or 4KB */ include/linux/mmc/mmc.h +9
-0@@ -272,6 +272,9 @@ struct _mmc_csd {* EXT_CSD fields */ +#define EXT_CSD_FFU_STATUS 26/* R */ +#define EXT_CSD_MODE_OPERATION_CODES 29/* W */ +#define EXT_CSD_MODE_CONFIG 30/* R/W */ #define EXT_CSD_FLUSH_CACHE 32/* W */ #define EXT_CSD_CACHE_CTRL 33/* R/W */ #define EXT_CSD_POWER_OFF_NOTIFICATION 34/* R/W */ @@ -290,6 +293,7 @@ struct _mmc_csd {#define EXT_CSD_SANITIZE_START 165/* W */ #define EXT_CSD_WR_REL_PARAM 166/* RO */ #define EXT_CSD_RPMB_MULT 168/* RO */ +#define EXT_CSD_FW_CONFIG 169/* R/W */ #define EXT_CSD_BOOT_WP 173/* R/W */ #define EXT_CSD_ERASE_GROUP_DEF 175/* R/W */ #define EXT_CSD_PART_CONFIG 179/* R/W */ @@ -326,6 +330,11 @@ struct _mmc_csd {#define EXT_CSD_GENERIC_CMD6_TIME 248/* RO */ #define EXT_CSD_CACHE_SIZE 249/* RO, 4 bytes */ #define EXT_CSD_PWR_CL_DDR_200_360 253/* RO */ +#define EXT_CSD_NUM_OF_FW_SEC_PROG 302/* RO, 4 bytes */ +#define EXT_CSD_FFU_ARG 487/* RO, 4 bytes */ +#define EXT_CSD_OPERATION_CODE_TIMEOUT 491/* RO */ +#define EXT_CSD_FFU_FEATURES 492/* RO */ +#define EXT_CSD_SUPPORTED_MODE 493/* RO */ #define EXT_CSD_TAG_UNIT_SIZE 498/* RO */ #define EXT_CSD_DATA_TAG_SUPPORT 499/* RO */ #define EXT_CSD_MAX_PACKED_WRITES 500/* RO */
RE: [PATCH 1/3] mmc: Support FFU for eMMC v5.0
Hi
I used the "git send-email..."
Thanks
Quoted 115 lines
-----Original Message-----
From: Jaehoon Chung [mailto:[email protected]]
Sent: Tuesday, November 11, 2014 5:08 AM
To: Avi Shchislowski; [email protected]
Cc: 'Chris Ball'; [email protected]; Alex Lemberg; [email protected]; open list; CPGS
Subject: Re: [PATCH 1/3] mmc: Support FFU for eMMC v5.0Hi,
how did you make the patch?
This patch didn't apply at Ulf's repository.drivers/mmc/core/mmc.c static int mmc_read_ext_csd(struct mmc_card
Quoted 12 lines · Avi Shchislowski
Add eMMC5.0 ffu
ext_csdfieldsSigned-off-by Avi Shchislowski <[email protected]>Signed-off-by Alex Lemberg <[email protected]>drivers/mmc/core/mmc.c index renamed from drivers/mmc/core/mmc.c +0
-01eda8dd..3b86ed3 100644
drivers/mmc/core/mmc.c +0
-0@@ -603,6 +603,15 @@ static int mmc_read_ext_csd(struct mmc_card*card, u8
*ext_csd)drivers/mmc/core/mmc.c static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd)
Quoted 7 lines
drivers/mmc/core/mmc.c +4
-0card->ext_csd.data_sector_size =512 ; } +/* eMMC v5 or later */ +if (card->ext_csd.rev >=7 ) { + card->ext_csd.ffu_capable = + ((ext_csd[EXT_CSD_SUPPORTED_MODE] & 0x1) ==
0x1) &&drivers/mmc/core/mmc.c static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) · line 610
Quoted 1 line
drivers/mmc/core/mmc.c +1
-0+ ((ext_csd[EXT_CSD_FW_CONFIG ] &0x1 ) ==0x0 );if (ext_csd[EXT_CSD_SUPPORTED_MODE] &
0x1)
&& !(ext_csd[EXT_CSD_FW_CONFIG] &0x1))card->ext_csd.ffu_capable = true;drivers/mmc/core/mmc.c static int mmc_read_ext_csd(struct mmc_card · line 611
Quoted 1 line
drivers/mmc/core/mmc.c +1
-0+ card->ext_csd.ffu_mode_op =ext_csd[EXT_CSD_FFU_FEATURES];
drivers/mmc/core/mmc.c static int mmc_read_ext_csd(struct mmc_card *card, u8 *ext_csd) · line 613
Quoted 2 lines
drivers/mmc/core/mmc.c +2
-0+ }else { + card->ext_csd.ffu_capable = false;Can be set to false by default before checking whether card is eMMC v5.0 or not.
card->ext_csd.ffu_capable= false;if (card->ext_csd.rev >= 7) { ... }Best Regards,
Jaehoon Chunginclude/linux/mmc/card.h struct mmc_ext_csd { · line 81
Quoted 10 lines
drivers/mmc/core/mmc.c +1
-0+ } out:return err; }include/linux/mmc/card.h index renamed from include/linux/mmc/card.h +0
-0d424b9d..dd01efe 100644
include/linux/mmc/card.h +0
-0@@ -81,6 +81,8 @@ struct mmc_ext_csd {bool hpi_en;/* HPI enablebit */
include/linux/mmc/card.h struct mmc_ext_csd {
Quoted 4 lines
include/linux/mmc/card.h +2
-0bool hpi;/* HPI support bit */ unsigned int hpi_cmd;/* cmd used as HPI */ +bool ffu_capable;/* FFU support */ +bool ffu_mode_op;/* FFU mode operation */
Quoted 26 lines
include/linux/mmc/card.h +0
-0bool bkops;/* background support bit */ bool bkops_en;/* background enable bit */ unsigned int data_sector_size;/* 512 bytes or 4KB */ include/linux/mmc/mmc.h index renamed from include/linux/mmc/mmc.h +0
-064ec963..787787b 100644
include/linux/mmc/mmc.h +4
-0@@ -272,6 +272,9 @@ struct _mmc_csd {* EXT_CSD fields */ +#define EXT_CSD_FFU_STATUS 26/* R */ +#define EXT_CSD_MODE_OPERATION_CODES 29/* W */ +#define EXT_CSD_MODE_CONFIG 30/* R/W */ #define EXT_CSD_FLUSH_CACHE 32/* W */ #define EXT_CSD_CACHE_CTRL 33/* R/W */ #define EXT_CSD_POWER_OFF_NOTIFICATION 34/* R/W */ @@ -290,6 +293,7 @@ struct _mmc_csd {#define EXT_CSD_SANITIZE_START 165/* W */ #define EXT_CSD_WR_REL_PARAM 166/* RO */ #define EXT_CSD_RPMB_MULT 168/* RO */ +#define EXT_CSD_FW_CONFIG 169/* R/W */ #define EXT_CSD_BOOT_WP 173/* R/W */ #define EXT_CSD_ERASE_GROUP_DEF 175/* R/W */ #define EXT_CSD_PART_CONFIG 179/* R/W */ include/linux/mmc/mmc.h struct _mmc_csd { · line 333
Quoted 4 lines
include/linux/mmc/mmc.h +1
-0@@ -326,6 +330,11 @@ struct _mmc_csd {#define EXT_CSD_GENERIC_CMD6_TIME 248/* RO */ #define EXT_CSD_CACHE_SIZE 249/* RO, 4 bytes */ #define EXT_CSD_PWR_CL_DDR_200_360 253/* RO */ +#define EXT_CSD_NUM_OF_FW_SEC_PROG 302/* RO, 4 bytes */
include/linux/mmc/mmc.h struct _mmc_csd { · line 334
Quoted 1 line
include/linux/mmc/mmc.h +1
-0+#define EXT_CSD_FFU_ARG 487/* RO, 4 bytes */
Quoted 7 lines
include/linux/mmc/mmc.h +3
-0+#define EXT_CSD_OPERATION_CODE_TIMEOUT 491/* RO */ +#define EXT_CSD_FFU_FEATURES 492/* RO */ +#define EXT_CSD_SUPPORTED_MODE 493/* RO */ #define EXT_CSD_TAG_UNIT_SIZE 498/* RO */ #define EXT_CSD_DATA_TAG_SUPPORT 499/* RO */ #define EXT_CSD_MAX_PACKED_WRITES 500/* RO */
PATCH 2/3 mmc: Support FFU for eMMC v5.0
eMMC5.0 FFU is using some of pre-existing mmc_test.c functionality.
In order to prevent code duplication, some code of mmc_test.c was moved to core.c and modified.
drivers/mmc/card/mmc_test.c +11 -86
@@ -13,6 +13,7 @@#include <linux/mmc/card.h> #include <linux/mmc/host.h> #include <linux/mmc/mmc.h> +#include <linux/mmc/core.h> #include <linux/slab.h> #include <linux/scatterlist.h>
@@ -189,43 +190,8 @@ static void mmc_test_prepare_mrq(struct mmc_test_card *test, struct mmc_request *mrq, struct scatterlist *sg, unsigned sg_len, unsigned dev_addr, unsigned blocks, unsigned blksz, int write) { - BUG_ON(!mrq || !mrq->cmd || !mrq->data || !mrq->stop); - -if (blocks >1 ) { - mrq->cmd->opcode = write ? -MMC_WRITE_MULTIPLE_BLOCK :MMC_READ_MULTIPLE_BLOCK ; - }else { - mrq->cmd->opcode = write ? -MMC_WRITE_BLOCK :MMC_READ_SINGLE_BLOCK ; - } - - mrq->cmd->arg = dev_addr; -if (!mmc_card_blockaddr (test->card)) - mrq->cmd->arg <<=9 ; - - mrq->cmd->flags =MMC_RSP_R1 |MMC_CMD_ADTC ; - -if (blocks ==1 ) - mrq->stop =NULL ; -else { - mrq->stop->opcode =MMC_STOP_TRANSMISSION ; - mrq->stop->arg =0 ; - mrq->stop->flags =MMC_RSP_R1B |MMC_CMD_AC ; - } - - mrq->data->blksz = blksz; - mrq->data->blocks = blocks; - mrq->data->flags = write ?MMC_DATA_WRITE :MMC_DATA_READ ; - mrq->data->sg = sg; - mrq->data->sg_len = sg_len; - -mmc_set_data_timeout (mrq->data, test->card); -} - -static int mmc_test_busy (struct mmc_command *cmd) -{ -return !(cmd->resp[0 ] &R1_READY_FOR_DATA ) || - (R1_CURRENT_STATE (cmd->resp[0 ]) ==R1_STATE_PRG ); + mmc_prepare_mrq(test->card, mrq, sg, sg_len, +dev_addr ,blocks ,blksz ,write ); }/*
@@ -233,30 +199,9 @@ static int mmc_test_busy(struct mmc_command *cmd)*/ static int mmc_test_wait_busy (struct mmc_test_card *test) { -int ret, busy; -struct mmc_command cmd = {0 }; - - busy =0 ; -do { -memset (&cmd,0 ,sizeof (struct mmc_command )); - - cmd.opcode =MMC_SEND_STATUS ; - cmd.arg = test->card->rca <<16 ; - cmd.flags =MMC_RSP_R1 |MMC_CMD_AC ; - - ret =mmc_wait_for_cmd (test->card->host, &cmd,0 ); -if (ret) -break ; - -if (!busy &&mmc_test_busy (&cmd)) { - busy =1 ; -if (test->card->host->caps &MMC_CAP_WAIT_WHILE_BUSY ) -pr_info ("%s: Warning: Host did not " -"wait for busy state to end.\n" , -mmc_hostname (test->card->host)); - } - }while (mmc_test_busy (&cmd)); +int ret; + ret =mmc_wait_busy (test->card);return ret; }
@@ -693,19 +638,9 @@ static int mmc_test_check_result(struct mmc_test_card *test, {int ret; -BUG_ON (!mrq || !mrq->cmd || !mrq->data); - ret =0 ; -if (!ret && mrq->cmd->error) - ret = mrq->cmd->error; -if (!ret && mrq->data->error) - ret = mrq->data->error; -if (!ret && mrq->stop && mrq->stop->error) - ret = mrq->stop->error; -if (!ret && mrq->data->bytes_xfered != - mrq->data->blocks * mrq->data->blksz) - ret =RESULT_FAIL ; + ret =mmc_check_result (mrq);if (ret == -EINVAL ) ret =RESULT_UNSUP_HOST ;
@@ -844,23 +779,13 @@ static int mmc_test_simple_transfer(struct mmc_test_card *test, struct scatterlist *sg, unsigned sg_len, unsigned dev_addr, unsigned blocks, unsigned blksz, int write) { - struct mmc_request mrq = {0}; -struct mmc_command cmd = {0 }; -struct mmc_command stop = {0 }; -struct mmc_data data = {0 }; - - mrq.cmd = &cmd; - mrq.data = &data; - mrq.stop = &stop; + int ret; -mmc_test_prepare_mrq (test, &mrq, sg, sg_len, dev_addr, + ret =mmc_simple_transfer (test->card, sg, sg_len, dev_addr, blocks, blksz, write); - -mmc_wait_for_req (test->card->host, &mrq); - -mmc_test_wait_busy (test); - -return mmc_test_check_result (test, &mrq); +if (ret == -EINVAL ) + ret =RESULT_UNSUP_HOST ; +return ret; }/*
drivers/mmc/core/core.c +128 -0
134 lines not on this page · raw message
include/linux/mmc/core.h +8 -0
@@ -196,6 +196,14 @@ extern void mmc_put_card(struct mmc_card *card);extern int mmc_flush_cache (struct mmc_card *);extern int mmc_detect_card_removed (struct mmc_host *host); +extern void mmc_prepare_mrq (struct mmc_card *card, +struct mmc_request *mrq,struct scatterlist *sg,unsigned sg_len, +unsigned dev_addr,unsigned blocks,unsigned blksz,int write); +extern int mmc_wait_busy (struct mmc_card *card); +extern int mmc_check_result (struct mmc_request *mrq); +extern int mmc_simple_transfer (struct mmc_card *card, +struct scatterlist *sg,unsigned sg_len,unsigned dev_addr, +unsigned blocks,unsigned blksz,int write);/** * mmc_claim_host - exclusively claim a host
Signature · 1 line
1.7.9.5
Re: [PATCH 2/3] mmc: Support FFU for eMMC v5.0
Hi,
Quoted 84 lines · Avi Shchislowski
eMMC5.0 FFU is using some of pre-existing
mmc_test.cfunctionality.
In order to prevent code duplication, some code ofmmc_test.cwas moved tocore.cand modified.Signed-off-by Avi Shchislowski <[email protected]>Signed-off-by Alex Lemberg <[email protected]>drivers/mmc/card/mmc_test.c +3
-53@@ -13,6 +13,7 @@#include <linux/mmc/card.h> #include <linux/mmc/host.h> #include <linux/mmc/mmc.h> +#include <linux/mmc/core.h> #include <linux/slab.h> #include <linux/scatterlist.h> @@ -189,43 +190,8 @@ static void mmc_test_prepare_mrq(struct mmc_test_card *test, struct mmc_request *mrq, struct scatterlist *sg, unsigned sg_len, unsigned dev_addr, unsigned blocks, unsigned blksz, int write) { - BUG_ON(!mrq || !mrq->cmd || !mrq->data || !mrq->stop); - -if (blocks >1 ) { - mrq->cmd->opcode = write ? -MMC_WRITE_MULTIPLE_BLOCK :MMC_READ_MULTIPLE_BLOCK ; - }else { - mrq->cmd->opcode = write ? -MMC_WRITE_BLOCK :MMC_READ_SINGLE_BLOCK ; - } - - mrq->cmd->arg = dev_addr; -if (!mmc_card_blockaddr (test->card)) - mrq->cmd->arg <<=9 ; - - mrq->cmd->flags =MMC_RSP_R1 |MMC_CMD_ADTC ; - -if (blocks ==1 ) - mrq->stop =NULL ; -else { - mrq->stop->opcode =MMC_STOP_TRANSMISSION ; - mrq->stop->arg =0 ; - mrq->stop->flags =MMC_RSP_R1B |MMC_CMD_AC ; - } - - mrq->data->blksz = blksz; - mrq->data->blocks = blocks; - mrq->data->flags = write ?MMC_DATA_WRITE :MMC_DATA_READ ; - mrq->data->sg = sg; - mrq->data->sg_len = sg_len; - -mmc_set_data_timeout (mrq->data, test->card); -} - -static int mmc_test_busy (struct mmc_command *cmd) -{ -return !(cmd->resp[0 ] &R1_READY_FOR_DATA ) || - (R1_CURRENT_STATE (cmd->resp[0 ]) ==R1_STATE_PRG ); + mmc_prepare_mrq(test->card, mrq, sg, sg_len, +dev_addr ,blocks ,blksz ,write ); }/* @@ -233,30 +199,9 @@ static int mmc_test_busy(struct mmc_command *cmd)*/ static int mmc_test_wait_busy (struct mmc_test_card *test) { -int ret, busy; -struct mmc_command cmd = {0 }; - - busy =0 ; - do { - memset(&cmd, 0, sizeof(struct mmc_command)); - - cmd.opcode = MMC_SEND_STATUS; - cmd.arg = test->card->rca << 16; - cmd.flags = MMC_RSP_R1 | MMC_CMD_AC; - - ret = mmc_wait_for_cmd(test->card->host, &cmd, 0); - if (ret) - break; - - if (!busy && mmc_test_busy(&cmd)) {
drivers/mmc/card/mmc_test.c static int mmc_test_busy(struct mmc_command *cmd) · line 204
Quoted 10 lines
drivers/mmc/card/mmc_test.c +2
-7- busy =1 ; -if (test->card->host->caps &MMC_CAP_WAIT_WHILE_BUSY ) -pr_info ("%s: Warning: Host did not " -"wait for busy state to end.\n" , -mmc_hostname (test->card->host)); - } - }while (mmc_test_busy (&cmd)); +int ret; + ret =mmc_wait_busy (test->card);
return mmc_wait_busy(test->card);
And mmc_test_wait_busy is wrapper function?
Why do you use the wrapper function?
Quoted 97 lines
drivers/mmc/card/mmc_test.c +6
-26return ret; }@@ -693,19 +638,9 @@ static int mmc_test_check_result(struct mmc_test_card *test, {int ret; -BUG_ON (!mrq || !mrq->cmd || !mrq->data); - ret =0 ; -if (!ret && mrq->cmd->error) - ret = mrq->cmd->error; -if (!ret && mrq->data->error) - ret = mrq->data->error; -if (!ret && mrq->stop && mrq->stop->error) - ret = mrq->stop->error; -if (!ret && mrq->data->bytes_xfered != - mrq->data->blocks * mrq->data->blksz) - ret =RESULT_FAIL ; + ret =mmc_check_result (mrq);if (ret == -EINVAL ) ret =RESULT_UNSUP_HOST ;@@ -844,23 +779,13 @@ static int mmc_test_simple_transfer(struct mmc_test_card *test, struct scatterlist *sg, unsigned sg_len, unsigned dev_addr, unsigned blocks, unsigned blksz, int write) { - struct mmc_request mrq = {0}; -struct mmc_command cmd = {0 }; -struct mmc_command stop = {0 }; -struct mmc_data data = {0 }; - - mrq.cmd = &cmd; - mrq.data = &data; - mrq.stop = &stop; + int ret; -mmc_test_prepare_mrq (test, &mrq, sg, sg_len, dev_addr, + ret =mmc_simple_transfer (test->card, sg, sg_len, dev_addr, blocks, blksz, write); - -mmc_wait_for_req (test->card->host, &mrq); - -mmc_test_wait_busy (test); - -return mmc_test_check_result (test, &mrq); +if (ret == -EINVAL ) + ret =RESULT_UNSUP_HOST ; +return ret; }/* drivers/mmc/core/core.c +36
-0@@ -2658,6 +2658,134 @@ int mmc_pm_notify(struct notifier_block *notify_block,return 0 ; }#endif +/* +* Fill in the mmc_request structure given a set of transfer parameters. +*/ +void mmc_prepare_mrq (struct mmc_card *card, +struct mmc_request *mrq,struct scatterlist *sg,unsigned sg_len, +unsigned dev_addr,unsigned blocks,unsigned blksz,int write) +{ +BUG_ON (!mrq || !mrq->cmd || !mrq->data || !mrq->stop); + +if (blocks >1 ) { + mrq->cmd->opcode = write ? +MMC_WRITE_MULTIPLE_BLOCK :MMC_READ_MULTIPLE_BLOCK ; + }else { + mrq->cmd->opcode = write ? +MMC_WRITE_BLOCK :MMC_READ_SINGLE_BLOCK ; + } + + mrq->cmd->arg = dev_addr; +if (!mmc_card_blockaddr (card)) + mrq->cmd->arg <<=9 ; + + mrq->cmd->flags =MMC_RSP_R1 |MMC_CMD_ADTC ; + +if (blocks ==1 ) + mrq->stop =NULL ; +else { + mrq->stop->opcode =MMC_STOP_TRANSMISSION ; + mrq->stop->arg =0 ; + mrq->stop->flags =MMC_RSP_R1B |MMC_CMD_AC ; + } + + mrq->data->blksz = blksz; + mrq->data->blocks = blocks; + mrq->data->flags = write ?MMC_DATA_WRITE :MMC_DATA_READ ; + mrq->data->sg = sg; + mrq->data->sg_len = sg_len;
drivers/mmc/core/core.c int mmc_pm_notify(struct notifier_block *notify_block, · line 2706
Quoted 10 lines
drivers/mmc/core/core.c +10
-0+ +mmc_set_data_timeout (mrq->data, card); +} +EXPORT_SYMBOL (mmc_prepare_mrq); + +static int mmc_busy (struct mmc_command *cmd) +{ +return !(cmd->resp[0 ] &R1_READY_FOR_DATA ) || + (R1_CURRENT_STATE (cmd->resp[0 ]) ==R1_STATE_PRG ); +}
What's function?
Quoted 31 lines
drivers/mmc/core/core.c +31
-0+ +/* +* Wait for the card to finish the busy state +*/ +int mmc_wait_busy (struct mmc_card *card) +{ +int ret, busy =0 ; +struct mmc_command cmd = {0 }; + +memset (&cmd,0 ,sizeof (struct mmc_command )); + cmd.opcode =MMC_SEND_STATUS ; + cmd.arg = card->rca <<16 ; + cmd.flags =MMC_RSP_SPI_R2 |MMC_RSP_R1 |MMC_CMD_AC ; + +do { + ret =mmc_wait_for_cmd (card->host, &cmd,0 ); +if (ret) +break ; + +if (!busy &&mmc_busy (&cmd)) { + busy =1 ; +if (card->host->caps &MMC_CAP_WAIT_WHILE_BUSY ) { +pr_warn ("%s: Warning: Host did not " +"wait for busy state to end.\n" , +mmc_hostname (card->host)); + } + } + + }while (mmc_busy (&cmd)); + +return ret;
drivers/mmc/core/core.c int mmc_pm_notify(struct notifier_block *notify_block, · line 2747
Quoted 10 lines
drivers/mmc/core/core.c +10
-0+} +EXPORT_SYMBOL (mmc_wait_busy); + +int mmc_check_result (struct mmc_request *mrq) +{ +int ret; + +BUG_ON (!mrq || !mrq->cmd || !mrq->data); + + ret =0 ;
Why do you initialized at here?
if ret is 0, why do you check "if (!ret && xxxx)" at the below condition?
Quoted 64 lines
drivers/mmc/core/core.c +41
-0+ +if (!ret && mrq->cmd->error) + ret = mrq->cmd->error; +if (!ret && mrq->data->error) + ret = mrq->data->error; +if (!ret && mrq->stop && mrq->stop->error) + ret = mrq->stop->error; +if (!ret && mrq->data->bytes_xfered != + mrq->data->blocks * mrq->data->blksz) + ret = -EPERM ; + +return ret; +} +EXPORT_SYMBOL (mmc_check_result); + +/* ++ * transfer with certain parameters +*/ +int mmc_simple_transfer (struct mmc_card *card, +struct scatterlist *sg,unsigned sg_len,unsigned dev_addr, +unsigned blocks,unsigned blksz,int write) +{ +struct mmc_request mrq = {0 }; +struct mmc_command cmd = {0 }; +struct mmc_command stop = {0 }; +struct mmc_data data = {0 }; + + mrq.cmd = &cmd; + mrq.data = &data; + mrq.stop = &stop; + +mmc_prepare_mrq (card, &mrq, sg, sg_len, dev_addr, + blocks, blksz, write); + +mmc_wait_for_req (card->host, &mrq); + +mmc_wait_busy (card); + +return mmc_check_result (&mrq); +} +EXPORT_SYMBOL (mmc_simple_transfer);/** * mmc_init_context_info() - init synchronization context include/linux/mmc/core.h +8
-0@@ -196,6 +196,14 @@ extern void mmc_put_card(struct mmc_card *card);extern int mmc_flush_cache (struct mmc_card *);extern int mmc_detect_card_removed (struct mmc_host *host); +extern void mmc_prepare_mrq (struct mmc_card *card, +struct mmc_request *mrq,struct scatterlist *sg,unsigned sg_len, +unsigned dev_addr,unsigned blocks,unsigned blksz,int write); +extern int mmc_wait_busy (struct mmc_card *card); +extern int mmc_check_result (struct mmc_request *mrq); +extern int mmc_simple_transfer (struct mmc_card *card, +struct scatterlist *sg,unsigned sg_len,unsigned dev_addr, +unsigned blocks,unsigned blksz,int write);/** * mmc_claim_host - exclusively claim a host