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

[Rafael J. Wysocki]

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