[v4,2/3] mmc: core: add random fault injection
diff mbox

Message ID 1311711119-6286-3-git-send-email-per.forlin@linaro.org
State New, archived
Headers show

Commit Message

Per Forlin July 26, 2011, 8:11 p.m. UTC
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.forlin@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(-)

Comments

Akinobu Mita July 26, 2011, 11:17 p.m. UTC | #1
2011/7/27 Per Forlin <per.forlin@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.
Per Forlin July 27, 2011, 9:02 a.m. UTC | #2
On 27 July 2011 01:17, Akinobu Mita <akinobu.mita@gmail.com> wrote:
> 2011/7/27 Per Forlin <per.forlin@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
Akinobu Mita July 27, 2011, 4:08 p.m. UTC | #3
2011/7/27 Per Forlin <per.forlin@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.

> 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.mita@gmail.com>
Per Forlin July 27, 2011, 7:46 p.m. UTC | #4
On 27 July 2011 18:08, Akinobu Mita <akinobu.mita@gmail.com> wrote:
> 2011/7/27 Per Forlin <per.forlin@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.mita@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

Patch
diff mbox

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/Kconfig.debug
index dd373c8..e29e36d 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1067,6 +1067,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
+	help
+	  Provide fault-injection capability for MMC IO.
+	  This will make the mmc core return data errors. This is
+	  useful to test the error handling in the mmc block device
+	  and to test how the mmc host driver handles retries from
+	  the block device.
+
 config FAULT_INJECTION_DEBUG_FS
 	bool "Debugfs entries for fault-injection capabilities"
 	depends on FAULT_INJECTION && SYSFS && DEBUG_FS