linux-kernel - Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL switcher is active

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 13 Oct 2015 08:19:04 +0100
From:	"Jon Medhurst (Tixy)" <tixy@...aro.org>
To:	Sudeep Holla <sudeep.holla@....com>
Cc:	Viresh Kumar <viresh.kumar@...aro.org>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>, linux-pm@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] cpufreq: arm_big_little: fix frequency check when bL
 switcher is active

On Mon, 2015-10-12 at 14:20 +0100, Sudeep Holla wrote:
> 
> On 08/10/15 10:23, Jon Medhurst (Tixy) wrote:
> [...]
> 
> > diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c
> > index f1e42f8..59115a4 100644
> > --- a/drivers/cpufreq/arm_big_little.c
> > +++ b/drivers/cpufreq/arm_big_little.c
> > @@ -149,6 +149,18 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
> >   			__func__, cpu, old_cluster, new_cluster, new_rate);
> >
> >   	ret = clk_set_rate(clk[new_cluster], new_rate * 1000);
> > +	if (!ret) {
> > +		/*
> > +		 * FIXME: clk_set_rate has to handle the case where clk_change_rate
> > +		 * can fail due to hardware or firmware issues. Until the clk core
> > +		 * layer is fixed, we can check here. In most of the cases we will
> > +		 * be reading only the cached value anyway. This needs to  be removed
> > +		 * once clk core is fixed.
> > +		 */
> > +		if (clk_get_rate(clk[new_cluster]) != new_rate * 1000)
> > +			ret = -EIO;
> > +	}
> > +
> >   	if (WARN_ON(ret)) {
> >   		pr_err("clk_set_rate failed: %d, new cluster: %d\n", ret,
> >   				new_cluster);
> > @@ -189,15 +201,6 @@ bL_cpufreq_set_rate(u32 cpu, u32 old_cluster, u32 new_cluster, u32 rate)
> >   		mutex_unlock(&cluster_lock[old_cluster]);
> >   	}
> >
> > -	/*
> > -	 * FIXME: clk_set_rate has to handle the case where clk_change_rate
> > -	 * can fail due to hardware or firmware issues. Until the clk core
> > -	 * layer is fixed, we can check here. In most of the cases we will
> > -	 * be reading only the cached value anyway. This needs to  be removed
> > -	 * once clk core is fixed.
> > -	 */
> > -	if (bL_cpufreq_get_rate(cpu) != new_rate)
> > -		return -EIO;
> >   	return 0;
> >   }
> >
> >
> >
> >
> 
> The above change looks good to me but with minor nit. You can get rid of
> if(!ret) check if you move the hunk after if (WARN_ON(ret))

But then we wouldn't get the WARN_ON and pr_err triggered when we detect
the clock rate isn't set, which surely is half the reason for the check
in the first place?

(P.S. I'm on holiday this week without access to my main computer, dev
boards or decent internet connectivity).

-- 
Tixy

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