> On Jan 30, 2023, at 1:13 PM, Bart Van Assche <bvanassche@xxxxxxx> wrote:
>
> Make the following minor changes which were reported by colleagues
> while reviewing this code:
> - Remove the parentheses from around the LOOP_DEFAULT_HW_Q_DEPTH
> definition since these are superfluous.
> - Accept other number formats than decimal, e.g. hexadecimal.
> - Do not set hw_queue_depth to an out-of-range value, even if that value
> won't be used.
> - Use the LOOP_DEFAULT_HW_Q_DEPTH macro in the kernel module parameter
> description to prevent that the description gets out of sync.
>
> This patch has been tested as follows:
>
> # modprobe -r loop
> # modprobe loop hw_queue_depth=-1
> modprobe: ERROR: could not insert 'loop': Invalid argument
> # modprobe loop hw_queue_depth=0
> modprobe: ERROR: could not insert 'loop': Invalid argument
> # modprobe loop hw_queue_depth=1; cat /sys/module/loop/parameters/hw_queue_depth
> 1
> # modprobe -r loop; modprobe loop; cat /sys/module/loop/parameters/hw_queue_depth hw_queue_depth=0x10
> 16
> # modprobe -r loop; modprobe loop; cat /sys/module/loop/parameters/hw_queue_depth hw_queue_depth=128
> 128
> # modprobe -r loop; modprobe loop hw_queue_depth=129; cat /sys/module/loop/parameters/hw_queue_depth
> 129
> # modprobe -r loop; modprobe loop hw_queue_depth=$((1<<32))
> modprobe: ERROR: could not insert 'loop': Numerical result out of range
>
> See also commit ef44c50837ab ("loop: allow user to set the queue
> depth").
>
> Cc: Chaitanya Kulkarni <kch@xxxxxxxxxx>
> Cc: Himanshu Madhani <himanshu.madhani@xxxxxxxxxx>
> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
> ---
> drivers/block/loop.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 1518a6423279..5f04235e4ff7 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -90,7 +90,7 @@ struct loop_cmd {
> };
>
> #define LOOP_IDLE_WORKER_TIMEOUT (60 * HZ)
> -#define LOOP_DEFAULT_HW_Q_DEPTH (128)
> +#define LOOP_DEFAULT_HW_Q_DEPTH 128
>
> static DEFINE_IDR(loop_index_idr);
> static DEFINE_MUTEX(loop_ctl_mutex);
> @@ -1792,9 +1792,15 @@ static int hw_queue_depth = LOOP_DEFAULT_HW_Q_DEPTH;
>
> static int loop_set_hw_queue_depth(const char *s, const struct kernel_param *p)
> {
> - int ret = kstrtoint(s, 10, &hw_queue_depth);
> + int qd, ret;
>
> - return (ret || (hw_queue_depth < 1)) ? -EINVAL : 0;
> + ret = kstrtoint(s, 0, &qd);
> + if (ret < 0)
> + return ret;
> + if (qd < 1)
> + return -EINVAL;
> + hw_queue_depth = qd;
> + return 0;
> }
>
> static const struct kernel_param_ops loop_hw_qdepth_param_ops = {
> @@ -1803,7 +1809,7 @@ static const struct kernel_param_ops loop_hw_qdepth_param_ops = {
> };
>
> device_param_cb(hw_queue_depth, &loop_hw_qdepth_param_ops, &hw_queue_depth, 0444);
> -MODULE_PARM_DESC(hw_queue_depth, "Queue depth for each hardware queue. Default: 128");
> +MODULE_PARM_DESC(hw_queue_depth, "Queue depth for each hardware queue. Default: " __stringify(LOOP_DEFAULT_HW_Q_DEPTH));
>
> MODULE_LICENSE("GPL");
> MODULE_ALIAS_BLOCKDEV_MAJOR(LOOP_MAJOR);
Reviewed-by: Himanshu Madhani <himanshu.madhani@xxxxxxxxxx <mailto:himanshu.madhani@xxxxxxxxxx>>
--
Himanshu Madhani Oracle Linux Engineering
