| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAKohpomaOvpdOYqAsOyPn0nRG0Mb68JscXhgBPDa6grqRAE8zA@mail.gmail.com>
Date: Tue, 28 Apr 2015 12:18:54 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Shilpasri G Bhat <shilpa.bhat@...ux.vnet.ibm.com>
Cc: "linuxppc-dev@...abs.org" <linuxppc-dev@...abs.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
"Rafael J. Wysocki" <rjw@...ysocki.net>,
Preeti U Murthy <preeti@...ux.vnet.ibm.com>,
"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] cpufreq: powernv: Register for OCC related
opal_message notification
On 28 April 2015 at 11:53, Shilpasri G Bhat
<shilpa.bhat@...ux.vnet.ibm.com> wrote:
> Changes from v1:
> - Add macros to define OCC_RESET, OCC_LOAD and OCC_THROTTLE
> - Define a structure to store chip id, chip mask which has bits set
> for cpus present in the chip, throttled state and a work_struct.
> - Modify powernv_cpufreq_throttle_check() to be called via smp_call()
Why ? I might have missed it but there should be some reasoning behind
what you are changing.
> - On Pmax throttling/unthrottling update 'chip.throttled' and not the
> global 'throttled' as Pmax capping is local to the chip.
> - Remove the condition which checks if local pstate is less than Pmin
> while checking for Psafe frequency. When OCC becomes active after
> reset we update 'thottled' to false and when the cpufreq governor
> initiates a pstate change, the local pstate will be in Psafe and we
> will be reporting a false positive when we are not throttled.
> - Schedule a kworker on receiving throttling/unthrottling OCC message
> for that chip and schedule on all chips after receiving active.
> - After an OCC reset all the cpus will be in Psafe frequency. So call
> target() and restore the frequency to policy->cur after OCC_ACTIVE
> and Pmax unthrottling
> - Taken care of Viresh and Preeti's comments.
That's a lot. I am not an expert here and so really can't comment on
the internals of ppc. But, is it patch solving a single problem ? I don't
know, I somehow got the impression that it can be split into multiple
(smaller & review-able) patches. Only if it makes sense. Your call.
> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> +void powernv_cpufreq_work_fn(struct work_struct *work)
> +{
> + struct chip *c = container_of(work, struct chip, throttle);
> + unsigned int cpu;
> +
> + smp_call_function_any(&c->mask,
> + powernv_cpufreq_throttle_check, NULL, 0);
> +
> + for_each_cpu(cpu, &c->mask) {
for_each_online_cpu ?
> + int index;
> + struct cpufreq_frequency_table *freq_table;
> + struct cpufreq_policy cpu_policy;
Name it policy.
> +
> + if (!cpu_online(cpu))
> + continue;
And you can kill this..
> + cpufreq_get_policy(&cpu_policy, cpu);
> + freq_table = cpufreq_frequency_get_table(cpu_policy.cpu);
Just do, policy->freq_table.
> +static int powernv_cpufreq_occ_msg(struct notifier_block *nb,
> + unsigned long msg_type, void *msg)
> +{
> + if (reason && reason <= 5)
> + pr_info("OCC: Chip %d Pmax reduced due to %s\n",
> + (int)chip_id, throttle_reason[reason]);
> + else
> + pr_info("OCC: Chip %d %s\n", (int)chip_id,
> + throttle_reason[reason]);
Blank line here. They are better for readability after blocks and loops.
> + for (i = 0; i < nr_chips; i++)
> + if (chips[i].id == (int)chip_id)
Why isn't .id 64 bit ?
> + schedule_work(&chips[i].throttle);
> + }
> + return 0;
> +}
> +
> +static struct notifier_block powernv_cpufreq_opal_nb = {
> + .notifier_call = powernv_cpufreq_occ_msg,
> + .next = NULL,
> + .priority = 0,
> +};
> +
> static void powernv_cpufreq_stop_cpu(struct cpufreq_policy *policy)
> {
> struct powernv_smp_call_data freq_data;
> @@ -414,6 +530,35 @@ static struct cpufreq_driver powernv_cpufreq_driver = {
> .attr = powernv_cpu_freq_attr,
> };
>
> +static int init_chip_info(void)
> +{
> + int chip[256], i = 0, cpu;
> + int prev_chip_id = INT_MAX;
> +
> + for_each_possible_cpu(cpu) {
> + int c = cpu_to_chip_id(cpu);
Does 'c' refer to id here ? Name it so then.
> +
> + if (prev_chip_id != c) {
> + prev_chip_id = c;
> + chip[nr_chips++] = c;
> + }
> + }
> +
> + chips = kmalloc_array(nr_chips, sizeof(struct chip), GFP_KERNEL);
> +
A blank line isn't preferred much here :). Sorry about these blank lines.
> + if (!chips)
> + return -ENOMEM;
> +
> + for (i = 0; i < nr_chips; i++) {
> + chips[i].id = chip[i];
> + cpumask_copy(&chips[i].mask, cpumask_of_node(chip[i]));
> + chips[i].throttled = false;
> + INIT_WORK(&chips[i].throttle, powernv_cpufreq_work_fn);
> + }
> +
> + return 0;
> +}
> +
> static int __init powernv_cpufreq_init(void)
> {
> int rc = 0;
> @@ -429,7 +574,13 @@ static int __init powernv_cpufreq_init(void)
> return rc;
> }
>
> + /* Populate chip info */
> + rc = init_chip_info();
> + if (rc)
> + return rc;
> +
> register_reboot_notifier(&powernv_cpufreq_reboot_nb);
> + opal_message_notifier_register(OPAL_MSG_OCC, &powernv_cpufreq_opal_nb);
> return cpufreq_register_driver(&powernv_cpufreq_driver);
> }
> module_init(powernv_cpufreq_init);
> --
> 1.9.3
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists