Re: [PATCH v2 2/3] mmc: core: add random fault injection

[Per Forlin]

Hi James,

On 20 July 2011 02:04, J Freyensee <james_p_freyensee@xxxxxxxxxxxxxxx> wrote:
> On 07/19/2011 02:31 PM, Per Forlin 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.
>>
>> Signed-off-by: Per Forlin<per.forlin@xxxxxxxxxx>
>> ---
>>  drivers/mmc/core/core.c    |   58
>> ++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/mmc/core/debugfs.c |    5 ++++
>>  include/linux/mmc/host.h   |    3 ++
>>  lib/Kconfig.debug          |   11 ++++++++
>>  4 files changed, 77 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index ab36c7b..3f822b4 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -23,6 +23,8 @@
>>  #include<linux/log2.h>
>>  #include<linux/regulator/consumer.h>
>>  #include<linux/pm_runtime.h>
>> +#include<linux/fault-inject.h>
>> +#include<linux/random.h>
>>
>
> As a suggestion, would you want to also use '#ifdef CONFIG_FAIL_MMC_REQUEST'
> for fault-inject.h and random.h?  If they are not used in a non-debug linux
> kernel configuration, could this possibly cause a little extra code bloat if
> they are a part of a production Linux kernel compile/configuration?
>
I try to avoid ifdefs in general but I see your point. I accept your
comment and add the ifdef.

>>  #include<linux/mmc/card.h>
>>  #include<linux/mmc/host.h>
>> @@ -82,6 +84,58 @@ static void mmc_flush_scheduled_work(void)
>>        flush_workqueue(workqueue);
>>  }
>>
>> +#ifdef CONFIG_FAIL_MMC_REQUEST
>> +
>> +static DECLARE_FAULT_ATTR(fail_mmc_request);
>> +
>> +static int __init setup_fail_mmc_request(char *str)
>
> Function comment header somewhere (here or the .h file?)
>
>> +{
>> +       return setup_fault_attr(&fail_mmc_request, str);
>> +}
>> +__setup("fail_mmc_request=", setup_fail_mmc_request);
>> +
>> +static void mmc_should_fail_request(struct mmc_host *host,
>> +                                   struct mmc_request *mrq)
>> +{
>
> Function comment header somewhere (here or the .h file)?
>
I will do.

>> +       struct mmc_command *cmd = mrq->cmd;
>> +       struct mmc_data *data = mrq->data;
>> +       static const int data_errors[] = {
>> +               -ETIMEDOUT,
>> +               -EILSEQ,
>> +               -EIO,
>> +       };
>> +
>> +       if (!data)
>> +               return;
>> +
>
> Should 'if (!cmd)' also be checked or is this guaranteed to always have a
> valid value for this function (and if there are certain commands in 'struct
> mmc_command *cmd = mrq->cmd' that will not work with this function then that
> is something that should be documented in the function comment header)?
>
cmd will always be set from where the mmc_should_fail_request() is called.
This code only inject data errors. For commands with no data struct
there will be no error inject. This is a choice I have made. It is
possible to extend this by injecting errors for cmds without data too
but it would require an additional flag in order to control inject
errors on data and cmd separately. I will add this information to the
function description.

>> +       if (cmd->error || data->error || !host->make_it_fail ||
>> +           !should_fail(&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;
>> +}
>> +
>> +static int __init fail_mmc_request_debugfs(void)
>> +{
>> +       return init_fault_attr_dentries(&fail_mmc_request,
>> +                                       "fail_mmc_request");
>> +}
>> +
>> +#else /* CONFIG_FAIL_MMC_REQUEST */
>> +
>> +static void mmc_should_fail_request(struct mmc_host *host,
>> +                                   struct mmc_request *mrq)
>> +{
>> +}
>> +
>> +static int __init fail_mmc_request_debugfs(void)
>> +{
>> +       return 0;
>> +}
>> +#endif /* CONFIG_FAIL_MMC_REQUEST */
>> +
>> +
>>  /**
>>   *    mmc_request_done - finish processing an MMC request
>>   *    @host: MMC host which completed request
>> @@ -108,6 +162,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",
>> @@ -2064,6 +2120,8 @@ static int __init mmc_init(void)
>>        if (ret)
>>                goto unregister_host_class;
>>
>> +       fail_mmc_request_debugfs();
>> +
>>        return 0;
>>
>>  unregister_host_class:
>> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
>> index 998797e..588e76f 100644
>> --- a/drivers/mmc/core/debugfs.c
>> +++ b/drivers/mmc/core/debugfs.c
>> @@ -188,6 +188,11 @@ 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;
>> +#endif
>>        return;
>>
>>  err_node:
>> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
>> index 771455f..250b46d 100644
>> --- a/include/linux/mmc/host.h
>> +++ b/include/linux/mmc/host.h
>> @@ -303,6 +303,9 @@ struct mmc_host {
>>
>>        struct mmc_async_req    *areq;          /* active async req */
>>
>> +#ifdef CONFIG_FAIL_MMC_REQUEST
>> +       u8                      make_it_fail;
>> +#endif
>>        unsigned long           private[0] ____cacheline_aligned;
>>  };
>>
>> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
>> index c768bcd..c2d1423 100644
>> --- a/lib/Kconfig.debug
>> +++ b/lib/Kconfig.debug
>> @@ -1057,6 +1057,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
>
>
> --
> J (James/Jay) Freyensee
> Storage Technology Group
> Intel Corporation
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/