@@ -1005,6 +1005,9 @@ static unsigned int powernv_fast_switch(struct cpufreq_policy *policy,structpowernv_smp_call_datafreq_data;index=cpufreq_table_find_index_dl(policy,target_freq);+if(unlikely(index<0))+index=get_nominal_index();+
AFAICT, you will get -1 here only if the freq table had no valid
frequencies (or the freq table is empty). Why would that happen ?
I agree too. There is no way we can get -1 with initialized cpu frequency table.
We don't initialize powernv-cpufreq if we don't have valid CPU frequency
entries. Is there any other way to suppress the Coverity tool warning apart from
ignoring it?
Thanks and Regards,
Shilpa
I agree too. There is no way we can get -1 with initialized cpu frequency table.
We don't initialize powernv-cpufreq if we don't have valid CPU frequency
entries. Is there any other way to suppress the Coverity tool warning apart from
ignoring it?
So IIUC, this warning is generated by an external tool after static
analysis of the code ?
If yes, then just ignore the warning. We shouldn't try fixing the
kernel because a tool isn't smart enough to catch intentional
ignorance of the return value here.
--
viresh
@@ -1005,6 +1005,9 @@ static unsigned int powernv_fast_switch(struct cpufreq_policy *policy,structpowernv_smp_call_datafreq_data;index=cpufreq_table_find_index_dl(policy,target_freq);+if(unlikely(index<0))+index=get_nominal_index();+
AFAICT, you will get -1 here only if the freq table had no valid
frequencies (or the freq table is empty). Why would that happen ?
Bugs?
Or if you ask for a target_freq that is higher than anything in the
table.
Or the API changes, and we forget to update this call site.
If you're saying that cpufreq_table_find_index_dl() can NEVER fail, then
write it so that it can never fail and change it to return unsigned int.
Having it potentially return -1, which is then used to index an array
and not handling that is just asking for bugs to happen.
cheers
AFAICT, you will get -1 here only if the freq table had no valid
frequencies (or the freq table is empty). Why would that happen ?
Bugs?
The cupfreq driver shouldn't have registered itself in that case (i.e.
if the cpufreq table is empty).
Or if you ask for a target_freq that is higher than anything in the
table.
You will still get a valid index in that case.
There is only once case where we return -1, when the cpufreq table
doesn't have any valid frequencies.
Or the API changes, and we forget to update this call site.
I am not sure we can do much about that right now.
If you're saying that cpufreq_table_find_index_dl() can NEVER fail,
Yes, if we have at least one valid frequency in the table, otherwise
the cpufreq driver shouldn't have registered itself.
then
write it so that it can never fail and change it to return unsigned int.
But what should we do when there is no frequency in the cpufreq table?
Just in case where a driver is buggy and tries to call this routine
for an invalid table.
Having it potentially return -1, which is then used to index an array
and not handling that is just asking for bugs to happen.
I understand what you are trying to say here, but I don't know what
can be done to prevent this here.
What we can do is change the return type to void and pass a int
pointer to the routine, but that wouldn't change anything at all. That
pointers variable can still have -1 in it.
--
viresh
From: "Rafael J. Wysocki" <rafael@kernel.org> Date: 2018-02-21 09:28:52
On Wed, Feb 21, 2018 at 6:54 AM, Viresh Kumar [off-list ref] wrote:
On 21-02-18, 16:39, Michael Ellerman wrote:
quoted
Viresh Kumar [off-list ref] writes:
quoted
quoted
AFAICT, you will get -1 here only if the freq table had no valid
frequencies (or the freq table is empty). Why would that happen ?
Bugs?
The cupfreq driver shouldn't have registered itself in that case (i.e.
if the cpufreq table is empty).
To be precise, ->init() should fail as that's where the table is
created. The registration fails as a result then.
But what if the bug is that ->init() doesn't fail when it should?
I guess the core could double check the frequency table after ->init()
if ->target_index is not NULL.
The overall point here is that if you get a negative index in
->fast_switch(), that's way too late anyway and we should be able to
catch that error much earlier.
To be precise, ->init() should fail as that's where the table is
created. The registration fails as a result then.
But what if the bug is that ->init() doesn't fail when it should?
I guess the core could double check the frequency table after ->init()
if ->target_index is not NULL.
The overall point here is that if you get a negative index in
->fast_switch(), that's way too late anyway and we should be able to
catch that error much earlier.
I don't want to end up doing double checking as some of it is already
done at init, but let me check on what can be done.
--
viresh
From: "Rafael J. Wysocki" <rafael@kernel.org> Date: 2018-02-21 10:17:18
On Wed, Feb 21, 2018 at 11:02 AM, Viresh Kumar [off-list ref] wrote:
On 21-02-18, 10:27, Rafael J. Wysocki wrote:
quoted
To be precise, ->init() should fail as that's where the table is
created. The registration fails as a result then.
But what if the bug is that ->init() doesn't fail when it should?
I guess the core could double check the frequency table after ->init()
if ->target_index is not NULL.
The overall point here is that if you get a negative index in
->fast_switch(), that's way too late anyway and we should be able to
catch that error much earlier.
I don't want to end up doing double checking as some of it is already
done at init, but let me check on what can be done.
The driver is expected to call cpufreq_table_validate_and_show() at
->init() time and fail ->init() if that fails.
That's kind of fragile, because it depends on the driver to do the right thing.
The driver is expected to call cpufreq_table_validate_and_show() at
->init() time and fail ->init() if that fails.
That's kind of fragile, because it depends on the driver to do the right thing.
That's exactly what I am trying to explore here, i.e. Call
validate/show from core instead of drivers.
--
viresh
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2018-02-21 13:15:46
"Rafael J. Wysocki" [off-list ref] writes:
On Wed, Feb 21, 2018 at 6:54 AM, Viresh Kumar [off-list ref] wrote:
quoted
On 21-02-18, 16:39, Michael Ellerman wrote:
quoted
Viresh Kumar [off-list ref] writes:
quoted
quoted
AFAICT, you will get -1 here only if the freq table had no valid
frequencies (or the freq table is empty). Why would that happen ?
Bugs?
The cupfreq driver shouldn't have registered itself in that case (i.e.
if the cpufreq table is empty).
To be precise, ->init() should fail as that's where the table is
created. The registration fails as a result then.
But what if the bug is that ->init() doesn't fail when it should?
I guess the core could double check the frequency table after ->init()
if ->target_index is not NULL.
The overall point here is that if you get a negative index in
->fast_switch(), that's way too late anyway and we should be able to
catch that error much earlier.
OK.
Still it's one thing for the driver to print a warning and bail out,
it's another to access off the front of an array and keep running using
some junk values, or oops (though not in this case because the array
happens to be static).
cheers
From: "Rafael J. Wysocki" <rafael@kernel.org> Date: 2018-02-21 13:25:14
On Wed, Feb 21, 2018 at 2:13 PM, Michael Ellerman [off-list ref] wrote:
"Rafael J. Wysocki" [off-list ref] writes:
quoted
On Wed, Feb 21, 2018 at 6:54 AM, Viresh Kumar [off-list ref] wrote:
quoted
On 21-02-18, 16:39, Michael Ellerman wrote:
quoted
Viresh Kumar [off-list ref] writes:
quoted
quoted
AFAICT, you will get -1 here only if the freq table had no valid
frequencies (or the freq table is empty). Why would that happen ?
Bugs?
The cupfreq driver shouldn't have registered itself in that case (i.e.
if the cpufreq table is empty).
To be precise, ->init() should fail as that's where the table is
created. The registration fails as a result then.
But what if the bug is that ->init() doesn't fail when it should?
I guess the core could double check the frequency table after ->init()
if ->target_index is not NULL.
The overall point here is that if you get a negative index in
->fast_switch(), that's way too late anyway and we should be able to
catch that error much earlier.
OK.
Still it's one thing for the driver to print a warning and bail out,
it's another to access off the front of an array and keep running using
some junk values, or oops (though not in this case because the array
happens to be static).
Well, let me rephrase. If ->fast_switch() runs, then it must not be
possible to get a negative index in it. That has to be guaranteed by
the core.
@@ -1005,6 +1005,9 @@ static unsigned int powernv_fast_switch(struct cpufreq_policy *policy,structpowernv_smp_call_datafreq_data;index=cpufreq_table_find_index_dl(policy,target_freq);+if(unlikely(index<0))+index=get_nominal_index();+
AFAICT, you will get -1 here only if the freq table had no valid
frequencies (or the freq table is empty). Why would that happen ?
I agree too. There is no way we can get -1 with initialized cpu frequency table.
We don't initialize powernv-cpufreq if we don't have valid CPU frequency
entries. Is there any other way to suppress the Coverity tool warning apart from
ignoring it?
In principle you could use BUG_ON(something_impossible) to annotate
that kind of thing to the static analysis tools, but that would
generate extra code.