| Message ID | af85927b-0da3-3495-2ed4-64bda91cd239@gmail.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | clk: mux: let clk_mux_val_to_index return U8_MAX in the error case | expand |
Quoting Heiner Kallweit (2023-04-11 23:19:04) > Currently this function may return a negative errno, but almost no > user checks for the error case. Only imx_clk_gpr_mux_get_parent() > does, but mentions in a comment that they'd prefer a dummy value. > Other users cast the negative errno to u8 instead, what may result > in unwanted results. > Let's deal with this by returning u8 and U8_MAX in the error case. > Then clk_core_get_parent_by_index() can detect that the index is > out of range. Is this causing problems for you?
On 13.04.2023 21:10, Stephen Boyd wrote: > Quoting Heiner Kallweit (2023-04-11 23:19:04) >> Currently this function may return a negative errno, but almost no >> user checks for the error case. Only imx_clk_gpr_mux_get_parent() >> does, but mentions in a comment that they'd prefer a dummy value. >> Other users cast the negative errno to u8 instead, what may result >> in unwanted results. >> Let's deal with this by returning u8 and U8_MAX in the error case. >> Then clk_core_get_parent_by_index() can detect that the index is >> out of range. > > Is this causing problems for you? I'm not directly impacted. I came across this when working on a driver with the following scenario: I have a clock muxer where I want to exclude one of the mux parents by using clk_mux.table. If by chance the boot loader used this mux parent, then clk_mux_val_to_index() would return an errno. Also within clk core the return value isn't checked in clk_mux_get_parent().
Quoting Heiner Kallweit (2023-04-13 12:27:27) > On 13.04.2023 21:10, Stephen Boyd wrote: > > Quoting Heiner Kallweit (2023-04-11 23:19:04) > >> Currently this function may return a negative errno, but almost no > >> user checks for the error case. Only imx_clk_gpr_mux_get_parent() > >> does, but mentions in a comment that they'd prefer a dummy value. > >> Other users cast the negative errno to u8 instead, what may result > >> in unwanted results. > >> Let's deal with this by returning u8 and U8_MAX in the error case. > >> Then clk_core_get_parent_by_index() can detect that the index is > >> out of range. > > > > Is this causing problems for you? > > I'm not directly impacted. I came across this when working on a driver > with the following scenario: > > I have a clock muxer where I want to exclude one of the mux parents > by using clk_mux.table. If by chance the boot loader used this mux > parent, then clk_mux_val_to_index() would return an errno. > Also within clk core the return value isn't checked in > clk_mux_get_parent(). > Ok. I'd rather go with a total rewrite of the get_parent clk_op[1] and fix this at the same time. [1] https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/log/?h=clk-parent-rewrite
On 13.04.2023 22:30, Stephen Boyd wrote: > Quoting Heiner Kallweit (2023-04-13 12:27:27) >> On 13.04.2023 21:10, Stephen Boyd wrote: >>> Quoting Heiner Kallweit (2023-04-11 23:19:04) >>>> Currently this function may return a negative errno, but almost no >>>> user checks for the error case. Only imx_clk_gpr_mux_get_parent() >>>> does, but mentions in a comment that they'd prefer a dummy value. >>>> Other users cast the negative errno to u8 instead, what may result >>>> in unwanted results. >>>> Let's deal with this by returning u8 and U8_MAX in the error case. >>>> Then clk_core_get_parent_by_index() can detect that the index is >>>> out of range. >>> >>> Is this causing problems for you? >> >> I'm not directly impacted. I came across this when working on a driver >> with the following scenario: >> >> I have a clock muxer where I want to exclude one of the mux parents >> by using clk_mux.table. If by chance the boot loader used this mux >> parent, then clk_mux_val_to_index() would return an errno. >> Also within clk core the return value isn't checked in >> clk_mux_get_parent(). >> > > Ok. I'd rather go with a total rewrite of the get_parent clk_op[1] and fix > this at the same time. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/log/?h=clk-parent-rewrite This would be even better. Thanks.
diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c index fa817c317..be0f317ee 100644 --- a/drivers/clk/clk-mux.c +++ b/drivers/clk/clk-mux.c @@ -40,8 +40,8 @@ static inline void clk_mux_writel(struct clk_mux *mux, u32 val) writel(val, mux->reg); } -int clk_mux_val_to_index(struct clk_hw *hw, const u32 *table, unsigned int flags, - unsigned int val) +u8 clk_mux_val_to_index(struct clk_hw *hw, const u32 *table, unsigned int flags, + unsigned int val) { int num_parents = clk_hw_get_num_parents(hw); @@ -51,7 +51,7 @@ int clk_mux_val_to_index(struct clk_hw *hw, const u32 *table, unsigned int flags for (i = 0; i < num_parents; i++) if (table[i] == val) return i; - return -EINVAL; + return U8_MAX; } if (val && (flags & CLK_MUX_INDEX_BIT)) @@ -61,7 +61,7 @@ int clk_mux_val_to_index(struct clk_hw *hw, const u32 *table, unsigned int flags val--; if (val >= num_parents) - return -EINVAL; + return U8_MAX; return val; } diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index faad3cdc1..62fb4118c 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -1069,8 +1069,8 @@ struct clk *clk_register_mux_table(struct device *dev, const char *name, NULL, (parent_data), (flags), (reg), (shift), \ BIT((width)) - 1, (clk_mux_flags), table, (lock)) -int clk_mux_val_to_index(struct clk_hw *hw, const u32 *table, unsigned int flags, - unsigned int val); +u8 clk_mux_val_to_index(struct clk_hw *hw, const u32 *table, unsigned int flags, + unsigned int val); unsigned int clk_mux_index_to_val(const u32 *table, unsigned int flags, u8 index); void clk_unregister_mux(struct clk *clk);
Currently this function may return a negative errno, but almost no user checks for the error case. Only imx_clk_gpr_mux_get_parent() does, but mentions in a comment that they'd prefer a dummy value. Other users cast the negative errno to u8 instead, what may result in unwanted results. Let's deal with this by returning u8 and U8_MAX in the error case. Then clk_core_get_parent_by_index() can detect that the index is out of range. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/clk/clk-mux.c | 8 ++++---- include/linux/clk-provider.h | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-)