| Message ID | 20200225134248.919889-1-maxime@cerno.tech (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | clk: Fix phase init check | expand |
Quoting Maxime Ripard (2020-02-25 05:42:48) > Commit 2760878662a2 ("clk: Bail out when calculating phase fails during clk > registration") introduced a check on error values at the time the clock is > registered to bail out when such an error occurs. > > However, it doesn't check whether the returned value is positive which will > happen if the driver returns a non-zero phase, and ends up returning that > to the caller on success, breaking the API that implies that the driver > should return 0 on success. I had to read this a few times to understand. I'll reword it to indicate that __clk_core_init() should return 0 on success and not the phase which is typically a positive value. > > Fixes: 2760878662a2 ("clk: Bail out when calculating phase fails during clk registration") > Signed-off-by: Maxime Ripard <maxime@cerno.tech> Thanks. I think we need a Reported-by: "kernelci.org bot" <bot@kernelci.org> tag added as well. > --- > drivers/clk/clk.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index ebfc1e2103cb..f122e9911b57 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -3344,6 +3344,7 @@ static int __clk_core_init(struct clk_core *core) > int ret; > struct clk_core *parent; > unsigned long rate; > + int phase; > > if (!core) > return -EINVAL; > @@ -3457,8 +3458,9 @@ static int __clk_core_init(struct clk_core *core) > * Since a phase is by definition relative to its parent, just > * query the current clock phase, or just assume it's in phase. > */ > - ret = clk_core_get_phase(core); > - if (ret < 0) { > + phase = clk_core_get_phase(core); > + if (phase < 0) { > + ret = phase; > pr_warn("%s: Failed to get phase for clk '%s'\n", __func__, > core->name); > goto out;
Quoting Maxime Ripard (2020-02-25 05:42:48) > Commit 2760878662a2 ("clk: Bail out when calculating phase fails during clk > registration") introduced a check on error values at the time the clock is > registered to bail out when such an error occurs. > > However, it doesn't check whether the returned value is positive which will > happen if the driver returns a non-zero phase, and ends up returning that > to the caller on success, breaking the API that implies that the driver > should return 0 on success. > > Fixes: 2760878662a2 ("clk: Bail out when calculating phase fails during clk registration") > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- Applied to clk-next
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index ebfc1e2103cb..f122e9911b57 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -3344,6 +3344,7 @@ static int __clk_core_init(struct clk_core *core) int ret; struct clk_core *parent; unsigned long rate; + int phase; if (!core) return -EINVAL; @@ -3457,8 +3458,9 @@ static int __clk_core_init(struct clk_core *core) * Since a phase is by definition relative to its parent, just * query the current clock phase, or just assume it's in phase. */ - ret = clk_core_get_phase(core); - if (ret < 0) { + phase = clk_core_get_phase(core); + if (phase < 0) { + ret = phase; pr_warn("%s: Failed to get phase for clk '%s'\n", __func__, core->name); goto out;
Commit 2760878662a2 ("clk: Bail out when calculating phase fails during clk registration") introduced a check on error values at the time the clock is registered to bail out when such an error occurs. However, it doesn't check whether the returned value is positive which will happen if the driver returns a non-zero phase, and ends up returning that to the caller on success, breaking the API that implies that the driver should return 0 on success. Fixes: 2760878662a2 ("clk: Bail out when calculating phase fails during clk registration") Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/clk/clk.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)