Re: [PATCH 6/8] raid6: defer implementation selection when built-in

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The boot-time RAID6 benchmark runs unconditionally when built-in, ignoring the `CONFIG_RAID6_PQ_BENCHMARK` setting and the single-algorithm optimization.
--

--- Patch [6]: [PATCH 6/8] raid6: defer implementation selection when built-in ---
commit 2c9ec4f0b44515f49715b67c2e70f8628c5abd61
Author: Christoph Hellwig <[email protected]>
Subject: raid6: defer implementation selection when built-in

This commit defers the RAID6 implementation selection to avoid holding up
early boot. It follows the pattern used in the RAID5 XOR code, allowing
full use of the timing subsystem for benchmarking.

> diff --git a/lib/raid/raid6/algos.c b/lib/raid/raid6/algos.c
> index 6f5c89ab2b17b..e7984bde5157f 100644
> --- a/lib/raid/raid6/algos.c
> +++ b/lib/raid/raid6/algos.c
> @@ -236,16 +236,6 @@ static int __init raid6_select_algo(void)
>  	int i, cycle;
>  	int error;
>  
> -	if (!IS_ENABLED(CONFIG_RAID6_PQ_BENCHMARK) || raid6_nr_algos == 1) {
> -		pr_info("raid6: skipped pq benchmark and selected %s\n",
> -			raid6_algos[raid6_nr_algos - 1]->name);
> -		static_call_update(raid6_gen_syndrome_impl,
> -				raid6_algos[raid6_nr_algos - 1]->gen_syndrome);
> -		static_call_update(raid6_xor_syndrome_impl,
> -				raid6_algos[raid6_nr_algos - 1]->xor_syndrome);
> -		return 0;
> -	}
> -
>  	/* prepare the buffer and fill it circularly with gfmul table */
>  	disk_ptr = kmalloc(PAGE_SIZE * RAID6_TEST_DISKS, GFP_KERNEL);

[ ... ]

> @@ -329,13 +319,40 @@ static int __init raid6_init(void)
>  	static_call_update(raid6_recov_datap_impl, raid6_recov_algo->datap);
>  	pr_info("raid6: using %s recovery algorithm\n", raid6_recov_algo->name);
>  
> +	if (!IS_ENABLED(CONFIG_RAID6_PQ_BENCHMARK) || raid6_nr_algos == 1) {
> +		pr_info("raid6: skipped pq benchmark and selected %s\n",
> +			raid6_algos[raid6_nr_algos - 1]->name);
> +		goto default_algo;
> +	}
> +
> +#ifdef MODULE
>  	return raid6_select_algo();
> +#else
> +	/*
> +	 * Pick the last registered implementation as the temporary default until
> +	 * calibration happens.
> +	 */
> +#endif

[ ... ]

> +/*
> + * When built-in we must register the default implementation before md
> + * initializes, but we don't want calibration to run that early as that
> + * would delay the boot process.
> + */
> +#ifndef MODULE
> +device_initcall(raid6_select_algo);
> +#endif

[Severity: Medium]
Will this unconditional device_initcall cause the benchmark to run even when
it is intentionally disabled in the configuration?

While raid6_init() checks CONFIG_RAID6_PQ_BENCHMARK and correctly skips the
early calibration phase, removing that same check from raid6_select_algo()
means this deferred call will unconditionally run the benchmarking loop later
in the boot process. It looks like this bypasses both the config setting and
the single-algorithm optimization entirely.

>  subsys_initcall(raid6_init);
>  module_exit(raid6_exit);
>  MODULE_LICENSE("GPL");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6