Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Tue, 12 Jul 2022 20:37:03 +0200 | Subject | Re: [PATCH] PM: QoS: Add check to make sure CPU freq is non-negative |
| |
On Thu, Jun 23, 2022 at 8:47 AM Shivnandan Kumar <quic_kshivnan@quicinc.com> wrote: > > CPU frequency should never be non-negative.
Do you mean "always be non-negative"?
> If some client driver calls freq_qos_update_request with some > value greater than INT_MAX, then it will set max CPU freq at > fmax but it will add plist node with some negative priority. > plist node has priority from INT_MIN (highest) to INT_MAX > (lowest). Once priority is set as negative, another client > will not be able to reduce max CPU frequency. Adding check > to make sure CPU freq is non-negative will fix this problem. > Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com> > > --- > kernel/power/qos.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/kernel/power/qos.c b/kernel/power/qos.c > index ec7e1e85923e..41e96fe34bfd 100644 > --- a/kernel/power/qos.c > +++ b/kernel/power/qos.c > @@ -531,7 +531,8 @@ int freq_qos_add_request(struct freq_constraints *qos, > { > int ret; > > - if (IS_ERR_OR_NULL(qos) || !req) > + if (IS_ERR_OR_NULL(qos) || !req || value < FREQ_QOS_MIN_DEFAULT_VALUE > + || value > FREQ_QOS_MAX_DEFAULT_VALUE)
Why do you check against the defaults?
> return -EINVAL; > > if (WARN(freq_qos_request_active(req), > @@ -563,7 +564,8 @@ EXPORT_SYMBOL_GPL(freq_qos_add_request); > */ > int freq_qos_update_request(struct freq_qos_request *req, s32 new_value) > { > - if (!req) > + if (!req || new_value < FREQ_QOS_MIN_DEFAULT_VALUE || > + new_value > FREQ_QOS_MAX_DEFAULT_VALUE) > return -EINVAL; > > if (WARN(!freq_qos_request_active(req), > --
I agree that it should guard against adding negative values, but I don't see why s32 can be greater than INT_MAX.
Also why don't you put the guard into freq_qos_apply() instead of duplicating it in the callers of that function?
| |