* Re: [Intel-gfx] Computation of return value being discarded in get_cpu_power() in drivers/platform/x86/intel_ips.c
@ 2021-06-10 11:55 ` Joonas Lahtinen
0 siblings, 0 replies; 17+ messages in thread
From: Joonas Lahtinen @ 2021-06-10 11:55 UTC (permalink / raw)
To: Hans de Goede, Colin Ian King, Jani Nikula, Mark Gross,
Rodrigo Vivi, Jesse Barnes
Cc: intel-gfx, linux-kernel, dri-devel, platform-driver-x86
(Address for Hans was corrupt in previous message, which confused my mail
client. Sorry for duplicate message, the other is without From: field).
+ 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
> Colin
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: Computation of return value being discarded in get_cpu_power() in drivers/platform/x86/intel_ips.c
@ 2021-06-10 11:55 ` Joonas Lahtinen
0 siblings, 0 replies; 17+ messages in thread
From: Joonas Lahtinen @ 2021-06-10 11:55 UTC (permalink / raw)
To: Hans de Goede, Colin Ian King, Jani Nikula, Mark Gross,
Rodrigo Vivi, Jesse Barnes
Cc: intel-gfx, linux-kernel, dri-devel, platform-driver-x86
(Address for Hans was corrupt in previous message, which confused my mail
client. Sorry for duplicate message, the other is without From: field).
+ 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
> Colin
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: Computation of return value being discarded in get_cpu_power() in drivers/platform/x86/intel_ips.c
2021-06-10 11:55 ` Joonas Lahtinen
@ 2021-06-10 15:40 ` Jesse Barnes
-1 siblings, 0 replies; 17+ messages in thread
From: Jesse Barnes @ 2021-06-10 15:40 UTC (permalink / raw)
To: Joonas Lahtinen
Cc: Mark Gross, intel-gfx, linux-kernel, platform-driver-x86,
Hans de Goede, dri-devel, Rodrigo Vivi, Colin Ian King
[-- Attachment #1: Type: text/plain, Size: 2763 bytes --]
It may be ok to drop this driver entirely now too; I doubt anyone is
relying on GPU turbo in Ironlake for anything critical anymore. That would
allow for some simplifications in i915 too if it's still supported.
Jesse
On Thu, Jun 10, 2021 at 4:56 AM Joonas Lahtinen <
joonas.lahtinen@linux.intel.com> wrote:
> (Address for Hans was corrupt in previous message, which confused my mail
> client. Sorry for duplicate message, the other is without From: field).
>
> + 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
>
> > Colin
> >
> >
>
[-- Attachment #2: Type: text/html, Size: 3682 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Intel-gfx] Computation of return value being discarded in get_cpu_power() in drivers/platform/x86/intel_ips.c
@ 2021-06-10 15:40 ` Jesse Barnes
0 siblings, 0 replies; 17+ messages in thread
From: Jesse Barnes @ 2021-06-10 15:40 UTC (permalink / raw)
To: Joonas Lahtinen
Cc: Mark Gross, intel-gfx, linux-kernel, platform-driver-x86,
dri-devel, Colin Ian King
[-- Attachment #1.1: Type: text/plain, Size: 2763 bytes --]
It may be ok to drop this driver entirely now too; I doubt anyone is
relying on GPU turbo in Ironlake for anything critical anymore. That would
allow for some simplifications in i915 too if it's still supported.
Jesse
On Thu, Jun 10, 2021 at 4:56 AM Joonas Lahtinen <
joonas.lahtinen@linux.intel.com> wrote:
> (Address for Hans was corrupt in previous message, which confused my mail
> client. Sorry for duplicate message, the other is without From: field).
>
> + 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
>
> > Colin
> >
> >
>
[-- Attachment #1.2: Type: text/html, Size: 3682 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Computation of return value being discarded in get_cpu_power() in drivers/platform/x86/intel_ips.c
2021-06-10 11:55 ` Joonas Lahtinen
(?)
@ 2021-06-10 15:43 ` Jesse Barnes
-1 siblings, 0 replies; 17+ messages in thread
From: Jesse Barnes @ 2021-06-10 15:43 UTC (permalink / raw)
To: Joonas Lahtinen
Cc: Hans de Goede, Colin Ian King, Jani Nikula, Mark Gross,
Rodrigo Vivi, platform-driver-x86, intel-gfx, dri-devel,
linux-kernel
Arg html email sorry. Resending plain text:
It may be ok to drop this driver entirely now too; I doubt anyone is
relying on GPU turbo in Ironlake for anything critical anymore. That
would allow for some simplifications in i915 too if it's still
supported.
On Thu, Jun 10, 2021 at 4:56 AM Joonas Lahtinen
<joonas.lahtinen@linux.intel.com> wrote:
>
> (Address for Hans was corrupt in previous message, which confused my mail
> client. Sorry for duplicate message, the other is without From: field).
>
> + 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
>
> > Colin
> >
> >
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Intel-gfx] Computation of return value being discarded in get_cpu_power() in drivers/platform/x86/intel_ips.c
@ 2021-06-10 15:43 ` Jesse Barnes
0 siblings, 0 replies; 17+ messages in thread
From: Jesse Barnes @ 2021-06-10 15:43 UTC (permalink / raw)
To: Joonas Lahtinen
Cc: Mark Gross, intel-gfx, linux-kernel, platform-driver-x86,
dri-devel, Colin Ian King
Arg html email sorry. Resending plain text:
It may be ok to drop this driver entirely now too; I doubt anyone is
relying on GPU turbo in Ironlake for anything critical anymore. That
would allow for some simplifications in i915 too if it's still
supported.
On Thu, Jun 10, 2021 at 4:56 AM Joonas Lahtinen
<joonas.lahtinen@linux.intel.com> wrote:
>
> (Address for Hans was corrupt in previous message, which confused my mail
> client. Sorry for duplicate message, the other is without From: field).
>
> + 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
>
> > Colin
> >
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: Computation of return value being discarded in get_cpu_power() in drivers/platform/x86/intel_ips.c
@ 2021-06-10 15:43 ` Jesse Barnes
0 siblings, 0 replies; 17+ messages in thread
From: Jesse Barnes @ 2021-06-10 15:43 UTC (permalink / raw)
To: Joonas Lahtinen
Cc: Mark Gross, intel-gfx, linux-kernel, platform-driver-x86,
Hans de Goede, dri-devel, Rodrigo Vivi, Colin Ian King
Arg html email sorry. Resending plain text:
It may be ok to drop this driver entirely now too; I doubt anyone is
relying on GPU turbo in Ironlake for anything critical anymore. That
would allow for some simplifications in i915 too if it's still
supported.
On Thu, Jun 10, 2021 at 4:56 AM Joonas Lahtinen
<joonas.lahtinen@linux.intel.com> wrote:
>
> (Address for Hans was corrupt in previous message, which confused my mail
> client. Sorry for duplicate message, the other is without From: field).
>
> + 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
>
> > Colin
> >
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: Computation of return value being discarded in get_cpu_power() in drivers/platform/x86/intel_ips.c
2021-06-10 11:55 ` Joonas Lahtinen
(?)
@ 2021-06-10 15:55 ` Hans de Goede
-1 siblings, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2021-06-10 15:55 UTC (permalink / raw)
To: Joonas Lahtinen, Colin Ian King, Jani Nikula, Mark Gross,
Rodrigo Vivi, Jesse Barnes
Cc: platform-driver-x86, intel-gfx, dri-devel, linux-kernel
Hi,
On 6/10/21 1:55 PM, Joonas Lahtinen wrote:
> (Address for Hans was corrupt in previous message, which confused my mail
> client. Sorry for duplicate message, the other is without From: field).
>
> + 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.
This has already been fixed (yesterday actually) in linux-next:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=for-next&id=13c3b4f76073d73dd81e418295902676153f6cb5
Regards,
Hans
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Intel-gfx] Computation of return value being discarded in get_cpu_power() in drivers/platform/x86/intel_ips.c
@ 2021-06-10 15:55 ` Hans de Goede
0 siblings, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2021-06-10 15:55 UTC (permalink / raw)
To: Joonas Lahtinen, Colin Ian King, Jani Nikula, Mark Gross,
Rodrigo Vivi, Jesse Barnes
Cc: intel-gfx, linux-kernel, dri-devel, platform-driver-x86
Hi,
On 6/10/21 1:55 PM, Joonas Lahtinen wrote:
> (Address for Hans was corrupt in previous message, which confused my mail
> client. Sorry for duplicate message, the other is without From: field).
>
> + 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.
This has already been fixed (yesterday actually) in linux-next:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=for-next&id=13c3b4f76073d73dd81e418295902676153f6cb5
Regards,
Hans
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: Computation of return value being discarded in get_cpu_power() in drivers/platform/x86/intel_ips.c
@ 2021-06-10 15:55 ` Hans de Goede
0 siblings, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2021-06-10 15:55 UTC (permalink / raw)
To: Joonas Lahtinen, Colin Ian King, Jani Nikula, Mark Gross,
Rodrigo Vivi, Jesse Barnes
Cc: intel-gfx, linux-kernel, dri-devel, platform-driver-x86
Hi,
On 6/10/21 1:55 PM, Joonas Lahtinen wrote:
> (Address for Hans was corrupt in previous message, which confused my mail
> client. Sorry for duplicate message, the other is without From: field).
>
> + 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.
This has already been fixed (yesterday actually) in linux-next:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=for-next&id=13c3b4f76073d73dd81e418295902676153f6cb5
Regards,
Hans
^ permalink raw reply [flat|nested] 17+ messages in thread