From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752718AbcGUXgy (ORCPT ); Thu, 21 Jul 2016 19:36:54 -0400 Received: from mail-pf0-f169.google.com ([209.85.192.169]:34736 "EHLO mail-pf0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752259AbcGUXgw (ORCPT ); Thu, 21 Jul 2016 19:36:52 -0400 From: Steve Muckle X-Google-Original-From: Steve Muckle Date: Thu, 21 Jul 2016 16:36:48 -0700 To: Viresh Kumar Cc: Steve Muckle , "Rafael J. Wysocki" , Peter Zijlstra , Ingo Molnar , linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, Vincent Guittot , Morten Rasmussen , Dietmar Eggemann , Juri Lelli , Patrick Bellasi Subject: Re: [PATCH v3 1/3] cpufreq: add cpufreq_driver_resolve_freq() Message-ID: <20160721233648.GV27987@graphite.smuckle.net> References: <1468441527-23534-1-git-send-email-smuckle@linaro.org> <1468441527-23534-2-git-send-email-smuckle@linaro.org> <20160721195926.GF3122@ubuntu> <2012245.HQXNKhffmu@vostro.rjw.lan> <20160721203041.GH3122@ubuntu> <20160721232131.GS27987@graphite.smuckle.net> <20160721233003.GM3122@ubuntu> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160721233003.GM3122@ubuntu> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jul 21, 2016 at 04:30:03PM -0700, Viresh Kumar wrote: > On 21-07-16, 16:21, Steve Muckle wrote: > > On Thu, Jul 21, 2016 at 01:30:41PM -0700, Viresh Kumar wrote: > > > Okay, but in that case shouldn't we do something like this: > > > > > > unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, > > > unsigned int target_freq) > > > { > > > target_freq = clamp_val(target_freq, policy->min, policy->max); > > > policy->cached_target_freq = target_freq; > > > > > > if (cpufreq_driver->target_index) { > > > policy->cached_resolved_idx = > > > cpufreq_frequency_table_target(policy, target_freq, > > > CPUFREQ_RELATION_L); > > > return policy->freq_table[policy->cached_resolved_idx].frequency; > > > } > > > > > > if (cpufreq_driver->resolve_freq) > > > return cpufreq_driver->resolve_freq(policy, target_freq); > > > } > > > > Thanks for the review. > > > > My thinking (noted in the commit text) was that the caller of > > cpufreq_driver_resolve_freq() would verify that the driver supported the > > proper calls before using this API. > > Okay, but the caller isn't doing that today. Right? There is no caller yet. > > This way it can be checked once, > > presumably in a governor's init routine. Checking the pointer over and > > over again in a fast path is wasteful. > > But we just can not assume the callers to always check that the driver > has a ->target() and no ->resolve_freq(), and in that case not to call > this routine. We would be forced to add a WARN_ON() in that case here > to make sure we aren't trying to access a NULL ->resolve_freq. Why not? Can we not catch that in code review? If somehow this slips past and someone tries to use a driver with schedutil that doesn't provide either target_index or resolve_freq, it's not like it'll be a rare crash. It'll die immediately and in a very obvious way. > Over that, it will be used for a very small number of drivers which > still use the ->target() callback and anyway we are going to do a > function call for them. We can add a likely() here if that helps, but > some sort of checking is surely required IMO. > > And, this is a core API, which can be used for other governor's > tomorrow :) As another alternative, this could be caught in cpufreq driver initialization? I believe you suggested that originally, but I avoided it as I didn't want to have to implement resolve_freq() for every target() style driver. It sounds like there aren't many though.