| Message ID | 20230330055203.43064-3-kch@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | null_blk: add modparam checks | expand |
On 3/30/23 14:51, Chaitanya Kulkarni wrote: > Right now we don't check for valid module parameter value for > submit_queue, that allows user to set negative values. > > Add a callback null_set_submit_queues() to error out when submit_queue > value is < 1 before module is loaded. > > Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com> > --- > drivers/block/null_blk/main.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c > index cf6937f4cfa1..9e3df92d0b98 100644 > --- a/drivers/block/null_blk/main.c > +++ b/drivers/block/null_blk/main.c > @@ -100,8 +100,18 @@ static int g_no_sched; > module_param_named(no_sched, g_no_sched, int, 0444); > MODULE_PARM_DESC(no_sched, "No io scheduler"); > > +static int null_set_submit_queues(const char *s, const struct kernel_param *kp) > +{ > + return null_param_store_int(s, kp->arg, 1, INT_MAX); > +} > + > +static const struct kernel_param_ops null_submit_queues_param_ops = { > + .set = null_set_submit_queues, > + .get = param_get_int, > +}; > + > static int g_submit_queues = 1; > -module_param_named(submit_queues, g_submit_queues, int, 0444); > +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_poll_queues = 1; I would do this: +#define NULL_PARAM(_name, _min, _max) \ +static int null_param_##_name##_set(const char *s, \ + const struct kernel_param *kp) \ +{ \ + return null_param_store_int(s, kp->arg, _min, _max); \ +} \ + \ +static const struct kernel_param_ops null_##_name##_param_ops = { \ + .set = null_param_##_name##_set, \ + .get = param_get_int, \ +} + And then have: +NULL_PARAM(submit_queues, 1, INT_MAX); +NULL_PARAM(poll_queues, 1, num_online_cpus()); +NULL_PARAM(queue_mode, NULL_Q_BIO, NULL_Q_MQ); +NULL_PARAM(gb, 1, INT_MAX); +NULL_PARAM(bs, 512, 4096); +NULL_PARAM(max_sectors, 1, INT_MAX); +NULL_PARAM(irqmode, NULL_IRQ_NONE, NULL_IRQ_TIMER); +NULL_PARAM(hw_qdepth, 1, INT_MAX); That can be done in a single patch and is overall a lot less code.
> I would do this: > > +#define NULL_PARAM(_name, _min, _max) \ > +static int null_param_##_name##_set(const char *s, \ > + const struct kernel_param *kp) \ > +{ \ > + return null_param_store_int(s, kp->arg, _min, _max); \ > +} \ > + \ > +static const struct kernel_param_ops null_##_name##_param_ops = { \ > + .set = null_param_##_name##_set, \ > + .get = param_get_int, \ > +} > + > > And then have: > > +NULL_PARAM(submit_queues, 1, INT_MAX); > +NULL_PARAM(poll_queues, 1, num_online_cpus()); > +NULL_PARAM(queue_mode, NULL_Q_BIO, NULL_Q_MQ); > +NULL_PARAM(gb, 1, INT_MAX); > +NULL_PARAM(bs, 512, 4096); > +NULL_PARAM(max_sectors, 1, INT_MAX); > +NULL_PARAM(irqmode, NULL_IRQ_NONE, NULL_IRQ_TIMER); > +NULL_PARAM(hw_qdepth, 1, INT_MAX); > > That can be done in a single patch and is overall a lot less code. > I did the same thing at first, however it doesn't allow us to print the right module parameter specific error message which I to add in this series especially for where this patch limits it nr_online_cpu(). let me send out V2 with right error messages ... -ck
On 3/31/23 04:01, Chaitanya Kulkarni wrote: > >> I would do this: >> >> +#define NULL_PARAM(_name, _min, _max) \ >> +static int null_param_##_name##_set(const char *s, \ >> + const struct kernel_param *kp) \ >> +{ \ >> + return null_param_store_int(s, kp->arg, _min, _max); \ >> +} \ >> + \ >> +static const struct kernel_param_ops null_##_name##_param_ops = { \ >> + .set = null_param_##_name##_set, \ >> + .get = param_get_int, \ >> +} >> + >> >> And then have: >> >> +NULL_PARAM(submit_queues, 1, INT_MAX); >> +NULL_PARAM(poll_queues, 1, num_online_cpus()); >> +NULL_PARAM(queue_mode, NULL_Q_BIO, NULL_Q_MQ); >> +NULL_PARAM(gb, 1, INT_MAX); >> +NULL_PARAM(bs, 512, 4096); >> +NULL_PARAM(max_sectors, 1, INT_MAX); >> +NULL_PARAM(irqmode, NULL_IRQ_NONE, NULL_IRQ_TIMER); >> +NULL_PARAM(hw_qdepth, 1, INT_MAX); >> >> That can be done in a single patch and is overall a lot less code. >> > > I did the same thing at first, however it doesn't allow us to print > the right module parameter specific error message which I > to add in this series especially for where this patch limits it > nr_online_cpu(). Given that your changes are checking the value ranges only, it would not be hard to craft a common error message and add it to the null_param_##_name##_set() definition. > > let me send out V2 with right error messages ... > > -ck > >
On 3/30/23 3:22?AM, Damien Le Moal wrote: > On 3/30/23 14:51, Chaitanya Kulkarni wrote: >> Right now we don't check for valid module parameter value for >> submit_queue, that allows user to set negative values. >> >> Add a callback null_set_submit_queues() to error out when submit_queue >> value is < 1 before module is loaded. >> >> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com> >> --- >> drivers/block/null_blk/main.c | 12 +++++++++++- >> 1 file changed, 11 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c >> index cf6937f4cfa1..9e3df92d0b98 100644 >> --- a/drivers/block/null_blk/main.c >> +++ b/drivers/block/null_blk/main.c >> @@ -100,8 +100,18 @@ static int g_no_sched; >> module_param_named(no_sched, g_no_sched, int, 0444); >> MODULE_PARM_DESC(no_sched, "No io scheduler"); >> >> +static int null_set_submit_queues(const char *s, const struct kernel_param *kp) >> +{ >> + return null_param_store_int(s, kp->arg, 1, INT_MAX); >> +} >> + >> +static const struct kernel_param_ops null_submit_queues_param_ops = { >> + .set = null_set_submit_queues, >> + .get = param_get_int, >> +}; >> + >> static int g_submit_queues = 1; >> -module_param_named(submit_queues, g_submit_queues, int, 0444); >> +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_poll_queues = 1; > > I would do this: > > +#define NULL_PARAM(_name, _min, _max) \ > +static int null_param_##_name##_set(const char *s, \ > + const struct kernel_param *kp) \ > +{ \ > + return null_param_store_int(s, kp->arg, _min, _max); \ > +} \ > + \ > +static const struct kernel_param_ops null_##_name##_param_ops = { \ > + .set = null_param_##_name##_set, \ > + .get = param_get_int, \ > +} > + > > And then have: > > +NULL_PARAM(submit_queues, 1, INT_MAX); > +NULL_PARAM(poll_queues, 1, num_online_cpus()); > +NULL_PARAM(queue_mode, NULL_Q_BIO, NULL_Q_MQ); > +NULL_PARAM(gb, 1, INT_MAX); > +NULL_PARAM(bs, 512, 4096); > +NULL_PARAM(max_sectors, 1, INT_MAX); > +NULL_PARAM(irqmode, NULL_IRQ_NONE, NULL_IRQ_TIMER); > +NULL_PARAM(hw_qdepth, 1, INT_MAX); > > That can be done in a single patch and is overall a lot less code. Agree, let's please try and reduce the number of patches in a series when it's not really useful to split things up.
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index cf6937f4cfa1..9e3df92d0b98 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -100,8 +100,18 @@ static int g_no_sched; module_param_named(no_sched, g_no_sched, int, 0444); MODULE_PARM_DESC(no_sched, "No io scheduler"); +static int null_set_submit_queues(const char *s, const struct kernel_param *kp) +{ + return null_param_store_int(s, kp->arg, 1, INT_MAX); +} + +static const struct kernel_param_ops null_submit_queues_param_ops = { + .set = null_set_submit_queues, + .get = param_get_int, +}; + static int g_submit_queues = 1; -module_param_named(submit_queues, g_submit_queues, int, 0444); +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_poll_queues = 1;
Right now we don't check for valid module parameter value for submit_queue, that allows user to set negative values. Add a callback null_set_submit_queues() to error out when submit_queue value is < 1 before module is loaded. Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com> --- drivers/block/null_blk/main.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)