Re: [PATCH v3] cpufreq: fix race on cpufreq online

[Viresh Kumar]

Don't use in-reply-to for single patches. It is mostly used when you are
updating a single patch in a patchset and it makes sense to continue the
discussion in the same thread. In this case, we have a fresh patchset and it
makes the same thread messy.

On 10-05-22, 23:42, Schspa Shi wrote:
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 80f535cc8a75..d93958dbdab8 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1337,12 +1337,12 @@ static int cpufreq_online(unsigned int cpu)
>  		down_write(&policy->rwsem);
>  		policy->cpu = cpu;
>  		policy->governor = NULL;
> -		up_write(&policy->rwsem);
>  	} else {
>  		new_policy = true;
>  		policy = cpufreq_policy_alloc(cpu);
>  		if (!policy)
>  			return -ENOMEM;
> +		down_write(&policy->rwsem);
>  	}

I am not sure, but maybe there were issues in calling init() with rwsem held, as
it may want to call some API from there.

>  	if (!new_policy && cpufreq_driver->online) {
> @@ -1382,7 +1382,6 @@ static int cpufreq_online(unsigned int cpu)
>  		cpumask_copy(policy->related_cpus, policy->cpus);
>  	}
>  
> -	down_write(&policy->rwsem);
>  	/*
>  	 * affected cpus must always be the one, which are online. We aren't
>  	 * managing offline cpus here.
> @@ -1533,7 +1532,7 @@ static int cpufreq_online(unsigned int cpu)
>  	for_each_cpu(j, policy->real_cpus)
>  		remove_cpu_dev_symlink(policy, get_cpu_device(j));
>  
> -	up_write(&policy->rwsem);
> +	cpumask_clear(policy->cpus);

I don't think you can do that safely. offline() or exit() may depend on
policy->cpus being set to all CPUs.

>  
>  out_offline_policy:
>  	if (cpufreq_driver->offline)
> @@ -1542,6 +1541,7 @@ static int cpufreq_online(unsigned int cpu)
>  out_exit_policy:
>  	if (cpufreq_driver->exit)
>  		cpufreq_driver->exit(policy);
> +	up_write(&policy->rwsem);
>  
>  out_free_policy:
>  	cpufreq_policy_free(policy);

Apart from the issues highlighted about, I think we are trying to fix an issue
locally which can happen at other places as well. Does something like this fix
your problem at hand ?

Untested.

-- 
viresh

-------------------------8<-------------------------