Re: [RFC][PATCH] cpufreq: Do not hold driver module references for additional policy CPUs

["Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>]

On 08/01/2013 05:38 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The cpufreq core is a little inconsistent in the way it uses the
> driver module refcount.
> 
> Namely, if __cpufreq_add_dev() is called for a CPU without siblings
> or generally a CPU for which a new policy object has to be created,
> it grabs a reference to the driver module to start with, but drops
> that reference before returning.  As a result, the driver module
> refcount is then equal to 0 after __cpufreq_add_dev() has returned.
> 
> On the other hand, if the given CPU is a sibling of some other
> CPU already having a policy, cpufreq_add_policy_cpu() is called
> to link the new CPU to the existing policy.  In that case,
> cpufreq_cpu_get() is called to obtain that policy and grabs a
> reference to the driver module, but that reference is not
> released and the module refcount will be different from 0 after
> __cpufreq_add_dev() returns (unless there is an error).  That
> prevents the driver module from being unloaded until
> __cpufreq_remove_dev() is called for all the CPUs that
> cpufreq_add_policy_cpu() was called for previously.
> 
> To remove that inconsistency make cpufreq_add_policy_cpu() execute
> cpufreq_cpu_put() for the given policy before returning, which
> decrements the driver module refcount so that it will be 0 after
> __cpufreq_add_dev() returns,

Removing the inconsistency is a good thing, but I think we should
make it consistent the other way around - make a CPU-online increment
the driver module refcount and decrement it only on CPU-offline.

The reason is, one should not be able to unload the back-end cpufreq
driver module when some CPUs are still being managed. Nasty things
will result if we allow that. For example, if we unload the module,
and then try to do a CPU offline, then the cpufreq hotplug notifier
won't even be called (because cpufreq_unregister_driver also
unregisters the hotplug notifier). And that might be troublesome.

Even worse, if we unload a cpufreq driver module and load a new
one and *then* try to offline the CPU, then the cpufreq_driver->exit()
function that we call during CPU offline will end up calling the
corresponding function of an entirely different driver! So the
->init() and ->exit() calls won't match.

These complications won't exist if we simply prevent unloading the
driver module as long as they are used in managing atleast one CPU.
So I think it would be good to make the code consistent that way.

Regards,
Srivatsa S. Bhat

> but also make it take a reference to
> the policy itself using kobject_get() and do not release that
> reference (unless there's an error or system resume is under way),
> which again is consistent with the "raw" __cpufreq_add_dev()
> behavior.
> 
> Accordingly, modify __cpufreq_remove_dev() to use kobject_put() to
> drop policy references taken by cpufreq_add_policy_cpu().
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> On top of current linux-pm.git/linux-next.
> 
> Thanks,
> Rafael
> 
> ---
>  drivers/cpufreq/cpufreq.c |   24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -908,8 +908,10 @@ static int cpufreq_add_policy_cpu(unsign
>  	unsigned long flags;
> 
>  	policy = cpufreq_cpu_get(sibling);
> -	WARN_ON(!policy);
> +	if (WARN_ON_ONCE(!policy))
> +		return -ENODATA;
> 
> +	kobject_get(&policy->kobj);
>  	if (has_target)
>  		__cpufreq_governor(policy, CPUFREQ_GOV_STOP);
> 
> @@ -932,14 +934,14 @@ static int cpufreq_add_policy_cpu(unsign
>  	/* Don't touch sysfs links during light-weight init */
>  	if (frozen) {
>  		/* Drop the extra refcount that we took above */
> -		cpufreq_cpu_put(policy);
> -		return 0;
> +		kobject_put(&policy->kobj);
> +	} else {
> +		ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
> +		if (ret)
> +			kobject_put(&policy->kobj);
>  	}
> 
> -	ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
> -	if (ret)
> -		cpufreq_cpu_put(policy);
> -
> +	cpufreq_cpu_put(policy);
>  	return ret;
>  }
>  #endif
> @@ -1298,10 +1300,14 @@ static int __cpufreq_remove_dev(struct d
>  		if (!frozen)
>  			cpufreq_policy_free(data);
>  	} else {
> -
> +		/*
> +		 * There are more CPUs using the same policy, so only drop the
> +		 * reference taken by cpufreq_add_policy_cpu() (unless the
> +		 * system is suspending).
> +		 */
>  		if (!frozen) {
>  			pr_debug("%s: removing link, cpu: %d\n", __func__, cpu);
> -			cpufreq_cpu_put(data);
> +			kobject_put(&data->kobj);
>  		}
> 
>  		if (cpufreq_driver->target) {
>