Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Fri, 29 Oct 2021 18:07:18 +0200 | Subject | Re: [PATCH] cpufreq: Fix implicit type conversion |
| |
On Fri, Oct 29, 2021 at 5:51 AM Jiasheng Jiang <jiasheng@iscas.ac.cn> wrote: > > The variable 'cpu' and 'j' are defined as unsigned int. > However in the for_each_cpu, their values are assigned to -1. > That doesn't make sense
Yes, it does.
The binary representation of -1 is an all-ones value of the size of int. It is perfectly valid to store that value in an unsigned int variable.
> and in the cpumask_next() they are implicitly type conversed to int.
However, the return type of cpumask_next() is unsigned int.
> It is universally accepted that the implicit type conversion is terrible.
I wouldn't say "terrible", but yes, it is risky when dealing with variables of different sizes and possible sign-extensions.
In this particular case, I don't see a problem.
> Also, having the good programming custom will set an example for > others. > Thus, it might be better to change the definition of 'cpu' and 'j' > from unsigned int to int. > > Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn> > --- > kernel/sched/cpufreq_schedutil.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > index 4f09afd..4aff4b6 100644 > --- a/kernel/sched/cpufreq_schedutil.c > +++ b/kernel/sched/cpufreq_schedutil.c > @@ -409,7 +409,7 @@ static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu, u64 time) > struct sugov_policy *sg_policy = sg_cpu->sg_policy; > struct cpufreq_policy *policy = sg_policy->policy; > unsigned long util = 0, max = 1; > - unsigned int j; > + int j; > > for_each_cpu(j, policy->cpus) { > struct sugov_cpu *j_sg_cpu = &per_cpu(sugov_cpu, j); > @@ -746,7 +746,7 @@ static int sugov_start(struct cpufreq_policy *policy) > { > struct sugov_policy *sg_policy = policy->governor_data; > void (*uu)(struct update_util_data *data, u64 time, unsigned int flags); > - unsigned int cpu; > + int cpu; > > sg_policy->freq_update_delay_ns = sg_policy->tunables->rate_limit_us * NSEC_PER_USEC; > sg_policy->last_freq_update_time = 0; > @@ -783,7 +783,7 @@ static int sugov_start(struct cpufreq_policy *policy) > static void sugov_stop(struct cpufreq_policy *policy) > { > struct sugov_policy *sg_policy = policy->governor_data; > - unsigned int cpu; > + int cpu; > > for_each_cpu(cpu, policy->cpus) > cpufreq_remove_update_util_hook(cpu); > -- > 2.7.4 >
| |