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=-4.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,UNWANTED_LANGUAGE_BODY autolearn=ham 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 131D9C5B57D for ; Wed, 3 Jul 2019 00:30:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C827F21993 for ; Wed, 3 Jul 2019 00:30:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1562113819; bh=/Z1D72tf0p5zCrv6Ah3wQNqTBcXh0FdTKkJDcF8jv8k=; h=References:In-Reply-To:From:Date:Subject:To:Cc:List-ID:From; b=dQhW9KRnaMfdcWlcj/QfppNy969mi1N7atgSurcGkpNDqd3KJHrm7Pkjgg0dCEQpQ Wlg3dz2yUGMvOfGpvXX3nodIxoMaq08uyRs68FVLHcJjaF1ptjwGwBlfnjWR2OmxBm e7iFRv0Oc9iSqzt0Pd1uhU0xIFoI8pG+ZKe5OeiA= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727337AbfGCAaT (ORCPT ); Tue, 2 Jul 2019 20:30:19 -0400 Received: from mail-ot1-f66.google.com ([209.85.210.66]:36825 "EHLO mail-ot1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727210AbfGCAaT (ORCPT ); Tue, 2 Jul 2019 20:30:19 -0400 Received: by mail-ot1-f66.google.com with SMTP id r6so482027oti.3 for ; Tue, 02 Jul 2019 17:30:18 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=HSmdlx0BcLkwVvJMMcbiNqSwmfTBqTBKyoQRyhSM4Mk=; b=ZdCcRR9p8bDWsjMxFXXm6S4558zaXMfy+8D3arULI0lDP2PxqCYjzJIxE4Sx1aQkEb l6EoI8/UhYVVTI/u2zJ0zrCtXkr9ul+anIcIZ7ocBYmI7GQXYLOcPLt9CQE8duqfnpWg XSdHOVs1SjeAU5Ik7kq/iEDiiZParEqzuRnS2p6xCS3s5qaiMkUYmOllnkcaotIWBqBX ICruuD8tLM7UwPesYwye2iOPprTVcSmc7D1QVcrbDi1SHCkuJbBL4PjePDOhE7puohH+ yCxKwi26DGln4k5kYWfAlybW/1GbtiiJrpjRqQCx2TQmBdbn+yWcD7jS2KIkLEHgsUHw QRjg== X-Gm-Message-State: APjAAAX67TMeuGF+FJkwtIcH6oxyrQ0e3Oq5ddmvZkLk+FUpiI78XgXT vWjeklXhGEX2rt5v8Wgz8QH4OsJ/qHrn4ETHl9UmOQ== X-Google-Smtp-Source: APXvYqz0CuFqzsYfwwipw8sThz3e17ZBEIFy/oxQ9CiOZR67gLLd893aarpAZSHOPovPtRSMUcMo/8KhliEKTHJ2Eu8= X-Received: by 2002:a05:6830:1516:: with SMTP id k22mr6057621otp.189.1562103855745; Tue, 02 Jul 2019 14:44:15 -0700 (PDT) MIME-Version: 1.0 References: <1561701029-3415-1-git-send-email-rui.zhang@intel.com> <1561701029-3415-5-git-send-email-rui.zhang@intel.com> In-Reply-To: <1561701029-3415-5-git-send-email-rui.zhang@intel.com> From: "Rafael J. Wysocki" Date: Tue, 2 Jul 2019 23:44:04 +0200 Message-ID: Subject: Re: [PATCH 04/13] intel_rapl: introduce struct rapl_private To: Zhang Rui Cc: "Rafael J. Wysocki" , Linux PM , "Pandruvada, Srinivas" Content-Type: text/plain; charset="UTF-8" Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org On Fri, Jun 28, 2019 at 7:50 AM Zhang Rui wrote: > > Introduce a new structure, rapl_private, to save the private data > for different RAPL Interface. The new structure is called rapl_priv in the patch. Besides, I would call it rapl_if_priv to indicate that this related to an interface. Also, IMO there should be a kerneldoc comment for it. > Signed-off-by: Zhang Rui > --- > drivers/powercap/intel_rapl.c | 58 +++++++++++++++++++++---------------------- > include/linux/intel_rapl.h | 7 ++++++ > 2 files changed, 35 insertions(+), 30 deletions(-) > > diff --git a/drivers/powercap/intel_rapl.c b/drivers/powercap/intel_rapl.c > index adb35ec..a804370 100644 > --- a/drivers/powercap/intel_rapl.c > +++ b/drivers/powercap/intel_rapl.c > @@ -75,6 +75,8 @@ enum unit_type { > TIME_UNIT, > }; > > +static struct rapl_priv rapl_msr_priv; Please add a comment describing this variable and its role. BTW, I'd rather call it rapl_if_msr. > + > /* per domain data, some are optional */ > #define NR_RAW_PRIMITIVES (NR_RAPL_PRIMITIVES - 2) > > @@ -155,17 +157,14 @@ static const char * const rapl_domain_names[] = { > "psys", > }; > > -static struct powercap_control_type *control_type; /* PowerCap Controller */ > -static struct rapl_domain *platform_rapl_domain; /* Platform (PSys) domain */ Why do these things need to go into rapl_priv? > - > /* caller to ensure CPU hotplug lock is held */ > -static struct rapl_package *rapl_find_package_domain(int cpu) > +static struct rapl_package *rapl_find_package_domain(int cpu, struct rapl_priv *priv) > { > int id = topology_logical_die_id(cpu); > struct rapl_package *rp; > > list_for_each_entry(rp, &rapl_packages, plist) { > - if (rp->id == id) > + if (rp->id == id && rp->priv->control_type == priv->control_type) > return rp; > } > > @@ -1090,12 +1089,12 @@ static void rapl_update_domain_data(struct rapl_package *rp) > > static void rapl_unregister_powercap(void) > { > - if (platform_rapl_domain) { > - powercap_unregister_zone(control_type, > - &platform_rapl_domain->power_zone); > - kfree(platform_rapl_domain); > + if (&rapl_msr_priv.platform_rapl_domain) { > + powercap_unregister_zone(rapl_msr_priv.control_type, > + &rapl_msr_priv.platform_rapl_domain->power_zone); > + kfree(rapl_msr_priv.platform_rapl_domain); > } > - powercap_unregister_control_type(control_type); > + powercap_unregister_control_type(rapl_msr_priv.control_type); > } > > static int rapl_package_register_powercap(struct rapl_package *rp) > @@ -1113,7 +1112,7 @@ static int rapl_package_register_powercap(struct rapl_package *rp) > nr_pl = find_nr_power_limit(rd); > pr_debug("register package domain %s\n", rp->name); > power_zone = powercap_register_zone(&rd->power_zone, > - control_type, > + rp->priv->control_type, > rp->name, NULL, > &zone_ops[rd->id], > nr_pl, > @@ -1140,7 +1139,7 @@ static int rapl_package_register_powercap(struct rapl_package *rp) > /* number of power limits per domain varies */ > nr_pl = find_nr_power_limit(rd); > power_zone = powercap_register_zone(&rd->power_zone, > - control_type, rd->name, > + rp->priv->control_type, rd->name, > rp->power_zone, > &zone_ops[rd->id], nr_pl, > &constraint_ops); > @@ -1161,7 +1160,7 @@ static int rapl_package_register_powercap(struct rapl_package *rp) > */ > while (--rd >= rp->domains) { > pr_debug("unregister %s domain %s\n", rp->name, rd->name); > - powercap_unregister_zone(control_type, &rd->power_zone); > + powercap_unregister_zone(rp->priv->control_type, &rd->power_zone); > } > > return ret; > @@ -1191,9 +1190,9 @@ static int __init rapl_register_psys(void) > rd->rpl[0].name = pl1_name; > rd->rpl[1].prim_id = PL2_ENABLE; > rd->rpl[1].name = pl2_name; > - rd->rp = rapl_find_package_domain(0); > + rd->rp = rapl_find_package_domain(0, &rapl_msr_priv); > > - power_zone = powercap_register_zone(&rd->power_zone, control_type, > + power_zone = powercap_register_zone(&rd->power_zone, rapl_msr_priv.control_type, > "psys", NULL, > &zone_ops[RAPL_DOMAIN_PLATFORM], > 2, &constraint_ops); > @@ -1203,17 +1202,17 @@ static int __init rapl_register_psys(void) > return PTR_ERR(power_zone); > } > > - platform_rapl_domain = rd; > + rapl_msr_priv.platform_rapl_domain = rd; > > return 0; > } > > static int __init rapl_register_powercap(void) > { > - control_type = powercap_register_control_type(NULL, "intel-rapl", NULL); > - if (IS_ERR(control_type)) { > + rapl_msr_priv.control_type = powercap_register_control_type(NULL, "intel-rapl", NULL); > + if (IS_ERR(rapl_msr_priv.control_type)) { > pr_debug("failed to register powercap control_type.\n"); > - return PTR_ERR(control_type); > + return PTR_ERR(rapl_msr_priv.control_type); > } > return 0; > } > @@ -1338,16 +1337,16 @@ static void rapl_remove_package(struct rapl_package *rp) > } > pr_debug("remove package, undo power limit on %s: %s\n", > rp->name, rd->name); > - powercap_unregister_zone(control_type, &rd->power_zone); > + powercap_unregister_zone(rp->priv->control_type, &rd->power_zone); > } > /* do parent zone last */ > - powercap_unregister_zone(control_type, &rd_package->power_zone); > + powercap_unregister_zone(rp->priv->control_type, &rd_package->power_zone); > list_del(&rp->plist); > kfree(rp); > } > > /* called from CPU hotplug notifier, hotplug lock held */ > -static struct rapl_package *rapl_add_package(int cpu) > +static struct rapl_package *rapl_add_package(int cpu, struct rapl_priv *priv) > { > int id = topology_logical_die_id(cpu); > struct rapl_package *rp; > @@ -1361,6 +1360,7 @@ static struct rapl_package *rapl_add_package(int cpu) > /* add the new package to the list */ > rp->id = id; > rp->lead_cpu = cpu; > + rp->priv = priv; > > if (topology_max_die_per_package() > 1) > snprintf(rp->name, PACKAGE_DOMAIN_NAME_LENGTH, > @@ -1399,9 +1399,9 @@ static int rapl_cpu_online(unsigned int cpu) > { > struct rapl_package *rp; > > - rp = rapl_find_package_domain(cpu); > + rp = rapl_find_package_domain(cpu, &rapl_msr_priv); > if (!rp) { > - rp = rapl_add_package(cpu); > + rp = rapl_add_package(cpu, &rapl_msr_priv); > if (IS_ERR(rp)) > return PTR_ERR(rp); > } > @@ -1414,7 +1414,7 @@ static int rapl_cpu_down_prep(unsigned int cpu) > struct rapl_package *rp; > int lead_cpu; > > - rp = rapl_find_package_domain(cpu); > + rp = rapl_find_package_domain(cpu, &rapl_msr_priv); > if (!rp) > return 0; > > @@ -1427,8 +1427,6 @@ static int rapl_cpu_down_prep(unsigned int cpu) > return 0; > } > > -static enum cpuhp_state pcap_rapl_online; > - > static void power_limit_state_save(void) > { > struct rapl_package *rp; > @@ -1538,7 +1536,7 @@ static int __init rapl_init(void) > rapl_cpu_online, rapl_cpu_down_prep); > if (ret < 0) > goto err_unreg; > - pcap_rapl_online = ret; > + rapl_msr_priv.pcap_rapl_online = ret; > > /* Don't bail out if PSys is not supported */ > rapl_register_psys(); > @@ -1550,7 +1548,7 @@ static int __init rapl_init(void) > return 0; > > err_unreg_all: > - cpuhp_remove_state(pcap_rapl_online); > + cpuhp_remove_state(rapl_msr_priv.pcap_rapl_online); > > err_unreg: > rapl_unregister_powercap(); > @@ -1560,7 +1558,7 @@ static int __init rapl_init(void) > static void __exit rapl_exit(void) > { > unregister_pm_notifier(&rapl_pm_notifier); > - cpuhp_remove_state(pcap_rapl_online); > + cpuhp_remove_state(rapl_msr_priv.pcap_rapl_online); > rapl_unregister_powercap(); > } > > diff --git a/include/linux/intel_rapl.h b/include/linux/intel_rapl.h > index 9471603..d6a8547 100644 > --- a/include/linux/intel_rapl.h > +++ b/include/linux/intel_rapl.h > @@ -88,6 +88,12 @@ struct rapl_domain { > struct rapl_package *rp; > }; > > +struct rapl_priv { > + struct powercap_control_type *control_type; > + struct rapl_domain *platform_rapl_domain; > + enum cpuhp_state pcap_rapl_online; > +}; > + > /* maximum rapl package domain name: package-%d-die-%d */ > #define PACKAGE_DOMAIN_NAME_LENGTH 30 > > @@ -108,6 +114,7 @@ struct rapl_package { > /* Track active cpus */ > struct cpumask cpumask; > char name[PACKAGE_DOMAIN_NAME_LENGTH]; > + struct rapl_priv *priv; > }; > > #endif /* __INTEL_RAPL_H__ */ > -- > 2.7.4 >