Re: [PATCH v5 3/3] PM: Introduce Intel PowerClamp Driver

[jacob pan <jacob.jun.pan@linux.intel.com>]

On Thu, 03 Jan 2013 19:51:04 -0800
Joe Perches <joe@perches.com> wrote:

> On Thu, 2013-01-03 at 07:10 -0800, Jacob Pan wrote:
> > Intel PowerClamp driver performs synchronized idle injection across
> > all online CPUs. The goal is to maintain a given package level
> > C-state ratio.
> 
> []
> 
> > +static int window_size_set(const char *arg, const struct
> > kernel_param *kp) +{
> > +	int ret = 0;
> > +	unsigned long new_window_size;
> > +
> > +	ret = kstrtoul(arg, 10, &new_window_size);
> > +	if (ret)
> > +		goto exit_win;
> > +	if (new_window_size > 10 || new_window_size < 2) {
> > +		pr_err("Invalid window size %lu, between 2-10\n",
> > +			new_window_size);
> > +		ret = -EINVAL;
> > +	}
> > +
> > +	window_size = new_window_size;
> 
> Possible assignment of known invalid windows size?
> Maybe you should add
> 	goto exit;
> after
> 	ret = -EINVAL;
> 
> or add
> 	new_window_size = clamp(new_window_size, 2ul, 10ul);
Good catch. The window size range 2-10 is somewhat arbitrary, greater
than 10 should also work just not recommended. I will reword that. But
it is good to clamp it as you suggested, i will do that for the
duration parameter also.

Thanks,

Jacob