Re: [PATCH] mmc: mmci: Support non-power-of-two block sizes for ux500v2 variant
On Mon, Nov 26, 2012 at 11:52 AM, Per Förlin per.for...@stericsson.com wrote:
On 11/26/2012 11:27 AM, Russell King - ARM Linux wrote:
On Mon, Nov 26, 2012 at 11:20:32AM +0100, Per Förlin wrote:
On 11/22/2012 06:37 PM, Russell King - ARM Linux wrote:
On Thu, Nov 22, 2012 at 06:28:30PM +0100, Per Forlin wrote:
On Wed, Nov 21, 2012 at 5:50 PM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
On Wed, Nov 21, 2012 at 05:13:55PM +0100, Per Forlin wrote:
On Wed, Nov 21, 2012 at 4:38 PM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
On Fri, Oct 12, 2012 at 04:02:02PM +0200, Ulf Hansson wrote:
/*
+ * Validate mmc prerequisites
+ */
+static int mmci_validate_data(struct mmci_host *host,
+ struct mmc_data *data)
+{
+ if (!data)
+ return 0;
+
+ if (!host-variant-non_power_of_2_blksize
+ !is_power_of_2(data-blksz)) {
+ dev_err(mmc_dev(host-mmc),
+ unsupported block size (%d bytes)\n,
data-blksz);
+ return -EINVAL;
+ }
+
+ if (data-sg-offset 3) {
+ dev_err(mmc_dev(host-mmc),
+ unsupported alginment (0x%x)\n,
data-sg-offset);
+ return -EINVAL;
+ }
Why? What's the reasoning behind this suddenly introduced restriction?
readsl()/writesl() copes just fine with non-aligned pointers. It may
be
that your DMA engine can not, but that's no business interfering with
non-DMA transfers, and no reason to fail such transfers.
If your DMA engine can't do that then its your DMA engine code which
should refuse to prepare the transfer.
Yes, that means problems with the way things are ordered - or it needs
a
proper API where DMA engine can export these kinds of properties.
The alignment constraint is related to PIO, sg_miter and that FIFO
access must be done with 4 bytes.
Total claptrap. No it isn't.
- sg_miter just deals with bytes, and number of bytes transferred; there
is no word assumptions in that code. Indeed many ATA disks transfer
by half-word accesses so such a restriction would be insane.
- the FIFO access itself needs to be 32-bit words, so readsl or writesl
(or their io* equivalents must be used).
- but - and this is the killer item to your argument as I said above -
readsl and writesl _can_ take misaligned pointers and cope with them
fine.
The actual alignment of the buffer address is totally irrelevant here.
What isn't irrelevant is the _number_ of bytes to be transferred, but
that's something totally different and completely unrelated from
data-sg-offset.
Let's try again :)
Keep in mind that the mmc -block layer is aligned so from that aspect
everything is fine.
SDIO may have any length or alignment but sg-len is always 1.
There is just one sg-element and one continues buffer.
sg-miter splits the continues buffer in chunks that may not be aligned
with 4 byte size. It depends on the start address alignment of the
buffer.
Is it more clear now?
Is this more clear: you may be passed a single buffer which is misaligned.
We cope with that just fine. There is *no* reason to reject that transfer
because readsl/writesl cope just fine with it.
The MMCI driver doesn't support alignment smaller than 4 bytes (it may
result in data corruption).
Explain yourself. That's what's lacking here. I'm explaining why I
think you're wrong, but you're just asserting all the time that I'm
wrong without giving any real details.
There are two options
1. Either one should fix the driver to support it. Currently the driver
only supports miss-alignment of the last sg-miter buffer.
2. Or be kind to inform the user that the alignment is not supported.
Look, it's very very simple.
If you have a multi-sg transfer, and the pointer starts off being
misaligned, the first transfer to the end of the page _MAY_ be a
non-word aligned number of bytes. _THAT_ is what you should be checking.
_THAT_ is what the limitation is in the driver. _NOT_ that the pointer
is misaligned.
There will be no multi-sg transfer that is misaligned because SDIO-fwk set
the sg-length to 1. Still the transfer may be multi-PAGE with sg-length 1
which is something that mmci driver cannot handle.
The intention of data-sg-offset 3 is to check for misaligned data. What
would you replace this check with?
Thanks
Per
I have tried to work out an if-statement to check this properly. Here
is my conclusion,
This only works if sg len is 1 (in the SDIO case)
if (WRITE)
align = sg-offset 3
else if (READ)
align = 0
if (sg-offset 3 (PAGE_SIZE - (sg-offset + align) host-size))
error; /* cannot be handled by mmci driver */
Is this if-statement align with your view of the alignment issue ?
BR
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: mmci: Support non-power-of-two block sizes for ux500v2 variant
On Wed, Nov 21, 2012 at 5:50 PM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
On Wed, Nov 21, 2012 at 05:13:55PM +0100, Per Forlin wrote:
On Wed, Nov 21, 2012 at 4:38 PM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
On Fri, Oct 12, 2012 at 04:02:02PM +0200, Ulf Hansson wrote:
/*
+ * Validate mmc prerequisites
+ */
+static int mmci_validate_data(struct mmci_host *host,
+ struct mmc_data *data)
+{
+ if (!data)
+ return 0;
+
+ if (!host-variant-non_power_of_2_blksize
+ !is_power_of_2(data-blksz)) {
+ dev_err(mmc_dev(host-mmc),
+ unsupported block size (%d bytes)\n, data-blksz);
+ return -EINVAL;
+ }
+
+ if (data-sg-offset 3) {
+ dev_err(mmc_dev(host-mmc),
+ unsupported alginment (0x%x)\n, data-sg-offset);
+ return -EINVAL;
+ }
Why? What's the reasoning behind this suddenly introduced restriction?
readsl()/writesl() copes just fine with non-aligned pointers. It may be
that your DMA engine can not, but that's no business interfering with
non-DMA transfers, and no reason to fail such transfers.
If your DMA engine can't do that then its your DMA engine code which
should refuse to prepare the transfer.
Yes, that means problems with the way things are ordered - or it needs a
proper API where DMA engine can export these kinds of properties.
The alignment constraint is related to PIO, sg_miter and that FIFO
access must be done with 4 bytes.
Total claptrap. No it isn't.
- sg_miter just deals with bytes, and number of bytes transferred; there
is no word assumptions in that code. Indeed many ATA disks transfer
by half-word accesses so such a restriction would be insane.
- the FIFO access itself needs to be 32-bit words, so readsl or writesl
(or their io* equivalents must be used).
- but - and this is the killer item to your argument as I said above -
readsl and writesl _can_ take misaligned pointers and cope with them
fine.
The actual alignment of the buffer address is totally irrelevant here.
What isn't irrelevant is the _number_ of bytes to be transferred, but
that's something totally different and completely unrelated from
data-sg-offset.
Let's try again :)
Keep in mind that the mmc -block layer is aligned so from that aspect
everything is fine.
SDIO may have any length or alignment but sg-len is always 1.
There is just one sg-element and one continues buffer.
sg-miter splits the continues buffer in chunks that may not be aligned
with 4 byte size. It depends on the start address alignment of the
buffer.
Is it more clear now?
BR
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: mmci: Support non-power-of-two block sizes for ux500v2 variant
On Wed, Nov 21, 2012 at 4:38 PM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
On Fri, Oct 12, 2012 at 04:02:02PM +0200, Ulf Hansson wrote:
/*
+ * Validate mmc prerequisites
+ */
+static int mmci_validate_data(struct mmci_host *host,
+ struct mmc_data *data)
+{
+ if (!data)
+ return 0;
+
+ if (!host-variant-non_power_of_2_blksize
+ !is_power_of_2(data-blksz)) {
+ dev_err(mmc_dev(host-mmc),
+ unsupported block size (%d bytes)\n, data-blksz);
+ return -EINVAL;
+ }
+
+ if (data-sg-offset 3) {
+ dev_err(mmc_dev(host-mmc),
+ unsupported alginment (0x%x)\n, data-sg-offset);
+ return -EINVAL;
+ }
Why? What's the reasoning behind this suddenly introduced restriction?
readsl()/writesl() copes just fine with non-aligned pointers. It may be
that your DMA engine can not, but that's no business interfering with
non-DMA transfers, and no reason to fail such transfers.
If your DMA engine can't do that then its your DMA engine code which
should refuse to prepare the transfer.
Yes, that means problems with the way things are ordered - or it needs a
proper API where DMA engine can export these kinds of properties.
The alignment constraint is related to PIO, sg_miter and that FIFO
access must be done with 4 bytes.
For a 8k buffer sg miter may return 3 buffer
1. 7 bytes
2. 4096
3. 4089
DMA can handle this because it will treat this a one buffer being 8 k.
PIO will do three transfer due to sg_miter (7, 4096, 4089).
One could change the driver to not use sg_miter and just access the 8k
buffer directly to avoid the issue.
BR
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] mmc: fix async request mechanism for sequential read scenarios
On Tue, Oct 30, 2012 at 1:19 PM, Konstantin Dorfman
kdorf...@codeaurora.org wrote:
Hello,
On 10/29/2012 11:40 PM, Per Forlin wrote:
Hi,
I would like to move the focus of my concerns from root cause analysis
to the actual patch,
My first reflection is that the patch is relatively complex and some
of the code looks duplicated.
Would it be possible to simplify it and re-use the current execution flow.
Is the following flow feasible?
in mmc_start_req():
--
if (rqc == NULL not_resend)
wait_for_both_mmc_and_arrival_of_new_req
else
wait_only_for_mmc
if (arrival_of_new_req) {
set flag to indicate fetch-new_req
return NULL;
}
-
in queue.c:
if (fetch-new_req)
don't overwrite previous req.
BR
Per
You are talking about using both waiting mechanisms, old (wait on
completion) and new - wait_event_interruptible()? But how done()
callback, called on host controller irq context, will differentiate
which one is relevant for the request?
I think it is better to use single wait point for mmc thread.
I have sketch a patch to better explain my point. It's not tested it
barely compiles :)
The patch tries to introduce your feature and still keep the same code
path. And it exposes an API that could be used by SDIO as well.
The intention of my sketch patch is only to explain what I tried to
visualize in the pseudo code previously in this thread.
The out come of your final patch should be documented here I think:
Documentation/mmc/mmc-async-req.txt
Here follows my patch sketch:
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index e360a97..08036a1 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -66,6 +66,8 @@ static int mmc_queue_thread(void *d)
spin_unlock_irq(q-queue_lock);
if (req || mq-mqrq_prev-req) {
+ if (!req)
+ mmc_prefetch_start(mq-mqrq_prev-mmc_active,
true);
set_current_state(TASK_RUNNING);
mq-issue_fn(mq, req);
} else {
@@ -79,11 +81,13 @@ static int mmc_queue_thread(void *d)
}
/* Current request becomes previous request and vice versa. */
- mq-mqrq_prev-brq.mrq.data = NULL;
- mq-mqrq_prev-req = NULL;
- tmp = mq-mqrq_prev;
- mq-mqrq_prev = mq-mqrq_cur;
- mq-mqrq_cur = tmp;
+ if (!mmc_prefetch_pending(mq-mqrq_prev-mmc_active)) {
+ mq-mqrq_prev-brq.mrq.data = NULL;
+ mq-mqrq_prev-req = NULL;
+ tmp = mq-mqrq_prev;
+ mq-mqrq_prev = mq-mqrq_cur;
+ mq-mqrq_cur = tmp;
+ }
} while (1);
up(mq-thread_sem);
@@ -109,10 +113,44 @@ static void mmc_request_fn(struct request_queue *q)
return;
}
+ if (mq-prefetch_enable) {
+ spin_lock(mq-prefetch_lock);
+ if (mq-prefetch_completion)
+ complete(mq-prefetch_completion);
+ mq-prefetch_pending = true;
+ spin_unlock(mq-prefetch_lock);
+ }
+
if (!mq-mqrq_cur-req !mq-mqrq_prev-req)
wake_up_process(mq-thread);
}
+static void mmc_req_init(struct mmc_async_req *areq, struct completion *compl)
+{
+ struct mmc_queue *mq =
+ container_of(areq-prefetch, struct mmc_queue, prefetch);
+
+ spin_lock(mq-prefetch_lock);
+ mq-prefetch_completion = compl;
+ if (mq-prefetch_pending)
+ complete(mq-prefetch_completion);
+ spin_unlock(mq-prefetch_lock);
+}
+
+static void mmc_req_start(struct mmc_async_req *areq, bool enable)
+{
+ struct mmc_queue *mq =
+ container_of(areq-prefetch, struct mmc_queue, prefetch);
+ mq-prefetch_enable = enable;
+}
+
+static bool mmc_req_pending(struct mmc_async_req *areq)
+{
+ struct mmc_queue *mq =
+ container_of(areq-prefetch, struct mmc_queue, prefetch);
+ return mq-prefetch_pending;
+}
+
static struct scatterlist *mmc_alloc_sg(int sg_len, int *err)
{
struct scatterlist *sg;
@@ -166,6 +204,12 @@ int mmc_init_queue(struct mmc_queue *mq, struct
mmc_card *card,
int ret;
struct mmc_queue_req *mqrq_cur = mq-mqrq[0];
struct mmc_queue_req *mqrq_prev = mq-mqrq[1];
+ spin_lock_init(mq-prefetch_lock);
+ mq-prefetch.wait_req_init = mmc_req_init;
+ mq-prefetch.wait_req_start = mmc_req_start;
+ mq-prefetch.wait_req_pending = mmc_req_pending;
+ mqrq_cur-mmc_active.prefetch = mq-prefetch;
+ mqrq_prev-mmc_active.prefetch = mq-prefetch;
if (mmc_dev(host)-dma_mask *mmc_dev(host)-dma_mask)
limit = *mmc_dev(host)-dma_mask;
diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card
Re: [PATCH v1] mmc: fix async request mechanism for sequential read scenarios
Hi,
On Tue, Oct 30, 2012 at 1:19 PM, Konstantin Dorfman
kdorf...@codeaurora.org wrote:
Hello,
On 10/29/2012 11:40 PM, Per Forlin wrote:
Hi,
I would like to move the focus of my concerns from root cause analysis
to the actual patch,
My first reflection is that the patch is relatively complex and some
of the code looks duplicated.
Would it be possible to simplify it and re-use the current execution flow.
Is the following flow feasible?
in mmc_start_req():
--
if (rqc == NULL not_resend)
wait_for_both_mmc_and_arrival_of_new_req
else
wait_only_for_mmc
if (arrival_of_new_req) {
set flag to indicate fetch-new_req
return NULL;
}
-
in queue.c:
if (fetch-new_req)
don't overwrite previous req.
BR
Per
You are talking about using both waiting mechanisms, old (wait on
completion) and new - wait_event_interruptible()? But how done()
callback, called on host controller irq context, will differentiate
which one is relevant for the request?
I think it is better to use single wait point for mmc thread.
Also in future plans to add second reason to wake up mmc thread from
waiting: this is urgent_request - this notification about next-to-fetch
read request, that should be executed out-of-order, using new eMMC HPI
feature (to stop currently running request).
I will publish this patch soon.
Your idea with fetch_new_req flag is good, I'll try to simplify current
patch and lets talk again.
I have not thought this through entirely. But it would be nice to add
a new func() in areq to add support for this a new mechanism. If the
func() is NULL the normal flow will be executed. This could be used in
the SDIO case too. At least it should be possible to use only the core
API and it still make sense without a mmc block driver on top.
How to implement this wait function?
There should be one single wait point, I agree.
One could share the mmc completion perhaps through the new func().
This mean both mmc-queue and mmc-host and completes the same
completion.
When waking up from the completion one needs to stop the new
func-wait-point and check if a new request is available. If yes, fetch
a new request and next time don't re-init the completion (this would
overwrite the mmc-host complete value).
Let's talk again when you have a new patch set.
BR
Per
Thanks,
--
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] mmc: fix async request mechanism for sequential read scenarios
Hi,
I would like to move the focus of my concerns from root cause analysis
to the actual patch,
My first reflection is that the patch is relatively complex and some
of the code looks duplicated.
Would it be possible to simplify it and re-use the current execution flow.
Is the following flow feasible?
in mmc_start_req():
--
if (rqc == NULL not_resend)
wait_for_both_mmc_and_arrival_of_new_req
else
wait_only_for_mmc
if (arrival_of_new_req) {
set flag to indicate fetch-new_req
return NULL;
}
-
in queue.c:
if (fetch-new_req)
don't overwrite previous req.
BR
Per
On Sun, Oct 28, 2012 at 2:12 PM, Konstantin Dorfman
kdorf...@codeaurora.org wrote:
Hello,
On 10/26/2012 02:07 PM, Venkatraman S wrote:
Actually there could a lot of reasons why block layer or CFQ would not have
inserted the request into the queue. i.e. you can see a lot of exit paths
where blk_peek_request returns NULL, even though there could be any request
pending in one of the CFQ requests queues.
Essentially you need to check what makes blk_fetch_request
(cfq_dispatch_requests() ) return NULL when there is an element in
queue, if at all.
This is not important why block layer causes to blk_fetch_request() (or
blk_peek_request()) to return NULL to the MMC layer, but what is really
important - MMC layer should always be able to fetch the new arrived
request ASAP, after block layer notification (mmc_request() ) and this
is what my fix goes to implement.
And the fix is not changing block layer behavior.
Thanks
--
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: fix async request mechanism for sequential read scenarios
On Sun, Oct 14, 2012 at 6:17 PM, Konstantin Dorfman kdorf...@codeaurora.org wrote: On Thu, 11 Oct 2012 17:19:01 +0200, Per Forlin per.l...@gmail.com wrote: Hello Per, I would like to start with some basic comments. 1. Is this read sequential issue specific to MMC? 2. Or is it common with all other block-drivers that gets data from the block layer (SCSI/SATA etc) ? If (#2) can the issue be addressed inside the block layer instead? BR Per This issue specific to MMC, others block drivers probably not using MMC mechanism for async request (or have more kernel threads for processing incoming blk requests). I think, since MMC actively fetches requests from block layer queue, the solution has nothing to do with block layer context. On Tue, Oct 2, 2012 at 5:39 PM, Konstantin Dorfman kdorf...@codeaurora.org wrote: The main assumption of the async request design is that the file system adds block requests to the block device queue asynchronously without waiting for completion (see the Rationale section of https://wiki.linaro.org/WorkingGroups/Kernel/Specs /StoragePerfMMC-async-req). We found out that in case of sequential read operations this is not the case, due to the read ahead mechanism. Would it be possible to improve this mechanism to achieve the same result? Allow an outstanding read ahead request on top of the current ongoing one. I need to look on this mechanism, but from first glance such behaviour may be result of libc/vfs/fs decisions and too complex comparing to the patch we are talking about. One observation I have made is that if setting the mmc_req_size to half READ_AHEAD changes the way block layer adds request to the MMC queue. Extract from https://wiki.linaro.org/WorkingGroups/Kernel/Specs/StoragePerfMMC-async-req#Unresolved_issues Forcing mmc host driver to set mmc_req_size 64k results in this behaviour. dd if=/dev/mmcblk0 of=/dev/null bs=4k count=256 [mmc_queue_thread] req d955f9b0 blocks 32 [mmc_queue_thread] req (null) blocks 0 [mmc_queue_thread] req (null) blocks 0 [mmc_queue_thread] req d955f9b0 blocks 64 [mmc_queue_thread] req (null) blocks 0 [mmc_queue_thread] req d955f8d8 blocks 128 [mmc_queue_thread] req (null) blocks 0 [mmc_queue_thread] req d955f9b0 blocks 128 [mmc_queue_thread] req d955f800 blocks 128 [mmc_queue_thread] req d955f8d8 blocks 128 [mmc_queue_thread] req d955fec0 blocks 128 [mmc_queue_thread] req d955f800 blocks 128 [mmc_queue_thread] req d955f9b0 blocks 128 [mmc_queue_thread] req d967cd30 blocks 128 This shows that the block layer can add request in a more asynchronous manner. I have not investigate that mechanism enough to say what can be done. Do you have an explanation to why the block layer behaves like this? BR Per -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1] mmc: fix async request mechanism for sequential read scenarios
On Mon, Oct 15, 2012 at 5:36 PM, Konstantin Dorfman
kdorf...@codeaurora.org wrote:
The main assumption of the async request design is that the file
system adds block requests to the block device queue asynchronously
without waiting for completion (see the Rationale section of
https://wiki.linaro.org/WorkingGroups/Kernel/Specs
/StoragePerfMMC-async-req).
We found out that in case of sequential read operations this is not
the case, due to the read ahead mechanism.
When mmcqt reports on completion of a request there should be
a context switch to allow the insertion of the next read ahead BIOs
to the block layer. Since the mmcqd tries to fetch another request
immediately after the completion of the previous request it gets NULL
and starts waiting for the completion of the previous request.
This wait on completion gives the FS the opportunity to insert the next
request but the MMC layer is already blocked on the previous request
completion and is not aware of the new request waiting to be fetched.
I thought that I could trigger a context switch in order to give
execution time for FS to add the new request to the MMC queue.
I made a simple hack to call yield() in case the request gets NULL. I
thought it may give the FS layer enough time to add a new request to
the MMC queue. This would not delay the MMC transfer since the yield()
is done in parallel with an ongoing transfer. Anyway it was just meant
to be a simple test.
One yield was not enough. Just for sanity check I added a msleep as
well and that was enough to let FS add a new request,
Would it be possible to gain throughput by delaying the fetch of new
request? Too avoid unnecessary NULL requests
If (ongoing request is read AND size is max read ahead AND new request
is NULL) yield();
BR
Per
This patch fixes the above behavior and allows the async request mechanism
to work in case of sequential read scenarios.
The main idea is to replace the blocking wait for a completion with an
event driven mechanism and add a new event of new_request.
When the block layer notifies the MMC layer on a new request, we check
for the above case where MMC layer is waiting on a previous request
completion and the current request is NULL.
In such a case the new_request event will be triggered to wakeup
the waiting thread. MMC layer will then fetch the new request
and after its preparation will go back to waiting on the completion.
Our tests showed that this fix improves the read sequential throughput
by 16%.
Signed-off-by: Konstantin Dorfman kdorf...@codeaurora.org
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 172a768..4d6431b 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -112,17 +112,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);
@@ -1224,6 +1213,7 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req
*mqrq,
}
mqrq-mmc_active.mrq = brq-mrq;
+ mqrq-mmc_active.mrq-sync_data = mq-sync_data;
mqrq-mmc_active.err_check = mmc_blk_err_check;
mmc_queue_bounce_pre(mqrq);
@@ -1284,9 +1274,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_data_req(card-host, areq, (int *)status);
+ if (!areq) {
+ if (status == MMC_BLK_NEW_REQUEST)
+ return status;
return 0;
+ }
mq_rq = container_of(areq, struct mmc_queue_req, mmc_active);
brq = mq_rq-brq;
@@ -1295,6 +1288,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_ON(1); /* should never get here */
case MMC_BLK_SUCCESS:
case MMC_BLK_PARTIAL:
/*
@@ -1367,7 +1362,8 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq,
struct request *rqc)
* prepare it again and resend.
*/
mmc_blk_rw_rq_prep(mq_rq, card, disable_multi, mq);
- mmc_start_req(card-host, mq_rq-mmc_active, NULL);
+ mmc_start_data_req(card-host, mq_rq-mmc_active,
+
Re: [PATCH] mmc: fix async request mechanism for sequential read scenarios
Hi Konstantin,
Thanks for addressing this issue. I have just started to look at this
patch and I haven't got into the details yet.
I would like to start with some basic comments.
1. Is this read sequential issue specific to MMC?
2. Or is it common with all other block-drivers that gets data from
the block layer (SCSI/SATA etc) ?
If (#2) can the issue be addressed inside the block layer instead?
BR
Per
On Tue, Oct 2, 2012 at 5:39 PM, Konstantin Dorfman
kdorf...@codeaurora.org wrote:
The main assumption of the async request design is that the file
system adds block requests to the block device queue asynchronously
without waiting for completion (see the Rationale section of
https://wiki.linaro.org/WorkingGroups/Kernel/Specs
/StoragePerfMMC-async-req).
We found out that in case of sequential read operations this is not
the case, due to the read ahead mechanism.
Would it be possible to improve this mechanism to achieve the same result?
Allow an outstanding read ahead request on top of the current ongoing one.
When mmcqt reports on completion of a request there should be
a context switch to allow the insertion of the next read ahead BIOs
to the block layer. Since the mmcqd tries to fetch another request
immediately after the completion of the previous request it gets NULL
and starts waiting for the completion of the previous request.
This wait on completion gives the FS the opportunity to insert the next
request but the MMC layer is already blocked on the previous request
completion and is not aware of the new request waiting to be fetched.
This patch fixes the above behavior and allows the async request mechanism
to work in case of sequential read scenarios.
The main idea is to replace the blocking wait for a completion with an
event driven mechanism and add a new event of new_request.
When the block layer notifies the MMC layer on a new request, we check
for the above case where MMC layer is waiting on a previous request
completion and the current request is NULL.
In such a case the new_request event will be triggered to wakeup
the waiting thread. MMC layer will then fetch the new request
and after its preparation will go back to waiting on the completion.
Our tests showed that this fix improves the read sequential throughput
by 16%.
Signed-off-by: Konstantin Dorfman kdorf...@codeaurora.org
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 172a768..cb32d4c 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -112,17 +112,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);
@@ -1224,6 +1213,7 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req
*mqrq,
}
mqrq-mmc_active.mrq = brq-mrq;
+ mqrq-mmc_active.mrq-sync_data = mq-sync_data;
mqrq-mmc_active.err_check = mmc_blk_err_check;
mmc_queue_bounce_pre(mqrq);
@@ -1284,9 +1274,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_data_req(card-host, areq, (int *) status);
+ if (!areq) {
+ if (status == MMC_BLK_NEW_PACKET)
+ return status;
return 0;
+ }
mq_rq = container_of(areq, struct mmc_queue_req, mmc_active);
brq = mq_rq-brq;
@@ -1295,6 +1288,9 @@ 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_PACKET:
+ BUG_ON(1); /* should never get here */
+ return MMC_BLK_NEW_PACKET;
case MMC_BLK_SUCCESS:
case MMC_BLK_PARTIAL:
/*
@@ -1367,7 +1363,8 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq,
struct request *rqc)
* prepare it again and resend.
*/
mmc_blk_rw_rq_prep(mq_rq, card, disable_multi, mq);
- mmc_start_req(card-host, mq_rq-mmc_active, NULL);
+ mmc_start_req(card-host, mq_rq-mmc_active,
+ (int *) status);
}
} while (ret);
@@ -1382,7 +1379,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq,
struct request
Re: mmc: queue: amend buffer swap for non-blocking transfer
Hi,
This is really a micro optimization. Still the patch looks correct.
CUR and PREV will have the same values before and after this patch.
Before this patch, NULL was assigned to NULL which is not necessary of course.
Reviewed-by: Per Forlin per.for...@stericsson.com
Thanks,
Per
On Fri, Sep 28, 2012 at 12:12 PM, Seungwon Jeon tgih@samsung.com wrote:
In case both 'req' and 'mq-mqrq_prev-req' are null, there is no request
to be processed. That means there is no need to switch buffer.
Switching buffer is required only after finishing 'issue_fn'.
Signed-off-by: Seungwon Jeon tgih@samsung.com
---
drivers/mmc/card/queue.c | 17 ++---
1 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index e360a97..fadf52e 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -68,6 +68,16 @@ static int mmc_queue_thread(void *d)
if (req || mq-mqrq_prev-req) {
set_current_state(TASK_RUNNING);
mq-issue_fn(mq, req);
+
+ /*
+* Current request becomes previous request
+* and vice versa.
+*/
+ mq-mqrq_prev-brq.mrq.data = NULL;
+ mq-mqrq_prev-req = NULL;
+ tmp = mq-mqrq_prev;
+ mq-mqrq_prev = mq-mqrq_cur;
+ mq-mqrq_cur = tmp;
} else {
if (kthread_should_stop()) {
set_current_state(TASK_RUNNING);
@@ -77,13 +87,6 @@ static int mmc_queue_thread(void *d)
schedule();
down(mq-thread_sem);
}
-
- /* Current request becomes previous request and vice versa. */
- mq-mqrq_prev-brq.mrq.data = NULL;
- mq-mqrq_prev-req = NULL;
- tmp = mq-mqrq_prev;
- mq-mqrq_prev = mq-mqrq_cur;
- mq-mqrq_cur = tmp;
} while (1);
up(mq-thread_sem);
--
1.7.0.4
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] MMC-4.5 Power OFF Notify Rework
Hi Ulf,
On Fri, Jun 15, 2012 at 9:22 AM, Ulf Hansson ulf.hans...@stericsson.com wrote:
On 06/15/2012 05:49 AM, Saugata Das wrote:
On 15 June 2012 00:36, Per Forlinper.l...@gmail.com wrote:
Hi Saugata,
I can have a go and test it. But first I would like to bring up 3
concerns that I have with this patch.
1. This patch should be sent to cc-stable in order to repair the bug
introduced in 3.4
I shall sent it out today.
2. Is the bus_ops for poweroff_notify really necessary since only mmc
use it?
This was recommended in the review from Ulf. If it is not adding to a
bug, I propose to keep it this way. Otherwise, we will be going in
circles :-)
Moreover it seems close related to sleep, which is implemented with bus_ops.
So I would say, keep as is. We might change later, then both sleep and
poweroff_notify together.
There are already bus_ops for suspend/resume,
power_save/power_restore and remove. It feels like it would be
possible to address poweroff_notify internally from mmc.c from theses
bus_ops. I would be nice to add this feature without having to touch
core.c.
For instance. Call mmc_suspend() from mmc_remove()
+++ b/drivers/mmc/core/mmc.c
@@ -1302,7 +1302,7 @@ static void mmc_remove(struct mmc_host *host)
+ __mmc_suspend(host, true);
mmc_remove_card(host-card);
@@ -1347,7 +1347,7 @@ static void mmc_detect(struct mmc_host *host)
-static int mmc_suspend(struct mmc_host *host)
+static int __mmc_suspend(struct mmc_host *host, bool remove)
The remove parameter takes care of the remove case.
@@ -1356,7 +1356,8 @@ static int mmc_suspend(struct mmc_host *host)
mmc_claim_host(host);
if (mmc_can_poweroff_notify(host-card)
- (host-caps2 MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND)) {
+ (host-caps2 MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND ||
+ remove)) {
err = mmc_poweroff_notify(host, MMC_PW_OFF_NOTIFY_SHORT);
} else {
if (mmc_card_can_sleep(host))
@@ -1372,6 +1373,11 @@ static int mmc_suspend(struct mmc_host *host)
+static int mmc_suspend(struct mmc_host *host)
+{
+ return __mmc_suspend(host, false);
+}
+
Calling mmc_suspend from mmc_remove() has another advantage since it
may issue SLEEP (CMD5) before the card is removed and power off. This
is recommended by eMMC Vendors in order to shutdown the eMMC safely to
prevent data corruption. When the platform shuts down the power to the
eMMC will be turned off no matter what.
May be for MMC-4.41 cards this approach can be taken. For MMC-4.5, it
has to be power OFF notify when the power is removed. Lets do it for
another patch, since the intention of this patch is to fix the issues
around power OFF notify.
I agree with you Saugata, the exact same sequence as in suspend can not be
used. The reason is simply that we do not want to consider
MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND for doing poweroff_notify here, as we
want in suspend.
Leave this to be fixed in a separate patch instead.
I have addressed this in my prototype patch above. The remove param in
__suspend tells if we are removing or just suspending. This approach
actually removes lines of code in core.c mmc_stop_host().
3. About the sleep and awake issue. This is not really related to
poweroff_notify() as I see it. I would recommend to use CMD 0 to
re-init the card safely after sleep in this patch. Then you could send
out a sleep/awake patch that address this separately. This would also
make #1 easier, send patch to cc-stable. Adding save/restore IOS is a
new feature and should not be sent to the cc-stable list. What do you
think?
The problem in sending CMD0 without power OFF notify is possibility of
some data loss in MMC-4.5 devices.
I don't see the problem here. You will be sending power OFF notify when you
can. The only difference is when you wake the device from sleep mode.
Instead of using CMD5, which may work is some cases and in some cases not
(without restoring ios). So using CMD0 as common way of waking up from
suspend should be fine. Unless I missed something of course. :-)
I'm in favour of simplifying this patch. CMD0 instead of CMD5 reduces
the number of lines in this patch. It also make this patch work :)
Using CMD 5 to wake up could be done in a separate patch.
Bottom line. If the patch is clean and works I'm happy.
I can help out and test the patch.
Regards,
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] MMC-4.5 Power OFF Notify Rework
On Fri, Jun 15, 2012 at 10:34 AM, Saugata Das saugata@linaro.org wrote:
On 15 June 2012 12:52, Ulf Hansson ulf.hans...@stericsson.com wrote:
On 06/15/2012 05:49 AM, Saugata Das wrote:
On 15 June 2012 00:36, Per Forlinper.l...@gmail.com wrote:
Hi Saugata,
I can have a go and test it. But first I would like to bring up 3
concerns that I have with this patch.
1. This patch should be sent to cc-stable in order to repair the bug
introduced in 3.4
I shall sent it out today.
2. Is the bus_ops for poweroff_notify really necessary since only mmc
use it?
This was recommended in the review from Ulf. If it is not adding to a
bug, I propose to keep it this way. Otherwise, we will be going in
circles :-)
Moreover it seems close related to sleep, which is implemented with bus_ops.
So I would say, keep as is. We might change later, then both sleep and
poweroff_notify together.
There are already bus_ops for suspend/resume,
power_save/power_restore and remove. It feels like it would be
possible to address poweroff_notify internally from mmc.c from theses
bus_ops. I would be nice to add this feature without having to touch
core.c.
For instance. Call mmc_suspend() from mmc_remove()
+++ b/drivers/mmc/core/mmc.c
@@ -1302,7 +1302,7 @@ static void mmc_remove(struct mmc_host *host)
+ __mmc_suspend(host, true);
mmc_remove_card(host-card);
@@ -1347,7 +1347,7 @@ static void mmc_detect(struct mmc_host *host)
-static int mmc_suspend(struct mmc_host *host)
+static int __mmc_suspend(struct mmc_host *host, bool remove)
@@ -1356,7 +1356,8 @@ static int mmc_suspend(struct mmc_host *host)
mmc_claim_host(host);
if (mmc_can_poweroff_notify(host-card)
- (host-caps2 MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND)) {
+ (host-caps2 MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND ||
+ remove)) {
err = mmc_poweroff_notify(host, MMC_PW_OFF_NOTIFY_SHORT);
} else {
if (mmc_card_can_sleep(host))
@@ -1372,6 +1373,11 @@ static int mmc_suspend(struct mmc_host *host)
+static int mmc_suspend(struct mmc_host *host)
+{
+ return __mmc_suspend(host, false);
+}
+
Calling mmc_suspend from mmc_remove() has another advantage since it
may issue SLEEP (CMD5) before the card is removed and power off. This
is recommended by eMMC Vendors in order to shutdown the eMMC safely to
prevent data corruption. When the platform shuts down the power to the
eMMC will be turned off no matter what.
May be for MMC-4.41 cards this approach can be taken. For MMC-4.5, it
has to be power OFF notify when the power is removed. Lets do it for
another patch, since the intention of this patch is to fix the issues
around power OFF notify.
I agree with you Saugata, the exact same sequence as in suspend can not be
used. The reason is simply that we do not want to consider
MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND for doing poweroff_notify here, as we
want in suspend.
Leave this to be fixed in a separate patch instead.
3. About the sleep and awake issue. This is not really related to
poweroff_notify() as I see it. I would recommend to use CMD 0 to
re-init the card safely after sleep in this patch. Then you could send
out a sleep/awake patch that address this separately. This would also
make #1 easier, send patch to cc-stable. Adding save/restore IOS is a
new feature and should not be sent to the cc-stable list. What do you
think?
The problem in sending CMD0 without power OFF notify is possibility of
some data loss in MMC-4.5 devices.
I don't see the problem here. You will be sending power OFF notify when you
can. The only difference is when you wake the device from sleep mode.
Instead of using CMD5, which may work is some cases and in some cases not
(without restoring ios). So using CMD0 as common way of waking up from
suspend should be fine. Unless I missed something of course. :-)
CMD0 is a reset. I expect with power OFF notify enable, the eMMC
device will postpone some control information update to its internal
non-volatile memory (e.g. some data structures which are kept in the
controller buffers and not stored in NAND). If we do a CMD0, then the
eMMC device will be reset and we may lose some data. In addition to
that, doing complete card initialization will increase the wakeup time
(for 4.4 devices).
Till now, we have done complete card initialization during resume
Yes, me and Ulf think we should still do a complete initialization, at
least for now and in this patch.
A separate patch may deal with resume awake CMD5 and IOS save/restore.
We may also discuss a clean up patch later on to reduce the number of
bus_ops. Sleep, awake, and poweroff_notify are MMC specific.
power_save/power_restore maps to suspend/resume. But let's not discuss
this now :)
BR
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
Re: [PATCH v5] MMC-4.5 Power OFF Notify Rework
Hi Girish and Suagata,
I have run some regression tests with this patch on our board (ux500
family) running suspend and resume of the eMMC 4.41 device.
The two patches I have looked at are:
1. mmc: core: Fix PowerOff Notify suspend/resume (merged)
2. MMC-4.5 Power OFF Notify Rework
With only patch #1 the eMMC doesn't power up after in resume() after
being suspended. The eMMC doesn't respond at all after suspend. It's
not powered up.
Running tests with #1 and #2, the card is powered up but it doesn't
wake up after CMD5. Commands that arrive are after resume/CMD5
timeouts. I even tried by increasing the awake timeout to 5 seconds
but i didn't help.
The eMMC on my board successfully suspends and resumes with patch #1
and #2 if waking up the card using CMD0 (mmc_card_init()) instead of
CMD5.
Have anyone else seen the same issue?
Have this patch been verified on a board together with eMMC 4.41 that
supports card power off.
BR
Per
On Tue, May 22, 2012 at 1:45 PM, Girish K S
girish.shivananja...@linaro.org wrote:
From: Saugata Das saugata@linaro.org
This is a rework of the existing POWER OFF NOTIFY patch. The current problem
with the patch comes from the ambiguity on the usage of POWER OFF NOTIFY
together with SLEEP and misunderstanding on the usage of MMC_POWER_OFF
power_mode from mmc_set_ios in different host controller drivers.
This new patch works around this problem by adding a new host CAP,
MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND, which when set sends a
POWER OFF NOTIFY from mmc_suspend instead of SLEEP. It is expected that host
controller drivers will set this CAP, if they switch off both Vcc and Vccq
from MMC_POWER_OFF condition within mmc_set_ios. However, note that there
is no harm in sending MMC_POWER_NOTIFY even if Vccq is not switched off.
This patch also sends POWER OFF NOTIFY from power management routines (e.g.
mmc_power_save_host, mmc_pm_notify/PM_SUSPEND_PREPARE, mmc_stop_host), which
does reinitialization of the eMMC on the return path of the power management
routines (e.g. mmc_power_restore_host, mmc_pm_notify/PM_POST_RESTORE,
mmc_start_host).
This patch sets POWER_OFF_NOTIFICATION to POWER_OFF_SHORT if it is sent from
the suspend sequence. If it is sent from shutdown sequence then it is set to
POWER_OFF_LONG.
Earlier implementation of PowerOff Notify as a core function is replaced as
a device's bus operation.
Signed-off-by: Saugata Das saugata@linaro.org
Signed-off-by: Girish K S girish.shivananja...@linaro.org
changes in v5:
modified the the handling of return value in mmc_poweroff_notify.
changes in v4:
As suggested in review,
- Moved mmc_can_poweroff_notify to core.c
- Moved mmc_claim_host, mmc_release_host outside mmc_poweroff_notify
- Added check for wrong initialization for poweroff_notify_type
- mmc_poweroff_notify is modified to take as 2nd parameter
changes in v3:
This version addresses the review comments given by Subhash and Ulf
changes in v2:
This version addresses the changes suggested by Ulf
---
drivers/mmc/core/core.c | 108 ++--
drivers/mmc/core/core.h | 1 +
drivers/mmc/core/mmc.c | 56 ---
drivers/mmc/host/dw_mmc.c | 5 --
drivers/mmc/host/sdhci.c | 9
include/linux/mmc/card.h | 5 +-
include/linux/mmc/core.h | 1 +
include/linux/mmc/host.h | 5 +--
include/linux/mmc/mmc.h | 7 +++
9 files changed, 104 insertions(+), 93 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 0b6141d..fe616b9 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1101,48 +1101,6 @@ void mmc_set_driver_type(struct mmc_host *host,
unsigned int drv_type)
mmc_host_clk_release(host);
}
-static void mmc_poweroff_notify(struct mmc_host *host)
-{
- struct mmc_card *card;
- unsigned int timeout;
- unsigned int notify_type = EXT_CSD_NO_POWER_NOTIFICATION;
- int err = 0;
-
- card = host-card;
- mmc_claim_host(host);
-
- /*
- * Send power notify command only if card
- * is mmc and notify state is powered ON
- */
- if (card mmc_card_mmc(card)
- (card-poweroff_notify_state == MMC_POWERED_ON)) {
-
- if (host-power_notify_type == MMC_HOST_PW_NOTIFY_SHORT) {
- notify_type = EXT_CSD_POWER_OFF_SHORT;
- timeout = card-ext_csd.generic_cmd6_time;
- card-poweroff_notify_state = MMC_POWEROFF_SHORT;
- } else {
- notify_type = EXT_CSD_POWER_OFF_LONG;
- timeout = card-ext_csd.power_off_longtime;
- card-poweroff_notify_state = MMC_POWEROFF_LONG;
- }
-
- err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
-
Re: [PATCH v5] MMC-4.5 Power OFF Notify Rework
Hi Saugata,
I can have a go and test it. But first I would like to bring up 3
concerns that I have with this patch.
1. This patch should be sent to cc-stable in order to repair the bug
introduced in 3.4
2. Is the bus_ops for poweroff_notify really necessary since only mmc
use it? There are already bus_ops for suspend/resume,
power_save/power_restore and remove. It feels like it would be
possible to address poweroff_notify internally from mmc.c from theses
bus_ops. I would be nice to add this feature without having to touch
core.c.
For instance. Call mmc_suspend() from mmc_remove()
+++ b/drivers/mmc/core/mmc.c
@@ -1302,7 +1302,7 @@ static void mmc_remove(struct mmc_host *host)
+ __mmc_suspend(host, true);
mmc_remove_card(host-card);
@@ -1347,7 +1347,7 @@ static void mmc_detect(struct mmc_host *host)
-static int mmc_suspend(struct mmc_host *host)
+static int __mmc_suspend(struct mmc_host *host, bool remove)
@@ -1356,7 +1356,8 @@ static int mmc_suspend(struct mmc_host *host)
mmc_claim_host(host);
if (mmc_can_poweroff_notify(host-card)
- (host-caps2 MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND)) {
+ (host-caps2 MMC_CAP2_POWER_OFF_VCCQ_DURING_SUSPEND ||
+remove)) {
err = mmc_poweroff_notify(host, MMC_PW_OFF_NOTIFY_SHORT);
} else {
if (mmc_card_can_sleep(host))
@@ -1372,6 +1373,11 @@ static int mmc_suspend(struct mmc_host *host)
+static int mmc_suspend(struct mmc_host *host)
+{
+ return __mmc_suspend(host, false);
+}
+
Calling mmc_suspend from mmc_remove() has another advantage since it
may issue SLEEP (CMD5) before the card is removed and power off. This
is recommended by eMMC Vendors in order to shutdown the eMMC safely to
prevent data corruption. When the platform shuts down the power to the
eMMC will be turned off no matter what.
3. About the sleep and awake issue. This is not really related to
poweroff_notify() as I see it. I would recommend to use CMD 0 to
re-init the card safely after sleep in this patch. Then you could send
out a sleep/awake patch that address this separately. This would also
make #1 easier, send patch to cc-stable. Adding save/restore IOS is a
new feature and should not be sent to the cc-stable list. What do you
think?
BR
/Per
On Thu, Jun 14, 2012 at 5:15 PM, Saugata Das saugata@linaro.org wrote:
On 14 June 2012 20:20, Ulf Hansson ulf.hans...@linaro.org wrote:
Hi Girish,
On 14 June 2012 15:21, Girish K S girish.shivananja...@linaro.org wrote:
On 14 June 2012 18:43, Per Forlin per.l...@gmail.com wrote:
Hi Girish and Suagata,
I have run some regression tests with this patch on our board (ux500
family) running suspend and resume of the eMMC 4.41 device.
The two patches I have looked at are:
1. mmc: core: Fix PowerOff Notify suspend/resume (merged)
2. MMC-4.5 Power OFF Notify Rework
With only patch #1 the eMMC doesn't power up after in resume() after
being suspended. The eMMC doesn't respond at all after suspend. It's
not powered up.
Running tests with #1 and #2, the card is powered up but it doesn't
wake up after CMD5. Commands that arrive are after resume/CMD5
timeouts. I even tried by increasing the awake timeout to 5 seconds
but i didn't help.
The eMMC on my board successfully suspends and resumes with patch #1
and #2 if waking up the card using CMD0 (mmc_card_init()) instead of
CMD5.
Have anyone else seen the same issue?
Have this patch been verified on a board together with eMMC 4.41 that
supports card power off.
This rework patch is still under progress. we are modifying it. In our
earlier discussions subhash has posted the
same issue and a solution for this. we should save ios context before
sleep and restore ios before awake. soon rework patch will be
posted with the above recomenedded solution.
I think the best solution is to always do mmc_card_init when doing
resume, it will be nice a simple.
Note that, with power OFF notify (MMC-4.5), there will be some pending
operation with the MMC controller. If we do mmc_card_init after
suspend, then there could be some data loss.
I have passed to Per the latest patch (Subhash reported that it is
working). I shall forward to you as well. Lets solve the issue. If you
can work around, without mmc_card_init after suspend, then you are
most welcome to update the patch :-)
Otherwise it will be somewhat tricky to keep track of what state we
are in, and if the ios should be restored or not.
Finally, I would be glad to help out in posting an updated version of
this patch, if that OK with you?
Kind regards
Ulf Hansson
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: BUG/PATCH? mmcblk devices don't suspend properly.
= mmc_runtime_resume,
.runtime_idle = mmc_runtime_idle,
+ .suspend = mmc_bus_suspend,
+ .resume = mmc_bus_resume,
};
#define MMC_PM_OPS_PTR (mmc_bus_pm_ops)
however I suspect we should remove the 'legacy' pointers at the same
time.(?).
This was pointed out earlier and a patch is posted but looks like it never
went into mmc tree -- http://comments.gmane.org/gmane.linux.kernel.mmc/9168
Also, while exploring this problem I could not find anything that would
cause
mmc_bus_suspend() to wait for an async request to complete. Maybe it is
there somewhere that I don't understand yet, and I cannot be sure that any
of
my symptoms could be explained by an async request continuing while the
hardware was powered off, but I wonder if something like this might be
needed
too:
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index 2517547..cd36c30 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -354,6 +354,8 @@ void mmc_queue_suspend(struct mmc_queue *mq)
spin_unlock_irqrestore(q-queue_lock, flags);
down(mq-thread_sem);
+ /* wait for current request to complete */
+ mmc_start_req(mq-card-host, NULL, NULL);
This shouldn't be necessary.
}
This looks good to me but I would prefer Per Forlin to ack on it.
My interpretation of the code is that the thread_sem blocks until
the mmc_qeueu_thread() has finished all request in the queue.
I refer to this code:
if (req || mq-mqrq_prev-req) {
set_current_state(TASK_RUNNING);
mq-issue_fn(mq, req);
} else {
if (kthread_should_stop()) {
set_current_state(TASK_RUNNING);
break;
}
up(mq-thread_sem);
schedule();
down(mq-thread_sem);
}
--
mmc_queue_thread() will always finish off a sequence of requests by
issuing a NULL request to complete the ongoing async request,
If both the new req fetched from the queue and the ongoing (previous)
req are NULL, it will goto sleep.
At this point mmc_queue_thread will increase the sem, and wake up the
mmc_queue_suspend().
Regards,
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] MMC: mmci: Provide bindings for Device Tree
Hi, I would like to see what the minimal required change is to support DT for mmci without factorization. 1. Minimal change in mmci. 2. Add mmci_dt.c which contains the DT-populate code. The factorization could be done as step 2 I think. What do you say? BR Per On Wed, Mar 14, 2012 at 3:19 PM, Lee Jones lee.jo...@linaro.org wrote: This patch-set splits out ux500 and ARM specific variants from mmci core code, applies generic bindings for use by all variants, then enables the ux500 variant for use with Device Tree. Documentation/devicetree/bindings/mmc/mmci.txt | 27 +++ arch/arm/boot/dts/db8500.dtsi | 6 +- arch/arm/boot/dts/snowball.dts | 30 +++- arch/arm/mach-ux500/board-mop500.c | 3 +- drivers/mmc/host/Makefile | 2 +- drivers/mmc/host/mmci-arm.c | 65 ++ drivers/mmc/host/mmci-ux500.c | 260 drivers/mmc/host/mmci.c | 194 +- drivers/mmc/host/mmci.h | 37 9 files changed, 473 insertions(+), 151 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] MMC: mmci: Add generic Device Tree bindings to mmci core code
On Wed, Mar 14, 2012 at 3:20 PM, Lee Jones lee.jo...@linaro.org wrote:
This adds the necessary bindings for collection of shared
attributes used in the mmci driver.
Signed-off-by: Lee Jones lee.jo...@linaro.org
---
drivers/mmc/host/mmci.c | 43 +++
1 files changed, 43 insertions(+), 0 deletions(-)
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 23b41a5..9132ca8 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -30,6 +30,7 @@
#include linux/dma-mapping.h
#include linux/amba/mmci.h
#include linux/pm_runtime.h
+#include linux/of_gpio.h
#include asm/div64.h
#include asm/io.h
@@ -1056,11 +1057,47 @@ static const struct mmc_host_ops mmci_ops = {
.get_cd = mmci_get_cd,
};
+#ifdef CONFIG_OF
+static void mmci_dt_populate_generic_pdata(struct device_node *np,
+ struct mmci_platform_data *pdata)
+{
+ const void *prop;
+ int len;
+
+ of_property_read_u32(np, wp-gpios, pdata-gpio_wp);
+ if (!pdata-gpio_wp)
+ pdata-gpio_wp = -1;
+
+ of_property_read_u32(np, cd-gpios, pdata-gpio_cd);
+ if (!pdata-gpio_cd)
+ pdata-gpio_cd = -1;
+
+ if (of_get_property(np, cd-invert, NULL))
+ pdata-cd_invert = true;
+ else
+ pdata-cd_invert = false;
+
+ of_property_read_u32(np, clock_frequency, pdata-f_max);
+ if (!pdata-f_max)
+ pr_warning(%s has no 'clock_frequency' property\n,
np-full_name);
+
+ if (of_get_property(np, mmc_cap_4_bit_data, NULL))
I have no previous experience with DT. Could you please bring some
light on this.
Is it really necessary to represent each bit in the CAP with a string?
To add CAP_ERASE for instance I need to change the code here and
update the DT, right?
BR
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] MMC: mmci: Provide bindings for Device Tree
On Thu, Mar 15, 2012 at 4:32 PM, Arnd Bergmann a...@arndb.de wrote: On Thursday 15 March 2012, Lee Jones wrote: I would like to see what the minimal required change is to support DT for mmci without factorization. 1. Minimal change in mmci. 2. Add mmci_dt.c which contains the DT-populate code. The factorization could be done as step 2 I think. What do you say? I'm wondering what the difference is as the work has already been done. It was Arnd's suggestion to separate out the two types of variants, and I'm quite fond of the new (fully featured) layout. Right, I usually prefer cleanups or other refactoring to be done first, and then features added on top. You could in theory add have just patches 3/4/5 all applied without the refactoring, but that I would be worried that this causes dependencies between the mmci driver and ux500 specific functionality like the stedma40_filter function. About DT I think I need to catch up on the DT-design a bit. I will try to catch up on the DT-implementation and maybe then the refactorization will make sense to me :) Board specific data in mmci-driver The stedma40 filter function is not really specific for ux500. ux500 use stedma40 but it should be possible to replace this DMA.IP with some other DMA-controller. This is board specific configuration. You should not need to change the mmci-driver just because the dma-driver has changed, right? Or will the board-configuration now be placed in mmci-ux500? Common DT populate code for all mmc host drivers Some parts of the DT-population is common for all the host driver, for instance setting the CAP and CAP2. This code should be moved to a common place to be used by other host drivers as well. About refactoring There are numbers of patches stashed up for MMCI waiting for verdict here http://git.kernel.org/?p=linux/kernel/git/linusw/linux-stericsson.git;a=shortlog;h=refs/heads/mmci. If doing a refactorization please rebase it on top of these patches. This needs to be done carefully, there may be more to considerations than just DT. Maybe the timing is better to do this for 4.5, just after 4.4 is closed. Then we can make sure that all new patches are made on top of the refactorized base. BR Per -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: core: Ensure clocks are always enabled before host interaction
On Tue, Jan 24, 2012 at 4:44 AM, Sujit Reddy Thumma sthu...@codeaurora.org wrote: Hi Linus Walleij, On 12/30/2011 7:44 AM, Linus Walleij wrote: On Mon, Dec 12, 2011 at 9:21 AM, Sujit Reddy Thumma sthu...@codeaurora.org wrote: Ensure clocks are always enabled before any interaction with the host controller driver. This makes sure that there is no race between host execution and the core layer turning off clocks in different context with clock gating framework. Signed-off-by: Sujit Reddy Thummasthu...@codeaurora.org I guess Per Förlin may not be available, but would have preferred to have his view on this as well, since he knows the semantics of pre/post-req. I have checked the implementation for pre-req and post-req in mmc host drivers. There is no interaction to the controller or card registers in these functions, but in future if drivers appeal to configure their controller in these functions then we must have clocks enabled. Per, if you are available can you comment on this? I just got back from 2 months of leave so I apologize for not being up to date. It makes sense to ensure clocking in pre-req() and post-req() implemented by this patch. My only concern from a throughput point of view is if the clocking adds an overhead, but AFAIK the clocking doesn't add an overhead (that would increase the prepare time). Currently the pre-req() and post-req() only prepares DMA for next transfer without interacting with the controller. The intention is to increase throughput by minimizing start latency for the next transfer. It's perfectly ok to do other useful preparations in these hooks as well. The hooks are generic. It should be possible to do clock dependent stuff in these hooks too, it's up to the host driver to do what's best. Acked-by: Per Forlin per.for...@stericsson.com Thanks, Per -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: block: release host in case of error
On Fri, Nov 25, 2011 at 1:03 PM, Adrian Hunter adrian.hun...@intel.com wrote:
On 24/11/11 20:58, Per Forlin wrote:
On Sun, Nov 20, 2011 at 9:50 PM, Per Forlin per.l...@gmail.com wrote:
Hi Adrian,
On Fri, Nov 18, 2011 at 10:56 AM, Per Förlin per.for...@stericsson.com
wrote:
On 11/17/2011 10:18 AM, Adrian Hunter wrote:
On 14/11/11 13:12, Per Forlin wrote:
Host is claimed as long as there are requests in the block queue
and all request are completed successfully. If an error occur release
the host in case someone else needs to claim it, for instance if the card
is removed during a transfer.
Signed-off-by: Per Forlin per.for...@stericsson.com
---
drivers/mmc/card/block.c | 37 +
1 files changed, 29 insertions(+), 8 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index c80bb6d..c21fd2c 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1158,6 +1158,28 @@ static int mmc_blk_cmd_err(struct mmc_blk_data
*md, struct mmc_card *card,
return ret;
}
+/*
+ * This function should be called to resend a request after failure.
+ * Prepares and starts the request.
+ */
+static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card
*card,
+ struct mmc_queue *mq,
+ struct mmc_queue_req
*mqrq,
+ int disable_multi,
+ struct mmc_async_req
*areq)
+{
+ /*
+ * Release host after failure in case the host is needed
+ * by someone else. For instance, if the card is removed the
+ * worker thread needs to claim the host in order to do mmc_rescan.
+ */
+ mmc_release_host(card-host);
+ mmc_claim_host(card-host);
Does this work? Won't the current thread win the race
to claim the host again?
Good question. I've tested it and I haven't seen any cases where current
has claimed the host again. Sujit has tested the patch as well.
But I can't say that your scenario can't happen. I will study the wake_up
and wait_queue code to see if I can find the answer.
mmc_release_host() - wake_up() - schedule(). If the waking process
has higher prio than current it will preempt current on NOSMP. If SMP,
current and waking process may be on separate CPUs and in that case
it's difficult to guarantee that the waking process will win the race.
I'm proposing to add yield() in order to give the waking process
better chances to win the race.
Here's a patch:
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index c21fd2c..add1c38 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1173,8 +1173,11 @@ static inline struct mmc_async_req
*mmc_blk_resend(struct mmc_card *card,
* by someone else. For instance, if the card is removed the
* worker thread needs to claim the host in order to do mmc_rescan.
*/
- mmc_release_host(card-host);
- mmc_claim_host(card-host);
+ if (mmc_card_rescan(card)) {
+ mmc_release_host(card-host);
+ yield();
+ mmc_claim_host(card-host);
+ }
mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
return mmc_start_req(card-host, areq, NULL);
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 271efea..83f03a3 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2059,6 +2059,8 @@ void mmc_rescan(struct work_struct *work)
if (host-rescan_disable)
return;
+ mmc_card_set_rescan(host-card);
+
/*
@@ -2101,6 +2103,7 @@
out:
+ mmc_card_clr_rescan(host-card);
}
---
I'm not sure if this patch-extension is really needed, it may only
make the patch more complex. If the race condition Adrian refers to is
unlikely, there may be a few extra retries before mmc_rescan get the
chance to claim the host.
I'm in favor of skipping my proposed extension and staying with the
original v1 patch.
Adrian, what do you say?
As far as I can see, if mmc block is checking / setting whether the
card has been removed, then mmc_blk_resend would not be needed.
I agree. The intention of this patch is only top let mmc_rescan claim the host.
Flow: card detect IRQ - mmc_detect_change - mmc_rescan - mmc_claim_host
If doing this check in mmc block instead this patch is not needed.
Let's wait and see what comes out of the patch mmc: Kill block
requests if card is removed.
Thanks,
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: core: Kill block requests if card is removed
Hi David, On Tue, Nov 22, 2011 at 9:18 PM, David Taylor dmtay...@skytex.net wrote: Sujit Reddy Thumma sthumma at codeaurora.org writes: Kill block requests when the host knows that the card is removed from the slot and is sure that subsequent requests are bound to fail. Do this silently so that the block layer doesn't output unnecessary error messages. This patch implements suggestion from Adrian Hunter, http://thread.gmane.org/gmane.linux.kernel.mmc/2714/focus=3474 Signed-off-by: Sujit Reddy Thumma sthumma at codeaurora.org --- drivers/mmc/card/queue.c | 5 + drivers/mmc/core/bus.c | 2 ++ include/linux/mmc/card.h | 3 +++ 3 files changed, 10 insertions(+), 0 deletions(-) Thanks, this patch worked nicely to control a seemingly endless series of driver complaints when the SD card was removed during a transfer. The OMAP4 I'm working on has hardware assisted detection of card removal, and this patch looks like it will be sufficient. What happens if you run dd if=/dev/zero of=/dev/mmcblk0 bs=1M count=1000 and during dd the card is ejected? When I test this, the host is claimed until the dd has tried to transfer all 1000MB. Regards, Per -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] mmc: Kill block requests if card is removed
On Thu, Nov 24, 2011 at 12:27 PM, Sujit Reddy Thumma
sthu...@codeaurora.org wrote:
Hi Per,
On 11/22/2011 6:40 PM, Per Forlin wrote:
Hi Sujit,
On Tue, Nov 22, 2011 at 11:56 AM, Sujit Reddy Thumma
sthu...@codeaurora.org wrote:
Kill block requests when the host knows that the card is
removed from the slot and is sure that subsequent requests
are bound to fail. Do this silently so that the block
layer doesn't output unnecessary error messages.
This patch implements suggestion from Adrian Hunter,
http://thread.gmane.org/gmane.linux.kernel.mmc/2714/focus=3474
Signed-off-by: Sujit Reddy Thummasthu...@codeaurora.org
---
Changes in v2:
- Changed the implementation with further comments from Adrian
- Set the card removed flag in bus notifier callbacks
- This patch is now dependent on patch from Per Forlin:
http://thread.gmane.org/gmane.linux.kernel.mmc/11128/focus=11211
---
drivers/mmc/card/block.c | 33 -
drivers/mmc/card/queue.c | 5 +
drivers/mmc/core/bus.c | 32 +++-
drivers/mmc/core/core.c | 8 +++-
include/linux/mmc/card.h | 3 +++
5 files changed, 74 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index edc379e..83956fa 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -648,8 +648,15 @@ static int mmc_blk_cmd_recovery(struct mmc_card
*card, struct request *req,
}
/* We couldn't get a response from the card. Give up. */
- if (err)
+ if (err) {
+ /*
+ * If the card didn't respond to status command,
+ * it is likely that card is gone. Flag it as removed,
+ * mmc_detect_change() cleans the rest.
+ */
+ mmc_card_set_removed(card);
return ERR_ABORT;
+ }
/* Flag ECC errors */
if ((status R1_CARD_ECC_FAILED) ||
@@ -1168,6 +1175,9 @@ static inline struct mmc_async_req
*mmc_blk_resend(struct mmc_card *card,
int disable_multi,
struct mmc_async_req
*areq)
{
+ struct mmc_blk_data *md = mmc_get_drvdata(card);
+ struct request *req = mqrq-req;
+ int ret;
/*
* Release host after failure in case the host is needed
* by someone else. For instance, if the card is removed the
@@ -1175,11 +1185,24 @@ static inline struct mmc_async_req
*mmc_blk_resend(struct mmc_card *card,
*/
mmc_release_host(card-host);
mmc_claim_host(card-host);
-
- mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
- return mmc_start_req(card-host, areq, NULL);
+ if (mmc_card_removed(card)) {
+ /*
+ * End the pending async request without starting
+ * it when card is removed.
+ */
+ spin_lock_irq(md-lock);
+ req-cmd_flags |= REQ_QUIET;
+ do {
+ ret = __blk_end_request(req,
+ -EIO, blk_rq_cur_bytes(req));
+ } while (ret);
+ spin_unlock_irq(md-lock);
+ return NULL;
+ } else {
+ mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
+ return mmc_start_req(card-host, areq, NULL);
+ }
mmc_blk_resend is only called to resend a request that has failed. If
the card is removed the request will still be issued, but when it
retries it will give up here.
Currently, we set the card_removed flag in two places:
1) If the host supports interrupt detection, mmc_detect_change() calls the
bus detect method and we set card removed flag in bus notifier call back.
2) When there is a transfer going on (host is claimed by mmcqd) and the card
is removed ungracefully, the driver sends an error code to the block layer.
mmc_blk_cmd_recovery() tries to send CMD13 to the card which in this case
fails and we set the card_removed flag.
So when we retry or send next async request we end it in mmc_blk_resend()
and there will be no subsequent request to the driver since we are
suppressing the requests in the queue layer.
So I guess there is no need to add further checks in the __mmc_start_req(),
which could be redundant as there are no calls to the core layer from block
layer once the card is found to be removed.
In summary the sequence I thought would be like this:
Case 1:
1) Transfer is going on (host claimed by mmcqd) and card is removed.
2) Driver is in middle of transaction, hence some kind of timeout error is
returned.
3) mmc_blk_cmd_recovery(): Block layer does error checking and sets the card
as removed.
4) Block layer then retries the request calling mmc_blk_resend().
5) Since the mmc_card_removed is true we end
Re: [PATCH] mmc: block: release host in case of error
On Sun, Nov 20, 2011 at 9:50 PM, Per Forlin per.l...@gmail.com wrote:
Hi Adrian,
On Fri, Nov 18, 2011 at 10:56 AM, Per Förlin per.for...@stericsson.com
wrote:
On 11/17/2011 10:18 AM, Adrian Hunter wrote:
On 14/11/11 13:12, Per Forlin wrote:
Host is claimed as long as there are requests in the block queue
and all request are completed successfully. If an error occur release
the host in case someone else needs to claim it, for instance if the card
is removed during a transfer.
Signed-off-by: Per Forlin per.for...@stericsson.com
---
drivers/mmc/card/block.c | 37 +
1 files changed, 29 insertions(+), 8 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index c80bb6d..c21fd2c 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1158,6 +1158,28 @@ static int mmc_blk_cmd_err(struct mmc_blk_data *md,
struct mmc_card *card,
return ret;
}
+/*
+ * This function should be called to resend a request after failure.
+ * Prepares and starts the request.
+ */
+static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
+ struct mmc_queue *mq,
+ struct mmc_queue_req *mqrq,
+ int disable_multi,
+ struct mmc_async_req *areq)
+{
+ /*
+ * Release host after failure in case the host is needed
+ * by someone else. For instance, if the card is removed the
+ * worker thread needs to claim the host in order to do mmc_rescan.
+ */
+ mmc_release_host(card-host);
+ mmc_claim_host(card-host);
Does this work? Won't the current thread win the race
to claim the host again?
Good question. I've tested it and I haven't seen any cases where current has
claimed the host again. Sujit has tested the patch as well.
But I can't say that your scenario can't happen. I will study the wake_up
and wait_queue code to see if I can find the answer.
mmc_release_host() - wake_up() - schedule(). If the waking process
has higher prio than current it will preempt current on NOSMP. If SMP,
current and waking process may be on separate CPUs and in that case
it's difficult to guarantee that the waking process will win the race.
I'm proposing to add yield() in order to give the waking process
better chances to win the race.
Here's a patch:
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index c21fd2c..add1c38 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1173,8 +1173,11 @@ static inline struct mmc_async_req
*mmc_blk_resend(struct mmc_card *card,
* by someone else. For instance, if the card is removed the
* worker thread needs to claim the host in order to do mmc_rescan.
*/
- mmc_release_host(card-host);
- mmc_claim_host(card-host);
+ if (mmc_card_rescan(card)) {
+ mmc_release_host(card-host);
+ yield();
+ mmc_claim_host(card-host);
+ }
mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
return mmc_start_req(card-host, areq, NULL);
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 271efea..83f03a3 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2059,6 +2059,8 @@ void mmc_rescan(struct work_struct *work)
if (host-rescan_disable)
return;
+ mmc_card_set_rescan(host-card);
+
/*
@@ -2101,6 +2103,7 @@
out:
+ mmc_card_clr_rescan(host-card);
}
---
I'm not sure if this patch-extension is really needed, it may only
make the patch more complex. If the race condition Adrian refers to is
unlikely, there may be a few extra retries before mmc_rescan get the
chance to claim the host.
I'm in favor of skipping my proposed extension and staying with the
original v1 patch.
Adrian, what do you say?
Thanks,
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] mmc: Kill block requests if card is removed
On Thu, Nov 24, 2011 at 7:48 PM, Sujit Reddy Thumma
sthu...@codeaurora.org wrote:
On Thu, Nov 24, 2011 at 12:27 PM, Sujit Reddy Thumma
sthu...@codeaurora.org wrote:
Hi Per,
On 11/22/2011 6:40 PM, Per Forlin wrote:
Hi Sujit,
On Tue, Nov 22, 2011 at 11:56 AM, Sujit Reddy Thumma
sthu...@codeaurora.org wrote:
Kill block requests when the host knows that the card is
removed from the slot and is sure that subsequent requests
are bound to fail. Do this silently so that the block
layer doesn't output unnecessary error messages.
This patch implements suggestion from Adrian Hunter,
http://thread.gmane.org/gmane.linux.kernel.mmc/2714/focus=3474
Signed-off-by: Sujit Reddy Thummasthu...@codeaurora.org
---
Changes in v2:
- Changed the implementation with further comments from Adrian
- Set the card removed flag in bus notifier callbacks
- This patch is now dependent on patch from Per Forlin:
http://thread.gmane.org/gmane.linux.kernel.mmc/11128/focus=11211
---
drivers/mmc/card/block.c | 33 -
drivers/mmc/card/queue.c | 5 +
drivers/mmc/core/bus.c | 32 +++-
drivers/mmc/core/core.c | 8 +++-
include/linux/mmc/card.h | 3 +++
5 files changed, 74 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index edc379e..83956fa 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -648,8 +648,15 @@ static int mmc_blk_cmd_recovery(struct mmc_card
*card, struct request *req,
}
/* We couldn't get a response from the card. Give up. */
- if (err)
+ if (err) {
+ /*
+ * If the card didn't respond to status command,
+ * it is likely that card is gone. Flag it as removed,
+ * mmc_detect_change() cleans the rest.
+ */
+ mmc_card_set_removed(card);
return ERR_ABORT;
+ }
/* Flag ECC errors */
if ((status R1_CARD_ECC_FAILED) ||
@@ -1168,6 +1175,9 @@ static inline struct mmc_async_req
*mmc_blk_resend(struct mmc_card *card,
int disable_multi,
struct mmc_async_req
*areq)
{
+ struct mmc_blk_data *md = mmc_get_drvdata(card);
+ struct request *req = mqrq-req;
+ int ret;
/*
* Release host after failure in case the host is needed
* by someone else. For instance, if the card is removed the
@@ -1175,11 +1185,24 @@ static inline struct mmc_async_req
*mmc_blk_resend(struct mmc_card *card,
*/
mmc_release_host(card-host);
mmc_claim_host(card-host);
-
- mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
- return mmc_start_req(card-host, areq, NULL);
+ if (mmc_card_removed(card)) {
+ /*
+ * End the pending async request without starting
+ * it when card is removed.
+ */
+ spin_lock_irq(md-lock);
+ req-cmd_flags |= REQ_QUIET;
+ do {
+ ret = __blk_end_request(req,
+ -EIO, blk_rq_cur_bytes(req));
+ } while (ret);
+ spin_unlock_irq(md-lock);
+ return NULL;
+ } else {
+ mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
+ return mmc_start_req(card-host, areq, NULL);
+ }
mmc_blk_resend is only called to resend a request that has failed. If
the card is removed the request will still be issued, but when it
retries it will give up here.
Currently, we set the card_removed flag in two places:
1) If the host supports interrupt detection, mmc_detect_change() calls
the
bus detect method and we set card removed flag in bus notifier call
back.
2) When there is a transfer going on (host is claimed by mmcqd) and the
card
is removed ungracefully, the driver sends an error code to the block
layer.
mmc_blk_cmd_recovery() tries to send CMD13 to the card which in this
case
fails and we set the card_removed flag.
So when we retry or send next async request we end it in
mmc_blk_resend()
and there will be no subsequent request to the driver since we are
suppressing the requests in the queue layer.
So I guess there is no need to add further checks in the
__mmc_start_req(),
which could be redundant as there are no calls to the core layer from
block
layer once the card is found to be removed.
In summary the sequence I thought would be like this:
Case 1:
1) Transfer is going on (host claimed by mmcqd) and card is removed.
2) Driver is in middle of transaction, hence some kind of timeout error
is
returned.
3) mmc_blk_cmd_recovery(): Block layer does error checking and sets the
card
as removed.
4) Block layer then retries
Re: [PATCH V2] mmc: Kill block requests if card is removed
Hi Sujit,
On Tue, Nov 22, 2011 at 11:56 AM, Sujit Reddy Thumma
sthu...@codeaurora.org wrote:
Kill block requests when the host knows that the card is
removed from the slot and is sure that subsequent requests
are bound to fail. Do this silently so that the block
layer doesn't output unnecessary error messages.
This patch implements suggestion from Adrian Hunter,
http://thread.gmane.org/gmane.linux.kernel.mmc/2714/focus=3474
Signed-off-by: Sujit Reddy Thumma sthu...@codeaurora.org
---
Changes in v2:
- Changed the implementation with further comments from Adrian
- Set the card removed flag in bus notifier callbacks
- This patch is now dependent on patch from Per Forlin:
http://thread.gmane.org/gmane.linux.kernel.mmc/11128/focus=11211
---
drivers/mmc/card/block.c | 33 -
drivers/mmc/card/queue.c | 5 +
drivers/mmc/core/bus.c | 32 +++-
drivers/mmc/core/core.c | 8 +++-
include/linux/mmc/card.h | 3 +++
5 files changed, 74 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index edc379e..83956fa 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -648,8 +648,15 @@ static int mmc_blk_cmd_recovery(struct mmc_card *card,
struct request *req,
}
/* We couldn't get a response from the card. Give up. */
- if (err)
+ if (err) {
+ /*
+ * If the card didn't respond to status command,
+ * it is likely that card is gone. Flag it as removed,
+ * mmc_detect_change() cleans the rest.
+ */
+ mmc_card_set_removed(card);
return ERR_ABORT;
+ }
/* Flag ECC errors */
if ((status R1_CARD_ECC_FAILED) ||
@@ -1168,6 +1175,9 @@ static inline struct mmc_async_req
*mmc_blk_resend(struct mmc_card *card,
int disable_multi,
struct mmc_async_req *areq)
{
+ struct mmc_blk_data *md = mmc_get_drvdata(card);
+ struct request *req = mqrq-req;
+ int ret;
/*
* Release host after failure in case the host is needed
* by someone else. For instance, if the card is removed the
@@ -1175,11 +1185,24 @@ static inline struct mmc_async_req
*mmc_blk_resend(struct mmc_card *card,
*/
mmc_release_host(card-host);
mmc_claim_host(card-host);
-
- mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
- return mmc_start_req(card-host, areq, NULL);
+ if (mmc_card_removed(card)) {
+ /*
+ * End the pending async request without starting
+ * it when card is removed.
+ */
+ spin_lock_irq(md-lock);
+ req-cmd_flags |= REQ_QUIET;
+ do {
+ ret = __blk_end_request(req,
+ -EIO, blk_rq_cur_bytes(req));
+ } while (ret);
+ spin_unlock_irq(md-lock);
+ return NULL;
+ } else {
+ mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
+ return mmc_start_req(card-host, areq, NULL);
+ }
mmc_blk_resend is only called to resend a request that has failed. If
the card is removed the request will still be issued, but when it
retries it will give up here.
You have added a check in mmc_wait_for_req(). What about this:
--
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index b526036..dcdcb9a 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -287,11 +287,17 @@ static void mmc_wait_done(struct mmc_request *mrq)
complete(mrq-completion);
}
-static void __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)
+static int __mmc_start_req(struct mmc_host *host, struct mmc_request *mrq)
{
- init_completion(mrq-completion);
- mrq-done = mmc_wait_done;
- mmc_start_request(host, mrq);
+ if (mmc_card_removed(host-card)) {
+ mrq-cmd-error = -ENOMEDIUM;
+ return -ENOMEDIUM;
+ }
+
+ init_completion(mrq-completion);
+ mrq-done = mmc_wait_done;
+ mmc_start_request(host, mrq);
+ return 0;
}
static void mmc_wait_for_req_done(struct mmc_host *host,
@@ -418,7 +424,8 @@ EXPORT_SYMBOL(mmc_start_req);
*/
void mmc_wait_for_req(struct mmc_host *host, struct mmc_request *mrq)
{
- __mmc_start_req(host, mrq);
+ if (__mmc_start_req(host, mrq))
+ return
mmc_wait_for_req_done(host, mrq);
}
EXPORT_SYMBOL(mmc_wait_for_req);
--
This patch will set error to -ENOMEDIUM for both mmc_start_req() and
mmc_wait_for_req()
mmc_blk_err_check() can check for -ENOMEDIUM and return something like
Re: [PATCH] mmc: block: release host in case of error
Hi Adrian,
On Fri, Nov 18, 2011 at 10:56 AM, Per Förlin per.for...@stericsson.com wrote:
On 11/17/2011 10:18 AM, Adrian Hunter wrote:
On 14/11/11 13:12, Per Forlin wrote:
Host is claimed as long as there are requests in the block queue
and all request are completed successfully. If an error occur release
the host in case someone else needs to claim it, for instance if the card
is removed during a transfer.
Signed-off-by: Per Forlin per.for...@stericsson.com
---
drivers/mmc/card/block.c | 37 +
1 files changed, 29 insertions(+), 8 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index c80bb6d..c21fd2c 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1158,6 +1158,28 @@ static int mmc_blk_cmd_err(struct mmc_blk_data *md,
struct mmc_card *card,
return ret;
}
+/*
+ * This function should be called to resend a request after failure.
+ * Prepares and starts the request.
+ */
+static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
+ struct mmc_queue *mq,
+ struct mmc_queue_req *mqrq,
+ int disable_multi,
+ struct mmc_async_req *areq)
+{
+ /*
+ * Release host after failure in case the host is needed
+ * by someone else. For instance, if the card is removed the
+ * worker thread needs to claim the host in order to do mmc_rescan.
+ */
+ mmc_release_host(card-host);
+ mmc_claim_host(card-host);
Does this work? Won't the current thread win the race
to claim the host again?
Good question. I've tested it and I haven't seen any cases where current has
claimed the host again. Sujit has tested the patch as well.
But I can't say that your scenario can't happen. I will study the wake_up and
wait_queue code to see if I can find the answer.
mmc_release_host() - wake_up() - schedule(). If the waking process
has higher prio than current it will preempt current on NOSMP. If SMP,
current and waking process may be on separate CPUs and in that case
it's difficult to guarantee that the waking process will win the race.
I'm proposing to add yield() in order to give the waking process
better chances to win the race.
Here's a patch:
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index c21fd2c..add1c38 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1173,8 +1173,11 @@ static inline struct mmc_async_req
*mmc_blk_resend(struct mmc_card *card,
* by someone else. For instance, if the card is removed the
* worker thread needs to claim the host in order to do mmc_rescan.
*/
- mmc_release_host(card-host);
- mmc_claim_host(card-host);
+ if (mmc_card_rescan(card)) {
+ mmc_release_host(card-host);
+ yield();
+ mmc_claim_host(card-host);
+ }
mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
return mmc_start_req(card-host, areq, NULL);
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 271efea..83f03a3 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2059,6 +2059,8 @@ void mmc_rescan(struct work_struct *work)
if (host-rescan_disable)
return;
+ mmc_card_set_rescan(host-card);
+
/*
@@ -2101,6 +2103,7 @@
out:
+ mmc_card_clr_rescan(host-card);
}
---
Thanks,
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] mmc: support BKOPS feature for eMMC
Hi Konstantin, On Sun, Nov 20, 2011 at 5:12 PM, Konstantin Dorfman kdorf...@codeaurora.org wrote: Hello Per, On Thu, Nov 17, 2011 at 4:01 PM, Konstantin Dorfman kdorf...@codeaurora.org wrote: Hello Jaenhoon, I have a few suggestions regarding the situation when Host starts BKOPS: 1) Host should start BKOPS (based on BKOPS_STATUS or URGENT_BKOPS event) when going to mmc_sleep, which means that the bus is in idle state (this also covers the case in mmc_queue_thread, because in this case no I/O request exists). It seems like checking the status periodically in addition to mmc_suspend is not needed. Since if the device was in idle and we performed a single BKOPS then there is no point in performing another BKOPS unless there was actually another I/O request. 2) Also this implies an answer to the question about need to interrupt BKOPS before powering off card - the answer is no. By the answer no you mean there is no risk of data corruption if cutting power in the middle of a BKOPS. When the card is powered up next time it will verify that bkops is in a defined state, and do recovery if necessary. An interesting comment I got from Sebastian is if this recovery mechanism affects card boot time. The question is: May the card boot up slower (due to recovery) next time if BKOPS was ongoing at power off? I assume this recovery time should be insignificant, but I don't know for sure. Let me explain proposed flow: The only trigger to start BKOPS command should be mmc_power_off() function, just before sending POWER_OFF_NOTIFICATION[34] and only when BKOPS is needed (by needed I understand situation, when URGENT_BKOPS event arrived or BKOPS_STATUS 0x2 or 0x3). The flow will wait till BKOPS successfully completed and than continue to powering off the card. Power off will never occur in the middle of BKOPS. Also do not need to start BKOPS when mmc_queue_thread() is in IDLE state (no requests exists), because in such case power off should be done to card. I wonder how this works with suspend. If suspend is called on the system, MMC should suspend quickly and not wait for the BKOPS to finish. For pm_runtime_suspend it's fine to return -EBUSY and defer pm_runtime_suspend until BKOPS is completed. mmc_power_on/off is too low level I think. What about adding this at the suspend/resume level instead? 3) Based on statistical data we have (day long test) it looks like we do not need to do any preventive BKOPS caused by BKOPS_STATUS less than critical (0x3). I proposed this preventive action but at that time I didn't have any data to back it up with. I've run some day long tests and what I could see is when BKOPS_LEVEL goes from 0 to 1, it goes back to 0 without having to start BKOPS. Can you confirm this with your tests as well? One explanation I got for this is that level of 1 only means the eMMC internal cache is fragmented and not the actual memory. We have such data from card vendors, but I plan to do similar tests to confirm. I will update about the results. Looking forward to see your results. Thanks, Per -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] mmc: support BKOPS feature for eMMC
On Thu, Nov 17, 2011 at 4:01 PM, Konstantin Dorfman
kdorf...@codeaurora.org wrote:
Hello Jaenhoon,
I have a few suggestions regarding the situation when Host starts BKOPS:
1) Host should start BKOPS (based on BKOPS_STATUS or URGENT_BKOPS event)
when going to mmc_sleep, which means that the bus is in idle state (this
also covers the case in mmc_queue_thread, because in this case no I/O
request exists). It seems like checking the status periodically in addition
to mmc_suspend is not needed. Since if the device was in idle and we
performed a single BKOPS then there is no point in performing another BKOPS
unless there was actually another I/O request.
2) Also this implies an answer to the question about need to interrupt BKOPS
before powering off card - the answer is no.
By the answer no you mean there is no risk of data corruption if
cutting power in the middle of a BKOPS. When the card is powered up
next time it will verify that bkops is in a defined state, and do
recovery if necessary. An interesting comment I got from Sebastian is
if this recovery mechanism affects card boot time.
The question is: May the card boot up slower (due to recovery) next
time if BKOPS was ongoing at power off?
I assume this recovery time should be insignificant, but I don't know for sure.
3) Based on statistical data we have (day long test) it looks like we do not
need to do any preventive BKOPS caused by BKOPS_STATUS less than critical
(0x3).
I proposed this preventive action but at that time I didn't have any
data to back it up with. I've run some day long tests and what I could
see is when BKOPS_LEVEL goes from 0 to 1, it goes back to 0 without
having to start BKOPS. Can you confirm this with your tests as well?
One explanation I got for this is that level of 1 only means the eMMC
internal cache is fragmented and not the actual memory.
Thanks,
Per
-Original Message-
From: linux-mmc-ow...@vger.kernel.org
[mailto:linux-mmc-ow...@vger.kernel.org] On Behalf Of Jaehoon Chung
Sent: Thursday, November 17, 2011 2:50 AM
To: linux-mmc
Cc: Chris Ball; Kyungmin Park; Hanumath Prasad; Per Forlin; Sebastian
Rasmussen; Dong, Chuanxiao; svenk...@ti.com
Subject: [PATCH v3] mmc: support BKOPS feature for eMMC
Enable eMMC background operations (BKOPS) feature.
If URGENT_BKOPS is set after a response, note that BKOPS
are required. After all I/O requests are finished, run
BKOPS if required. Should read/write operations be requested
during BKOPS, first issue HPI to interrupt the ongoing BKOPS
and then service the request.
If you want to enable this feature, set MMC_CAP2_BKOPS.
Future considerations
* Check BKOPS_LEVEL=1 and start BKOPS in a preventive manner.
* Interrupt ongoing BKOPS before powering off the card.
Signed-off-by: Jaehoon Chung jh80.ch...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
CC: Hanumath Prasad hanumath.pra...@stericsson.com
---
Changelog V3:
- move the bkops setting's location in mmc_blk_issue_rw_rq
- modify condition checking
- bkops_en is assigned ext_csd[EXT_CSD_BKOPS_EN] instead of 1
- remove the unused code
- change pr_debug instead of pr_warn in mmc_send_hpi_cmd
- Add the Future consideration suggested by Per
Changelog V2:
- Use EXCEPTION_STATUS instead of URGENT_BKOPS
- Add function to check Exception_status(for eMMC4.5)
- remove unnecessary code.
drivers/mmc/card/block.c | 10 +
drivers/mmc/card/queue.c | 4 ++
drivers/mmc/core/core.c | 87
drivers/mmc/core/mmc.c | 8
drivers/mmc/core/mmc_ops.c | 6 +++-
include/linux/mmc/card.h | 12 ++
include/linux/mmc/core.h | 3 ++
include/linux/mmc/host.h | 1 +
include/linux/mmc/mmc.h | 14 +++
9 files changed, 144 insertions(+), 1 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index c80bb6d..9d19ebb 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1188,6 +1188,16 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq,
struct request *rqc)
type = rq_data_dir(req) == READ ? MMC_BLK_READ :
MMC_BLK_WRITE;
mmc_queue_bounce_post(mq_rq);
+ /*
+ * Check BKOPS urgency from each R1 response
+ */
+ if (mmc_card_mmc(card)
+ (brq-cmd.resp[0] R1_EXCEPTION_EVENT)) {
+ if (mmc_is_exception_event(card,
+ EXT_CSD_URGENT_BKOPS))
+ mmc_card_set_need_bkops(card);
+ }
+
switch (status) {
case MMC_BLK_SUCCESS:
case MMC_BLK_PARTIAL:
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index dcad59c..20bb4a5 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
Re: [PATCH] mmc: core: Kill block requests if card is removed
On Mon, Nov 14, 2011 at 8:52 AM, Per Forlin per.l...@gmail.com wrote:
On Mon, Nov 14, 2011 at 5:19 AM, Sujit Reddy Thumma
sthu...@codeaurora.org wrote:
On 11/10/2011 7:50 PM, Per Forlin wrote:
On Thu, Nov 10, 2011 at 10:35 AM, Adrian Hunteradrian.hun...@intel.com
wrote:
On 10/11/11 06:02, Sujit Reddy Thumma wrote:
Hi,
Hi Adrian,
On Wed, Nov 9, 2011 at 10:34 AM, Adrian Hunteradrian.hun...@intel.com
wrote:
On 09/11/11 06:31, Sujit Reddy Thumma wrote:
Kill block requests when the host knows that the card is
removed from the slot and is sure that subsequent requests
are bound to fail. Do this silently so that the block
layer doesn't output unnecessary error messages.
This patch implements suggestion from Adrian Hunter,
http://thread.gmane.org/gmane.linux.kernel.mmc/2714/focus=3474
Signed-off-by: Sujit Reddy Thummasthu...@codeaurora.org
---
It is safer to have zero initialisations so I suggest inverting
the meaning of the state bit i.e.
Makes sense. Will take care in V2.
#define MMC_STATE_CARD_GONE (17) /* card removed from
the
slot */
Also it would be nice is a request did not start if the card is
gone. For the non-async case, that is easy:
As far as I understand Sujit's patch it will stop new requests from
being issued, blk_fetch_request(q) returns NULL.
Yes, Per you are right. The ongoing requests will fail at the controller
driver layer and only the new requests coming to MMC queue layer will be
blocked.
Adrian's suggestion is to stop all the requests reaching host controller
layer by mmc core. This seems to be a good approach but the problem here
is
the host driver should inform mmc core that card is gone.
Adrian, do you agree if we need to change all the host drivers to set
the
MMC_STATE_CARD_GONE flag as soon as the card detect irq handler detects
the
card as removed?
Typically a card detect interrupt queues a rescan which in turn will have
to wait to claim the host. In the meantime, in the async case, the block
driver will not release the host until the queue is empty.
Here is a patch that let async-mmc release host and reclaim it in case of
error.
When the host is released mmc_rescan will claim the host and run.
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index cf73877..8952e63 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1209,6 +1209,28 @@ static int mmc_blk_cmd_err(struct mmc_blk_data
*md, struct mmc_card *card,
return ret;
}
+/*
+ * This function should be called to resend a request after failure.
+ * Prepares and starts the request.
+ */
+static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
+ struct mmc_queue *mq,
+ struct mmc_queue_req
*mqrq,
+ int disable_multi,
+ struct mmc_async_req
*areq)
+{
+ /*
+ * Release host after failure in case the host is needed
+ * by someone else. For instance, if the card is removed the
+ * worker thread needs to claim the host in order to do
mmc_rescan.
+ */
+ mmc_release_host(card-host);
+ mmc_claim_host(card-host);
+
+ mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
+ return mmc_start_req(card-host, areq, NULL);
+}
+
static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
{
struct mmc_blk_data *md = mq-data;
@@ -1308,14 +1330,14 @@ static int mmc_blk_issue_rw_rq(struct
mmc_queue *mq, struct request *rqc)
break;
}
- if (ret) {
+ if (ret)
/*
* In case of a incomplete request
* prepare it again and resend.
*/
- mmc_blk_rw_rq_prep(mq_rq, card, disable_multi,
mq);
- mmc_start_req(card-host,mq_rq-mmc_active,
NULL);
- }
+ mmc_blk_prep_start(card, mq, mq_rq, disable_multi,
+ mq_rq-mmc_active);
+
:%s/mmc_blk_prep_start/mmc_blk_resend
I'll update.
} while (ret);
return 1;
@@ -1327,10 +1349,9 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue
*mq, struct request *rqc)
spin_unlock_irq(md-lock);
start_new_req:
- if (rqc) {
- mmc_blk_rw_rq_prep(mq-mqrq_cur, card, 0, mq);
- mmc_start_req(card-host,mq-mqrq_cur-mmc_active, NULL);
- }
+ if (rqc)
+ mmc_blk_prep_start(card, mq, mq-mqrq_cur, 0,
+ mq-mqrq_cur-mmc_active);
return 0;
}
Thanks Per. This looks good. Can we split this into a different patch?
Yes I agree. My intention
[PATCH] ARM: mmci: add capabilities2 for MMC_CAP2
Signed-off-by: Per Forlin per.for...@stericsson.com
---
This patch depends on patches adding CAP2 available on mmc-next.
Chris, would you mind merging this through your tree when it
has been accepted.
drivers/mmc/host/mmci.c |1 +
include/linux/amba/mmci.h |2 ++
2 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 4602771..c1cf80c 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1231,6 +1231,7 @@ static int __devinit mmci_probe(struct amba_device *dev,
if (host-vcc == NULL)
mmc-ocr_avail = plat-ocr_mask;
mmc-caps = plat-capabilities;
+ mmc-caps2 = plat-capabilities2;
/*
* We can do SGIO
diff --git a/include/linux/amba/mmci.h b/include/linux/amba/mmci.h
index 2111481..3749e13 100644
--- a/include/linux/amba/mmci.h
+++ b/include/linux/amba/mmci.h
@@ -30,6 +30,7 @@ struct dma_chan;
* @cd_invert: true if the gpio_cd pin value is active low
* @capabilities: the capabilities of the block as implemented in
* this platform, signify anything MMC_CAP_* from mmc/host.h
+ * @capabilities2: more capabilities, MMC_CAP2_* from mmc/host.h
* @dma_filter: function used to select an appropriate RX and TX
* DMA channel to be used for DMA, if and only if you're deploying the
* generic DMA engine
@@ -52,6 +53,7 @@ struct mmci_platform_data {
int gpio_cd;
boolcd_invert;
unsigned long capabilities;
+ unsigned long capabilities2;
bool (*dma_filter)(struct dma_chan *chan, void *filter_param);
void *dma_rx_param;
void *dma_tx_param;
--
1.6.3.3
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] mmc: mmc_test: align max_seg_size
If max_seg_size is unaligned, mmc_test_map_sg() may create sg element sizes that are not aligned with 512 byte. Fix, align max_seg_size at mmc_test_area_init(). Signed-off-by: Per Forlin per.for...@stericsson.com --- drivers/mmc/card/mmc_test.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/mmc/card/mmc_test.c b/drivers/mmc/card/mmc_test.c index b038c4a..5848998 100644 --- a/drivers/mmc/card/mmc_test.c +++ b/drivers/mmc/card/mmc_test.c @@ -1581,6 +1581,7 @@ static int mmc_test_area_init(struct mmc_test_card *test, int erase, int fill) t-max_segs = test-card-host-max_segs; t-max_seg_sz = test-card-host-max_seg_size; + t-max_seg_sz -= t-max_seg_sz % 512; t-max_tfr = t-max_sz; if (t-max_tfr 9 test-card-host-max_blk_count) -- 1.6.3.3 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] mmc: support BKOPS feature for eMMC
On Fri, Nov 11, 2011 at 9:42 AM, Dong, Chuanxiao
chuanxiao.d...@intel.com wrote:
-Original Message-
From: Per Forlin [mailto:per.l...@gmail.com]
Sent: Friday, November 11, 2011 4:32 PM
To: Dong, Chuanxiao
Cc: Jaehoon Chung; linux-mmc; Jae hoon Chung; Chris Ball; Kyungmin Park;
Hanumath Prasad; Sebastian Rasmussen; svenk...@ti.com
Subject: Re: [PATCH v2] mmc: support BKOPS feature for eMMC
On Fri, Nov 11, 2011 at 8:48 AM, Per Forlin per.l...@gmail.com wrote:
On Fri, Nov 11, 2011 at 7:51 AM, Dong, Chuanxiao
chuanxiao.d...@intel.com wrote:
Hi Jaehoon,
-Original Message-
From: Jaehoon Chung [mailto:jh80.ch...@samsung.com]
Sent: Wednesday, November 02, 2011 6:29 PM
To: linux-mmc
Cc: Chris Ball; Kyungmin Park; Hanumath Prasad; Sebastian Rasmussen; Per
Forlin;
svenk...@ti.com; Dong, Chuanxiao
Subject: [PATCH v2] mmc: support BKOPS feature for eMMC
Enable eMMC background operations (BKOPS) feature.
If URGENT_BKOPS is set after a response, note that BKOPS
are required. After all I/O requests are finished, run
BKOPS if required. Should read/write operations be requested
during BKOPS, first issue HPI to interrupt the ongoing BKOPS
and then service the request.
If you want to enable this feature, set MMC_CAP2_BKOPS.
Signed-off-by: Jaehoon Chung jh80.ch...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
CC: Hanumath Prasad hanumath.pra...@stericsson.com
---
Changelog V2:
- Use EXCEPTION_STATUS instead of URGENT_BKOPS
- Add function to check Exception_status(for eMMC4.5)
- remove unnecessary code.
drivers/mmc/card/block.c | 9 +
drivers/mmc/card/queue.c | 4 ++
drivers/mmc/core/core.c | 87
drivers/mmc/core/mmc.c | 9 -
drivers/mmc/core/mmc_ops.c | 4 ++
include/linux/mmc/card.h | 12 ++
include/linux/mmc/core.h | 3 ++
include/linux/mmc/host.h | 1 +
include/linux/mmc/mmc.h | 14 +++
9 files changed, 142 insertions(+), 1 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index a1cb21f..fbfb405 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1192,6 +1192,15 @@ static int mmc_blk_issue_rw_rq(struct
mmc_queue
*mq, struct request *rqc)
case MMC_BLK_SUCCESS:
case MMC_BLK_PARTIAL:
/*
+ * Check BKOPS urgency from each R1 response
+ */
+ if (mmc_card_mmc(card)
+ (brq-cmd.resp[0]
R1_EXCEPTION_EVENT)) {
+ if (mmc_is_exception_event(card,
+ EXT_CSD_URGENT_BK
OPS))
+ mmc_card_set_need_bkops(card
);
+ }
+ /*
Have you thought about moving this into mmc_wait_for_req_done()? We can
check if the command is a R1 or R1B or not, and set BKOPS bit in there if
needed. I
was just thinking if we put such code here, we may only cover the successful
scenario but not for the failed cases. Putting in mmc_wait_for_req_done can
cover
all cases.
Good point! I'm in favor of this change.
Or, put the check before the switch-case.
If we put in mmc_wait_for_req_done, then we can catch all R1 response command
result not only for the READ/WRITE command, right?
Yes I agree. The only reason for _not_ moving it to
wait_for_req_done() would be if this response (URGENT_BKOPS) is
directly connected to request handled by mmc_blk_issue_rw_rq() and no
one else. I don't know the answer.
ERASE command is also using R1 response.
I wasn't aware of this. Reading the R1 response should be done in
wait_for_req_done().
Checking if the response is (URGENT_BKOPS) could still be done in
mmc_blk_issue_rw_rq(), if this particular response only happens here.
Otherwise that needs to be done in wait_for_req_done() too.
Thanks,
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: core: Kill block requests if card is removed
On Thu, Nov 10, 2011 at 10:35 AM, Adrian Hunter adrian.hun...@intel.com wrote:
On 10/11/11 06:02, Sujit Reddy Thumma wrote:
Hi,
Hi Adrian,
On Wed, Nov 9, 2011 at 10:34 AM, Adrian Hunteradrian.hun...@intel.com
wrote:
On 09/11/11 06:31, Sujit Reddy Thumma wrote:
Kill block requests when the host knows that the card is
removed from the slot and is sure that subsequent requests
are bound to fail. Do this silently so that the block
layer doesn't output unnecessary error messages.
This patch implements suggestion from Adrian Hunter,
http://thread.gmane.org/gmane.linux.kernel.mmc/2714/focus=3474
Signed-off-by: Sujit Reddy Thummasthu...@codeaurora.org
---
It is safer to have zero initialisations so I suggest inverting
the meaning of the state bit i.e.
Makes sense. Will take care in V2.
#define MMC_STATE_CARD_GONE (17) /* card removed from the
slot */
Also it would be nice is a request did not start if the card is
gone. For the non-async case, that is easy:
As far as I understand Sujit's patch it will stop new requests from
being issued, blk_fetch_request(q) returns NULL.
Yes, Per you are right. The ongoing requests will fail at the controller
driver layer and only the new requests coming to MMC queue layer will be
blocked.
Adrian's suggestion is to stop all the requests reaching host controller
layer by mmc core. This seems to be a good approach but the problem here is
the host driver should inform mmc core that card is gone.
Adrian, do you agree if we need to change all the host drivers to set the
MMC_STATE_CARD_GONE flag as soon as the card detect irq handler detects the
card as removed?
Typically a card detect interrupt queues a rescan which in turn will have
to wait to claim the host. In the meantime, in the async case, the block
driver will not release the host until the queue is empty.
Here is a patch that let async-mmc release host and reclaim it in case of error.
When the host is released mmc_rescan will claim the host and run.
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index cf73877..8952e63 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1209,6 +1209,28 @@ static int mmc_blk_cmd_err(struct mmc_blk_data
*md, struct mmc_card *card,
return ret;
}
+/*
+ * This function should be called to resend a request after failure.
+ * Prepares and starts the request.
+ */
+static inline struct mmc_async_req *mmc_blk_resend(struct mmc_card *card,
+ struct mmc_queue *mq,
+ struct mmc_queue_req *mqrq,
+ int disable_multi,
+ struct mmc_async_req *areq)
+{
+ /*
+* Release host after failure in case the host is needed
+* by someone else. For instance, if the card is removed the
+* worker thread needs to claim the host in order to do mmc_rescan.
+*/
+ mmc_release_host(card-host);
+ mmc_claim_host(card-host);
+
+ mmc_blk_rw_rq_prep(mqrq, card, disable_multi, mq);
+ return mmc_start_req(card-host, areq, NULL);
+}
+
static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
{
struct mmc_blk_data *md = mq-data;
@@ -1308,14 +1330,14 @@ static int mmc_blk_issue_rw_rq(struct
mmc_queue *mq, struct request *rqc)
break;
}
- if (ret) {
+ if (ret)
/*
* In case of a incomplete request
* prepare it again and resend.
*/
- mmc_blk_rw_rq_prep(mq_rq, card, disable_multi, mq);
- mmc_start_req(card-host, mq_rq-mmc_active, NULL);
- }
+ mmc_blk_prep_start(card, mq, mq_rq, disable_multi,
+ mq_rq-mmc_active);
+
} while (ret);
return 1;
@@ -1327,10 +1349,9 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue
*mq, struct request *rqc)
spin_unlock_irq(md-lock);
start_new_req:
- if (rqc) {
- mmc_blk_rw_rq_prep(mq-mqrq_cur, card, 0, mq);
- mmc_start_req(card-host, mq-mqrq_cur-mmc_active, NULL);
- }
+ if (rqc)
+ mmc_blk_prep_start(card, mq, mq-mqrq_cur, 0,
+ mq-mqrq_cur-mmc_active);
return 0;
}
-
The block
driver will see errors and will use a send status command which will fail
so the request will be aborted, but the next request will be started
anyway.
There are two problems:
1. When do we reliably know that the card has really gone?
At present, the detect method for SD and MMC is just the send status
command, which is what the block driver is doing i.e. if the
[RFC] mmc: block: complete req before blk_part_switch
From: Per Forlin per.for...@stericsson.com
Finish off any previous request before switching partition.
Can there be an ongoing request when mmc_blk_part_switch() is called?
In that case it might fail if calling mmc_switch when there is an ongoing
request.
I'm sending this as an RFC because I haven't been able to verify if this
can happen, but to me it looks suspicious. host-areq is set if there is a
previous request in the pipeline.
In case this issue is confirmed I'll send a real patch.
Signed-off-by: Per Forlin per.for...@stericsson.com
---
drivers/mmc/card/block.c | 24
1 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index 4c1a648..ca993b1 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -437,12 +437,20 @@ static const struct block_device_operations mmc_bdops = {
#endif
};
+/* returns true if current partion, otherwise false */
+static inline bool mmc_blk_part_is_current(struct mmc_card *card,
+ struct mmc_blk_data *md)
+{
+ struct mmc_blk_data *main_md = mmc_get_drvdata(card);
+ return main_md-part_curr == md-part_type;
+}
+
static inline int mmc_blk_part_switch(struct mmc_card *card,
struct mmc_blk_data *md)
{
int ret;
struct mmc_blk_data *main_md = mmc_get_drvdata(card);
- if (main_md-part_curr == md-part_type)
+ if (mmc_blk_part_is_current(card, md))
return 0;
if (mmc_card_mmc(card)) {
@@ -454,7 +462,7 @@ static inline int mmc_blk_part_switch(struct mmc_card *card,
card-ext_csd.part_time);
if (ret)
return ret;
-}
+ }
main_md-part_curr = md-part_type;
return 0;
@@ -1188,6 +1196,12 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq, struct
request *req)
/* claim host only for the first request */
mmc_claim_host(card-host);
+ if (card-host-areq req
+ (req-cmd_flags (REQ_DISCARD | REQ_FLUSH) ||
+!mmc_blk_part_is_current(card, md)))
+ /* complete ongoing async transfer */
+ mmc_blk_issue_rw_rq(mq, NULL);
+
ret = mmc_blk_part_switch(card, md);
if (ret) {
ret = 0;
@@ -1195,17 +1209,11 @@ static int mmc_blk_issue_rq(struct mmc_queue *mq,
struct request *req)
}
if (req req-cmd_flags REQ_DISCARD) {
- /* complete ongoing async transfer before issuing discard */
- if (card-host-areq)
- mmc_blk_issue_rw_rq(mq, NULL);
if (req-cmd_flags REQ_SECURE)
ret = mmc_blk_issue_secdiscard_rq(mq, req);
else
ret = mmc_blk_issue_discard_rq(mq, req);
} else if (req req-cmd_flags REQ_FLUSH) {
- /* complete ongoing async transfer before issuing flush */
- if (card-host-areq)
- mmc_blk_issue_rw_rq(mq, NULL);
ret = mmc_blk_issue_flush(mq, req);
} else {
ret = mmc_blk_issue_rw_rq(mq, req);
--
1.7.0.4
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] mmc: support BKOPS feature for eMMC
Hi Jaehoon, On Wed, Nov 2, 2011 at 11:28 AM, Jaehoon Chung jh80.ch...@samsung.com wrote: Enable eMMC background operations (BKOPS) feature. If URGENT_BKOPS is set after a response, note that BKOPS are required. After all I/O requests are finished, run BKOPS if required. Should read/write operations be requested during BKOPS, first issue HPI to interrupt the ongoing BKOPS and then service the request. If you want to enable this feature, set MMC_CAP2_BKOPS. Signed-off-by: Jaehoon Chung jh80.ch...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com CC: Hanumath Prasad hanumath.pra...@stericsson.com --- Changelog V2: - Use EXCEPTION_STATUS instead of URGENT_BKOPS - Add function to check Exception_status(for eMMC4.5) - remove unnecessary code. Will there be a V3 soon? I have tested this patch with my minor changes on top and as far as I can tell it looks ok. I'm ready to accept this patch if those minor updates are made in V3. Thanks, Per -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] mmc: support BKOPS feature for eMMC
On Fri, Nov 11, 2011 at 7:51 AM, Dong, Chuanxiao
chuanxiao.d...@intel.com wrote:
Hi Jaehoon,
-Original Message-
From: Jaehoon Chung [mailto:jh80.ch...@samsung.com]
Sent: Wednesday, November 02, 2011 6:29 PM
To: linux-mmc
Cc: Chris Ball; Kyungmin Park; Hanumath Prasad; Sebastian Rasmussen; Per
Forlin;
svenk...@ti.com; Dong, Chuanxiao
Subject: [PATCH v2] mmc: support BKOPS feature for eMMC
Enable eMMC background operations (BKOPS) feature.
If URGENT_BKOPS is set after a response, note that BKOPS
are required. After all I/O requests are finished, run
BKOPS if required. Should read/write operations be requested
during BKOPS, first issue HPI to interrupt the ongoing BKOPS
and then service the request.
If you want to enable this feature, set MMC_CAP2_BKOPS.
Signed-off-by: Jaehoon Chung jh80.ch...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
CC: Hanumath Prasad hanumath.pra...@stericsson.com
---
Changelog V2:
- Use EXCEPTION_STATUS instead of URGENT_BKOPS
- Add function to check Exception_status(for eMMC4.5)
- remove unnecessary code.
drivers/mmc/card/block.c | 9 +
drivers/mmc/card/queue.c | 4 ++
drivers/mmc/core/core.c | 87
drivers/mmc/core/mmc.c | 9 -
drivers/mmc/core/mmc_ops.c | 4 ++
include/linux/mmc/card.h | 12 ++
include/linux/mmc/core.h | 3 ++
include/linux/mmc/host.h | 1 +
include/linux/mmc/mmc.h | 14 +++
9 files changed, 142 insertions(+), 1 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index a1cb21f..fbfb405 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1192,6 +1192,15 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue
*mq, struct request *rqc)
case MMC_BLK_SUCCESS:
case MMC_BLK_PARTIAL:
/*
+ * Check BKOPS urgency from each R1 response
+ */
+ if (mmc_card_mmc(card)
+ (brq-cmd.resp[0] R1_EXCEPTION_EVENT)) {
+ if (mmc_is_exception_event(card,
+ EXT_CSD_URGENT_BKOPS))
+ mmc_card_set_need_bkops(card);
+ }
+ /*
Have you thought about moving this into mmc_wait_for_req_done()? We can check
if the command is a R1 or R1B or not, and set BKOPS bit in there if needed. I
was just thinking if we put such code here, we may only cover the successful
scenario but not for the failed cases. Putting in mmc_wait_for_req_done can
cover all cases.
Good point! I'm in favor of this change.
Thanks,
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: core: Kill block requests if card is removed
Hi Adrian,
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 5278ffb..91d7721 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -259,7 +259,7 @@ static void mmc_wait_for_req_done(struct mmc_host *host,
wait_for_completion(mrq-completion);
cmd = mrq-cmd;
- if (!cmd-error || !cmd-retries)
+ if (!cmd-error || !cmd-retries ||
mmc_card_gone(host-card))
host-card will be NULL
static void mmc_remove(struct mmc_host *host)
{
BUG_ON(!host);
BUG_ON(!host-card);
mmc_remove_card(host-card);
host-card = NULL;
}
card is not freed until later.
Please ignore this part. I jumped to conclusions. I had another look
and there can't be any incoming requests when host-card is NULL.
I need to study device_del() further, in order to understand the details.
Regards,
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] mmc: support BKOPS feature for eMMC
Hi Jaehoon,
On Tue, Nov 8, 2011 at 1:43 PM, Jae hoon Chung jh80.ch...@gmail.com wrote:
Hi Per.
2011/11/5 Per Forlin per.l...@gmail.com:
Hi Jaehoon,
On Wed, Nov 2, 2011 at 11:28 AM, Jaehoon Chung jh80.ch...@samsung.com
wrote:
Enable eMMC background operations (BKOPS) feature.
If URGENT_BKOPS is set after a response, note that BKOPS
are required. After all I/O requests are finished, run
BKOPS if required. Should read/write operations be requested
during BKOPS, first issue HPI to interrupt the ongoing BKOPS
and then service the request.
If you want to enable this feature, set MMC_CAP2_BKOPS.
Signed-off-by: Jaehoon Chung jh80.ch...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
CC: Hanumath Prasad hanumath.pra...@stericsson.com
---
Changelog V2:
- Use EXCEPTION_STATUS instead of URGENT_BKOPS
- Add function to check Exception_status(for eMMC4.5)
- remove unnecessary code.
drivers/mmc/card/block.c | 9 +
drivers/mmc/card/queue.c | 4 ++
drivers/mmc/core/core.c | 87
drivers/mmc/core/mmc.c | 9 -
drivers/mmc/core/mmc_ops.c | 4 ++
include/linux/mmc/card.h | 12 ++
include/linux/mmc/core.h | 3 ++
include/linux/mmc/host.h | 1 +
include/linux/mmc/mmc.h | 14 +++
9 files changed, 142 insertions(+), 1 deletions(-)
diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index a1cb21f..fbfb405 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -1192,6 +1192,15 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq,
struct request *rqc)
case MMC_BLK_SUCCESS:
case MMC_BLK_PARTIAL:
/*
+ * Check BKOPS urgency from each R1 response
+ */
+ if (mmc_card_mmc(card)
+ (brq-cmd.resp[0] R1_EXCEPTION_EVENT)) {
+ if (mmc_is_exception_event(card,
+ EXT_CSD_URGENT_BKOPS))
+ mmc_card_set_need_bkops(card);
+ }
+ /*
* A block was successfully transferred.
*/
mmc_blk_reset_success(md, type);
diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
index dcad59c..20bb4a5 100644
--- a/drivers/mmc/card/queue.c
+++ b/drivers/mmc/card/queue.c
@@ -61,6 +61,9 @@ static int mmc_queue_thread(void *d)
spin_unlock_irq(q-queue_lock);
if (req || mq-mqrq_prev-req) {
+ if (mmc_card_doing_bkops(mq-card))
+ mmc_interrupt_bkops(mq-card);
+
set_current_state(TASK_RUNNING);
mq-issue_fn(mq, req);
} else {
@@ -68,6 +71,7 @@ static int mmc_queue_thread(void *d)
set_current_state(TASK_RUNNING);
break;
}
+ mmc_start_bkops(mq-card);
up(mq-thread_sem);
schedule();
down(mq-thread_sem);
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 5278ffb..41ea923 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -238,6 +238,50 @@ mmc_start_request(struct mmc_host *host, struct
mmc_request *mrq)
host-ops-request(host, mrq);
}
+/**
+ * mmc_start_bkops - start BKOPS for supported cards
+ * @card: MMC card to start BKOPS
+ *
+ * Start background operations whenever requested.
+ * when the urgent BKOPS bit is set in a R1 command response
+ * then background operations should be started immediately.
+*/
+void mmc_start_bkops(struct mmc_card *card)
+{
+ int err;
+ unsigned long flags;
+
+ card-host-caps2 |= MMC_CAP2_BKOPS;
I guess this should be set by the host driver?
You're right...it's my mistake...i will remove this..
+ if ((!card || !card-ext_csd.bkops_en)
+ !(card-host-caps2 MMC_CAP2_BKOPS))
I don't really understand this. If card is NULL it will seg-fault.
Do you mean:
if (!card)
return
if (!card-ext_csd.bkops_en ||
!(card-host-caps2 MMC_CAP2_BKOPS))
return
Do you really need to check card != NULL? Consider BUG_ON
Maybe if (!card !card-ext_csd.bkops_en) is correctly...i think...
If card is NULL it will seg-fault.
Did you mean: if (card !card-ext_csd.bkops_en)
I would suggest.
# return if card is NULL
if (!card)
return
# if card is set, check the values.
if (!card-ext_csd.bkops_en ||
!(card-host-caps2 MMC_CAP2_BKOPS))
return
+ return;
+
+ /*
+ * If card is already doing bkops or need for
+ * bkops flag
Re: [PATCH v2] mmc: support BKOPS feature for eMMC
On Tue, Nov 8, 2011 at 2:53 PM, Per Forlin per.l...@gmail.com wrote: Hi Jaehoon, On Tue, Nov 8, 2011 at 1:43 PM, Jae hoon Chung jh80.ch...@gmail.com wrote: Hi Per. 2011/11/5 Per Forlin per.l...@gmail.com: Hi Jaehoon, On Wed, Nov 2, 2011 at 11:28 AM, Jaehoon Chung jh80.ch...@samsung.com wrote: Enable eMMC background operations (BKOPS) feature. If URGENT_BKOPS is set after a response, note that BKOPS are required. After all I/O requests are finished, run BKOPS if required. Should read/write operations be requested during BKOPS, first issue HPI to interrupt the ongoing BKOPS and then service the request. If you want to enable this feature, set MMC_CAP2_BKOPS. I'm thinking if it's good to add something in this commit message about what has been proposed in this thread. The scope of this patch is good and any additional optimizations could be added separately later on. Future considerations * Check BKOPS_LEVEL=1 and start BKOPS in a preventive manner. * Interrupt ongoing BKOPS before powering off the card. Regards, Per -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: support BKOPS feature for eMMC
On Fri, Oct 28, 2011 at 12:25 AM, Sebastian Rasmussen seb...@gmail.com wrote: Hi! Well, you kind of need both. Periodical check is a complement, not a replacement. Then we are indeed in agreement. Must BKOPS always be deferred until performance is impacted? No, of course not. As you say doing BKOPS too late is not good, but also doing them too often is probably not wise either (will cause latency for interrupting BKOPS for too many requests in that case). About starvation. What happens if the BKOPS never have time to finish because new writes are coming in all the time. Is it possible to starve the BKOPS-operation? Will it come to a point when BKOPS must run without interruption? The 4.5 spec says that if the level is at critical then some operations may extend beyond their original timeouts due to undelayable maintenance operations. So I can not forsee that an eMMC might stop working because the level reached critical and the host did not start BKOPS periodically. So as far as I understand it you may HPI interrupt BKOPS at critical level in order to issue CMD25 (WRITE_MULTIPLE_BLOCK), but there is a good chance that this write will take a long time to complete. One question is: Is it worth running BKOPS if the BKOPS_STATUS is only 1? I could run some tests comparing write performance when status is 0,1,2. The spec is likely intentionally arbitrary about what these levels mean in order to allow vendors to implement those differently. I don't know what the best place would be for a BKOPS_STATUS check. What comes to my mind is to use the same credentials that are used for power save. That seems like a good option, yes. Suspend? if suspend is requested one might check BKOPS_STATUS and return -EBUSY if BKOPS needs to be started. Maybe, one could also choose to do this based on the level of course. However now that I'm thinking about it, I don't think Jaehoon has taken care of the case where suspend happens while BKOPS are running in the background. I guess one has to issue HPI in that case to stop the BKOPS before going in to suspend, otherwise one risks cutting power to the card while BKOPS are running. Sebastian, Would you recommend to _not_ cut power while BKOPS? Is this documented anywhere? Or is it so that the BKOPS will resume when the card is powered up again? Thanks, Per -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: support BKOPS feature for eMMC
Jaehoon Chung jh80.chung at samsung.com writes: +++ b/drivers/mmc/core/core.c @@ -238,6 +238,50 @@ mmc_start_request(struct mmc_host *host, struct mmc_request *mrq) host-ops-request(host, mrq); } +/** + * mmc_start_bkops - start BKOPS for supported cards + * @card: MMC card to start BKOPS + * + * Start background operations whenever requested. + * when the urgent BKOPS bit is set in a R1 command response + * then background operations should be started immediately. +*/ This patch only starts BKOPS if it's urgent or critical. I would be preferable to run bkops periodically and only when the card is idle to minimize the risk of reaching URGENT. The specs says: - Hosts shall still read the full status from the BKOPS_STATUS byte periodically and start background operations as needed. - I'm thinking of checking BKOPS_STATUS when the card is idle and then run bkops even if level is only 1 (Operations outstanding – non critical). Would this make sense? Regards, Per -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: support BKOPS feature for eMMC
Hi Sebastian, On Thu, Oct 27, 2011 at 10:51 PM, Sebastian Rasmussen seb...@gmail.com wrote: Hi! This patch only starts BKOPS if it's urgent or critical. Almost, it starts BKOPS when it is urgent, which per spec means level 2 or 3, i.e. when performance is impacted or when it is critical. Better use the specs terminology as far as possible to relieve everyone of confusion. I would be preferable to run bkops periodically and only when the card is idle to minimize the risk of reaching URGENT. Well, you kind of need both. Yes, this is what I mean. If the URGENT_BKOPS is set meaning (status 2 or 3) there is no escape, current solution will do fine. Periodical check is a complement, not a replacement. Must BKOPS always be deferred until performance is impacted? About starvation. What happens if the BKOPS never have time to finish because new writes are coming in all the time. Is it possible to starve the BKOPS-operation? Will it come to a point when BKOPS must run without interruption? extent that the block device is never idling then you would definitely require this patch, right? Otherwise you may end up wasting the time between the last command sent and when the device has been deemed to be idle for long enough. So basically the OP's patch fixes the case where, e.g. sustained (re-)writing, keeps the block device busy until and after it reaches the critical BKOPS level, while your proposal takes care of the other case where the block device is not accessed all the time. The specs says: - Hosts shall still read the full status from the BKOPS_STATUS byte periodically and start background operations as needed. - Sure, if there is idle time to do it, then why not. If there is no idle time and URGENT_BKOPS is asserted, then the framework can not wait until the next idle period, but should issue BKOPS as soon as possible after the current command is finished. I'm thinking of checking BKOPS_STATUS when the card is idle and then run bkops even if level is only 1 (Operations outstanding – non critical). Would this make sense? I guess this boils down to how you define idle..? Does it mean immediately after the current command has been fully serviced, or does it mean some arbitrary time after the last sent command has been fully serviced? My assumption is that you are refering to the latter, in which case certain access patterns may cause problems. So, how do _you_ define idle? :) My first point was just to raise my concern and start a discussion about this. If this turns out to be a good idea I be happy to look more into the details. One question is: Is it worth running BKOPS if the BKOPS_STATUS is only 1? I could run some tests comparing write performance when status is 0,1,2. I don't know what the best place would be for a BKOPS_STATUS check. What comes to my mind is to use the same credentials that are used for power save. I need to look more closely into this in order to propose a patch. Suspend? if suspend is requested one might check BKOPS_STATUS and return -EBUSY if BKOPS needs to be started. Regards, Per -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 2/3] mmc: core: add random fault injection
On 14 September 2011 01:37, Akinobu Mita akinobu.m...@gmail.com wrote:
2011/9/14 Per Forlin per.for...@linaro.org:
Hi Akinobu,
On 13 September 2011 16:19, Per Forlin per.for...@linaro.org wrote:
On 13 September 2011 15:12, Akinobu Mita akinobu.m...@gmail.com wrote:
2011/8/19 Per Forlin per.for...@stericsson.com:
+#ifdef KERNEL
+/*
+ * Internal function. Pass the boot param fail_mmc_request to
+ * the setup fault injection attributes routine.
+ */
+static int __init setup_fail_mmc_request(char *str)
+{
+ return setup_fault_attr(fail_mmc_request, str);
+}
+__setup(fail_mmc_request=, setup_fail_mmc_request);
+#endif /* KERNEL */
You attempt to enable __setup() only when mmc_core is built into
the kernel. Does it really work? I cannot find any drivers using
KERNEL macro.
Your right it doesn't work. I think I change from ifndef MODULE to
ifdef KERNEL at one point.
It should be ifndef MODULE
It's simple and I like this solution.
It's simple and the patch would be just two lines.
The reason for changing my mind is that it may be useful to be able to
pass the fault injection attributes even when mmc_core is a module.
module_param is more complicated than this. Also the parameter is only
usefull when when mmc_core is built into the kernel (it's useless when
mmc_core is built as a module).
If you want to enable fault injection for the mmc_core module at load
time (during mmc initialisation) the param must be used.
modprobe mmc_core fail_request=1,1,1,1
As soon as the module is loaded there is no need for the module param anymore.
You can use module_param_cb() instead of __setup() without #ifdef KERNEL.
When mmc_core is built into the kernel, you can specify the parameter
with mmc_core.fail_mmc_request=...
I am considering using module_param() with perm = 0 (not visible in
sysfs). The purpose of the param is to set fault attributes during
kerne boot time or module load time. After the kernel boot all can be
set under debug fs, therefore no need to make the module param
visible.
What do you think about this? I have not tested it yet.
--
...
--
It's only necessary to call setup_fault_attr() once for all hosts.
Here it's called one time for each host. I think it's ok since the
routine is small and used for debugging purposes.
I could use a static bool to indicate whether setup_fault_attr() has
already been issued.
+ if (fail_mmc_request !setup_fault_attr_done)
module_param_cb() doesn't work as you expected?
I made a silly mistake thinking the set() hook would only be issued if
setting the param via sysfs. I turned out that I just mistyped the
boot-param name, sorry.
I now have a working test with module_param_cb() implemented.
Thanks,
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/3] mmc: rectify boot param option for fault injection
Akinobu Mita reported that the boot option for mmc fault injection is never compiled in due to a fauly ifdef KERNEL that is never set. A correct ifdef would be ifndef MODULE. This patch set adds a module_param_cb() and thereby no ifndef MODULE is needed. Using a module_param makes it possible to pass default fault attributes for external modules too. This patch set is for 3.2 Change log: v2 - use module_param_cb() to set default fault attributes - fix spelling of documentation in patch #3 Per Forlin (3): fault-inject: export setup_fault_attr() mmc: add module param to set fault injection attributes fault-injection: update documentation with the mmc module param Documentation/fault-injection/fault-injection.txt |2 +- drivers/mmc/core/debugfs.c| 38 +++-- lib/fault-inject.c|3 +- 3 files changed, 23 insertions(+), 20 deletions(-) -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/3] fault-inject: export setup_fault_attr()
mmc_core module needs to use setup_fault_attr() in order
to set fault injection attributes during module load time.
Signed-off-by: Per Forlin per.for...@linaro.org
---
lib/fault-inject.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/lib/fault-inject.c b/lib/fault-inject.c
index 328d433..4f75540 100644
--- a/lib/fault-inject.c
+++ b/lib/fault-inject.c
@@ -14,7 +14,7 @@
* setup_fault_attr() is a helper function for various __setup handlers, so it
* returns 0 on error, because that is what __setup handlers do.
*/
-int __init setup_fault_attr(struct fault_attr *attr, char *str)
+int setup_fault_attr(struct fault_attr *attr, char *str)
{
unsigned long probability;
unsigned long interval;
@@ -36,6 +36,7 @@ int __init setup_fault_attr(struct fault_attr *attr, char
*str)
return 1;
}
+EXPORT_SYMBOL_GPL(setup_fault_attr);
static void fail_dump(struct fault_attr *attr)
{
--
1.7.4.1
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/3] mmc: add module param to set fault injection attributes
Replace setup(fail_mmc_request) and faulty ifdef KERNEL with
a module_param_cb(). The module param mmc_core.fail_request
may be used to set the fault injection attributes during boot time
or module load time.
Signed-off-by: Per Forlin per.for...@linaro.org
---
drivers/mmc/core/debugfs.c | 38 --
1 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index 5acd707..d80e234 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -20,6 +20,25 @@
#include core.h
#include mmc_ops.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+
+static DECLARE_FAULT_ATTR(fail_default_attr);
+static char *fail_request;
+static int fail_mmc_request_param_set(const char *val,
+ const struct kernel_param *kp)
+{
+ setup_fault_attr(fail_default_attr, (char *) val);
+ return 0;
+}
+
+static const struct kernel_param_ops fail_mmc_request_param_ops = {
+ .set = fail_mmc_request_param_set
+};
+module_param_cb(fail_request, fail_mmc_request_param_ops,
+ fail_request, 0);
+
+#endif /* CONFIG_FAIL_MMC_REQUEST */
+
/* The debugfs functions are optimized away when CONFIG_DEBUG_FS isn't set. */
static int mmc_ios_show(struct seq_file *s, void *data)
{
@@ -159,23 +178,6 @@ static int mmc_clock_opt_set(void *data, u64 val)
return 0;
}
-#ifdef CONFIG_FAIL_MMC_REQUEST
-
-static DECLARE_FAULT_ATTR(fail_mmc_request);
-
-#ifdef KERNEL
-/*
- * Internal function. Pass the boot param fail_mmc_request to
- * the setup fault injection attributes routine.
- */
-static int __init setup_fail_mmc_request(char *str)
-{
- return setup_fault_attr(fail_mmc_request, str);
-}
-__setup(fail_mmc_request=, setup_fail_mmc_request);
-#endif /* KERNEL */
-#endif /* CONFIG_FAIL_MMC_REQUEST */
-
DEFINE_SIMPLE_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
%llu\n);
@@ -207,7 +209,7 @@ void mmc_add_host_debugfs(struct mmc_host *host)
goto err_node;
#endif
#ifdef CONFIG_FAIL_MMC_REQUEST
- host-fail_mmc_request = fail_mmc_request;
+ host-fail_mmc_request = fail_default_attr;
if (IS_ERR(fault_create_debugfs_attr(fail_mmc_request,
root,
host-fail_mmc_request)))
--
1.7.4.1
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/3] fault-injection: update documentation with the mmc module param
Signed-off-by: Per Forlin per.for...@linaro.org --- Documentation/fault-injection/fault-injection.txt |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/Documentation/fault-injection/fault-injection.txt b/Documentation/fault-injection/fault-injection.txt index 70f924e..ba4be8b 100644 --- a/Documentation/fault-injection/fault-injection.txt +++ b/Documentation/fault-injection/fault-injection.txt @@ -121,7 +121,7 @@ use the boot option: failslab= fail_page_alloc= fail_make_request= - fail_mmc_request=interval,probability,space,times + mmc_core.fail_request=interval,probability,space,times How to add new fault injection capability - -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] mmc: add module param to set fault injection attributes
On 14 September 2011 12:05, Akinobu Mita akinobu.m...@gmail.com wrote:
2011/9/14 Per Forlin per.for...@linaro.org:
+#ifdef CONFIG_FAIL_MMC_REQUEST
+
+static DECLARE_FAULT_ATTR(fail_default_attr);
+static char *fail_request;
This is not used anymore and ...
Yes of course. Will remove it.
+static int fail_mmc_request_param_set(const char *val,
+ const struct kernel_param *kp)
+{
+ setup_fault_attr(fail_default_attr, (char *) val);
+ return 0;
+}
+
+static const struct kernel_param_ops fail_mmc_request_param_ops = {
+ .set = fail_mmc_request_param_set
+};
+module_param_cb(fail_request, fail_mmc_request_param_ops,
+ fail_request, 0);
you can change this third parameter to NULL.
Absolutely..
Thanks,
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] mmc: add module param to set fault injection attributes
On 14 September 2011 12:18, Per Forlin per.for...@linaro.org wrote:
On 14 September 2011 12:05, Akinobu Mita akinobu.m...@gmail.com wrote:
2011/9/14 Per Forlin per.for...@linaro.org:
+#ifdef CONFIG_FAIL_MMC_REQUEST
+
+static DECLARE_FAULT_ATTR(fail_default_attr);
+static char *fail_request;
This is not used anymore and ...
Yes of course. Will remove it.
+static int fail_mmc_request_param_set(const char *val,
+ const struct kernel_param *kp)
+{
+ setup_fault_attr(fail_default_attr, (char *) val);
I am thinking of returning failure here if setup_fault_attr() fails.
if (setup_fault_attr(fail_default_attr, (char *) val) == 0)
return -EINVAL;
There will be a printk(KERN_WARNING FAULT_INJECTION: failed to parse
arguments) it setup() fails. Is it too harsh to return failure?
Regards,
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] mmc: add module param to set fault injection attributes
On 14 September 2011 12:38, Per Forlin per.for...@linaro.org wrote:
On 14 September 2011 12:18, Per Forlin per.for...@linaro.org wrote:
On 14 September 2011 12:05, Akinobu Mita akinobu.m...@gmail.com wrote:
2011/9/14 Per Forlin per.for...@linaro.org:
+#ifdef CONFIG_FAIL_MMC_REQUEST
+
+static DECLARE_FAULT_ATTR(fail_default_attr);
+static char *fail_request;
This is not used anymore and ...
Yes of course. Will remove it.
+static int fail_mmc_request_param_set(const char *val,
+ const struct kernel_param *kp)
+{
+ setup_fault_attr(fail_default_attr, (char *) val);
I am thinking of returning failure here if setup_fault_attr() fails.
if (setup_fault_attr(fail_default_attr, (char *) val) == 0)
return -EINVAL;
There will be a printk(KERN_WARNING FAULT_INJECTION: failed to parse
arguments) it setup() fails. Is it too harsh to return failure?
If error is returned here the kernel prints: invalid for parameter
`mmc_core.fail_request'
This piece of information is a reason for returning error on failure.
Regards,
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 0/3] mmc: rectify boot param option for fault injection
Akinobu Mita reported that the boot option for mmc fault injection is never compiled in due to a fauly ifdef KERNEL that is never set. A correct ifdef would be ifndef MODULE. This patch set adds a module_param_cb() and thereby no ifndef MODULE is needed. Using a module_param makes it possible to pass default fault attributes for external modules too. This patch set is for 3.2 Change log: v2 - use module_param_cb() to set default fault attributes - fix spelling of documentation in patch #3 v3 - remove unused variable and return error if invalid boot param. Per Forlin (3): fault-inject: export setup_fault_attr() mmc: add module param to set fault injection attributes fault-injection: update documentation with the mmc module param Documentation/fault-injection/fault-injection.txt |2 +- drivers/mmc/core/debugfs.c| 38 +++-- lib/fault-inject.c|3 +- 3 files changed, 23 insertions(+), 20 deletions(-) -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/3] fault-inject: export setup_fault_attr()
mmc_core module needs to use setup_fault_attr() in order
to set fault injection attributes during module load time.
Signed-off-by: Per Forlin per.for...@linaro.org
---
lib/fault-inject.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/lib/fault-inject.c b/lib/fault-inject.c
index 328d433..4f75540 100644
--- a/lib/fault-inject.c
+++ b/lib/fault-inject.c
@@ -14,7 +14,7 @@
* setup_fault_attr() is a helper function for various __setup handlers, so it
* returns 0 on error, because that is what __setup handlers do.
*/
-int __init setup_fault_attr(struct fault_attr *attr, char *str)
+int setup_fault_attr(struct fault_attr *attr, char *str)
{
unsigned long probability;
unsigned long interval;
@@ -36,6 +36,7 @@ int __init setup_fault_attr(struct fault_attr *attr, char
*str)
return 1;
}
+EXPORT_SYMBOL_GPL(setup_fault_attr);
static void fail_dump(struct fault_attr *attr)
{
--
1.7.4.1
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/3] mmc: add module param to set fault injection attributes
Replace setup(fail_mmc_request) and faulty ifdef KERNEL with
a module_param_cb(). The module param mmc_core.fail_request
may be used to set the fault injection attributes during boot time
or module load time.
Signed-off-by: Per Forlin per.for...@linaro.org
---
drivers/mmc/core/debugfs.c | 38 --
1 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index 5acd707..13e44c9 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -20,6 +20,25 @@
#include core.h
#include mmc_ops.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+
+static DECLARE_FAULT_ATTR(fail_default_attr);
+static int fail_mmc_request_param_set(const char *val,
+ const struct kernel_param *kp)
+{
+ if (setup_fault_attr(fail_default_attr, (char *) val) == 0)
+ return -EINVAL;
+
+ return 0;
+}
+
+static const struct kernel_param_ops fail_mmc_request_param_ops = {
+ .set = fail_mmc_request_param_set
+};
+module_param_cb(fail_request, fail_mmc_request_param_ops, NULL, 0);
+
+#endif /* CONFIG_FAIL_MMC_REQUEST */
+
/* The debugfs functions are optimized away when CONFIG_DEBUG_FS isn't set. */
static int mmc_ios_show(struct seq_file *s, void *data)
{
@@ -159,23 +178,6 @@ static int mmc_clock_opt_set(void *data, u64 val)
return 0;
}
-#ifdef CONFIG_FAIL_MMC_REQUEST
-
-static DECLARE_FAULT_ATTR(fail_mmc_request);
-
-#ifdef KERNEL
-/*
- * Internal function. Pass the boot param fail_mmc_request to
- * the setup fault injection attributes routine.
- */
-static int __init setup_fail_mmc_request(char *str)
-{
- return setup_fault_attr(fail_mmc_request, str);
-}
-__setup(fail_mmc_request=, setup_fail_mmc_request);
-#endif /* KERNEL */
-#endif /* CONFIG_FAIL_MMC_REQUEST */
-
DEFINE_SIMPLE_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
%llu\n);
@@ -207,7 +209,7 @@ void mmc_add_host_debugfs(struct mmc_host *host)
goto err_node;
#endif
#ifdef CONFIG_FAIL_MMC_REQUEST
- host-fail_mmc_request = fail_mmc_request;
+ host-fail_mmc_request = fail_default_attr;
if (IS_ERR(fault_create_debugfs_attr(fail_mmc_request,
root,
host-fail_mmc_request)))
--
1.7.4.1
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/3] fault-injection: update documentation with the mmc module param
Signed-off-by: Per Forlin per.for...@linaro.org --- Documentation/fault-injection/fault-injection.txt |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/Documentation/fault-injection/fault-injection.txt b/Documentation/fault-injection/fault-injection.txt index 70f924e..ba4be8b 100644 --- a/Documentation/fault-injection/fault-injection.txt +++ b/Documentation/fault-injection/fault-injection.txt @@ -121,7 +121,7 @@ use the boot option: failslab= fail_page_alloc= fail_make_request= - fail_mmc_request=interval,probability,space,times + mmc_core.fail_request=interval,probability,space,times How to add new fault injection capability - -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] omap: hsmmc: Normalize dma cleanup operations
On 14 September 2011 15:40, S, Venkatraman svenk...@ti.com wrote: On Tue, Sep 13, 2011 at 1:26 AM, Per Forlin per.for...@linaro.org wrote: On 1 September 2011 21:05, Venkatraman S svenk...@ti.com wrote: Reuse omap_hsmmc_dma_cleanup even for normal dma teardown in omap_hsmmc_dma_cb. Consolidate multiple points of dma unmap into a single location in post_req function, to prevent double unmapping. It's optional to use pre_req() and post_req(). The SDIO framework doesn't utilise these hooks. For instance this wont work together with SDIO-wlan on the pandaboard. If pre_req() has been issued it's fine to defer dma_unmap() until post_req(). If pre_req() is not called the driver should call dma_unmap() directly. In that case, can the actual 'request' function just call pre_req and post_req (at the beginning and at the end), if host_cookie is not set ? Which request() function do your refer to? The mmc_host_ops.request() returns before the transfer is finished. Apart from that, I would say yes. It's nice to have the same code path for valid cookie and invalid cookie scenario. Note that the host_cookie could be set but with an invalid value. host driver: 1. validate cookie 2. if invalid cookie call pre_req() 3. run request 4. done - if (has called pre_req() #2) - call post_req(). A general fix would be to put this logic in mmc_wait_for_req() in core.c, but at this level it's not possible to validate the cookie. In the current implementation the host driver controls the cookie value. /Per -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 2/3] mmc: core: add random fault injection
On 13 September 2011 15:12, Akinobu Mita akinobu.m...@gmail.com wrote:
2011/8/19 Per Forlin per.for...@stericsson.com:
+#ifdef KERNEL
+/*
+ * Internal function. Pass the boot param fail_mmc_request to
+ * the setup fault injection attributes routine.
+ */
+static int __init setup_fail_mmc_request(char *str)
+{
+ return setup_fault_attr(fail_mmc_request, str);
+}
+__setup(fail_mmc_request=, setup_fail_mmc_request);
+#endif /* KERNEL */
You attempt to enable __setup() only when mmc_core is built into
the kernel. Does it really work? I cannot find any drivers using
KERNEL macro.
Your right it doesn't work. I think I change from ifndef MODULE to
ifdef KERNEL at one point.
It should be ifndef MODULE
You can use module_param_cb() instead of __setup() without #ifdef KERNEL.
When mmc_core is built into the kernel, you can specify the parameter
with mmc_core.fail_mmc_request=...
Thanks, this is the proper way to do it.
Sorry for pointing that out too late.
I think this patch is queued for 3.2 so there should be time to fix it still.
Thanks again,
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 2/3] mmc: core: add random fault injection
Hi Akinobu,
On 13 September 2011 16:19, Per Forlin per.for...@linaro.org wrote:
On 13 September 2011 15:12, Akinobu Mita akinobu.m...@gmail.com wrote:
2011/8/19 Per Forlin per.for...@stericsson.com:
+#ifdef KERNEL
+/*
+ * Internal function. Pass the boot param fail_mmc_request to
+ * the setup fault injection attributes routine.
+ */
+static int __init setup_fail_mmc_request(char *str)
+{
+ return setup_fault_attr(fail_mmc_request, str);
+}
+__setup(fail_mmc_request=, setup_fail_mmc_request);
+#endif /* KERNEL */
You attempt to enable __setup() only when mmc_core is built into
the kernel. Does it really work? I cannot find any drivers using
KERNEL macro.
Your right it doesn't work. I think I change from ifndef MODULE to
ifdef KERNEL at one point.
It should be ifndef MODULE
You can use module_param_cb() instead of __setup() without #ifdef KERNEL.
When mmc_core is built into the kernel, you can specify the parameter
with mmc_core.fail_mmc_request=...
I am considering using module_param() with perm = 0 (not visible in
sysfs). The purpose of the param is to set fault attributes during
kerne boot time or module load time. After the kernel boot all can be
set under debug fs, therefore no need to make the module param
visible.
What do you think about this? I have not tested it yet.
--
+++ b/drivers/mmc/core/debugfs.c
@@ -20,6 +20,14 @@
#include core.h
#include mmc_ops.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+
+static DECLARE_FAULT_ATTR(fail_mmc_default_attr);
+static char *fail_mmc_request;
+module_param(fail_mmc_request, charp, 0);
+
+#endif /* CONFIG_FAIL_MMC_REQUEST */
+
/* The debugfs functions are optimized away when CONFIG_DEBUG_FS isn't set. */
static int mmc_ios_show(struct seq_file *s, void *data)
{
@@ -159,23 +167,6 @@ static int mmc_clock_opt_set(void *data, u64 val)
return 0;
}
-#ifdef CONFIG_FAIL_MMC_REQUEST
-
-static DECLARE_FAULT_ATTR(fail_mmc_request);
-
-#ifdef KERNEL
-/*
- * Internal function. Pass the boot param fail_mmc_request to
- * the setup fault injection attributes routine.
- */
-static int __init setup_fail_mmc_request(char *str)
-{
- return setup_fault_attr(fail_mmc_request, str);
-}
-__setup(fail_mmc_request=, setup_fail_mmc_request);
-#endif /* KERNEL */
-#endif /* CONFIG_FAIL_MMC_REQUEST */
-
DEFINE_SIMPLE_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
%llu\n);
@@ -207,7 +198,9 @@ void mmc_add_host_debugfs(struct mmc_host *host)
goto err_node;
#endif
#ifdef CONFIG_FAIL_MMC_REQUEST
- host-fail_mmc_request = fail_mmc_request;
+ if (fail_mmc_request)
+ setup_fault_attr(fail_mmc_default_attr, fail_mmc_request);
+ host-fail_mmc_request = fail_mmc_default_attr;
if (IS_ERR(fault_create_debugfs_attr(fail_mmc_request,
root,
host-fail_mmc_request)))
--
It's only necessary to call setup_fault_attr() once for all hosts.
Here it's called one time for each host. I think it's ok since the
routine is small and used for debugging purposes.
I could use a static bool to indicate whether setup_fault_attr() has
already been issued.
+ if (fail_mmc_request !setup_fault_attr_done)
Regards,
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] fault-inject: export setup_fault_attr()
mmc_core module needs to use setup_fault_attr() in order
to set fault injection attributes during module load time.
Signed-off-by: Per Forlin per.for...@linaro.org
---
lib/fault-inject.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/lib/fault-inject.c b/lib/fault-inject.c
index 328d433..4f75540 100644
--- a/lib/fault-inject.c
+++ b/lib/fault-inject.c
@@ -14,7 +14,7 @@
* setup_fault_attr() is a helper function for various __setup handlers, so it
* returns 0 on error, because that is what __setup handlers do.
*/
-int __init setup_fault_attr(struct fault_attr *attr, char *str)
+int setup_fault_attr(struct fault_attr *attr, char *str)
{
unsigned long probability;
unsigned long interval;
@@ -36,6 +36,7 @@ int __init setup_fault_attr(struct fault_attr *attr, char
*str)
return 1;
}
+EXPORT_SYMBOL_GPL(setup_fault_attr);
static void fail_dump(struct fault_attr *attr)
{
--
1.7.4.1
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] mmc: add module param to set fault injection attributes
Replace setup(fail_mmc_request) and faulty ifdef KERNEL with
a simple module_param(). The module param mmc_core.fail_request
may be used to set the fault injection attributes during boot time
or module load time.
Signed-off-by: Per Forlin per.for...@linaro.org
---
drivers/mmc/core/debugfs.c | 29 +++--
1 files changed, 11 insertions(+), 18 deletions(-)
diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index 5acd707..bb72ba2 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -20,6 +20,14 @@
#include core.h
#include mmc_ops.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+
+static DECLARE_FAULT_ATTR(fail_default_attr);
+static char *fail_request;
+module_param(fail_request, charp, 0);
+
+#endif /* CONFIG_FAIL_MMC_REQUEST */
+
/* The debugfs functions are optimized away when CONFIG_DEBUG_FS isn't set. */
static int mmc_ios_show(struct seq_file *s, void *data)
{
@@ -159,23 +167,6 @@ static int mmc_clock_opt_set(void *data, u64 val)
return 0;
}
-#ifdef CONFIG_FAIL_MMC_REQUEST
-
-static DECLARE_FAULT_ATTR(fail_mmc_request);
-
-#ifdef KERNEL
-/*
- * Internal function. Pass the boot param fail_mmc_request to
- * the setup fault injection attributes routine.
- */
-static int __init setup_fail_mmc_request(char *str)
-{
- return setup_fault_attr(fail_mmc_request, str);
-}
-__setup(fail_mmc_request=, setup_fail_mmc_request);
-#endif /* KERNEL */
-#endif /* CONFIG_FAIL_MMC_REQUEST */
-
DEFINE_SIMPLE_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
%llu\n);
@@ -207,7 +198,9 @@ void mmc_add_host_debugfs(struct mmc_host *host)
goto err_node;
#endif
#ifdef CONFIG_FAIL_MMC_REQUEST
- host-fail_mmc_request = fail_mmc_request;
+ if (fail_request)
+ setup_fault_attr(fail_default_attr, fail_request);
+ host-fail_mmc_request = fail_default_attr;
if (IS_ERR(fault_create_debugfs_attr(fail_mmc_request,
root,
host-fail_mmc_request)))
--
1.7.4.1
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] fault-injection: update documenation with the mmc module param
Signed-off-by: Per Forlin per.for...@linaro.org --- Documentation/fault-injection/fault-injection.txt |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/Documentation/fault-injection/fault-injection.txt b/Documentation/fault-injection/fault-injection.txt index 70f924e..ba4be8b 100644 --- a/Documentation/fault-injection/fault-injection.txt +++ b/Documentation/fault-injection/fault-injection.txt @@ -121,7 +121,7 @@ use the boot option: failslab= fail_page_alloc= fail_make_request= - fail_mmc_request=interval,probability,space,times + mmc_core.fail_request=interval,probability,space,times How to add new fault injection capability - -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mmc: omap_hsmmc: DMA unmap only once in case of MMC error
On 1 September 2011 21:19, S, Venkatraman svenk...@ti.com wrote:
On Mon, Aug 29, 2011 at 3:38 AM, Per Forlin per.for...@linaro.org wrote:
Reported by Russell King:
mmcblk0: error -84 transferring data, sector 149201, nr 64,
cmd response 0x900, card status 0xb00
mmcblk0: retrying using single block read
WARNING: at /home/rmk/git/linux-2.6-rmk/lib/dma-debug.c:811 check_unmap
omap_hsmmc omap_hsmmc.0: DMA-API: device driver tries to free DMA memory
it has not allocated [device address=0x80933000] [size=20480 bytes]
-
In case of an error dma_unmap() is issued in omap_hsmmc_dma_cleanup()
and then again in omap_hsmmc_post_req(). Resolve this by clearing the
host_cookie to indicate there is no DMA mapped memory to unmap.
Signed-off-by: Per Forlin per.for...@linaro.org
---
Bug fix on 3.1-rc3
drivers/mmc/host/omap_hsmmc.c | 7 +--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 21e4a79..31d9817 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -1011,6 +1011,7 @@ static void omap_hsmmc_dma_cleanup(struct
omap_hsmmc_host *host, int errno)
host-data-sg_len,
omap_hsmmc_get_dma_dir(host, host-data));
omap_free_dma(dma_ch);
+ host-data-host_cookie = 0;
}
host-data = NULL;
}
@@ -1576,8 +1577,10 @@ static void omap_hsmmc_post_req(struct mmc_host *mmc,
struct mmc_request *mrq,
struct mmc_data *data = mrq-data;
if (host-use_dma) {
- dma_unmap_sg(mmc_dev(host-mmc), data-sg, data-sg_len,
- omap_hsmmc_get_dma_dir(host, data));
+ if (data-host_cookie)
+ dma_unmap_sg(mmc_dev(host-mmc), data-sg,
+ data-sg_len,
+ omap_hsmmc_get_dma_dir(host, data));
data-host_cookie = 0;
}
}
--
1.7.4.1
I just posted a patch [1] which just consolidates all unmapping to
just post_req.
This would obviously prevent double unmapping, and keeps all the related
dma operations at a single place. Let me know if that works better.
I'm in favour of such patch. My patch only tries to do the minimal bug fix.
I have been out of office the last 10 days or so. I'll look at your
patch later today.
Thanks,
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] omap: hsmmc: Normalize dma cleanup operations
On 1 September 2011 21:05, Venkatraman S svenk...@ti.com wrote: Reuse omap_hsmmc_dma_cleanup even for normal dma teardown in omap_hsmmc_dma_cb. Consolidate multiple points of dma unmap into a single location in post_req function, to prevent double unmapping. It's optional to use pre_req() and post_req(). The SDIO framework doesn't utilise these hooks. For instance this wont work together with SDIO-wlan on the pandaboard. If pre_req() has been issued it's fine to defer dma_unmap() until post_req(). If pre_req() is not called the driver should call dma_unmap() directly. Regards, Per -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] mmc: clarifications on host.post_req()
Fixes for 3.1. This patchset doesn't fix any bugs in 3.1 but it improves the documentation in order to prevent new bugs. Per Forlin (2): mmc: core: clarify how to use post_req in case of errors mmc: mmci: simplify err check in mmci_post_request drivers/mmc/core/core.c |6 ++ drivers/mmc/host/mmci.c |2 +- include/linux/mmc/host.h |3 +++ 3 files changed, 10 insertions(+), 1 deletions(-) -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] mmc: mmci: simplify err check in mmci_post_request
The error condition indicates that mmci_post_request() should cleanup
after the mmci_pre_request(). In this case the resources allocated by
device_prep_slave_sg() are freed by calling dmaengine_terminate_all().
dma_unmap_sg() should always be performed if the host_cookie is set.
Signed-off-by: Per Forlin per.for...@linaro.org
---
drivers/mmc/host/mmci.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 56e9a41..40e4c05 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -529,7 +529,7 @@ static void mmci_post_request(struct mmc_host *mmc, struct
mmc_request *mrq,
if (chan) {
if (err)
dmaengine_terminate_all(chan);
- if (err || data-host_cookie)
+ if (data-host_cookie)
dma_unmap_sg(mmc_dev(host-mmc), data-sg,
data-sg_len, dir);
mrq-data-host_cookie = 0;
--
1.7.4.1
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 00/12] use nonblock mmc requests to minimize latency
On 26 August 2011 18:28, Santosh santosh.shilim...@ti.com wrote:
+ Balaji,
On Friday 26 August 2011 09:49 PM, Russell King - ARM Linux wrote:
I'm not sure who's responsible for this, but the nonblocking MMC stuff is
broken on OMAPs HSMMC:
mmcblk0: error -84 transferring data, sector 149201, nr 64, cmd response
0x900, card status 0xb00
mmcblk0: retrying using single block read
[ cut here ]
WARNING: at /home/rmk/git/linux-2.6-rmk/lib/dma-debug.c:811
check_unmap+0x1ac/0x764()
omap_hsmmc omap_hsmmc.0: DMA-API: device driver tries to free DMA memory
it has not allocated [device address=0x80933000] [size=20480 bytes]
Modules linked in:
Backtrace:
[c0017874] (dump_backtrace+0x0/0x10c) from [c02ce8ac]
(dump_stack+0x18/0x1c)
r7:c1abfcb8 r6:c0186248 r5:c037de51 r4:032b
[c02ce894] (dump_stack+0x0/0x1c) from [c0039ed4]
(warn_slowpath_common+0x58/0x70)
[c0039e7c] (warn_slowpath_common+0x0/0x70) from [c0039f90]
(warn_slowpath_fmt+0x38/0x40)
r8:c1abfd50 r7: r6:5000 r5: r4:80933000
[c0039f58] (warn_slowpath_fmt+0x0/0x40) from [c0186248]
(check_unmap+0x1ac/0x764)
r3:c0367d55 r2:c037e24d
[c018609c] (check_unmap+0x0/0x764) from [c0186978]
(debug_dma_unmap_sg+0x100/0x134)
[c0186878] (debug_dma_unmap_sg+0x0/0x134) from [c0019770]
(dma_unmap_sg+0x24/0x7c)
[c001974c] (dma_unmap_sg+0x0/0x7c) from [c0207220]
(omap_hsmmc_post_req+0x48/0x54)
[c02071d8] (omap_hsmmc_post_req+0x0/0x54) from [c01fb644]
(mmc_start_req+0x9c/0x128)
r4:c1a76000
[c01fb5a8] (mmc_start_req+0x0/0x128) from [c02049fc]
(mmc_blk_issue_rw_rq+0x80/0x460)
r8:c1ab5c00 r7:0001 r6:c1ab5824 r5:c1ab5824 r4:c1ab5c00
[c020497c] (mmc_blk_issue_rw_rq+0x0/0x460) from [c02050a8]
(mmc_blk_issue_rq+0x2cc/0x2fc)
[c0204ddc] (mmc_blk_issue_rq+0x0/0x2fc) from [c02056c4]
(mmc_queue_thread+0xa0/0x104)
[c0205624] (mmc_queue_thread+0x0/0x104) from [c0054f8c]
(kthread+0x88/0x90)
[c0054f04] (kthread+0x0/0x90) from [c003d9a4] (do_exit+0x0/0x618)
r7:0013 r6:c003d9a4 r5:c0054f04 r4:c1a7bc7c
---[ end trace 3314ad56daf5d14f ]---
Luckily thinks continue to work, but clearly releasing DMA mappings which
drivers don't own is bad news. Unfortunately, I don't have the bandwidth
to be able to investigate this at present - well, I could do but then I'd
have to drop working on the OMAP4 vs generic suspend/resume code and learn
about something I've no current clue about.
Please continue your help on generic suspend.
Can someone please investigate and fix whatever is broken.
Will find somebody to look at this issue.
I had a look at this and my guess is that the same mapped DMA memory
is unmapped twice. First the memory is unmapped in
omap_hsmmc_dma_cleanup() due to the mmc error, then later in
omap_hsmmc_post_req(). Here is a patch to resolve that. I haven't had
the chance to test it yet though.
---
drivers/mmc/host/omap_hsmmc.c |7 +--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 21e4a79..7f40767 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -1011,6 +1011,7 @@ static void omap_hsmmc_dma_cleanup(struct
omap_hsmmc_host *host, int errno)
host-data-sg_len,
omap_hsmmc_get_dma_dir(host, host-data));
omap_free_dma(dma_ch);
+ data-host_cookie = 0;
}
host-data = NULL;
}
@@ -1576,8 +1577,10 @@ static void omap_hsmmc_post_req(struct mmc_host
*mmc, struct mmc_request *mrq,
struct mmc_data *data = mrq-data;
if (host-use_dma) {
- dma_unmap_sg(mmc_dev(host-mmc), data-sg, data-sg_len,
-omap_hsmmc_get_dma_dir(host, data));
+ if (data-host_cookie)
+ dma_unmap_sg(mmc_dev(host-mmc), data-sg,
+data-sg_len,
+omap_hsmmc_get_dma_dir(host, data));
data-host_cookie = 0;
I also checked the mmci code for this same issue and mmci doesn't have
the same bug.
MMCI works fine in 3.1-rc2 even though the code is wrong. I propose
this change.
@@ -529,7 +529,7 @@ static void mmci_post_request(struct mmc_host
*mmc, struct mmc_request *mrq,
if (chan) {
if (err)
dmaengine_terminate_all(chan);
- if (err || data-host_cookie)
+ if (data-host_cookie)
I'll post these patches as soon as I have managed to test them or
sooner if I get a tested-by.
Regards,
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 00/12] use nonblock mmc requests to minimize latency
On 28 August 2011 13:11, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
On Sun, Aug 28, 2011 at 12:50:54PM +0200, Per Forlin wrote:
On 26 August 2011 18:28, Santosh santosh.shilim...@ti.com wrote:
+ Balaji,
On Friday 26 August 2011 09:49 PM, Russell King - ARM Linux wrote:
I'm not sure who's responsible for this, but the nonblocking MMC stuff is
broken on OMAPs HSMMC:
mmcblk0: error -84 transferring data, sector 149201, nr 64, cmd response
0x900, card status 0xb00
mmcblk0: retrying using single block read
[ cut here ]
WARNING: at /home/rmk/git/linux-2.6-rmk/lib/dma-debug.c:811
check_unmap+0x1ac/0x764()
omap_hsmmc omap_hsmmc.0: DMA-API: device driver tries to free DMA memory
it has not allocated [device address=0x80933000] [size=20480
bytes]
Modules linked in:
Backtrace:
[c0017874] (dump_backtrace+0x0/0x10c) from [c02ce8ac]
(dump_stack+0x18/0x1c)
r7:c1abfcb8 r6:c0186248 r5:c037de51 r4:032b
[c02ce894] (dump_stack+0x0/0x1c) from [c0039ed4]
(warn_slowpath_common+0x58/0x70)
[c0039e7c] (warn_slowpath_common+0x0/0x70) from [c0039f90]
(warn_slowpath_fmt+0x38/0x40)
r8:c1abfd50 r7: r6:5000 r5: r4:80933000
[c0039f58] (warn_slowpath_fmt+0x0/0x40) from [c0186248]
(check_unmap+0x1ac/0x764)
r3:c0367d55 r2:c037e24d
[c018609c] (check_unmap+0x0/0x764) from [c0186978]
(debug_dma_unmap_sg+0x100/0x134)
[c0186878] (debug_dma_unmap_sg+0x0/0x134) from [c0019770]
(dma_unmap_sg+0x24/0x7c)
[c001974c] (dma_unmap_sg+0x0/0x7c) from [c0207220]
(omap_hsmmc_post_req+0x48/0x54)
[c02071d8] (omap_hsmmc_post_req+0x0/0x54) from [c01fb644]
(mmc_start_req+0x9c/0x128)
r4:c1a76000
[c01fb5a8] (mmc_start_req+0x0/0x128) from [c02049fc]
(mmc_blk_issue_rw_rq+0x80/0x460)
r8:c1ab5c00 r7:0001 r6:c1ab5824 r5:c1ab5824 r4:c1ab5c00
[c020497c] (mmc_blk_issue_rw_rq+0x0/0x460) from [c02050a8]
(mmc_blk_issue_rq+0x2cc/0x2fc)
[c0204ddc] (mmc_blk_issue_rq+0x0/0x2fc) from [c02056c4]
(mmc_queue_thread+0xa0/0x104)
[c0205624] (mmc_queue_thread+0x0/0x104) from [c0054f8c]
(kthread+0x88/0x90)
[c0054f04] (kthread+0x0/0x90) from [c003d9a4] (do_exit+0x0/0x618)
r7:0013 r6:c003d9a4 r5:c0054f04 r4:c1a7bc7c
---[ end trace 3314ad56daf5d14f ]---
Luckily thinks continue to work, but clearly releasing DMA mappings which
drivers don't own is bad news. Unfortunately, I don't have the bandwidth
to be able to investigate this at present - well, I could do but then I'd
have to drop working on the OMAP4 vs generic suspend/resume code and learn
about something I've no current clue about.
Please continue your help on generic suspend.
Can someone please investigate and fix whatever is broken.
Will find somebody to look at this issue.
I had a look at this and my guess is that the same mapped DMA memory
is unmapped twice. First the memory is unmapped in
omap_hsmmc_dma_cleanup() due to the mmc error, then later in
omap_hsmmc_post_req(). Here is a patch to resolve that. I haven't had
the chance to test it yet though.
---
drivers/mmc/host/omap_hsmmc.c | 7 +--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 21e4a79..7f40767 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -1011,6 +1011,7 @@ static void omap_hsmmc_dma_cleanup(struct
omap_hsmmc_host *host, int errno)
host-data-sg_len,
omap_hsmmc_get_dma_dir(host, host-data));
omap_free_dma(dma_ch);
+ data-host_cookie = 0;
}
host-data = NULL;
}
@@ -1576,8 +1577,10 @@ static void omap_hsmmc_post_req(struct mmc_host
*mmc, struct mmc_request *mrq,
struct mmc_data *data = mrq-data;
if (host-use_dma) {
- dma_unmap_sg(mmc_dev(host-mmc), data-sg, data-sg_len,
- omap_hsmmc_get_dma_dir(host, data));
+ if (data-host_cookie)
+ dma_unmap_sg(mmc_dev(host-mmc), data-sg,
+ data-sg_len,
+ omap_hsmmc_get_dma_dir(host, data));
data-host_cookie = 0;
I also checked the mmci code for this same issue and mmci doesn't have
the same bug.
MMCI works fine in 3.1-rc2 even though the code is wrong. I propose
this change.
@@ -529,7 +529,7 @@ static void mmci_post_request(struct mmc_host
*mmc, struct mmc_request *mrq,
if (chan) {
if (err)
dmaengine_terminate_all(chan);
- if (err || data-host_cookie)
+ if (data-host_cookie)
I'll post these patches as soon as I have managed to test them or
sooner if I get a tested-by.
Looking at MMCI, are you sure all those DMA engine terminate calls are
correct?
I should probably do so together with adding better
[PATCH] mmc: omap_hsmmc: DMA unmap only once in case of MMC error
Reported by Russell King:
mmcblk0: error -84 transferring data, sector 149201, nr 64,
cmd response 0x900, card status 0xb00
mmcblk0: retrying using single block read
WARNING: at /home/rmk/git/linux-2.6-rmk/lib/dma-debug.c:811 check_unmap
omap_hsmmc omap_hsmmc.0: DMA-API: device driver tries to free DMA memory
it has not allocated [device address=0x80933000] [size=20480 bytes]
-
In case of an error dma_unmap() is issued in omap_hsmmc_dma_cleanup()
and then again in omap_hsmmc_post_req(). Resolve this by clearing the
host_cookie to indicate there is no DMA mapped memory to unmap.
Signed-off-by: Per Forlin per.for...@linaro.org
---
Bug fix on 3.1-rc3
drivers/mmc/host/omap_hsmmc.c |7 +--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 21e4a79..31d9817 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -1011,6 +1011,7 @@ static void omap_hsmmc_dma_cleanup(struct omap_hsmmc_host
*host, int errno)
host-data-sg_len,
omap_hsmmc_get_dma_dir(host, host-data));
omap_free_dma(dma_ch);
+ host-data-host_cookie = 0;
}
host-data = NULL;
}
@@ -1576,8 +1577,10 @@ static void omap_hsmmc_post_req(struct mmc_host *mmc,
struct mmc_request *mrq,
struct mmc_data *data = mrq-data;
if (host-use_dma) {
- dma_unmap_sg(mmc_dev(host-mmc), data-sg, data-sg_len,
-omap_hsmmc_get_dma_dir(host, data));
+ if (data-host_cookie)
+ dma_unmap_sg(mmc_dev(host-mmc), data-sg,
+data-sg_len,
+omap_hsmmc_get_dma_dir(host, data));
data-host_cookie = 0;
}
}
--
1.7.4.1
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH --mmotm v8 0/3] Make fault injection available for MMC IO
Hi Chris, It's no longer necessary to merge this through the mm-tree since Akinobu's patch fault-injection: add ability to export fault_attr in arbitrary directory is in mainline. Chris, would you mind merging the fault-injection patches in this patchset to mmc-next once the mmc part of this patchset is acked and accepted? Regards, Per On 9 August 2011 14:07, Per Forlin per.for...@linaro.org wrote: change log: v2 - Resolve build issue in mmc core.c due to multiple init_module by removing the fault inject module. - Export fault injection functions to make them available for modules - Update fault injection documentation on MMC IO v3 - add function descriptions in core.c - use export GPL for fault injection functions v4 - make the fault_attr per host. This prepares for upcoming patch from Akinobu that adds support for creating debugfs entries in arbitrary directory. v5 - Make use of fault_create_debugfs_attr() in Akinobu's patch fault-injection: add ability to export fault_attr in v6 - Fix typo in commit message in patch export fault injection functions v7 - Don't compile in boot param setup function if mmc-core is built as module. v8 - Update fault injection documentation. Add fail_mmc_request to boot option section. Per Forlin (3): fault-inject: export fault injection functions mmc: core: add random fault injection fault injection: add documentation on MMC IO fault injection Documentation/fault-injection/fault-injection.txt | 8 +++- drivers/mmc/core/core.c | 44 + drivers/mmc/core/debugfs.c | 27 + include/linux/mmc/host.h | 7 +++ lib/Kconfig.debug | 11 + lib/fault-inject.c | 2 + 6 files changed, 98 insertions(+), 1 deletions(-) -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH --mmotm v8 2/3] mmc: core: add random fault injection
On 19 August 2011 13:40, Linus Walleij linus.wall...@linaro.org wrote:
On Tue, Aug 9, 2011 at 2:07 PM, Per Forlin per.for...@linaro.org wrote:
This adds support to inject data errors after a completed host transfer.
The mmc core will return error even though the host transfer is successful.
This simple fault injection proved to be very useful to test the
non-blocking error handling in the mmc_blk_issue_rw_rq().
Random faults can also test how the host driver handles pre_req()
and post_req() in case of errors.
Good idea!
Thanks.
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 89bdeae..a4996b0 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -25,6 +25,11 @@
#include linux/pm_runtime.h
#include linux/suspend.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+#include linux/fault-inject.h
+#include linux/random.h
+#endif
You don't need to #ifdef around the #include stuff, and if you
do, something is wrong with those headers. It's just a bunch of defines
that aren't used in some circumstances. Stack them with the others,
simply, just #ifdef the code below.
I added them after suggestion from J Freyensee. I am also in favor of
no ifdefs here. I'll remove them in the next patchset unless James has
any strong objections.
@@ -83,6 +88,43 @@ static void mmc_flush_scheduled_work(void)
flush_workqueue(workqueue);
}
+#ifdef CONFIG_FAIL_MMC_REQUEST
+
+/*
+ * Internal function. Inject random data errors.
+ * If mmc_data is NULL no errors are injected.
+ */
+static void mmc_should_fail_request(struct mmc_host *host,
+ struct mmc_request *mrq)
+{
+ struct mmc_command *cmd = mrq-cmd;
+ struct mmc_data *data = mrq-data;
+ static const int data_errors[] = {
+ -ETIMEDOUT,
+ -EILSEQ,
+ -EIO,
+ };
+
+ if (!data)
+ return;
+
+ if (cmd-error || data-error ||
+ !should_fail(host-fail_mmc_request, data-blksz *
data-blocks))
+ return;
+
+ data-error = data_errors[random32() % ARRAY_SIZE(data_errors)];
+ data-bytes_xfered = (random32() % (data-bytes_xfered 9)) 9;
+}
+
+#else /* CONFIG_FAIL_MMC_REQUEST */
+
+static void mmc_should_fail_request(struct mmc_host *host,
+ struct mmc_request *mrq)
Should be static inline so we know it will be folded in and nullified
by the compiler, lots of kernel code use that pattern.
I'll fix.
diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index f573753..189581d 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -13,6 +13,9 @@
#include linux/seq_file.h
#include linux/slab.h
#include linux/stat.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+#include linux/fault-inject.h
+#endif
No #ifdef:ing...
I'll remove it.
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 0f83858..ee472fe 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -12,6 +12,9 @@
#include linux/leds.h
#include linux/sched.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+#include linux/fault-inject.h
+#endif
Neither here...
dito
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 47879c7..ebff0c9 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
I'm contemplating if we should create drivers/mmc/Kconfig.debug
and stash this in there instead, i.e. also move out MMC_DEBUG
from drivers/mmc/Kconfig and add to that?
It seems more apropriate to select this from the MMC subsystem.
However the core of fault injection is in lib/
So maybe a simple:
config FAIL_MMC_REQUEST
bool
select FAULT_INJECTION
That can then be selected by a debug option in the MMC subsystem?
I fear it may be hard to find this otherwise...
(NB: I have very little clue how the Kconfig.debug files get sourced
into the Kbuild so I might be misguided...)
The FAIL_MMC_REQUEST sits right next to the rest of the fail injection
functions.
config FAILSLAB
depends on FAULT_INJECTION
depends on SLAB || SLUB
config FAIL_PAGE_ALLOC
depends on FAULT_INJECTION
config FAIL_MAKE_REQUEST
depends on FAULT_INJECTION BLOCK
config FAIL_IO_TIMEOUT
depends on FAULT_INJECTION BLOCK
config FAIL_MMC_REQUEST
select DEBUG_FS
depends on FAULT_INJECTION MMC
I think the proper place is to have it here together with the rest.
@@ -1090,6 +1090,17 @@ config FAIL_IO_TIMEOUT
Only works with drivers that use the generic timeout handling,
for others it wont do anything.
+config FAIL_MMC_REQUEST
+ bool Fault-injection capability for MMC IO
+ select DEBUG_FS
+ depends on FAULT_INJECTION MMC
Isn't:
depends on MMC
select FAULT_INJECTION
Simpler to use? Now you have to select fault injection first
to even see this option right?
In menuconfig you have to select
[PATCH v9 0/3] mmc: make fault injection available for MMC IO
From: Per Forlin per.for...@linaro.org change log: v2 - Resolve build issue in mmc core.c due to multiple init_module by removing the fault inject module. - Export fault injection functions to make them available for modules - Update fault injection documentation on MMC IO v3 - add function descriptions in core.c - use export GPL for fault injection functions v4 - make the fault_attr per host. This prepares for upcoming patch from Akinobu that adds support for creating debugfs entries in arbitrary directory. v5 - Make use of fault_create_debugfs_attr() in Akinobu's patch fault-injection: add ability to export fault_attr in v6 - Fix typo in commit message in patch export fault injection functions v7 - Don't compile in boot param setup function if mmc-core is built as module. v8 - Update fault injection documentation. Add fail_mmc_request to boot option section. v9 - remove ifdef around include files and inline empty function, comments from Linus Walleij. Per Forlin (3): fault-inject: export fault injection functions mmc: core: add random fault injection fault injection: add documentation on MMC IO fault injection Documentation/fault-injection/fault-injection.txt |8 - drivers/mmc/core/core.c | 41 + drivers/mmc/core/debugfs.c| 25 + include/linux/mmc/host.h |5 +++ lib/Kconfig.debug | 11 ++ lib/fault-inject.c|2 + 6 files changed, 91 insertions(+), 1 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 1/3] fault-inject: export fault injection functions
From: Per Forlin per.for...@linaro.org export symbols should_fail() and fault_create_debugfs_attr() in order to let modules utilize the fault injection Signed-off-by: Per Forlin per.for...@linaro.org Acked-by: Akinobu Mita akinobu.m...@gmail.com --- lib/fault-inject.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/lib/fault-inject.c b/lib/fault-inject.c index f193b77..328d433 100644 --- a/lib/fault-inject.c +++ b/lib/fault-inject.c @@ -130,6 +130,7 @@ bool should_fail(struct fault_attr *attr, ssize_t size) return true; } +EXPORT_SYMBOL_GPL(should_fail); #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS @@ -243,5 +244,6 @@ fail: return ERR_PTR(-ENOMEM); } +EXPORT_SYMBOL_GPL(fault_create_debugfs_attr); #endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */ -- 1.6.3.3 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 3/3] fault injection: add documentation on MMC IO fault injection
From: Per Forlin per.for...@linaro.org Add description on how to enable random fault injection for MMC IO Signed-off-by: Per Forlin per.for...@linaro.org Acked-by: Akinobu Mita akinobu.m...@gmail.com --- Documentation/fault-injection/fault-injection.txt |8 +++- 1 files changed, 7 insertions(+), 1 deletions(-) diff --git a/Documentation/fault-injection/fault-injection.txt b/Documentation/fault-injection/fault-injection.txt index 82a5d25..70f924e 100644 --- a/Documentation/fault-injection/fault-injection.txt +++ b/Documentation/fault-injection/fault-injection.txt @@ -21,6 +21,11 @@ o fail_make_request /sys/block/device/make-it-fail or /sys/block/device/partition/make-it-fail. (generic_make_request()) +o fail_mmc_request + + injects MMC data errors on devices permitted by setting + debugfs entries under /sys/kernel/debug/mmc0/fail_mmc_request + Configure fault-injection capabilities behavior --- @@ -115,7 +120,8 @@ use the boot option: failslab= fail_page_alloc= - fail_make_request=interval,probability,space,times + fail_make_request= + fail_mmc_request=interval,probability,space,times How to add new fault injection capability - -- 1.6.3.3 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v9 2/3] mmc: core: add random fault injection
From: Per Forlin per.for...@linaro.org
This adds support to inject data errors after a completed host transfer.
The mmc core will return error even though the host transfer is successful.
This simple fault injection proved to be very useful to test the
non-blocking error handling in the mmc_blk_issue_rw_rq().
Random faults can also test how the host driver handles pre_req()
and post_req() in case of errors.
Signed-off-by: Per Forlin per.for...@linaro.org
Acked-by: Akinobu Mita akinobu.m...@gmail.com
---
drivers/mmc/core/core.c| 41 +
drivers/mmc/core/debugfs.c | 25 +
include/linux/mmc/host.h |5 +
lib/Kconfig.debug | 11 +++
4 files changed, 82 insertions(+), 0 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 91a0a74..d704dfa 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -24,6 +24,8 @@
#include linux/regulator/consumer.h
#include linux/pm_runtime.h
#include linux/suspend.h
+#include linux/fault-inject.h
+#include linux/random.h
#include linux/mmc/card.h
#include linux/mmc/host.h
@@ -83,6 +85,43 @@ static void mmc_flush_scheduled_work(void)
flush_workqueue(workqueue);
}
+#ifdef CONFIG_FAIL_MMC_REQUEST
+
+/*
+ * Internal function. Inject random data errors.
+ * If mmc_data is NULL no errors are injected.
+ */
+static void mmc_should_fail_request(struct mmc_host *host,
+ struct mmc_request *mrq)
+{
+ struct mmc_command *cmd = mrq-cmd;
+ struct mmc_data *data = mrq-data;
+ static const int data_errors[] = {
+ -ETIMEDOUT,
+ -EILSEQ,
+ -EIO,
+ };
+
+ if (!data)
+ return;
+
+ if (cmd-error || data-error ||
+ !should_fail(host-fail_mmc_request, data-blksz * data-blocks))
+ return;
+
+ data-error = data_errors[random32() % ARRAY_SIZE(data_errors)];
+ data-bytes_xfered = (random32() % (data-bytes_xfered 9)) 9;
+}
+
+#else /* CONFIG_FAIL_MMC_REQUEST */
+
+static inline void mmc_should_fail_request(struct mmc_host *host,
+ struct mmc_request *mrq)
+{
+}
+
+#endif /* CONFIG_FAIL_MMC_REQUEST */
+
/**
* mmc_request_done - finish processing an MMC request
* @host: MMC host which completed request
@@ -109,6 +148,8 @@ void mmc_request_done(struct mmc_host *host, struct
mmc_request *mrq)
cmd-error = 0;
host-ops-request(host, mrq);
} else {
+ mmc_should_fail_request(host, mrq);
+
led_trigger_event(host-led, LED_OFF);
pr_debug(%s: req done (CMD%u): %d: %08x %08x %08x %08x\n,
diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index 998797e..5acd707 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -12,6 +12,7 @@
#include linux/seq_file.h
#include linux/slab.h
#include linux/stat.h
+#include linux/fault-inject.h
#include linux/mmc/card.h
#include linux/mmc/host.h
@@ -158,6 +159,23 @@ static int mmc_clock_opt_set(void *data, u64 val)
return 0;
}
+#ifdef CONFIG_FAIL_MMC_REQUEST
+
+static DECLARE_FAULT_ATTR(fail_mmc_request);
+
+#ifdef KERNEL
+/*
+ * Internal function. Pass the boot param fail_mmc_request to
+ * the setup fault injection attributes routine.
+ */
+static int __init setup_fail_mmc_request(char *str)
+{
+ return setup_fault_attr(fail_mmc_request, str);
+}
+__setup(fail_mmc_request=, setup_fail_mmc_request);
+#endif /* KERNEL */
+#endif /* CONFIG_FAIL_MMC_REQUEST */
+
DEFINE_SIMPLE_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
%llu\n);
@@ -188,6 +206,13 @@ void mmc_add_host_debugfs(struct mmc_host *host)
root, host-clk_delay))
goto err_node;
#endif
+#ifdef CONFIG_FAIL_MMC_REQUEST
+ host-fail_mmc_request = fail_mmc_request;
+ if (IS_ERR(fault_create_debugfs_attr(fail_mmc_request,
+root,
+host-fail_mmc_request)))
+ goto err_node;
+#endif
return;
err_node:
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 1d09562..4c4bddf 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -12,6 +12,7 @@
#include linux/leds.h
#include linux/sched.h
+#include linux/fault-inject.h
#include linux/mmc/core.h
#include linux/mmc/pm.h
@@ -302,6 +303,10 @@ struct mmc_host {
struct mmc_async_req*areq; /* active async req */
+#ifdef CONFIG_FAIL_MMC_REQUEST
+ struct fault_attr fail_mmc_request;
+#endif
+
unsigned long private[0] cacheline_aligned;
};
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index c0cb9c4..1c7dbbf 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1070,6
Re: [PATCH --mmotm v5 1/3] fault-inject: export fault injection functions
On 8 August 2011 22:07, Per Forlin per.for...@linaro.org wrote: export symbols fault_should_fail() and fault_create_debugfs_attr() in order to let modules utilize the fault injection This patch is already merged in mainline too. Unfortunately I left a typo here. It says fault_should_fail() in the commit message but the function in the patch is called only should_fail(). This is already in rc1 so I guess we have to live with this typo. -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH --mmotm v5 0/3] Make fault injection available for MMC IO
On 9 August 2011 02:51, Akinobu Mita akinobu.m...@gmail.com wrote: All three patches look good. Acked-by: Akinobu Mita akinobu.m...@gmail.com 2011/8/9 Per Forlin per.for...@linaro.org: This patchset is sent to the mm-tree because it depends on Akinobu's patch fault-injection: add ability to export fault_attr in... That patch has already been merged in mainline. Please drop this patchset. Patch #1 fault-injection: export fault injection functions is merged too. There is no need to merge this through mm-tree anymore. All fault-injection patches needed by MMC fault injection code are merged. I'll repost the patchset to mmc-next when mmc-next has moved to 3.1 code base. Thanks, Per -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH --mmotm v5 0/3] Make fault injection available for MMC IO
On 9 August 2011 11:24, Akinobu Mita akinobu.m...@gmail.com wrote: 2011/8/9 Per Forlin per.for...@linaro.org: Patch #1 fault-injection: export fault injection functions is merged Maybe you are looking at wrong tree. I can't find it in Linus' tree or mmotm patches. Thanks for double checking! I looked at the wrong tree. What a mess I am creating. Do you think it would be possible to get only the export fault-injection patch in 3.1? I know it's not a bugfix so I guess it wont be accepted. I'll prepare v6 of this patch-set. Thanks for your help, Per -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH --mmotm v6 0/3] Make fault injection available for MMC IO
change log: v2 - Resolve build issue in mmc core.c due to multiple init_module by removing the fault inject module. - Export fault injection functions to make them available for modules - Update fault injection documentation on MMC IO v3 - add function descriptions in core.c - use export GPL for fault injection functions v4 - make the fault_attr per host. This prepares for upcoming patch from Akinobu that adds support for creating debugfs entries in arbitrary directory. v5 - Make use of fault_create_debugfs_attr() in Akinobu's patch fault-injection: add ability to export fault_attr in v6 - Fix typo in commit message in patch export fault injection functions Per Forlin (3): fault-inject: export fault injection functions mmc: core: add random fault injection fault injection: add documentation on MMC IO fault injection Documentation/fault-injection/fault-injection.txt |5 ++ drivers/mmc/core/core.c | 44 + drivers/mmc/core/debugfs.c| 24 +++ include/linux/mmc/host.h |7 +++ lib/Kconfig.debug | 11 + lib/fault-inject.c|2 + 6 files changed, 93 insertions(+), 0 deletions(-) -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH --mmotm v6 1/3] fault-inject: export fault injection functions
export symbols should_fail() and fault_create_debugfs_attr() in order to let modules utilize the fault injection Signed-off-by: Per Forlin per.for...@linaro.org Acked-by: Akinobu Mita akinobu.m...@gmail.com --- lib/fault-inject.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/lib/fault-inject.c b/lib/fault-inject.c index f193b77..328d433 100644 --- a/lib/fault-inject.c +++ b/lib/fault-inject.c @@ -130,6 +130,7 @@ bool should_fail(struct fault_attr *attr, ssize_t size) return true; } +EXPORT_SYMBOL_GPL(should_fail); #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS @@ -243,5 +244,6 @@ fail: return ERR_PTR(-ENOMEM); } +EXPORT_SYMBOL_GPL(fault_create_debugfs_attr); #endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */ -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH --mmotm v6 3/3] fault injection: add documentation on MMC IO fault injection
Add description on how to enable random fault injection for MMC IO Signed-off-by: Per Forlin per.for...@linaro.org Acked-by: Akinobu Mita akinobu.m...@gmail.com --- Documentation/fault-injection/fault-injection.txt |5 + 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/Documentation/fault-injection/fault-injection.txt b/Documentation/fault-injection/fault-injection.txt index 82a5d25..10571df 100644 --- a/Documentation/fault-injection/fault-injection.txt +++ b/Documentation/fault-injection/fault-injection.txt @@ -21,6 +21,11 @@ o fail_make_request /sys/block/device/make-it-fail or /sys/block/device/partition/make-it-fail. (generic_make_request()) +o fail_mmc_request + + injects MMC data errors on devices permitted by setting + debugfs entries under /sys/kernel/debug/mmc0/fail_mmc_request + Configure fault-injection capabilities behavior --- -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH --mmotm v6 2/3] mmc: core: add random fault injection
This adds support to inject data errors after a completed host transfer.
The mmc core will return error even though the host transfer is successful.
This simple fault injection proved to be very useful to test the
non-blocking error handling in the mmc_blk_issue_rw_rq().
Random faults can also test how the host driver handles pre_req()
and post_req() in case of errors.
Signed-off-by: Per Forlin per.for...@linaro.org
Acked-by: Akinobu Mita akinobu.m...@gmail.com
---
drivers/mmc/core/core.c| 44
drivers/mmc/core/debugfs.c | 24
include/linux/mmc/host.h |7 +++
lib/Kconfig.debug | 11 +++
4 files changed, 86 insertions(+), 0 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 89bdeae..a4996b0 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -25,6 +25,11 @@
#include linux/pm_runtime.h
#include linux/suspend.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+#include linux/fault-inject.h
+#include linux/random.h
+#endif
+
#include linux/mmc/card.h
#include linux/mmc/host.h
#include linux/mmc/mmc.h
@@ -83,6 +88,43 @@ static void mmc_flush_scheduled_work(void)
flush_workqueue(workqueue);
}
+#ifdef CONFIG_FAIL_MMC_REQUEST
+
+/*
+ * Internal function. Inject random data errors.
+ * If mmc_data is NULL no errors are injected.
+ */
+static void mmc_should_fail_request(struct mmc_host *host,
+ struct mmc_request *mrq)
+{
+ struct mmc_command *cmd = mrq-cmd;
+ struct mmc_data *data = mrq-data;
+ static const int data_errors[] = {
+ -ETIMEDOUT,
+ -EILSEQ,
+ -EIO,
+ };
+
+ if (!data)
+ return;
+
+ if (cmd-error || data-error ||
+ !should_fail(host-fail_mmc_request, data-blksz * data-blocks))
+ return;
+
+ data-error = data_errors[random32() % ARRAY_SIZE(data_errors)];
+ data-bytes_xfered = (random32() % (data-bytes_xfered 9)) 9;
+}
+
+#else /* CONFIG_FAIL_MMC_REQUEST */
+
+static void mmc_should_fail_request(struct mmc_host *host,
+ struct mmc_request *mrq)
+{
+}
+
+#endif /* CONFIG_FAIL_MMC_REQUEST */
+
/**
* mmc_request_done - finish processing an MMC request
* @host: MMC host which completed request
@@ -109,6 +151,8 @@ void mmc_request_done(struct mmc_host *host, struct
mmc_request *mrq)
cmd-error = 0;
host-ops-request(host, mrq);
} else {
+ mmc_should_fail_request(host, mrq);
+
led_trigger_event(host-led, LED_OFF);
pr_debug(%s: req done (CMD%u): %d: %08x %08x %08x %08x\n,
diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index f573753..548c2e7 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -13,6 +13,9 @@
#include linux/seq_file.h
#include linux/slab.h
#include linux/stat.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+#include linux/fault-inject.h
+#endif
#include linux/mmc/card.h
#include linux/mmc/host.h
@@ -159,6 +162,20 @@ static int mmc_clock_opt_set(void *data, u64 val)
return 0;
}
+#ifdef CONFIG_FAIL_MMC_REQUEST
+
+static DECLARE_FAULT_ATTR(fail_mmc_request);
+/*
+ * Internal function. Pass the boot param fail_mmc_request to
+ * the setup fault injection attributes routine.
+ */
+static int __init setup_fail_mmc_request(char *str)
+{
+ return setup_fault_attr(fail_mmc_request, str);
+}
+__setup(fail_mmc_request=, setup_fail_mmc_request);
+#endif
+
DEFINE_SIMPLE_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
%llu\n);
@@ -189,6 +206,13 @@ void mmc_add_host_debugfs(struct mmc_host *host)
root, host-clk_delay))
goto err_node;
#endif
+#ifdef CONFIG_FAIL_MMC_REQUEST
+ host-fail_mmc_request = fail_mmc_request;
+ if (IS_ERR(fault_create_debugfs_attr(fail_mmc_request,
+root,
+host-fail_mmc_request)))
+ goto err_node;
+#endif
return;
err_node:
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 0f83858..ee472fe 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -12,6 +12,9 @@
#include linux/leds.h
#include linux/sched.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+#include linux/fault-inject.h
+#endif
#include linux/mmc/core.h
#include linux/mmc/pm.h
@@ -304,6 +307,10 @@ struct mmc_host {
struct mmc_async_req*areq; /* active async req */
+#ifdef CONFIG_FAIL_MMC_REQUEST
+ struct fault_attr fail_mmc_request;
+#endif
+
unsigned long private[0] cacheline_aligned;
};
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 47879c7..ebff0c9 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1090,6 +1090,17
[PATCH --mmotm v7 0/3] Make fault injection available for MMC IO
change log: v2 - Resolve build issue in mmc core.c due to multiple init_module by removing the fault inject module. - Export fault injection functions to make them available for modules - Update fault injection documentation on MMC IO v3 - add function descriptions in core.c - use export GPL for fault injection functions v4 - make the fault_attr per host. This prepares for upcoming patch from Akinobu that adds support for creating debugfs entries in arbitrary directory. v5 - Make use of fault_create_debugfs_attr() in Akinobu's patch fault-injection: add ability to export fault_attr in v6 - Fix typo in commit message in patch export fault injection functions v7 - Don't compile in boot param setup function if mmc-core is built as module. Per Forlin (3): fault-inject: export fault injection functions mmc: core: add random fault injection fault injection: add documentation on MMC IO fault injection Documentation/fault-injection/fault-injection.txt |5 ++ drivers/mmc/core/core.c | 44 + drivers/mmc/core/debugfs.c| 27 + include/linux/mmc/host.h |7 +++ lib/Kconfig.debug | 11 + lib/fault-inject.c|2 + 6 files changed, 96 insertions(+), 0 deletions(-) -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH --mmotm v7 1/3] fault-inject: export fault injection functions
export symbols should_fail() and fault_create_debugfs_attr() in order to let modules utilize the fault injection Signed-off-by: Per Forlin per.for...@linaro.org Acked-by: Akinobu Mita akinobu.m...@gmail.com --- lib/fault-inject.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/lib/fault-inject.c b/lib/fault-inject.c index f193b77..328d433 100644 --- a/lib/fault-inject.c +++ b/lib/fault-inject.c @@ -130,6 +130,7 @@ bool should_fail(struct fault_attr *attr, ssize_t size) return true; } +EXPORT_SYMBOL_GPL(should_fail); #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS @@ -243,5 +244,6 @@ fail: return ERR_PTR(-ENOMEM); } +EXPORT_SYMBOL_GPL(fault_create_debugfs_attr); #endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */ -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH --mmotm v7 3/3] fault injection: add documentation on MMC IO fault injection
Add description on how to enable random fault injection for MMC IO Signed-off-by: Per Forlin per.for...@linaro.org Acked-by: Akinobu Mita akinobu.m...@gmail.com --- Documentation/fault-injection/fault-injection.txt |5 + 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/Documentation/fault-injection/fault-injection.txt b/Documentation/fault-injection/fault-injection.txt index 82a5d25..10571df 100644 --- a/Documentation/fault-injection/fault-injection.txt +++ b/Documentation/fault-injection/fault-injection.txt @@ -21,6 +21,11 @@ o fail_make_request /sys/block/device/make-it-fail or /sys/block/device/partition/make-it-fail. (generic_make_request()) +o fail_mmc_request + + injects MMC data errors on devices permitted by setting + debugfs entries under /sys/kernel/debug/mmc0/fail_mmc_request + Configure fault-injection capabilities behavior --- -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH --mmotm v7 2/3] mmc: core: add random fault injection
This adds support to inject data errors after a completed host transfer.
The mmc core will return error even though the host transfer is successful.
This simple fault injection proved to be very useful to test the
non-blocking error handling in the mmc_blk_issue_rw_rq().
Random faults can also test how the host driver handles pre_req()
and post_req() in case of errors.
Signed-off-by: Per Forlin per.for...@linaro.org
Acked-by: Akinobu Mita akinobu.m...@gmail.com
---
drivers/mmc/core/core.c| 44
drivers/mmc/core/debugfs.c | 27 +++
include/linux/mmc/host.h |7 +++
lib/Kconfig.debug | 11 +++
4 files changed, 89 insertions(+), 0 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 89bdeae..a4996b0 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -25,6 +25,11 @@
#include linux/pm_runtime.h
#include linux/suspend.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+#include linux/fault-inject.h
+#include linux/random.h
+#endif
+
#include linux/mmc/card.h
#include linux/mmc/host.h
#include linux/mmc/mmc.h
@@ -83,6 +88,43 @@ static void mmc_flush_scheduled_work(void)
flush_workqueue(workqueue);
}
+#ifdef CONFIG_FAIL_MMC_REQUEST
+
+/*
+ * Internal function. Inject random data errors.
+ * If mmc_data is NULL no errors are injected.
+ */
+static void mmc_should_fail_request(struct mmc_host *host,
+ struct mmc_request *mrq)
+{
+ struct mmc_command *cmd = mrq-cmd;
+ struct mmc_data *data = mrq-data;
+ static const int data_errors[] = {
+ -ETIMEDOUT,
+ -EILSEQ,
+ -EIO,
+ };
+
+ if (!data)
+ return;
+
+ if (cmd-error || data-error ||
+ !should_fail(host-fail_mmc_request, data-blksz * data-blocks))
+ return;
+
+ data-error = data_errors[random32() % ARRAY_SIZE(data_errors)];
+ data-bytes_xfered = (random32() % (data-bytes_xfered 9)) 9;
+}
+
+#else /* CONFIG_FAIL_MMC_REQUEST */
+
+static void mmc_should_fail_request(struct mmc_host *host,
+ struct mmc_request *mrq)
+{
+}
+
+#endif /* CONFIG_FAIL_MMC_REQUEST */
+
/**
* mmc_request_done - finish processing an MMC request
* @host: MMC host which completed request
@@ -109,6 +151,8 @@ void mmc_request_done(struct mmc_host *host, struct
mmc_request *mrq)
cmd-error = 0;
host-ops-request(host, mrq);
} else {
+ mmc_should_fail_request(host, mrq);
+
led_trigger_event(host-led, LED_OFF);
pr_debug(%s: req done (CMD%u): %d: %08x %08x %08x %08x\n,
diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index f573753..189581d 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -13,6 +13,9 @@
#include linux/seq_file.h
#include linux/slab.h
#include linux/stat.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+#include linux/fault-inject.h
+#endif
#include linux/mmc/card.h
#include linux/mmc/host.h
@@ -159,6 +162,23 @@ static int mmc_clock_opt_set(void *data, u64 val)
return 0;
}
+#ifdef CONFIG_FAIL_MMC_REQUEST
+
+static DECLARE_FAULT_ATTR(fail_mmc_request);
+
+#ifdef KERNEL
+/*
+ * Internal function. Pass the boot param fail_mmc_request to
+ * the setup fault injection attributes routine.
+ */
+static int __init setup_fail_mmc_request(char *str)
+{
+ return setup_fault_attr(fail_mmc_request, str);
+}
+__setup(fail_mmc_request=, setup_fail_mmc_request);
+#endif /* KERNEL */
+#endif /* CONFIG_FAIL_MMC_REQUEST */
+
DEFINE_SIMPLE_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
%llu\n);
@@ -189,6 +209,13 @@ void mmc_add_host_debugfs(struct mmc_host *host)
root, host-clk_delay))
goto err_node;
#endif
+#ifdef CONFIG_FAIL_MMC_REQUEST
+ host-fail_mmc_request = fail_mmc_request;
+ if (IS_ERR(fault_create_debugfs_attr(fail_mmc_request,
+root,
+host-fail_mmc_request)))
+ goto err_node;
+#endif
return;
err_node:
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 0f83858..ee472fe 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -12,6 +12,9 @@
#include linux/leds.h
#include linux/sched.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+#include linux/fault-inject.h
+#endif
#include linux/mmc/core.h
#include linux/mmc/pm.h
@@ -304,6 +307,10 @@ struct mmc_host {
struct mmc_async_req*areq; /* active async req */
+#ifdef CONFIG_FAIL_MMC_REQUEST
+ struct fault_attr fail_mmc_request;
+#endif
+
unsigned long private[0] cacheline_aligned;
};
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 47879c7..ebff0c9 100644
[PATCH --mmotm v8 0/3] Make fault injection available for MMC IO
change log: v2 - Resolve build issue in mmc core.c due to multiple init_module by removing the fault inject module. - Export fault injection functions to make them available for modules - Update fault injection documentation on MMC IO v3 - add function descriptions in core.c - use export GPL for fault injection functions v4 - make the fault_attr per host. This prepares for upcoming patch from Akinobu that adds support for creating debugfs entries in arbitrary directory. v5 - Make use of fault_create_debugfs_attr() in Akinobu's patch fault-injection: add ability to export fault_attr in v6 - Fix typo in commit message in patch export fault injection functions v7 - Don't compile in boot param setup function if mmc-core is built as module. v8 - Update fault injection documentation. Add fail_mmc_request to boot option section. Per Forlin (3): fault-inject: export fault injection functions mmc: core: add random fault injection fault injection: add documentation on MMC IO fault injection Documentation/fault-injection/fault-injection.txt |8 +++- drivers/mmc/core/core.c | 44 + drivers/mmc/core/debugfs.c| 27 + include/linux/mmc/host.h |7 +++ lib/Kconfig.debug | 11 + lib/fault-inject.c|2 + 6 files changed, 98 insertions(+), 1 deletions(-) -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH --mmotm v8 1/3] fault-inject: export fault injection functions
export symbols should_fail() and fault_create_debugfs_attr() in order to let modules utilize the fault injection Signed-off-by: Per Forlin per.for...@linaro.org Acked-by: Akinobu Mita akinobu.m...@gmail.com --- lib/fault-inject.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/lib/fault-inject.c b/lib/fault-inject.c index f193b77..328d433 100644 --- a/lib/fault-inject.c +++ b/lib/fault-inject.c @@ -130,6 +130,7 @@ bool should_fail(struct fault_attr *attr, ssize_t size) return true; } +EXPORT_SYMBOL_GPL(should_fail); #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS @@ -243,5 +244,6 @@ fail: return ERR_PTR(-ENOMEM); } +EXPORT_SYMBOL_GPL(fault_create_debugfs_attr); #endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */ -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH --mmotm v8 2/3] mmc: core: add random fault injection
This adds support to inject data errors after a completed host transfer.
The mmc core will return error even though the host transfer is successful.
This simple fault injection proved to be very useful to test the
non-blocking error handling in the mmc_blk_issue_rw_rq().
Random faults can also test how the host driver handles pre_req()
and post_req() in case of errors.
Signed-off-by: Per Forlin per.for...@linaro.org
Acked-by: Akinobu Mita akinobu.m...@gmail.com
---
drivers/mmc/core/core.c| 44
drivers/mmc/core/debugfs.c | 27 +++
include/linux/mmc/host.h |7 +++
lib/Kconfig.debug | 11 +++
4 files changed, 89 insertions(+), 0 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 89bdeae..a4996b0 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -25,6 +25,11 @@
#include linux/pm_runtime.h
#include linux/suspend.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+#include linux/fault-inject.h
+#include linux/random.h
+#endif
+
#include linux/mmc/card.h
#include linux/mmc/host.h
#include linux/mmc/mmc.h
@@ -83,6 +88,43 @@ static void mmc_flush_scheduled_work(void)
flush_workqueue(workqueue);
}
+#ifdef CONFIG_FAIL_MMC_REQUEST
+
+/*
+ * Internal function. Inject random data errors.
+ * If mmc_data is NULL no errors are injected.
+ */
+static void mmc_should_fail_request(struct mmc_host *host,
+ struct mmc_request *mrq)
+{
+ struct mmc_command *cmd = mrq-cmd;
+ struct mmc_data *data = mrq-data;
+ static const int data_errors[] = {
+ -ETIMEDOUT,
+ -EILSEQ,
+ -EIO,
+ };
+
+ if (!data)
+ return;
+
+ if (cmd-error || data-error ||
+ !should_fail(host-fail_mmc_request, data-blksz * data-blocks))
+ return;
+
+ data-error = data_errors[random32() % ARRAY_SIZE(data_errors)];
+ data-bytes_xfered = (random32() % (data-bytes_xfered 9)) 9;
+}
+
+#else /* CONFIG_FAIL_MMC_REQUEST */
+
+static void mmc_should_fail_request(struct mmc_host *host,
+ struct mmc_request *mrq)
+{
+}
+
+#endif /* CONFIG_FAIL_MMC_REQUEST */
+
/**
* mmc_request_done - finish processing an MMC request
* @host: MMC host which completed request
@@ -109,6 +151,8 @@ void mmc_request_done(struct mmc_host *host, struct
mmc_request *mrq)
cmd-error = 0;
host-ops-request(host, mrq);
} else {
+ mmc_should_fail_request(host, mrq);
+
led_trigger_event(host-led, LED_OFF);
pr_debug(%s: req done (CMD%u): %d: %08x %08x %08x %08x\n,
diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index f573753..189581d 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -13,6 +13,9 @@
#include linux/seq_file.h
#include linux/slab.h
#include linux/stat.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+#include linux/fault-inject.h
+#endif
#include linux/mmc/card.h
#include linux/mmc/host.h
@@ -159,6 +162,23 @@ static int mmc_clock_opt_set(void *data, u64 val)
return 0;
}
+#ifdef CONFIG_FAIL_MMC_REQUEST
+
+static DECLARE_FAULT_ATTR(fail_mmc_request);
+
+#ifdef KERNEL
+/*
+ * Internal function. Pass the boot param fail_mmc_request to
+ * the setup fault injection attributes routine.
+ */
+static int __init setup_fail_mmc_request(char *str)
+{
+ return setup_fault_attr(fail_mmc_request, str);
+}
+__setup(fail_mmc_request=, setup_fail_mmc_request);
+#endif /* KERNEL */
+#endif /* CONFIG_FAIL_MMC_REQUEST */
+
DEFINE_SIMPLE_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
%llu\n);
@@ -189,6 +209,13 @@ void mmc_add_host_debugfs(struct mmc_host *host)
root, host-clk_delay))
goto err_node;
#endif
+#ifdef CONFIG_FAIL_MMC_REQUEST
+ host-fail_mmc_request = fail_mmc_request;
+ if (IS_ERR(fault_create_debugfs_attr(fail_mmc_request,
+root,
+host-fail_mmc_request)))
+ goto err_node;
+#endif
return;
err_node:
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 0f83858..ee472fe 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -12,6 +12,9 @@
#include linux/leds.h
#include linux/sched.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+#include linux/fault-inject.h
+#endif
#include linux/mmc/core.h
#include linux/mmc/pm.h
@@ -304,6 +307,10 @@ struct mmc_host {
struct mmc_async_req*areq; /* active async req */
+#ifdef CONFIG_FAIL_MMC_REQUEST
+ struct fault_attr fail_mmc_request;
+#endif
+
unsigned long private[0] cacheline_aligned;
};
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 47879c7..ebff0c9 100644
[PATCH --mmotm v8 3/3] fault injection: add documentation on MMC IO fault injection
Add description on how to enable random fault injection for MMC IO Signed-off-by: Per Forlin per.for...@linaro.org Acked-by: Akinobu Mita akinobu.m...@gmail.com --- Documentation/fault-injection/fault-injection.txt |8 +++- 1 files changed, 7 insertions(+), 1 deletions(-) diff --git a/Documentation/fault-injection/fault-injection.txt b/Documentation/fault-injection/fault-injection.txt index 82a5d25..70f924e 100644 --- a/Documentation/fault-injection/fault-injection.txt +++ b/Documentation/fault-injection/fault-injection.txt @@ -21,6 +21,11 @@ o fail_make_request /sys/block/device/make-it-fail or /sys/block/device/partition/make-it-fail. (generic_make_request()) +o fail_mmc_request + + injects MMC data errors on devices permitted by setting + debugfs entries under /sys/kernel/debug/mmc0/fail_mmc_request + Configure fault-injection capabilities behavior --- @@ -115,7 +120,8 @@ use the boot option: failslab= fail_page_alloc= - fail_make_request=interval,probability,space,times + fail_make_request= + fail_mmc_request=interval,probability,space,times How to add new fault injection capability - -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH --mmotm v5 1/3] fault-inject: export fault injection functions
export symbols fault_should_fail() and fault_create_debugfs_attr() in order to let modules utilize the fault injection Signed-off-by: Per Forlin per.for...@linaro.org --- lib/fault-inject.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/lib/fault-inject.c b/lib/fault-inject.c index f193b77..328d433 100644 --- a/lib/fault-inject.c +++ b/lib/fault-inject.c @@ -130,6 +130,7 @@ bool should_fail(struct fault_attr *attr, ssize_t size) return true; } +EXPORT_SYMBOL_GPL(should_fail); #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS @@ -243,5 +244,6 @@ fail: return ERR_PTR(-ENOMEM); } +EXPORT_SYMBOL_GPL(fault_create_debugfs_attr); #endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */ -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH --mmotm v5 2/3] mmc: core: add random fault injection
This adds support to inject data errors after a completed host transfer.
The mmc core will return error even though the host transfer is successful.
This simple fault injection proved to be very useful to test the
non-blocking error handling in the mmc_blk_issue_rw_rq().
Random faults can also test how the host driver handles pre_req()
and post_req() in case of errors.
Signed-off-by: Per Forlin per.for...@linaro.org
Acked-by: Akinobu Mita akinobu.m...@gmail.com
---
drivers/mmc/core/core.c| 44
drivers/mmc/core/debugfs.c | 24
include/linux/mmc/host.h |7 +++
lib/Kconfig.debug | 11 +++
4 files changed, 86 insertions(+), 0 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 89bdeae..a4996b0 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -25,6 +25,11 @@
#include linux/pm_runtime.h
#include linux/suspend.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+#include linux/fault-inject.h
+#include linux/random.h
+#endif
+
#include linux/mmc/card.h
#include linux/mmc/host.h
#include linux/mmc/mmc.h
@@ -83,6 +88,43 @@ static void mmc_flush_scheduled_work(void)
flush_workqueue(workqueue);
}
+#ifdef CONFIG_FAIL_MMC_REQUEST
+
+/*
+ * Internal function. Inject random data errors.
+ * If mmc_data is NULL no errors are injected.
+ */
+static void mmc_should_fail_request(struct mmc_host *host,
+ struct mmc_request *mrq)
+{
+ struct mmc_command *cmd = mrq-cmd;
+ struct mmc_data *data = mrq-data;
+ static const int data_errors[] = {
+ -ETIMEDOUT,
+ -EILSEQ,
+ -EIO,
+ };
+
+ if (!data)
+ return;
+
+ if (cmd-error || data-error ||
+ !should_fail(host-fail_mmc_request, data-blksz * data-blocks))
+ return;
+
+ data-error = data_errors[random32() % ARRAY_SIZE(data_errors)];
+ data-bytes_xfered = (random32() % (data-bytes_xfered 9)) 9;
+}
+
+#else /* CONFIG_FAIL_MMC_REQUEST */
+
+static void mmc_should_fail_request(struct mmc_host *host,
+ struct mmc_request *mrq)
+{
+}
+
+#endif /* CONFIG_FAIL_MMC_REQUEST */
+
/**
* mmc_request_done - finish processing an MMC request
* @host: MMC host which completed request
@@ -109,6 +151,8 @@ void mmc_request_done(struct mmc_host *host, struct
mmc_request *mrq)
cmd-error = 0;
host-ops-request(host, mrq);
} else {
+ mmc_should_fail_request(host, mrq);
+
led_trigger_event(host-led, LED_OFF);
pr_debug(%s: req done (CMD%u): %d: %08x %08x %08x %08x\n,
diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index f573753..548c2e7 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -13,6 +13,9 @@
#include linux/seq_file.h
#include linux/slab.h
#include linux/stat.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+#include linux/fault-inject.h
+#endif
#include linux/mmc/card.h
#include linux/mmc/host.h
@@ -159,6 +162,20 @@ static int mmc_clock_opt_set(void *data, u64 val)
return 0;
}
+#ifdef CONFIG_FAIL_MMC_REQUEST
+
+static DECLARE_FAULT_ATTR(fail_mmc_request);
+/*
+ * Internal function. Pass the boot param fail_mmc_request to
+ * the setup fault injection attributes routine.
+ */
+static int __init setup_fail_mmc_request(char *str)
+{
+ return setup_fault_attr(fail_mmc_request, str);
+}
+__setup(fail_mmc_request=, setup_fail_mmc_request);
+#endif
+
DEFINE_SIMPLE_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
%llu\n);
@@ -189,6 +206,13 @@ void mmc_add_host_debugfs(struct mmc_host *host)
root, host-clk_delay))
goto err_node;
#endif
+#ifdef CONFIG_FAIL_MMC_REQUEST
+ host-fail_mmc_request = fail_mmc_request;
+ if (IS_ERR(fault_create_debugfs_attr(fail_mmc_request,
+root,
+host-fail_mmc_request)))
+ goto err_node;
+#endif
return;
err_node:
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 0f83858..ee472fe 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -12,6 +12,9 @@
#include linux/leds.h
#include linux/sched.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+#include linux/fault-inject.h
+#endif
#include linux/mmc/core.h
#include linux/mmc/pm.h
@@ -304,6 +307,10 @@ struct mmc_host {
struct mmc_async_req*areq; /* active async req */
+#ifdef CONFIG_FAIL_MMC_REQUEST
+ struct fault_attr fail_mmc_request;
+#endif
+
unsigned long private[0] cacheline_aligned;
};
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 47879c7..ebff0c9 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1090,6 +1090,17
[PATCH --mmotm v5 3/3] fault injection: add documentation on MMC IO fault injection
Add description on how to enable random fault injection for MMC IO Signed-off-by: Per Forlin per.for...@linaro.org --- Documentation/fault-injection/fault-injection.txt |5 + 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/Documentation/fault-injection/fault-injection.txt b/Documentation/fault-injection/fault-injection.txt index 82a5d25..10571df 100644 --- a/Documentation/fault-injection/fault-injection.txt +++ b/Documentation/fault-injection/fault-injection.txt @@ -21,6 +21,11 @@ o fail_make_request /sys/block/device/make-it-fail or /sys/block/device/partition/make-it-fail. (generic_make_request()) +o fail_mmc_request + + injects MMC data errors on devices permitted by setting + debugfs entries under /sys/kernel/debug/mmc0/fail_mmc_request + Configure fault-injection capabilities behavior --- -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/3] mmc: core: add random fault injection
On 27 July 2011 01:17, Akinobu Mita akinobu.m...@gmail.com wrote:
2011/7/27 Per Forlin per.for...@linaro.org:
This adds support to inject data errors after a completed host transfer.
The mmc core will return error even though the host transfer is successful.
This simple fault injection proved to be very useful to test the
non-blocking error handling in the mmc_blk_issue_rw_rq().
Random faults can also test how the host driver handles pre_req()
and post_req() in case of errors.
Looks good but I have one question.
@@ -304,6 +307,10 @@ struct mmc_host {
struct mmc_async_req *areq; /* active async req */
+#ifdef CONFIG_FAIL_MMC_REQUEST
+ u8 make_it_fail;
+ struct fault_attr fail_mmc_request;
+#endif
unsigned long private[0] cacheline_aligned;
};
I think make_it_fail is not needed anymore because if fail_attr is
defined per-host then you can enable it by setting probability=0
or times=0 per-host.
Yes, if there are many debugfs entries, one for each host make_if_fail
is no longer necessary.
There is an issue with patch v4 now when fault_attr is per-host.
Without your patch the entry is still created at the root but there
are many instances of fault-attr. I think it's better to wait for your
patch to make it into the mmc-next tree before submitting my patch. I
will prepare a patch v5 that depends on your upcoming changes in
fault-inject with a note that states the dependency.
Would you mind adding patch 1/3 (export_symbol_gpl) to your
patch-set since it depends on the new function names in your patch?
If not, I can resend the patch on top of your changes to match the new
function names if you prefer to have this patch separate.
Please let me know.
Thanks,
Per
--
To unsubscribe from this list: send the line unsubscribe linux-mmc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/3] mmc: core: add random fault injection
On 27 July 2011 18:08, Akinobu Mita akinobu.m...@gmail.com wrote: 2011/7/27 Per Forlin per.for...@linaro.org: There is an issue with patch v4 now when fault_attr is per-host. Without your patch the entry is still created at the root but there are many instances of fault-attr. I think it's better to wait for your patch to make it into the mmc-next tree before submitting my patch. I will prepare a patch v5 that depends on your upcoming changes in fault-inject with a note that states the dependency. Or you can prepare a patch for -mm and ask Andrew to add it to the -mm tree. Thanks for the tip, Would you mind adding patch 1/3 (export_symbol_gpl) to your patch-set since it depends on the new function names in your patch? If not, I can resend the patch on top of your changes to match the new function names if you prefer to have this patch separate. I recommend it to be a part of your patchset. The new function name (fault_create_debugfs_attr) is not likely to change for a time. You can add Acked-by: Akinobu Mita akinobu.m...@gmail.com Beside fault_create_debugfs_attr() the other function is should_fail. I presume this name will be changed too, and start with the fault_? Regards, Per -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] mmc: core: add random fault injection
On 26 July 2011 03:41, Akinobu Mita akinobu.m...@gmail.com wrote: 2011/7/26 Per Forlin per.for...@linaro.org: And I know that init_fault_attr_dentries() can only create a subdirectory in debugfs root directory. But I have a patch which support for creating it in arbitrary directory. Could you take a look at this? (Note that this patch is based on mmotm and not yet tested) I looked at your patch and it raised two questions. I can't use FAULT_ATTR_INITIALIZER since mmc_host is allocated on the heap. It looks like setup_fault_attr(attr, str) will fail if str is NULL. How can I initialise the fault_attrs if not stack allocated? About the boot param initialisation of fault attr. There can only be one fault_mmc_request boot param for the entire kernel but there is one fault_attr per host, and there may be many hosts. It would be convenient if setup_fault_attrs would take (attr, boot_param_name), look up boot_param_name and use that otherwise set default values. I think you can define one default fail_attr for boot time configuration and copy it to per-host fail_attr in mmc_add_host_debugfs(). You're right. This works fine. I'll prepare a new version of my patch. -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] mmc: core: add random fault injection
On 25 July 2011 17:58, Akinobu Mita akinobu.m...@gmail.com wrote: 2011/7/21 Per Forlin per.for...@linaro.org: This adds support to inject data errors after a completed host transfer. The mmc core will return error even though the host transfer is successful. This simple fault injection proved to be very useful to test the non-blocking error handling in the mmc_blk_issue_rw_rq(). Random faults can also test how the host driver handles pre_req() and post_req() in case of errors. Looks good to me. @@ -82,6 +87,66 @@ static void mmc_flush_scheduled_work(void) flush_workqueue(workqueue); } +#ifdef CONFIG_FAIL_MMC_REQUEST + +static DECLARE_FAULT_ATTR(fail_mmc_request); I think the fail_attr should be defined for each mmc_host like make_it_fail in struct mmc_host and debugfs entries should also be created in a subdirectory of mmc host debugfs. And I know that init_fault_attr_dentries() can only create a subdirectory in debugfs root directory. But I have a patch which support for creating it in arbitrary directory. Could you take a look at this? (Note that this patch is based on mmotm and not yet tested) I tested your patch on a Snowball board. I had two active mmc cards and did various fault injection configurations on the two cards together with dd. I did only test MMC IO fault injection and not any of the other fault injection clients. Tested-by: Per Forlin per.for...@linaro.org Regards, Per -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 0/3] Make fault injection available for MMC IO
The first version of this patch is a part of mmc non-blocking v9 patchset: [PATCH v9 11/12] mmc: core: add random fault injection change log: v2 - Resolve build issue in mmc core.c due to multiple init_module by removing the fault inject module. - Export fault injection functions to make them available for modules - Update fault injection documentation on MMC IO v3 - add function descriptions in core.c - use export GPL for fault injection functions v4 - make the fault_attr per host. This prepares for upcoming patch from Akinobu that adds support for creating debugfs entries in arbitrary directory. Per Forlin (3): fault-inject: make fault injection available for modules mmc: core: add random fault injection fault injection: add documentation on MMC IO fault injection Documentation/fault-injection/fault-injection.txt |5 ++ drivers/mmc/core/core.c | 45 + drivers/mmc/core/debugfs.c| 26 include/linux/mmc/host.h |7 +++ lib/Kconfig.debug | 11 + lib/fault-inject.c|2 + 6 files changed, 96 insertions(+), 0 deletions(-) -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 1/3] fault-inject: make fault injection available for modules
export symbols should_fail() and init_fault_attr_dentries() in order to make modules use the fault injection functionality Signed-off-by: Per Forlin per.for...@linaro.org --- lib/fault-inject.c |2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/lib/fault-inject.c b/lib/fault-inject.c index 7e65af7..27783da 100644 --- a/lib/fault-inject.c +++ b/lib/fault-inject.c @@ -131,6 +131,7 @@ bool should_fail(struct fault_attr *attr, ssize_t size) return true; } +EXPORT_SYMBOL_GPL(should_fail); #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS @@ -311,5 +312,6 @@ fail: cleanup_fault_attr_dentries(attr); return -ENOMEM; } +EXPORT_SYMBOL_GPL(init_fault_attr_dentries); #endif /* CONFIG_FAULT_INJECTION_DEBUG_FS */ -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 3/3] fault injection: add documentation on MMC IO fault injection
Add description on how to enable random fault injection for MMC IO Signed-off-by: Per Forlin per.for...@linaro.org --- Documentation/fault-injection/fault-injection.txt |5 + 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/Documentation/fault-injection/fault-injection.txt b/Documentation/fault-injection/fault-injection.txt index 7be15e4..27eede4 100644 --- a/Documentation/fault-injection/fault-injection.txt +++ b/Documentation/fault-injection/fault-injection.txt @@ -21,6 +21,11 @@ o fail_make_request /sys/block/device/make-it-fail or /sys/block/device/partition/make-it-fail. (generic_make_request()) +o fail_mmc_request + + injects MMC data errors on devices permitted by setting + /sys/kernel/debug/mmc0/make-it-fail + Configure fault-injection capabilities behavior --- -- 1.7.4.1 -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 2/3] mmc: core: add random fault injection
This adds support to inject data errors after a completed host transfer.
The mmc core will return error even though the host transfer is successful.
This simple fault injection proved to be very useful to test the
non-blocking error handling in the mmc_blk_issue_rw_rq().
Random faults can also test how the host driver handles pre_req()
and post_req() in case of errors.
Signed-off-by: Per Forlin per.for...@linaro.org
---
drivers/mmc/core/core.c| 45
drivers/mmc/core/debugfs.c | 26 +
include/linux/mmc/host.h |7 ++
lib/Kconfig.debug | 11 ++
4 files changed, 89 insertions(+), 0 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index f091b43..c9195b0 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -24,6 +24,11 @@
#include linux/regulator/consumer.h
#include linux/pm_runtime.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+#include linux/fault-inject.h
+#include linux/random.h
+#endif
+
#include linux/mmc/card.h
#include linux/mmc/host.h
#include linux/mmc/mmc.h
@@ -82,6 +87,44 @@ static void mmc_flush_scheduled_work(void)
flush_workqueue(workqueue);
}
+#ifdef CONFIG_FAIL_MMC_REQUEST
+
+/*
+ * Internal function. Inject random data errors.
+ * If mmc_data is NULL no errors are injected.
+ */
+static void mmc_should_fail_request(struct mmc_host *host,
+ struct mmc_request *mrq)
+{
+ struct mmc_command *cmd = mrq-cmd;
+ struct mmc_data *data = mrq-data;
+ static const int data_errors[] = {
+ -ETIMEDOUT,
+ -EILSEQ,
+ -EIO,
+ };
+
+ if (!data)
+ return;
+
+ if (cmd-error || data-error || !host-make_it_fail ||
+ !should_fail(host-fail_mmc_request, data-blksz * data-blocks))
+ return;
+
+ data-error = data_errors[random32() % ARRAY_SIZE(data_errors)];
+ data-bytes_xfered = (random32() % (data-bytes_xfered 9)) 9;
+}
+
+#else /* CONFIG_FAIL_MMC_REQUEST */
+
+static void mmc_should_fail_request(struct mmc_host *host,
+ struct mmc_request *mrq)
+{
+}
+
+#endif /* CONFIG_FAIL_MMC_REQUEST */
+
+
/**
* mmc_request_done - finish processing an MMC request
* @host: MMC host which completed request
@@ -108,6 +151,8 @@ void mmc_request_done(struct mmc_host *host, struct
mmc_request *mrq)
cmd-error = 0;
host-ops-request(host, mrq);
} else {
+ mmc_should_fail_request(host, mrq);
+
led_trigger_event(host-led, LED_OFF);
pr_debug(%s: req done (CMD%u): %d: %08x %08x %08x %08x\n,
diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index 998797e..5f885a4 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -12,6 +12,9 @@
#include linux/seq_file.h
#include linux/slab.h
#include linux/stat.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+#include linux/fault-inject.h
+#endif
#include linux/mmc/card.h
#include linux/mmc/host.h
@@ -158,6 +161,20 @@ static int mmc_clock_opt_set(void *data, u64 val)
return 0;
}
+#ifdef CONFIG_FAIL_MMC_REQUEST
+
+static DECLARE_FAULT_ATTR(fail_mmc_request);
+/*
+ * Internal function. Pass the boot param fail_mmc_request to
+ * the setup fault injection attributes routine.
+ */
+static int __init setup_fail_mmc_request(char *str)
+{
+ return setup_fault_attr(fail_mmc_request, str);
+}
+__setup(fail_mmc_request=, setup_fail_mmc_request);
+#endif
+
DEFINE_SIMPLE_ATTRIBUTE(mmc_clock_fops, mmc_clock_opt_get, mmc_clock_opt_set,
%llu\n);
@@ -188,6 +205,15 @@ void mmc_add_host_debugfs(struct mmc_host *host)
root, host-clk_delay))
goto err_node;
#endif
+#ifdef CONFIG_FAIL_MMC_REQUEST
+ if (!debugfs_create_u8(make-it-fail, S_IRUSR | S_IWUSR,
+ root, host-make_it_fail))
+ goto err_node;
+ host-fail_mmc_request = fail_mmc_request;
+ if (init_fault_attr_dentries(host-fail_mmc_request,
+fail_mmc_request))
+ goto err_node;
+#endif
return;
err_node:
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 0f83858..3b57f4b 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -12,6 +12,9 @@
#include linux/leds.h
#include linux/sched.h
+#ifdef CONFIG_FAIL_MMC_REQUEST
+#include linux/fault-inject.h
+#endif
#include linux/mmc/core.h
#include linux/mmc/pm.h
@@ -304,6 +307,10 @@ struct mmc_host {
struct mmc_async_req*areq; /* active async req */
+#ifdef CONFIG_FAIL_MMC_REQUEST
+ u8 make_it_fail;
+ struct fault_attr fail_mmc_request;
+#endif
unsigned long private[0] cacheline_aligned;
};
diff --git a/lib/Kconfig.debug b/lib
Re: [PATCH v3 2/3] mmc: core: add random fault injection
On 25 July 2011 17:58, Akinobu Mita akinobu.m...@gmail.com wrote: 2011/7/21 Per Forlin per.for...@linaro.org: This adds support to inject data errors after a completed host transfer. The mmc core will return error even though the host transfer is successful. This simple fault injection proved to be very useful to test the non-blocking error handling in the mmc_blk_issue_rw_rq(). Random faults can also test how the host driver handles pre_req() and post_req() in case of errors. Looks good to me. Thanks, @@ -82,6 +87,66 @@ static void mmc_flush_scheduled_work(void) flush_workqueue(workqueue); } +#ifdef CONFIG_FAIL_MMC_REQUEST + +static DECLARE_FAULT_ATTR(fail_mmc_request); I think the fail_attr should be defined for each mmc_host like make_it_fail in struct mmc_host and debugfs entries should also be created in a subdirectory of mmc host debugfs. I looked at blk-core.c and used the same code here. Current code creates the entry under the debugfs root. This means one fail_attr for all hosts. I agree, it's more clean to move the fail_attr to the host-debugfs-entry which require the fail_attr to be stored same way as make_it_fail. And I know that init_fault_attr_dentries() can only create a subdirectory in debugfs root directory. But I have a patch which support for creating it in arbitrary directory. Could you take a look at this? (Note that this patch is based on mmotm and not yet tested) Thanks, I will check it out. Regards, Per -- To unsubscribe from this list: send the line unsubscribe linux-mmc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
