From patchwork Fri Feb 8 05:09:13 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Viresh Kumar X-Patchwork-Id: 2114861 Return-Path: X-Original-To: patchwork-linux-pm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id 39EFC3FCD5 for ; Fri, 8 Feb 2013 05:09:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751529Ab3BHFJP (ORCPT ); Fri, 8 Feb 2013 00:09:15 -0500 Received: from mail-ob0-f177.google.com ([209.85.214.177]:45591 "EHLO mail-ob0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751237Ab3BHFJO (ORCPT ); Fri, 8 Feb 2013 00:09:14 -0500 Received: by mail-ob0-f177.google.com with SMTP id wc18so3476469obb.8 for ; Thu, 07 Feb 2013 21:09:13 -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=VHYSxwwZQGS4TmPnf64+POf8BFH7HD3hYYQYg0py0W4=; b=U8HkPmsJxIobHrKpPLZ+7Dj6fGl4gykRVEg/1x5JdyrR+I6p6wpRkrcBKDtX0lAnZj zLAPnzH27JA+5TtM18gtNDIto/7kLQOjcvzDLHlU9cNDI71hr5BVnyuSpPMqgAogCqzm UbVlJCF+DRVLbc2ctYjcAEWEpQKiwM9GjXGGcerGurJZHGbsOeDNJo2c69MgY3gE6vZP 4QUVlVlWYLf7Udv+lxq/oppsn7sePqYPQK4SPX/KkOAAZOfVJ3bLd3kBRQSqCBs4sAo2 YqN6qwBfALouNUNiC9ZpzCJ11Q1UepOgfYxbLBRKjUU44o5A0FYR8MqhoC+2NMfegpkB Ocnw== MIME-Version: 1.0 X-Received: by 10.60.11.71 with SMTP id o7mr3155002oeb.69.1360300153550; Thu, 07 Feb 2013 21:09:13 -0800 (PST) Received: by 10.182.22.65 with HTTP; Thu, 7 Feb 2013 21:09:13 -0800 (PST) In-Reply-To: <1947746.IyFppZQEx8@vostro.rjw.lan> References: <2158181.aoVXdpLJe2@vostro.rjw.lan> <1947746.IyFppZQEx8@vostro.rjw.lan> Date: Fri, 8 Feb 2013 10:39:13 +0530 Message-ID: Subject: Re: [PATCH 0/4] CPUFreq Fixes for 3.9 From: Viresh Kumar To: "Rafael J. Wysocki" Cc: valdis.kletnieks@vt.edu, artem.savkov@gmail.com, cpufreq@vger.kernel.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, linaro-dev@lists.linaro.org, robin.randhawa@arm.com, Steve.Bannister@arm.com, Liviu.Dudau@arm.com, dirk.brandewie@gmail.com X-Gm-Message-State: ALoCoQmRk7z6r8AG4zwtrA7AFmzGG9HSapFPZ+T9a8x43cD0HHDcoLQhNmQyEH8h1XIH+SHI4YZi Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org On 8 February 2013 04:37, Rafael J. Wysocki wrote: > BTW, there still are locking problems in linux-next. Why do we need > to take cpufreq_driver_lock() around driver->init() in cpufreq_add_dev(), > in particular? I thought a bit more and realized there is no such limitation on cpufreq_driver->ops about calling routines which can sleep. And thus we shoudln't have locks around any of these. I have got a patch for it, that i would fold-back into the original patch that introduced locking fixes (attached too for testing): From: Viresh Kumar Date: Fri, 8 Feb 2013 10:35:31 +0530 Subject: [PATCH] cpufreq: Remove unnecessary locking I have placed some locks intentionally around calls to driver->ops (init/exit), which look to be wrong as these calls can call routines that potentially sleep. Lets remove these locks. Signed-off-by: Viresh Kumar Tested-by: Artem Savkov --- drivers/cpufreq/cpufreq.c | 7 ------- 1 file changed, 7 deletions(-) cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus); @@ -1100,10 +1095,8 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif wait_for_completion(cmp); pr_debug("wait complete\n"); - spin_lock_irqsave(&cpufreq_driver_lock, flags); if (driver->exit) driver->exit(data); - spin_unlock_irqrestore(&cpufreq_driver_lock, flags); free_cpumask_var(data->related_cpus); free_cpumask_var(data->cpus); diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 5d8a422..04aab05 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -795,10 +795,8 @@ static int cpufreq_add_dev_interface(unsigned int cpu, if (ret) { pr_debug("setting policy failed\n"); - spin_lock_irqsave(&cpufreq_driver_lock, flags); if (driver->exit) driver->exit(policy); - spin_unlock_irqrestore(&cpufreq_driver_lock, flags); } return ret; @@ -920,17 +918,14 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) init_completion(&policy->kobj_unregister); INIT_WORK(&policy->update, handle_update); - spin_lock_irqsave(&cpufreq_driver_lock, flags); /* call driver. From then on the cpufreq must be able * to accept all calls to ->verify and ->setpolicy for this CPU */ ret = driver->init(policy); if (ret) { pr_debug("initialization failed\n"); - spin_unlock_irqrestore(&cpufreq_driver_lock, flags); goto err_set_policy_cpu; } - spin_unlock_irqrestore(&cpufreq_driver_lock, flags); /* related cpus should atleast have policy->cpus */