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

[Per Forlin]

On 14 September 2011 01:37, Akinobu Mita <akinobu.mita@xxxxxxxxx> wrote:
> 2011/9/14 Per Forlin <per.forlin@xxxxxxxxxx>:
>> Hi Akinobu,
>>
>> On 13 September 2011 16:19, Per Forlin <per.forlin@xxxxxxxxxx> wrote:
>>> On 13 September 2011 15:12, Akinobu Mita <akinobu.mita@xxxxxxxxx> wrote:
>>>> 2011/8/19 Per Forlin <per.forlin@xxxxxxxxxxxxxx>:
>>>>
>>>>> +#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-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/