Date: Mon, 18 Dec 2017 14:21:38 -0800
From: Andrew Morton <akpm@linux-foundation.org>
To: Pravin Shedge <pravin.shedge4linux@gmail.com>
Cc: fkostenzer@live.at, andriy.shevchenko@linux.intel.com,
        geert@linux-m68k.org, paul.gortmaker@windriver.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] lib: add module unload support to sort tests
Message-Id: <20171218142138.f1b84a8c1ca072b15b54af33@linux-foundation.org>
In-Reply-To: <1513504167-4118-1-git-send-email-pravin.shedge4linux@gmail.com>
References: <1513504167-4118-1-git-send-email-pravin.shedge4linux@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1865
Lines: 66

On Sun, 17 Dec 2017 15:19:27 +0530 Pravin Shedge <pravin.shedge4linux@gmail.com> wrote:

> test_sort.c perform array-based and linked list sort test. Code allows to
> compile either as a loadable modules or builtin into the kernel.
> 
> Current code is not allow to unload the test_sort.ko module after
> successful completion.
> 
> This patch add support to unload the "test_sort.ko" module.
> 
> ...
>
> --- a/lib/test_sort.c
> +++ b/lib/test_sort.c
> @@ -13,11 +13,12 @@ static int __init cmpint(const void *a, const void *b)
>  
>  static int __init test_sort_init(void)
>  {
> -	int *a, i, r = 1, err = -ENOMEM;
> +	int *a, i, r = 1;
> +	int err = -EAGAIN; /* Fail will directly unload the module */
>  
>  	a = kmalloc_array(TEST_LEN, sizeof(*a), GFP_KERNEL);
>  	if (!a)
> -		return err;
> +		return -ENOMEM;
>  
>  	for (i = 0; i < TEST_LEN; i++) {
>  		r = (r * 725861) % 6599;
> @@ -26,13 +27,12 @@ static int __init test_sort_init(void)
>  
>  	sort(a, TEST_LEN, sizeof(*a), cmpint, NULL);
>  
> -	err = -EINVAL;
>  	for (i = 0; i < TEST_LEN-1; i++)
>  		if (a[i] > a[i+1]) {
>  			pr_err("test has failed\n");
> +			err = -EINVAL;
>  			goto exit;
>  		}
> -	err = 0;
>  	pr_info("test passed\n");
>  exit:
>  	kfree(a);

I'm struggling to understand this.

It seems that the current pattern for lib/test_*.c is:

- if test fails, module_init() handler returns -EFOO
- if test succeeds, module_init() handler returns 0

So the module will be auto-unloaded if it failed and will require an
rmmod if it succeeded.

Correct?

And it appears that lib/test_sort.c currently implements the above. 
And that your patch changes it so that the module_init() handler always
returns -EFOO, so the module will be aut-unloaded whether or not the
testing passed.

Correct?

If so, why do you think we shiould alter lib/test_sort.c to behave in
this atypical fashion?