[PATCH v1] mtd: nandbiterrs: Have init function return 0 on success

[Marc Gonzalez]

On 19/12/2016 13:20, Boris Brezillon wrote:

> On Tue, 13 Dec 2016 15:36:07 +0100
> Marc Gonzalez <marc_gonzalez at sigmadesigns.com> wrote:
> 
>> The init function currently returns -EIO on success. This behavior
>> was probably chosen in order to avoid a subsequent rmmod, but this
>> complicates failure detection from user-space.
>>
>> Signed-off-by: Marc Gonzalez <marc_gonzalez at sigmadesigns.com>
>> ---
>> I'm not sure failures are reported as expected. I would expect
>> the test to report a failure if the driver cannot fix less than
>> $STRENGTH bit flips, but it doesn't, AFAICT.
>> cf. incremental_errors_test which sets err to 0 in the 
>> "After %d biterrors per subpage, read reported error %d\n"
>> code path.
> 
> I'm not strongly opposed to this change, but please note that it's
> changing the module behavior, and some people might depend on this
> rather unusual 'module probe never succeeds' thing.
> 
> If all maintainers are okay with that, then I'll ack the patch, but I'd
> still prefer if you could switch to the userspace equivalent (recently
> added in mtd-utils) to do your regression tests.
> 
> One last thing: I'd really like to remove the in-kernel MTD tests at
> some point (assuming all the tests have been ported to mtd-utils, or
> kselftest), so it's probably not a good idea to design something that
> is based on it.

I don't have strong feelings about this patch, either.

I just found that the nandbiterrs module doesn't handle insertion
the same way as other test modules. As discussed on IRC, I was
asked to provide an automatic regression test which can be run
without user intervention.

With nandbiterrs in its current form, I can't really determine
whether the test has succeeded or failed.

Here's my current script:

set -ex
modprobe -a tango_nand ubifs
ls -l /dev/mtd*
sleep 1
time modprobe mtd_readtest    dev=0 && rmmod mtd_readtest
time modprobe mtd_readtest    dev=1 && rmmod mtd_readtest
time modprobe mtd_pagetest    dev=1 && rmmod mtd_pagetest
time modprobe mtd_speedtest   dev=1 && rmmod mtd_speedtest
time modprobe mtd_stresstest  dev=1 && rmmod mtd_stresstest
time modprobe mtd_nandbiterrs dev=1 || true # ignore return code
time flash_erase /dev/mtd1 0 0
ubiattach -m 1 -d 7
ubimkvol /dev/ubi7 -s 400MiB -N foo
mkdir /mnt/ubifs
mount -t ubifs /dev/ubi7_0 /mnt/ubifs
time bonnie++ -d /mnt/ubifs/ -u root -n 50
sleep 1
umount /mnt/ubifs
ubirmvol /dev/ubi7 -N foo
ubidetach -d 7
modprobe -r tango_nand ubifs

(The sleeps are to prevent kernel messages and user-space messages
from inter-mixing. This script is supposed to be run on a console.)

I did see David's announcement of mtd-utils-2.0.0-rc1, but right now
QA is using a 2014 buildroot-based system; so until buildroot includes
these tools, and we upgrade QA to a modern buildroot, they are stuck
with the kernel modules.

Regards.


>> ---
>>  drivers/mtd/tests/nandbiterrs.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/mtd/tests/nandbiterrs.c b/drivers/mtd/tests/nandbiterrs.c
>> index f26dec896afa..41050bcae9f1 100644
>> --- a/drivers/mtd/tests/nandbiterrs.c
>> +++ b/drivers/mtd/tests/nandbiterrs.c
>> @@ -403,7 +403,6 @@ static int __init mtd_nandbiterrs_init(void)
>>  	if (err)
>>  		goto exit_error;
>>  
>> -	err = -EIO;
>>  	pr_info("finished successfully.\n");
>>  	printk(KERN_INFO "==================================================\n");
>>