From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.6 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DB203C433E1 for ; Fri, 26 Jun 2020 15:58:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id B7BB420773 for ; Fri, 26 Jun 2020 15:58:00 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="m8J34OgF" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728537AbgFZP54 (ORCPT ); Fri, 26 Jun 2020 11:57:56 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57034 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728491AbgFZP54 (ORCPT ); Fri, 26 Jun 2020 11:57:56 -0400 Received: from mail-wm1-x344.google.com (mail-wm1-x344.google.com [IPv6:2a00:1450:4864:20::344]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id EFD39C03E97A for ; Fri, 26 Jun 2020 08:57:55 -0700 (PDT) Received: by mail-wm1-x344.google.com with SMTP id l17so9308292wmj.0 for ; Fri, 26 Jun 2020 08:57:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=SLVYA+KPlPsy9yYgUjFYIqSkRv4wawTtFWm+qvVkpo4=; b=m8J34OgFfXzLvgc2if7QNbh2n9WDF0es4wCI5PdUUYAIWFt1H+DMIQ8TdH8szFYi3Z OhziB1Ey6NE4hEfulz5iFal5p5A7Sc7x4SAP5aUJbDfLwn7RhY/7b74L1JLOy9xEXkTB HT/ARs/NhMlaXXLdlepTcS1Gr9xCxFnc4KaqGrzKtcw+JFtafDJfyB7nAjQb/sdTQIse wd/v6QR7DlZnmNvj8bTL8Rpiwzjx+JuSGn53xTDo7qJbeUpH9kUdnkq39ODpH8H+B1RS qOj1vVvu/dzbZXcNvxmIT7D5cQgP8LO1E4K+NcP4IscV6XuNXT9oimRBsqC9SxdlHjxG +SLw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=SLVYA+KPlPsy9yYgUjFYIqSkRv4wawTtFWm+qvVkpo4=; b=WCffV08lglJ2w+QBkNTS2f/sLvnixn8t8DwYYifqLAhmZZBP5eLZWwb28o0QG5p7Zy A0rxhuC+m5sIhO1R48HTvnHe+i3394fysPW8ROSMXf20PEmcFMe2DAA1oi2mpJxs4qLF 2F0qh6azT2uUUzvyPytkWPqrWjYaBV6fQ8Df/XNz65P3P46JMdKUutSdYHZWgD8i/sGC QM0CgWxQyogarQXlNNvDbTsrpkXqZpkxlg2Me2tcNmdLXI3TnGiRcJzmSOuAwBS6QJJi FMJBFlUtiscORI4sykDQra43M/PaOrXPY0aE5Gj7ibgZZBKywJAtrIWHgUZA5qwdPVzQ LXPw== X-Gm-Message-State: AOAM530L53N5vYviMxD3KDOqfGl//IXOjECpbDbNaLD7X8e2DTMB5OZI BecBVEM6NGBOBsqw869gYgtYpw== X-Google-Smtp-Source: ABdhPJyN/TqWJB0s+HOEo3+G7pkP5DR7DUqjxT+LOyXk/S8tZH/iVagIQRM3AUWWBY3aR03bTU2d5g== X-Received: by 2002:a1c:7e49:: with SMTP id z70mr4013279wmc.24.1593187074525; Fri, 26 Jun 2020 08:57:54 -0700 (PDT) Received: from google.com ([2a00:79e0:d:110:d6cc:2030:37c1:9964]) by smtp.gmail.com with ESMTPSA id f1sm17429843wmj.12.2020.06.26.08.57.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 26 Jun 2020 08:57:53 -0700 (PDT) Date: Fri, 26 Jun 2020 16:57:50 +0100 From: Quentin Perret To: Viresh Kumar Cc: Rafael Wysocki , Jonathan Corbet , linux-pm@vger.kernel.org, Vincent Guittot , kernel-team@android.com, tkjos@google.com, adharmap@codeaurora.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH V3 3/3] cpufreq: Specify default governor on command line Message-ID: <20200626155750.GA540785@google.com> References: <7eb38608b2b32c0c72dfb160c51206ec42e74e35.1593143118.git.viresh.kumar@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7eb38608b2b32c0c72dfb160c51206ec42e74e35.1593143118.git.viresh.kumar@linaro.org> Sender: linux-doc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-doc@vger.kernel.org On Friday 26 Jun 2020 at 09:21:44 (+0530), Viresh Kumar wrote: > index e798a1193bdf..93c6399c1a42 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -50,6 +50,9 @@ static LIST_HEAD(cpufreq_governor_list); > #define for_each_governor(__governor) \ > list_for_each_entry(__governor, &cpufreq_governor_list, governor_list) > > +static char cpufreq_param_governor[CPUFREQ_NAME_LEN]; > +static char default_governor[CPUFREQ_NAME_LEN]; > + > /** > * The "cpufreq driver" - the arch- or hardware-dependent low > * level driver of CPUFreq support, and its spinlock. This lock > @@ -1061,7 +1064,6 @@ __weak struct cpufreq_governor *cpufreq_default_governor(void) > > static int cpufreq_init_policy(struct cpufreq_policy *policy) > { > - struct cpufreq_governor *def_gov = cpufreq_default_governor(); > struct cpufreq_governor *gov = NULL; > unsigned int pol = CPUFREQ_POLICY_UNKNOWN; > bool put_governor = false; > @@ -1071,22 +1073,29 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy) > /* Update policy governor to the one used before hotplug. */ > gov = get_governor(policy->last_governor); > if (gov) { > - put_governor = true; > pr_debug("Restoring governor %s for cpu %d\n", > - policy->governor->name, policy->cpu); > - } else if (def_gov) { > - gov = def_gov; > + gov->name, policy->cpu); > } else { > - return -ENODATA; > + gov = get_governor(default_governor); > + } > + > + if (gov) { > + put_governor = true; > + } else { > + gov = cpufreq_default_governor(); > + if (!gov) > + return -ENODATA; > } As mentioned on patch 01, doing put_module() below if gov != NULL would avoid this dance with put_governor, but this works too, so no strong opinion. > + > } else { > + > /* Use the default policy if there is no last_policy. */ > if (policy->last_policy) { > pol = policy->last_policy; > - } else if (def_gov) { > - pol = cpufreq_parse_policy(def_gov->name); > + } else { > + pol = cpufreq_parse_policy(default_governor); > /* > - * In case the default governor is neiter "performance" > + * In case the default governor is neither "performance" > * nor "powersave", fall back to the initial policy > * value set by the driver. > */ > @@ -2796,13 +2805,22 @@ EXPORT_SYMBOL_GPL(cpufreq_unregister_driver); > > static int __init cpufreq_core_init(void) > { > + struct cpufreq_governor *gov = cpufreq_default_governor(); > + char *name = gov->name; > + > if (cpufreq_disabled()) > return -ENODEV; > > cpufreq_global_kobject = kobject_create_and_add("cpufreq", &cpu_subsys.dev_root->kobj); > BUG_ON(!cpufreq_global_kobject); > > + if (strlen(cpufreq_param_governor)) > + name = cpufreq_param_governor; > + > + strncpy(default_governor, name, CPUFREQ_NAME_LEN); Do we need both cpufreq_param_governor and default_governor? Could we move everything to only one of them? Something a little bit like that maybe? ---8<--- diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 93c6399c1a42..a0e90b567e1e 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -50,7 +50,6 @@ static LIST_HEAD(cpufreq_governor_list); #define for_each_governor(__governor) \ list_for_each_entry(__governor, &cpufreq_governor_list, governor_list) -static char cpufreq_param_governor[CPUFREQ_NAME_LEN]; static char default_governor[CPUFREQ_NAME_LEN]; /** @@ -2814,13 +2813,11 @@ static int __init cpufreq_core_init(void) cpufreq_global_kobject = kobject_create_and_add("cpufreq", &cpu_subsys.dev_root->kobj); BUG_ON(!cpufreq_global_kobject); - if (strlen(cpufreq_param_governor)) - name = cpufreq_param_governor; - - strncpy(default_governor, name, CPUFREQ_NAME_LEN); + if (!strlen(default_governor)) + strncpy(default_governor, name, CPUFREQ_NAME_LEN); return 0; } module_param(off, int, 0444); -module_param_string(default_governor, cpufreq_param_governor, CPUFREQ_NAME_LEN, 0444); +module_param_string(default_governor, default_governor, CPUFREQ_NAME_LEN, 0444); --->8--- Also, one thing to keep in mind with this version (or the one you suggested) is that if the command line parameter is not valid, we will not fallback on the builtin default for the ->setpolicy() case. But I suppose one might argue this is a reasonable behaviour, so no objection from me. Otherwise, apart from these nits, I gave this a go on my setup, with builtin and modular governors & drivers, and everything worked exactly as expected. Thanks Viresh for the help! Quentin