From patchwork Mon Sep 18 17:39:59 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bo Yan X-Patchwork-Id: 9957235 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 0B532601E9 for ; Mon, 18 Sep 2017 17:43:55 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id EE71E285DC for ; Mon, 18 Sep 2017 17:43:54 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id E351E28D87; Mon, 18 Sep 2017 17:43:54 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A0ACC285DC for ; Mon, 18 Sep 2017 17:43:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754991AbdIRRnl (ORCPT ); Mon, 18 Sep 2017 13:43:41 -0400 Received: from hqemgate15.nvidia.com ([216.228.121.64]:1207 "EHLO hqemgate15.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752279AbdIRRnk (ORCPT ); Mon, 18 Sep 2017 13:43:40 -0400 Received: from hqpgpgate101.nvidia.com (Not Verified[216.228.121.13]) by hqemgate15.nvidia.com id ; Mon, 18 Sep 2017 10:43:13 -0700 Received: from HQMAIL108.nvidia.com ([172.20.161.6]) by hqpgpgate101.nvidia.com (PGP Universal service); Mon, 18 Sep 2017 10:43:14 -0700 X-PGP-Universal: processed; by hqpgpgate101.nvidia.com on Mon, 18 Sep 2017 10:43:14 -0700 Received: from [172.17.185.27] (172.17.185.27) by HQMAIL108.nvidia.com (172.18.146.13) with Microsoft SMTP Server (TLS) id 15.0.1293.2; Mon, 18 Sep 2017 17:39:24 +0000 Subject: Re: [PATCH] cpufreq: cpufreq_stats: make last_index signed int To: Viresh Kumar CC: , , References: <1505506402-11497-1-git-send-email-byan@nvidia.com> <20170918015017.GC17030@ubuntu> X-Nvconfidentiality: public From: Bo Yan Message-ID: <961bce0a-5162-5e8d-3474-5b2161cf92c4@nvidia.com> Date: Mon, 18 Sep 2017 10:39:59 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20170918015017.GC17030@ubuntu> X-Originating-IP: [172.17.185.27] X-ClientProxiedBy: HQMAIL103.nvidia.com (172.20.187.11) To HQMAIL108.nvidia.com (172.18.146.13) Content-Language: en-US Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 09/17/2017 06:50 PM, Viresh Kumar wrote: > On 15-09-17, 13:13, Bo Yan wrote: >> It is possible for last_index to get a -1 if current frequency >> is not found in the freq table when stats is created. If the >> function "cpufreq_stats_update" is called before last_index is >> updated with a valid value, the "-1" will be used as index to >> update stats->time_in_state, triggering an exception. > > No, that's not how it works AFAIK and if it did, then can you explain how your > solution fixes it? > > AFAIK, what happens right now is that stats->last_index eventually stores > 2147483647 (uint max) and the exception comes while accessing that value. > > While with your change, it will become -1 and accessing array[-1] is fine by C > standards, though it is still the wrong thing to do as you are accessing > something outside of the array. > > We should just check last_index == -1 before calling cpufreq_stats_update(), > which is already done by one of the callers. > Currently, the "last_index" is being checked before cpufreq_stats_update(stats) inside function "cpufreq_stats_record_transition", so it's taken care of. However, the function "show_time_in_state" also calls cpufreq_stats_update, the similar check should be done there too, like this: stats->freq_table[i], (unsigned long long) This is only needed when policy->cur is not in frequency table when stats table is created, in which case, stats->last_index will get -1, then user does a "cat time_in_state" before any frequency transition. Does this make sense? diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c index e75880eb037d..15305b5ec322 100644 --- a/drivers/cpufreq/cpufreq_stats.c +++ b/drivers/cpufreq/cpufreq_stats.c @@ -62,7 +62,8 @@ static ssize_t show_time_in_state(struct cpufreq_policy *policy, char *buf) if (policy->fast_switch_enabled) return 0; - cpufreq_stats_update(stats); + if ((int)stats->last_index >= 0) + cpufreq_stats_update(stats); for (i = 0; i < stats->state_num; i++) { len += sprintf(buf + len, "%u %llu\n",