linux-kernel - Re: [PATCH v2 9/10] cpufreq: governor: Rearrange governor data structures

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]

Date:	Fri, 05 Feb 2016 23:47:55 +0100
From:	"Rafael J. Wysocki" <rjw@...ysocki.net>
To:	Viresh Kumar <viresh.kumar@...aro.org>
Cc:	Linux PM list <linux-pm@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
	Juri Lelli <juri.lelli@....com>,
	Steve Muckle <steve.muckle@...aro.org>,
	Saravana Kannan <skannan@...eaurora.org>
Subject: Re: [PATCH v2 9/10] cpufreq: governor: Rearrange governor data structures

On Friday, February 05, 2016 02:43:57 PM Viresh Kumar wrote:
> On 05-02-16, 03:21, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > 
> > The struct policy_dbs_info objects representing per-policy governor
> > data are not accessible directly from the corresponding policy
> > objects.  To access them, one has to get a pointer to the
> > struct cpu_dbs_info of policy->cpu and use the "shared" field of
> > that which isn't really straightforward.
> > 
> > To address that rearrange the governor data structures so the
> > governor_data pointer in struct cpufreq_policy will point to
> > struct policy_dbs_info and that will contain a pointer to
> > struct dbs_data.
> 
> IMHO, this patch has done way too much over what's mentioned here.
> 
> > Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
> > @@ -297,16 +298,22 @@ static int alloc_policy_dbs_info(struct
> >  	/* Allocate memory for the common information for policy->cpus */
> >  	policy_dbs = kzalloc(sizeof(*policy_dbs), GFP_KERNEL);
> >  	if (!policy_dbs)
> > -		return -ENOMEM;
> > -
> > -	/* Set policy_dbs for all CPUs, online+offline */
> > -	for_each_cpu(j, policy->related_cpus)
> > -		gov->get_cpu_cdbs(j)->shared = policy_dbs;
> > +		return NULL;
> >  
> > +	policy_dbs->policy = policy;
> 
> Value of policy_dbs->policy was used to verify the state machine of
> the governor and so was updated only in start/stop.
> 
> You have moved it to INIT first (which shouldn't have been part of
> this patch at the least),

Why?

> and then there is no reasoning given on why
> that isn't required as part of the state machine now, which I believe
> is still required the way it was.

No, it isn't required.  The whole "state machine" isn't required IMO.

The only user of this is the cpufreq core, so why does the code here have to
double check what the core is doing?

Thanks,
Rafael