From: joonas.lahtinen@linux.intel.com
To: Hans de Goede <hdegoede@redhat.com>,
Colin Ian King <colin.king@canonical.com>,
Jani Nikula <jani.nikula@linux.intel.com>,
Mark Gross <mgross@linux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@intel.com>,
Jesse Barnes <jsbarnes@google.com>
Cc: platform-driver-x86@vger.kernel.org,
intel-gfx@lists.freedesktop.org,
"dri-devel@lists.freedesktop.org"
<dri-devel@lists.freedesktop.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: Computation of return value being discarded in get_cpu_power() in drivers/platform/x86/intel_ips.c
Date: Thu, 10 Jun 2021 14:51:11 +0300 [thread overview]
Message-ID: <162332587143.15408.5683489401507477378@jlahtine-mobl.ger.corp.intel.com> (raw)
In-Reply-To: <548dd463-3942-00a1-85c3-232897dea1a3@canonical.com>
+ Jesse
Quoting Colin Ian King (2021-06-09 14:50:07)
> Hi,
>
> I was reviewing some old unassigned variable warnings from static
> analysis by Coverity and found an issue introduced with the following
> commit:
>
> commit aa7ffc01d254c91a36bf854d57a14049c6134c72
> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> Date: Fri May 14 15:41:14 2010 -0700
>
> x86 platform driver: intelligent power sharing driver
>
> The analysis is as follows:
>
> drivers/platform/x86/intel_ips.c
>
> 871 static u32 get_cpu_power(struct ips_driver *ips, u32 *last, int period)
> 872 {
> 873 u32 val;
> 874 u32 ret;
> 875
> 876 /*
> 877 * CEC is in joules/65535. Take difference over time to
> 878 * get watts.
> 879 */
> 880 val = thm_readl(THM_CEC);
> 881
> 882 /* period is in ms and we want mW */
> 883 ret = (((val - *last) * 1000) / period);
>
> Unused value (UNUSED_VALUE)
> assigned_value: Assigning value from ret * 1000U / 65535U to ret here,
> but that stored value is not used.
>
> 884 ret = (ret * 1000) / 65535;
> 885 *last = val;
> 886
> 887 return 0;
> 888 }
>
> I'm really not sure why ret is being calculated on lines 883,884 and not
> being used. Should that be *last = ret on line 885? Looks suspect anyhow.
According to git blame code seems to have been disabled intentionally by the
following commit:
commit 96f3823f537088c13735cfdfbf284436c802352a
Author: Jesse Barnes <jbarnes@virtuousgeek.org>
Date: Tue Oct 5 14:50:59 2010 -0400
[PATCH 2/2] IPS driver: disable CPU turbo
The undocumented interface we're using for reading CPU power seems to be
overreporting power. Until we figure out how to correct it, disable CPU
turbo and power reporting to be safe. This will keep the CPU within default
limits and still allow us to increase GPU frequency as needed.
Maybe wrap the code after thm_readl() in #if 0 in case somebody ends up
wanting to fix it? Or eliminate completely.
In theory the thm_readl() may affect the system behavior so would not
remove that for extra paranoia.
Regards, Joonas
WARNING: multiple messages have this Message-ID (diff)
From: joonas.lahtinen@linux.intel.com
To: Hans de Goede <hdegoede@redhat.com>,
Colin Ian King <colin.king@canonical.com>,
Jani Nikula <jani.nikula@linux.intel.com>,
Mark Gross <mgross@linux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@intel.com>,
Jesse Barnes <jsbarnes@google.com>
Cc: intel-gfx@lists.freedesktop.org,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"dri-devel@lists.freedesktop.org"
<dri-devel@lists.freedesktop.org>,
platform-driver-x86@vger.kernel.org
Subject: Re: Computation of return value being discarded in get_cpu_power() in drivers/platform/x86/intel_ips.c
Date: Thu, 10 Jun 2021 14:51:11 +0300 [thread overview]
Message-ID: <162332587143.15408.5683489401507477378@jlahtine-mobl.ger.corp.intel.com> (raw)
In-Reply-To: <548dd463-3942-00a1-85c3-232897dea1a3@canonical.com>
+ Jesse
Quoting Colin Ian King (2021-06-09 14:50:07)
> Hi,
>
> I was reviewing some old unassigned variable warnings from static
> analysis by Coverity and found an issue introduced with the following
> commit:
>
> commit aa7ffc01d254c91a36bf854d57a14049c6134c72
> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> Date: Fri May 14 15:41:14 2010 -0700
>
> x86 platform driver: intelligent power sharing driver
>
> The analysis is as follows:
>
> drivers/platform/x86/intel_ips.c
>
> 871 static u32 get_cpu_power(struct ips_driver *ips, u32 *last, int period)
> 872 {
> 873 u32 val;
> 874 u32 ret;
> 875
> 876 /*
> 877 * CEC is in joules/65535. Take difference over time to
> 878 * get watts.
> 879 */
> 880 val = thm_readl(THM_CEC);
> 881
> 882 /* period is in ms and we want mW */
> 883 ret = (((val - *last) * 1000) / period);
>
> Unused value (UNUSED_VALUE)
> assigned_value: Assigning value from ret * 1000U / 65535U to ret here,
> but that stored value is not used.
>
> 884 ret = (ret * 1000) / 65535;
> 885 *last = val;
> 886
> 887 return 0;
> 888 }
>
> I'm really not sure why ret is being calculated on lines 883,884 and not
> being used. Should that be *last = ret on line 885? Looks suspect anyhow.
According to git blame code seems to have been disabled intentionally by the
following commit:
commit 96f3823f537088c13735cfdfbf284436c802352a
Author: Jesse Barnes <jbarnes@virtuousgeek.org>
Date: Tue Oct 5 14:50:59 2010 -0400
[PATCH 2/2] IPS driver: disable CPU turbo
The undocumented interface we're using for reading CPU power seems to be
overreporting power. Until we figure out how to correct it, disable CPU
turbo and power reporting to be safe. This will keep the CPU within default
limits and still allow us to increase GPU frequency as needed.
Maybe wrap the code after thm_readl() in #if 0 in case somebody ends up
wanting to fix it? Or eliminate completely.
In theory the thm_readl() may affect the system behavior so would not
remove that for extra paranoia.
Regards, Joonas
WARNING: multiple messages have this Message-ID (diff)
From: joonas.lahtinen@linux.intel.com
To: Hans de Goede <hdegoede@redhat.com>,
Colin Ian King <colin.king@canonical.com>,
Jani Nikula <jani.nikula@linux.intel.com>,
Mark Gross <mgross@linux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@intel.com>,
Jesse Barnes <jsbarnes@google.com>
Cc: intel-gfx@lists.freedesktop.org,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"dri-devel@lists.freedesktop.org"
<dri-devel@lists.freedesktop.org>,
platform-driver-x86@vger.kernel.org
Subject: Re: [Intel-gfx] Computation of return value being discarded in get_cpu_power() in drivers/platform/x86/intel_ips.c
Date: Thu, 10 Jun 2021 14:51:11 +0300 [thread overview]
Message-ID: <162332587143.15408.5683489401507477378@jlahtine-mobl.ger.corp.intel.com> (raw)
In-Reply-To: <548dd463-3942-00a1-85c3-232897dea1a3@canonical.com>
+ Jesse
Quoting Colin Ian King (2021-06-09 14:50:07)
> Hi,
>
> I was reviewing some old unassigned variable warnings from static
> analysis by Coverity and found an issue introduced with the following
> commit:
>
> commit aa7ffc01d254c91a36bf854d57a14049c6134c72
> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> Date: Fri May 14 15:41:14 2010 -0700
>
> x86 platform driver: intelligent power sharing driver
>
> The analysis is as follows:
>
> drivers/platform/x86/intel_ips.c
>
> 871 static u32 get_cpu_power(struct ips_driver *ips, u32 *last, int period)
> 872 {
> 873 u32 val;
> 874 u32 ret;
> 875
> 876 /*
> 877 * CEC is in joules/65535. Take difference over time to
> 878 * get watts.
> 879 */
> 880 val = thm_readl(THM_CEC);
> 881
> 882 /* period is in ms and we want mW */
> 883 ret = (((val - *last) * 1000) / period);
>
> Unused value (UNUSED_VALUE)
> assigned_value: Assigning value from ret * 1000U / 65535U to ret here,
> but that stored value is not used.
>
> 884 ret = (ret * 1000) / 65535;
> 885 *last = val;
> 886
> 887 return 0;
> 888 }
>
> I'm really not sure why ret is being calculated on lines 883,884 and not
> being used. Should that be *last = ret on line 885? Looks suspect anyhow.
According to git blame code seems to have been disabled intentionally by the
following commit:
commit 96f3823f537088c13735cfdfbf284436c802352a
Author: Jesse Barnes <jbarnes@virtuousgeek.org>
Date: Tue Oct 5 14:50:59 2010 -0400
[PATCH 2/2] IPS driver: disable CPU turbo
The undocumented interface we're using for reading CPU power seems to be
overreporting power. Until we figure out how to correct it, disable CPU
turbo and power reporting to be safe. This will keep the CPU within default
limits and still allow us to increase GPU frequency as needed.
Maybe wrap the code after thm_readl() in #if 0 in case somebody ends up
wanting to fix it? Or eliminate completely.
In theory the thm_readl() may affect the system behavior so would not
remove that for extra paranoia.
Regards, Joonas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2021-06-10 11:51 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-09 11:50 Computation of return value being discarded in get_cpu_power() in drivers/platform/x86/intel_ips.c Colin Ian King
2021-06-09 11:50 ` [Intel-gfx] " Colin Ian King
2021-06-09 11:50 ` Colin Ian King
2021-06-10 11:51 ` joonas.lahtinen [this message]
2021-06-10 11:51 ` [Intel-gfx] " joonas.lahtinen
2021-06-10 11:51 ` joonas.lahtinen
2021-06-10 11:55 ` Joonas Lahtinen
2021-06-10 11:55 ` [Intel-gfx] " Joonas Lahtinen
2021-06-10 11:55 ` Joonas Lahtinen
2021-06-10 15:40 ` Jesse Barnes
2021-06-10 15:40 ` [Intel-gfx] " Jesse Barnes
2021-06-10 15:43 ` Jesse Barnes
2021-06-10 15:43 ` [Intel-gfx] " Jesse Barnes
2021-06-10 15:43 ` Jesse Barnes
2021-06-10 15:55 ` Hans de Goede
2021-06-10 15:55 ` [Intel-gfx] " Hans de Goede
2021-06-10 15:55 ` Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=162332587143.15408.5683489401507477378@jlahtine-mobl.ger.corp.intel.com \
--to=joonas.lahtinen@linux.intel.com \
--cc=colin.king@canonical.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=hdegoede@redhat.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@linux.intel.com \
--cc=jsbarnes@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mgross@linux.intel.com \
--cc=platform-driver-x86@vger.kernel.org \
--cc=rodrigo.vivi@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.