Re: [PATCH V2 1/1] null_blk: add moddule parameter check

[Damien Le Moal <dlemoal@kernel.org>]

On 4/10/23 14:13, Chaitanya Kulkarni wrote:
>  static bool g_virt_boundary = false;
>  module_param_named(virt_boundary, g_virt_boundary, bool, 0444);
>  MODULE_PARM_DESC(virt_boundary, "Require a virtual boundary for the device. Default: False");
> @@ -86,11 +113,13 @@ module_param_named(no_sched, g_no_sched, int, 0444);
>  MODULE_PARM_DESC(no_sched, "No io scheduler");
>  
>  static int g_submit_queues = 1;
> -module_param_named(submit_queues, g_submit_queues, int, 0444);
> +NULL_PARAM(submit_queues, 1, INT_MAX);

INT_MAX ? That is a little large... Why not using CONFIG_NR_CPUS ? Or
CONFIG_NR_CPUS * 2 to leave some room for experiments.

> +device_param_cb(submit_queues, &null_submit_queues_param_ops, &g_submit_queues, 0444);
>  MODULE_PARM_DESC(submit_queues, "Number of submission queues");

[...]

>  static int g_bs = 512;
> -module_param_named(bs, g_bs, int, 0444);
> +NULL_PARAM(bs, 1, PAGE_SIZE);

Allowing min=1 here does not make sense. This should be 512. And you could add a
"bool is_power_of_2" argument to null_param_store_int() to check that the value
is a power of 2.

> +device_param_cb(bs, &null_bs_param_ops, &g_bs, 0444);
>  MODULE_PARM_DESC(bs, "Block size (in bytes)");

[...]

>  static int g_hw_queue_depth = 64;
> -module_param_named(hw_queue_depth, g_hw_queue_depth, int, 0444);
> +NULL_PARAM(hw_qdepth, 1, INT_MAX);

INT_MAX here is a little silly. Way too large to make any sense in my opinion.
NVMe allows up to 65536, why not use the same value here ?

> +device_param_cb(hw_queue_depth, &null_hw_qdepth_param_ops, &g_hw_queue_depth, 0444);
>  MODULE_PARM_DESC(hw_queue_depth, "Queue depth for each hardware queue. Default: 64");
>  
>  static bool g_use_per_node_hctx;