From patchwork Tue Feb 5 07:08:44 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Viresh Kumar X-Patchwork-Id: 2096381 Return-Path: X-Original-To: patchwork-linux-pm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 5A28CDF24C for ; Tue, 5 Feb 2013 07:08:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751137Ab3BEHIp (ORCPT ); Tue, 5 Feb 2013 02:08:45 -0500 Received: from mail-ob0-f173.google.com ([209.85.214.173]:56518 "EHLO mail-ob0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750973Ab3BEHIo (ORCPT ); Tue, 5 Feb 2013 02:08:44 -0500 Received: by mail-ob0-f173.google.com with SMTP id dn14so7344381obc.32 for ; Mon, 04 Feb 2013 23:08:44 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:x-received:in-reply-to:references:date:message-id :subject:from:to:cc:content-type:x-gm-message-state; bh=fJxTtgeByyTDSFiERNViG0O7QqlW3f2vvxTUTOzxdag=; b=RzhhIDAeD0EoMu2HEyc7Z6KQWa1BQn7yWser0eyOMPFjzJSchLodtBgLAcWFaQQNAS Nvw/LjBDUTRQg7zHizP0wjX0mSEmgOTFYsqG4la4QX665LARl3XrwOkDYc7+ODrgx3Qb T5nE/hKpVMXDSqcNmYDMZoe0mhiLyN964NvZ3+xLD9yRMSIX5w0zUDKTtdjhWtXJU/IO mX68O/FlO1htUFpeJot76K5h7dwRWHLhA5egfbiudXcwwxbYHuIxcg2Q5vcNy3UEA4Am X3Ii9S/XUa7qWIO2QV/BaXiOln/HGEaq+BotIMjPAD/zrfC5BYsUcaPmouU/H6IOfiOi ONFg== MIME-Version: 1.0 X-Received: by 10.60.5.231 with SMTP id v7mr18606424oev.62.1360048124399; Mon, 04 Feb 2013 23:08:44 -0800 (PST) Received: by 10.182.22.65 with HTTP; Mon, 4 Feb 2013 23:08:44 -0800 (PST) In-Reply-To: References: <511033EA.7090604@gmail.com> <3463348.tWNCISaOfi@vostro.rjw.lan> <51104CC7.8030800@gmail.com> <2585398.78DPBR35EF@vostro.rjw.lan> Date: Tue, 5 Feb 2013 12:38:44 +0530 Message-ID: Subject: Re: BUG in bleeding edge c560f3d From: Viresh Kumar To: "Rafael J. Wysocki" Cc: Dirk Brandewie , cpufreq@vger.kernel.org, Linux PM list X-Gm-Message-State: ALoCoQk1ihyJF0iEvn2JeSlMSi6aZWrRRKXeru/ajbdMUHpvg8QR3hpPukovRoEORRVBo8B2xBhg Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org On 5 February 2013 11:15, Viresh Kumar wrote: > On 5 February 2013 08:13, Rafael J. Wysocki wrote: >> On Monday, February 04, 2013 04:05:27 PM Dirk Brandewie wrote: > >>> Rebased a couple of hours ago testing now. ATM it looks like it is related to >>> cpufreq_stats handling of the sysfs files >> >> This looks like a problem with commit b8eed8a. Viresh? > > I had some direct chat with Dirk to understand his problem. He has got lots > of minor changes, which by themselves looks incorrect, for ex: > - He is required to cpufreq_frequency_table_put_attr() from his drivers > init() code to make it NULL... But it should already have been NULL as per > my understanding. I really want him to test his driver without any additional > target/set_policy() patches he has.. to see if linux-next works for > him or not. > > He will do this testing tomorrow and will let us know of any issues he faces. > > We have already tested linux-next and these patches on ARM TC2 and STE > u8500 (by Fabio), with reboots and lots of hotplugging. > > Though i am still going to review my own patches once more to see if there is > scope for improvement. The system on which Dirk is testing his patches has following configuration: 1 Package, 4 cpus, 8 virtual cores... Though they share their clock line in some way, it is handled by the cpufreq driver only and for the outside world (cpufreq core), it looks as there are 8 independent cpus, and so 8 policy structures. I tried to reproduce the issue at my end by removing extra cpus from my clusters and just keeping one per cluster alive from DT. And i *wasn't* able to reproduce the problem. Though i was able to find a small bug/issue in the current code for which i have following patch (attached too): @Dirk: Please give this one a try. Atleast on my system with various configurations, i couldn't see any different this patch has made, but is more logical to me. commit 9bdd6d47403e696d05953870019e791806f8d6bf Author: Viresh Kumar Date: Tue Feb 5 12:28:18 2013 +0530 cpufreq: Don't remove sysfs link for policy->cpu "cpufreq" directory in policy->cpu is never created using sysfs_create_link(), but using kobject_init_and_add(). And so we shouldn't call sysfs_remove_link() for policy->cpu(). sysfs stuff for policy->cpu is automatically removed when we call kobject_put() for dying policy. Signed-off-by: Viresh Kumar --- drivers/cpufreq/cpufreq.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) sysfs_remove_link(&cpu_dev->kobj, "cpufreq"); @@ -1072,7 +1074,6 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif pr_debug("%s: removing link, cpu: %d\n", __func__, cpu); cpufreq_cpu_put(data); unlock_policy_rwsem_write(cpu); - sysfs_remove_link(&dev->kobj, "cpufreq"); /* If cpu is last user of policy, free policy */ if (cpus == 1) { diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 7aacfbf..9567451 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1047,7 +1047,9 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif cpus = cpumask_weight(data->cpus); cpumask_clear_cpu(cpu, data->cpus); - if (unlikely((cpu == data->cpu) && (cpus > 1))) { + if (cpu != data->cpu) { + sysfs_remove_link(&dev->kobj, "cpufreq"); + } else if (cpus > 1) { /* first sibling now owns the new sysfs dir */ cpu_dev = get_cpu_device(cpumask_first(data->cpus));