From patchwork Mon Jul 29 09:41:45 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: "Srivatsa S. Bhat" X-Patchwork-Id: 2834870 Return-Path: X-Original-To: patchwork-linux-pm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id BDA999F9BE for ; Mon, 29 Jul 2013 09:45:35 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 2FD9820260 for ; Mon, 29 Jul 2013 09:45:34 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 98DDE20254 for ; Mon, 29 Jul 2013 09:45:32 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751386Ab3G2Jp2 (ORCPT ); Mon, 29 Jul 2013 05:45:28 -0400 Received: from e28smtp05.in.ibm.com ([122.248.162.5]:54291 "EHLO e28smtp05.in.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754046Ab3G2JpZ (ORCPT ); Mon, 29 Jul 2013 05:45:25 -0400 Received: from /spool/local by e28smtp05.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 29 Jul 2013 15:09:29 +0530 Received: from d28dlp02.in.ibm.com (9.184.220.127) by e28smtp05.in.ibm.com (192.168.1.135) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Mon, 29 Jul 2013 15:09:26 +0530 Received: from d28relay03.in.ibm.com (d28relay03.in.ibm.com [9.184.220.60]) by d28dlp02.in.ibm.com (Postfix) with ESMTP id 9082A3940058; Mon, 29 Jul 2013 15:15:12 +0530 (IST) Received: from d28av05.in.ibm.com (d28av05.in.ibm.com [9.184.220.67]) by d28relay03.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r6T9kGjd42860774; Mon, 29 Jul 2013 15:16:16 +0530 Received: from d28av05.in.ibm.com (loopback [127.0.0.1]) by d28av05.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r6T9jGlm026382; Mon, 29 Jul 2013 19:45:17 +1000 Received: from srivatsabhat.in.ibm.com (srivatsabhat.in.ibm.com [9.124.35.141] (may be forged)) by d28av05.in.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with ESMTP id r6T9jGSg026327; Mon, 29 Jul 2013 19:45:16 +1000 Message-ID: <51F638D9.7050904@linux.vnet.ibm.com> Date: Mon, 29 Jul 2013 15:11:45 +0530 From: "Srivatsa S. Bhat" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120828 Thunderbird/15.0 MIME-Version: 1.0 To: "Rafael J. Wysocki" CC: =?UTF-8?B?VG9yYWxmIEbDtnJzdGVy?= , cpufreq@vger.kernel.org, Linux PM list Subject: Re: stable 3-10-3: strange output of "lsmod | grep ^acpi_cpufreq" References: <51F40612.2050403@gmx.de> <2111514.pxW5saG1J3@vostro.rjw.lan> <18359786.D7glpto546@vostro.rjw.lan> <2368277.VjYrsHUseA@vostro.rjw.lan> In-Reply-To: <2368277.VjYrsHUseA@vostro.rjw.lan> X-TM-AS-MML: No X-Content-Scanned: Fidelis XPS MAILER x-cbid: 13072909-8256-0000-0000-0000088E91B9 Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org X-Spam-Status: No, score=-8.4 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 07/29/2013 04:50 AM, Rafael J. Wysocki wrote: > On Monday, July 29, 2013 12:43:59 AM Rafael J. Wysocki wrote: >> On Monday, July 29, 2013 12:11:18 AM Rafael J. Wysocki wrote: >>> On Sunday, July 28, 2013 12:21:22 PM Toralf Förster wrote: >>>> On 07/28/2013 01:39 AM, Rafael J. Wysocki wrote: >>>>> On Saturday, July 27, 2013 07:40:34 PM Toralf Förster wrote: >>>>>> it gives at a ThinkPad T420: >>>>>> >>>>>> tfoerste@n22 ~/tmp $ lsmod | grep ^acpi_cpufreq >>>>>> acpi_cpufreq 12902 2147483647 >>>>> >>>>> That is -1, which indicates some module refcount woes. >>>> >>>> yes, ofc. >>>> >>>> The issue apears after 1 s2ram/resume cycle, before s2ram the refcount is 1. >>>> >>>>> I definitely can't see that with the mainline on my machines. >>>> >>>> It is in mainline too. >>> >>> Does the appended patch help? >> >> Actually, something as simple as this also should help: >> >> --- >> From: Rafael J. Wysocki >> Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume >> >> Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the >> driver module refcount, __cpufreq_remove_dev() causes that refcount >> to become negative after a suspend/resume cycle, for example. >> >> To prevent this from happening make __cpufreq_remove_dev() put >> the policy kobject only instead of calling cpufreq_cpu_put(). > > Having a deeper look at it, though, I see that in fact the whole > cpufreq_cpu_put() is needed if the CPU is not the last one for the given > policy and is not needed at all otherwise (as described in the changelog > of the patch below). > > Srivatsa, does this make sense to you? > Code-wise, your patch looks good to me. But one thing in the existing code struck me as a little strange. I'm assuming that the module_get() is used in the cpufreq core to ensure that until the cpufreq core is managing (atleast one) CPU(s), the cpufreq backend driver module (eg: acpi-cpufreq) can't be removed. But the cpufreq_add_dev() function does a module *put* at the end of initializing a fresh CPU. 1057 kobject_uevent(&policy->kobj, KOBJ_ADD); 1058 module_put(cpufreq_driver->owner); 1059 pr_debug("initialization complete\n"); 1060 1061 return 0; So at that point, the module refcount of the cpufreq backend driver will become zero, since we dropped it. Perhaps that was not the original intention. Looking further (with respect to the problem we are discussing), the root-cause is that a fresh CPU never does a cpufreq_cpu_get() for itself, but does it only for the other CPUs in its policy->cpus mask. From cpufreq_add_dev_symlink(): 814 for_each_cpu(j, policy->cpus) { 815 struct cpufreq_policy *managed_policy; 816 struct device *cpu_dev; 817 818 if (j == cpu) 819 continue; 820 821 pr_debug("CPU %u already managed, adding link\n", j); 822 managed_policy = cpufreq_cpu_get(cpu); 823 cpu_dev = get_cpu_device(j); So, that 'continue' statement skips bumping the refcount on behalf of the master CPU. So, I wonder if it would be a good idea to instead allow that CPU to take a module refcount as well. That way, the problem that Toralf reported would go away, and at the same time, we can ensure that as long as the cpufreq core is managing CPUs, the cpufreq-backend-driver module refcount never drops to zero. Something like this: Thoughts? Regards, Srivatsa S. Bhat > --- > From: Rafael J. Wysocki > Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume > > Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the > driver module refcount, __cpufreq_remove_dev() causes that refcount > to become negative for the acpi-cpufreq driver after a suspend/resume > cycle. > > This is not the only bad thing that happens there, however, because > kobject_put() should only be called for the policy kobject at this > point if the CPU is not the last one for that policy. > > Namely, if the given CPU is the last one for that policy, the > policy kobject's refcount should be 1 at this point, as set by > cpufreq_add_dev_interface(), and only needs to be dropped once for > the kobject to go away. This actually happens under the cpu == 1 > check, so it need not be done before by cpufreq_cpu_put(). > > On the other hand, if the given CPU is not the last one for that > policy, this means that cpufreq_add_policy_cpu() has been called > at least once for that policy and cpufreq_cpu_get() has been > called for it too. To balance that cpufreq_cpu_get(), we need to > call pufreq_cpu_put() in that case. > > Thus, to fix the described problem and keep the reference > counters balanced in both cases, move the cpufreq_cpu_get() call > in __cpufreq_remove_dev() to the code path executed only for > CPUs that share the policy with other CPUs. > > Reported-by: Toralf Förster > Signed-off-by: Rafael J. Wysocki > --- > drivers/cpufreq/cpufreq.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > Index: linux-pm/drivers/cpufreq/cpufreq.c > =================================================================== > --- linux-pm.orig/drivers/cpufreq/cpufreq.c > +++ linux-pm/drivers/cpufreq/cpufreq.c > @@ -1181,7 +1181,6 @@ static int __cpufreq_remove_dev(struct d > __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); > > pr_debug("%s: removing link, cpu: %d\n", __func__, cpu); > - cpufreq_cpu_put(data); > > /* If cpu is last user of policy, free policy */ > if (cpus == 1) { > @@ -1205,9 +1204,12 @@ static int __cpufreq_remove_dev(struct d > free_cpumask_var(data->related_cpus); > free_cpumask_var(data->cpus); > kfree(data); > - } else if (cpufreq_driver->target) { > - __cpufreq_governor(data, CPUFREQ_GOV_START); > - __cpufreq_governor(data, CPUFREQ_GOV_LIMITS); > + } else { > + cpufreq_cpu_put(data); > + if (cpufreq_driver->target) { > + __cpufreq_governor(data, CPUFREQ_GOV_START); > + __cpufreq_governor(data, CPUFREQ_GOV_LIMITS); > + } > } > > per_cpu(cpufreq_policy_cpu, cpu) = -1; > Reviewed-by: Srivatsa S. Bhat --- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index a4ad733..ecfbc52 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -878,9 +878,14 @@ static int cpufreq_add_dev_interface(unsigned int cpu, } write_unlock_irqrestore(&cpufreq_driver_lock, flags); + /* Bump up the refcount for this CPU */ + policy = cpufreq_cpu_get(cpu); + ret = cpufreq_add_dev_symlink(cpu, policy); - if (ret) + if (ret) { + cpufreq_cpu_put(policy); goto err_out_kobj_put; + } memcpy(&new_policy, policy, sizeof(struct cpufreq_policy)); /* assure that the starting sequence is run in __cpufreq_set_policy */