Messages in this thread | | | Subject | Re: [PATCH] interconnect: Check for valid path in icc_set_bw() | From | Georgi Djakov <> | Date | Sat, 21 Dec 2019 04:43:04 +0200 |
| |
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;
| |