| Message ID | 20220623064605.2538969-1-quic_kshivnan@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | PM: QoS: Add check to make sure CPU freq is non-negative | expand |
Gentle reminder, Thanks, Shivnandan On 6/23/2022 12:16 PM, Shivnandan Kumar wrote: > CPU frequency should never 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) > 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),
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?
Hi Rafael, Thanks for taking the time to review my patch and providing feedback. Please find answer inline. Thanks, Shivnandan On 7/13/2022 12:07 AM, Rafael J. Wysocki wrote: > On Thu, Jun 23, 2022 at 8:47 AM Shivnandan Kumar > <quic_kshivnan@quicinc.com> wrote: >> CPU frequency should never be negative. > Do you mean "always be non-negative"? Yes,corrected subject now. > >> 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? Want to make sure to guard against negative value. > >> 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. yes, checking against negative values will be sufficient. I will share patch v2 with only check against negative values. > > Also why don't you put the guard into freq_qos_apply() instead of > duplicating it in the callers of that function? Because function freq_qos_remove_request calls freq_qos_apply with PM_QOS_DEFAULT_VALUE which is actually negative. So I do not want to break that.
On Wed, Jul 13, 2022 at 10:37 AM Shivnandan Kumar <quic_kshivnan@quicinc.com> wrote: > > Hi Rafael, > > > Thanks for taking the time to review my patch and providing feedback. > > Please find answer inline. > > Thanks, > > Shivnandan > > On 7/13/2022 12:07 AM, Rafael J. Wysocki wrote: > > On Thu, Jun 23, 2022 at 8:47 AM Shivnandan Kumar > > <quic_kshivnan@quicinc.com> wrote: > >> CPU frequency should never be negative. > > Do you mean "always be non-negative"? > Yes,corrected subject now. > > > >> 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? > Want to make sure to guard against negative value. > > > >> 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. > yes, checking against negative values will be sufficient. > I will share patch v2 with only check against negative values. > > > > Also why don't you put the guard into freq_qos_apply() instead of > > duplicating it in the callers of that function? > Because function freq_qos_remove_request calls freq_qos_apply with > PM_QOS_DEFAULT_VALUE which is actually negative. > So I do not want to break that. OK
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) 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),
CPU frequency should never 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(-)