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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <553F1C51.4060901@linux.vnet.ibm.com>
Date:	Tue, 28 Apr 2015 11:06:17 +0530
From:	Shilpasri G Bhat <shilpa.bhat@...ux.vnet.ibm.com>
To:	Viresh Kumar <viresh.kumar@...aro.org>
CC:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"linuxppc-dev@...abs.org" <linuxppc-dev@...abs.org>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>
Subject: Re: [PATCH 2/2] cpufreq: powernv: Register for OCC related opal_message
 notification

Hi Viresh,

On 04/27/2015 10:02 AM, Viresh Kumar wrote:
> On 22 April 2015 at 22:34, Shilpasri G Bhat
> <shilpa.bhat@...ux.vnet.ibm.com> wrote:
>> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> 
>> +static char throttle_reason[6][50] = { "No throttling",
> 
> Don't need to mention 6 here.
> 
> And the max length you need right now is 27, so maybe s/50/30 ?
> 
> Also, start 'No Throttling' in a new line, like below.

Will do.
> 
>> +                                       "Power Cap",
>> +                                       "Processor Over Temperature",
>> +                                       "Power Supply Failure",
>> +                                       "OverCurrent",
> 
> s/OverCurrent/Over Current/ ?

Okay.
> 
>> +                                       "OCC Reset"
>> +                                    };
>> +
>> +static int powernv_cpufreq_occ_msg(struct notifier_block *nb,
>> +               unsigned long msg_type, void *msg)
>> +{
>> +       struct opal_msg *occ_msg = msg;
>> +       uint64_t token;
>> +       uint64_t chip_id, reason;
>> +
>> +       if (msg_type != OPAL_MSG_OCC)
>> +               return 0;
> 
> Blank line here.

Okay
> 
>> +       token = be64_to_cpu(occ_msg->params[0]);
> 
> Here as well..
> 
>> +       switch (token) {
>> +       case 0:
>> +               occ_reset = true;
>> +               /*
>> +                * powernv_cpufreq_throttle_check() is called in
>> +                * target() callback which can detect the throttle state
>> +                * for governors like ondemand.
>> +                * But static governors will not call target() often thus
>> +                * report throttling here.
>> +                */
> 
> Now, do I understand correctly that this notifier will be called as
> soon as we switch throttling state ?
> 
> If yes, then do we still need the throttle_check() routine you added
> earlier ? Maybe not.

We cannot remove throttle_check() routine for the following reasons:
1) To report old firmware bugs which do not restore frequency control to host
after an OCC reset.
2) In BMC based boxes if OCC crashes currently firmware will not send 'reset'
and 'load' messages, in such cases throttle_check() will be sufficient to
monitor a throttled state caused by 'reset'.
3) Throttle reporting in old firmwares which do not have this notification.

> 
>> +               if (!throttled) {
>> +                       throttled = true;
>> +                       pr_crit("CPU Frequency is throttled\n");
>> +               }
>> +               pr_info("OCC in Reset\n");
>> +               break;
>> +       case 1:
>> +               pr_info("OCC is Loaded\n");
>> +               break;
>> +       case 2:
>> +               chip_id = be64_to_cpu(occ_msg->params[1]);
>> +               reason = be64_to_cpu(occ_msg->params[2]);
> 
> Blank line here.

Okay
> 
>> +               if (occ_reset) {
>> +                       occ_reset = false;
>> +                       throttled = false;
>> +                       pr_info("OCC is Active\n");
>> +                       /* Sanity check for static governors */
>> +                       powernv_cpufreq_throttle_check(smp_processor_id());
>> +               } else if (reason) {
>> +                       throttled = true;
>> +                       pr_info("Pmax reduced due to %s on chip %x\n",
>> +                                       throttle_reason[reason], (int)chip_id);
>> +               } else {
>> +                       throttled = false;
>> +                       pr_info("%s on chip %x\n",
>> +                                       throttle_reason[reason], (int)chip_id);
>> +               }
> 
> Run checkpatch with --strict option, and you will see some warnings.

Okay will do.

Thanks and Regards,
Shilpa

--
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

Powered by Openwall GNU/*/Linux Powered by OpenVZ