| Message ID | 20191220171310.24169-1-georgi.djakov@linaro.org |
|---|---|
| State | Accepted |
| Commit | 7d7899c5297be8cd6095bcddecbbedccb34dd161 |
| Headers | show |
| Series | interconnect: Check for valid path in icc_set_bw() | expand |
On Fri 20 Dec 09:13 PST 2019, Georgi Djakov wrote: > Use IS_ERR() to ensure that the path passed to icc_set_bw() is valid. > > Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org> > --- > drivers/interconnect/core.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c > index 63c164264b73..14a6f7ade44a 100644 > --- a/drivers/interconnect/core.c > +++ b/drivers/interconnect/core.c > @@ -498,6 +498,11 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw) > if (!path || !path->num_nodes) > return 0; > > + if (IS_ERR(path)) { This is a sign of a logical error, and the print is likely to be ignored/lost in the noise. So I think the response should aid to help the developer hitting this to resolve the issue. So I think this is more visible and more useful as: if (WARN_ON(IS_ERR(path))) return -EINVAL; PS. Doesn't path->num_nodes == 0 fall in this category as well? When would you have a path object with no nodes passed to this function? Regards, Bjorn > + pr_err("%s: invalid path=%ld\n", __func__, PTR_ERR(path)); > + return -EINVAL; > + } > + > mutex_lock(&icc_lock); > > old_avg = path->reqs[0].avg_bw;
Hi Bjorn, Thanks for the comments! On 20.12.19 21:04, Bjorn Andersson wrote: > On Fri 20 Dec 09:13 PST 2019, Georgi Djakov wrote: > >> Use IS_ERR() to ensure that the path passed to icc_set_bw() is valid. >> >> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org> >> --- >> drivers/interconnect/core.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c >> index 63c164264b73..14a6f7ade44a 100644 >> --- a/drivers/interconnect/core.c >> +++ b/drivers/interconnect/core.c >> @@ -498,6 +498,11 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw) >> if (!path || !path->num_nodes) >> return 0; >> >> + if (IS_ERR(path)) { > > This is a sign of a logical error, and the print is likely to be > ignored/lost in the noise. So I think the response should aid to help > the developer hitting this to resolve the issue. > > So I think this is more visible and more useful as: > > if (WARN_ON(IS_ERR(path))) > return -EINVAL; That's actually what i had in mind initially, but then started wondering whether this isn't a bit too noisy. But oh well, let's scream loud if something is done incorrectly. > > PS. Doesn't path->num_nodes == 0 fall in this category as well? When > would you have a path object with no nodes passed to this function? Yes, will make the warning cover this case too. Thanks, Georgi > > Regards, > Bjorn > >> + pr_err("%s: invalid path=%ld\n", __func__, PTR_ERR(path)); >> + return -EINVAL; >> + } >> + >> mutex_lock(&icc_lock); >> >> old_avg = path->reqs[0].avg_bw;
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c index 63c164264b73..14a6f7ade44a 100644 --- a/drivers/interconnect/core.c +++ b/drivers/interconnect/core.c @@ -498,6 +498,11 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw) if (!path || !path->num_nodes) return 0; + if (IS_ERR(path)) { + pr_err("%s: invalid path=%ld\n", __func__, PTR_ERR(path)); + return -EINVAL; + } + mutex_lock(&icc_lock); old_avg = path->reqs[0].avg_bw;
Use IS_ERR() to ensure that the path passed to icc_set_bw() is valid. Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org> --- drivers/interconnect/core.c | 5 +++++ 1 file changed, 5 insertions(+)