linux-kernel - Re: [PATCH v2 2/2] cpufreq: powernv: Register for OCC related opal

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] [day] [month] [year] [list]

Date:	Tue, 28 Apr 2015 14:37:37 +0530
From:	Shilpasri G Bhat <shilpa.bhat@...ux.vnet.ibm.com>
To:	Viresh Kumar <viresh.kumar@...aro.org>
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 04/28/2015 02:23 PM, Viresh Kumar wrote:
> On 28 April 2015 at 13:48, Shilpasri G Bhat
> <shilpa.bhat@...ux.vnet.ibm.com> wrote:
>> My bad I haven't added explicit comment to state reason behind this change.
>>
>> I modified the definition of *throttle_check() to match the function definition
>> to be called via smp_call() instead of adding an additional wrapper around
>> *throttle_check().
>>
>> OCC is a chip entity and any local throttle state changes should be associated
>> to cpus belonging to that chip. The *throttle_check() will read the core
>> register PMSR to verify throttling. All the cores in a chip will have the same
>> throttled state as they are managed by a the same OCC in that chip.
>>
>> smp_call() is required to ensure *throttle_check() is called on a cpu belonging
>> to the chip for which we have received throttled/unthrottled notification. We
>> could be handling throttled/unthrottled notification of 'chip1' in 'chip2' so do
>> an smp_call() on 'chip1'.
> 
> Okay. Lets talk about the code that is already present in mainline. Isn't that
> suffering from this issue ? If yes, then you need to bugfix that separately.

Nope. The upstream code does not have this issue as it does not have checks to
detect unthrottling state. The unthrottling i.e, 'throttled=false' is being
handled only in this patchset.

Yes this can be fixed separately.

> 
>> We are irq_disabled in powernv_cpufreq_occ_msg() the notification handler.
>> Thus the use of kworker to do an smp_call and restore policy->cur.
>>
>> OCC_RESET is global event it affects frequency of all chips. Pmax capping is
>> local event, it affects the frequency of a chip.
>>
> 
>>> 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.
>>
>> All the changes introduced in this patch is centered around opal_message
>> notification handler powernv_cpufreq_occ_msg(). I can split it into multiple
>> patches but it all will be relevant only to solve the above problem.
> 
> And that's what I meant here. Yes, this all is solving a central problem, but
> a patch must be divided into separate, independently working, entities.
> 

Yup agree. Will do.

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