linux-kernel - Re: [PATCH] power: introduce library for device-specific OPPs

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: <87fwwh7isu.fsf@deeprootsystems.com>
Date:	Thu, 07 Oct 2010 15:13:37 -0700
From:	Kevin Hilman <khilman@...prootsystems.com>
To:	"Menon\, Nishanth" <nm@...com>
Cc:	"Rafael J. Wysocki" <rjw@...k.pl>,
	linux-pm <linux-pm@...ts.linux-foundation.org>,
	lkml <linux-kernel@...r.kernel.org>,
	l-o <linux-omap@...r.kernel.org>,
	l-a <linux-arm-kernel@...ts.infradead.org>,
	Paul <paulmck@...ux.vnet.ibm.com>
Subject: Re: [PATCH] power: introduce library for device-specific OPPs

"Menon, Nishanth" <nm@...com> writes:

>> -----Original Message-----
>> From: Rafael J. Wysocki [mailto:rjw@...k.pl]
>> Sent: Thursday, October 07, 2010 4:55 PM
>
>
>> 
>> Hi,
>> 
>> On Wednesday, October 06, 2010, Nishanth Menon wrote:
>> > SoCs have a standard set of tuples consisting of frequency and
>> > voltage pairs that the device will support per voltage domain. These
>> > are called Operating Performance Points or OPPs. The actual
>> > definitions of OPP varies over silicon versions. For a specific domain,
>> > we can have a set of {frequency, voltage} pairs. As the kernel boots
>> > and more information is available, a default set of these are activated
>> > based on the precise nature of device. Further on operation, based on
>> > conditions prevailing in the system (such as temperature), some OPP
>> > availability may be temporarily controlled by the SoC frameworks.
>> >
>> > To implement an OPP, some sort of power management support is necessary
>> > hence this library depends on CONFIG_PM.
>> 
>> The patch generally looks good to me, I only have a couple of cosmetic
> Thanks for the great reviews.. It did bump up the resultant patch.
>
>> remarks
>> (below).
>> 
>> ...
>> > +static int opp_set_availability(struct device *dev, unsigned long freq,
>> > +		bool availability_req)
>> > +{
>> > +	struct device_opp *tmp_dev_opp, *dev_opp = NULL;
>> > +	struct opp *new_opp, *tmp_opp, *opp = ERR_PTR(-ENODEV);
>> > +	int r = 0;
>> > +
>> > +	/* keep the node allocated */
>> > +	new_opp = kmalloc(sizeof(struct opp), GFP_KERNEL);
>> > +	if (!new_opp) {
>> > +		pr_warning("Unable to allocate opp\n");
>> 
>> Please add an identification string to the messages, something like
>> "OPP: Unable to allocat object\n" (and similarly in the other messages).
>> That would help to find the source of a message in case there's any
>> problem.
>
> pr_fmt has been reformatted for this. The actual message which will appear
> is as follows:
> opp_set_availability: Unable to allocate opp
>
> is'nt that good enough considering that all functions are opp_ prefixed? 
> I can modify pr_fmt to add "OPP:" but I kinda think it is redundant. But I
> have no strong opinions on that and look forward to your recommendations.

Even more informative would be to use dev_warn() and include the func.
That way we'll even know which device has the problem.

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