| Message ID | 20181215005519.3695-1-marek.vasut+renesas@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | clk: vc5: Abort clock configuration without upstream clock | expand |
Hi Marek, Thank you for the patch. On Saturday, 15 December 2018 02:55:19 EET Marek Vasut wrote: > In case the upstream clock are not set, which can happen in case the > VC5 has no valid upstream clock, the $src variable is used uninited > by regmap_update_bits(). Check for this condition and return -EINVAL > in such case. Note that the probe() function will fail in this case, so vc5_mux_set_parent() won't be reached. > Note that in case the VC5 has no valid upstream clock, the VC5 can > not operate correctly. That is a hardware property of the VC5. The > internal oscilator present in some VC5 models is also considered > upstream clock. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Alexey Firago <alexey_firago@mentor.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Stephen Boyd <sboyd@kernel.org> > Cc: linux-renesas-soc@vger.kernel.org > --- > NOTE: This is an updated version of: > https://patchwork.kernel.org/patch/10731699/ > --- > drivers/clk/clk-versaclock5.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c > index 5b393e711e94..b10801506518 100644 > --- a/drivers/clk/clk-versaclock5.c > +++ b/drivers/clk/clk-versaclock5.c > @@ -262,8 +262,10 @@ static int vc5_mux_set_parent(struct clk_hw *hw, u8 > index) > > if (vc5->clk_mux_ins == VC5_MUX_IN_XIN) > src = VC5_PRIM_SRC_SHDN_EN_XTAL; > - if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN) > + else if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN) > src = VC5_PRIM_SRC_SHDN_EN_CLKIN; > + else > + return -EINVAL; > } I'd rather go for Stephen's approach if the goal is just to silence a warning for a condition that can't happen in practice. > return regmap_update_bits(vc5->regmap, VC5_PRIM_SRC_SHDN, mask, src);
On 12/16/2018 08:19 AM, Laurent Pinchart wrote: > Hi Marek, > > Thank you for the patch. > > On Saturday, 15 December 2018 02:55:19 EET Marek Vasut wrote: >> In case the upstream clock are not set, which can happen in case the >> VC5 has no valid upstream clock, the $src variable is used uninited >> by regmap_update_bits(). Check for this condition and return -EINVAL >> in such case. > > Note that the probe() function will fail in this case, so vc5_mux_set_parent() > won't be reached. > >> Note that in case the VC5 has no valid upstream clock, the VC5 can >> not operate correctly. That is a hardware property of the VC5. The >> internal oscilator present in some VC5 models is also considered >> upstream clock. >> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> >> Cc: Alexey Firago <alexey_firago@mentor.com> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> Cc: Stephen Boyd <sboyd@kernel.org> >> Cc: linux-renesas-soc@vger.kernel.org >> --- >> NOTE: This is an updated version of: >> https://patchwork.kernel.org/patch/10731699/ >> --- >> drivers/clk/clk-versaclock5.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c >> index 5b393e711e94..b10801506518 100644 >> --- a/drivers/clk/clk-versaclock5.c >> +++ b/drivers/clk/clk-versaclock5.c >> @@ -262,8 +262,10 @@ static int vc5_mux_set_parent(struct clk_hw *hw, u8 >> index) >> >> if (vc5->clk_mux_ins == VC5_MUX_IN_XIN) >> src = VC5_PRIM_SRC_SHDN_EN_XTAL; >> - if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN) >> + else if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN) >> src = VC5_PRIM_SRC_SHDN_EN_CLKIN; >> + else >> + return -EINVAL; >> } > > I'd rather go for Stephen's approach if the goal is just to silence a warning > for a condition that can't happen in practice. Sure, probe will fail, but it's safer to handle the possibility that probe() is broken and this code is reached by properly handling the failure instead of doing something obviously wrong (like configuring the hardware with value 0).
Quoting Marek Vasut (2018-12-16 09:14:29) > On 12/16/2018 08:19 AM, Laurent Pinchart wrote: > > Hi Marek, > > > > Thank you for the patch. > > > > On Saturday, 15 December 2018 02:55:19 EET Marek Vasut wrote: > >> In case the upstream clock are not set, which can happen in case the > >> VC5 has no valid upstream clock, the $src variable is used uninited > >> by regmap_update_bits(). Check for this condition and return -EINVAL > >> in such case. > > > > Note that the probe() function will fail in this case, so vc5_mux_set_parent() > > won't be reached. > > > >> Note that in case the VC5 has no valid upstream clock, the VC5 can > >> not operate correctly. That is a hardware property of the VC5. The > >> internal oscilator present in some VC5 models is also considered > >> upstream clock. > >> > >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > >> Cc: Alexey Firago <alexey_firago@mentor.com> > >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> Cc: Stephen Boyd <sboyd@kernel.org> > >> Cc: linux-renesas-soc@vger.kernel.org > >> --- > >> NOTE: This is an updated version of: > >> https://patchwork.kernel.org/patch/10731699/ > >> --- > >> drivers/clk/clk-versaclock5.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c > >> index 5b393e711e94..b10801506518 100644 > >> --- a/drivers/clk/clk-versaclock5.c > >> +++ b/drivers/clk/clk-versaclock5.c > >> @@ -262,8 +262,10 @@ static int vc5_mux_set_parent(struct clk_hw *hw, u8 > >> index) > >> > >> if (vc5->clk_mux_ins == VC5_MUX_IN_XIN) > >> src = VC5_PRIM_SRC_SHDN_EN_XTAL; > >> - if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN) > >> + else if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN) > >> src = VC5_PRIM_SRC_SHDN_EN_CLKIN; > >> + else > >> + return -EINVAL; > >> } > > > > I'd rather go for Stephen's approach if the goal is just to silence a warning > > for a condition that can't happen in practice. > > Sure, probe will fail, but it's safer to handle the possibility that > probe() is broken and this code is reached by properly handling the > failure instead of doing something obviously wrong (like configuring the > hardware with value 0). I'm fine with this approach. Laurent?
Hi Stephen, On Saturday, 29 December 2018 00:30:16 EET Stephen Boyd wrote: > Quoting Marek Vasut (2018-12-16 09:14:29) > > On 12/16/2018 08:19 AM, Laurent Pinchart wrote: > > > On Saturday, 15 December 2018 02:55:19 EET Marek Vasut wrote: > > >> In case the upstream clock are not set, which can happen in case the > > >> VC5 has no valid upstream clock, the $src variable is used uninited > > >> by regmap_update_bits(). Check for this condition and return -EINVAL > > >> in such case. > > > > > > Note that the probe() function will fail in this case, so > > > vc5_mux_set_parent() won't be reached. > > > > > >> Note that in case the VC5 has no valid upstream clock, the VC5 can > > >> not operate correctly. That is a hardware property of the VC5. The > > >> internal oscilator present in some VC5 models is also considered > > >> upstream clock. > > >> > > >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > > >> Cc: Alexey Firago <alexey_firago@mentor.com> > > >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > >> Cc: Stephen Boyd <sboyd@kernel.org> > > >> Cc: linux-renesas-soc@vger.kernel.org > > >> --- > > >> > > >> NOTE: This is an updated version of: > > >> https://patchwork.kernel.org/patch/10731699/ > > >> > > >> --- > > >> > > >> drivers/clk/clk-versaclock5.c | 4 +++- > > >> 1 file changed, 3 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/clk/clk-versaclock5.c > > >> b/drivers/clk/clk-versaclock5.c > > >> index 5b393e711e94..b10801506518 100644 > > >> --- a/drivers/clk/clk-versaclock5.c > > >> +++ b/drivers/clk/clk-versaclock5.c > > >> @@ -262,8 +262,10 @@ static int vc5_mux_set_parent(struct clk_hw *hw, > > >> u8> index) > > >> > > >> if (vc5->clk_mux_ins == VC5_MUX_IN_XIN) > > >> src = VC5_PRIM_SRC_SHDN_EN_XTAL; > > >> - if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN) > > >> + else if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN) > > >> src = VC5_PRIM_SRC_SHDN_EN_CLKIN; > > >> + else > > >> + return -EINVAL; > > >> } > > > > > > I'd rather go for Stephen's approach if the goal is just to silence a > > > warning for a condition that can't happen in practice. > > > > Sure, probe will fail, but it's safer to handle the possibility that > > probe() is broken and this code is reached by properly handling the > > failure instead of doing something obviously wrong (like configuring the > > hardware with value 0). > > I'm fine with this approach. Laurent? So am I. I might add a comment in the vc5_mu_set_parent() function to explain this can't happen.
Quoting Laurent Pinchart (2018-12-29 08:34:18) > Hi Stephen, > > On Saturday, 29 December 2018 00:30:16 EET Stephen Boyd wrote: > > Quoting Marek Vasut (2018-12-16 09:14:29) > > > On 12/16/2018 08:19 AM, Laurent Pinchart wrote: > > > > On Saturday, 15 December 2018 02:55:19 EET Marek Vasut wrote: > > > >> In case the upstream clock are not set, which can happen in case the > > > >> VC5 has no valid upstream clock, the $src variable is used uninited > > > >> by regmap_update_bits(). Check for this condition and return -EINVAL > > > >> in such case. > > > > > > > > Note that the probe() function will fail in this case, so > > > > vc5_mux_set_parent() won't be reached. > > > > > > > >> Note that in case the VC5 has no valid upstream clock, the VC5 can > > > >> not operate correctly. That is a hardware property of the VC5. The > > > >> internal oscilator present in some VC5 models is also considered > > > >> upstream clock. > > > >> > > > >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > > > >> Cc: Alexey Firago <alexey_firago@mentor.com> > > > >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> Cc: Stephen Boyd <sboyd@kernel.org> > > > >> Cc: linux-renesas-soc@vger.kernel.org > > > >> --- > > > >> > > > >> NOTE: This is an updated version of: > > > >> https://patchwork.kernel.org/patch/10731699/ > > > >> > > > >> --- > > > >> > > > >> drivers/clk/clk-versaclock5.c | 4 +++- > > > >> 1 file changed, 3 insertions(+), 1 deletion(-) > > > >> > > > >> diff --git a/drivers/clk/clk-versaclock5.c > > > >> b/drivers/clk/clk-versaclock5.c > > > >> index 5b393e711e94..b10801506518 100644 > > > >> --- a/drivers/clk/clk-versaclock5.c > > > >> +++ b/drivers/clk/clk-versaclock5.c > > > >> @@ -262,8 +262,10 @@ static int vc5_mux_set_parent(struct clk_hw *hw, > > > >> u8> index) > > > >> > > > >> if (vc5->clk_mux_ins == VC5_MUX_IN_XIN) > > > >> src = VC5_PRIM_SRC_SHDN_EN_XTAL; > > > >> - if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN) > > > >> + else if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN) > > > >> src = VC5_PRIM_SRC_SHDN_EN_CLKIN; > > > >> + else > > > >> + return -EINVAL; > > > >> } > > > > > > > > I'd rather go for Stephen's approach if the goal is just to silence a > > > > warning for a condition that can't happen in practice. > > > > > > Sure, probe will fail, but it's safer to handle the possibility that > > > probe() is broken and this code is reached by properly handling the > > > failure instead of doing something obviously wrong (like configuring the > > > hardware with value 0). > > > > I'm fine with this approach. Laurent? > > So am I. I might add a comment in the vc5_mu_set_parent() function to explain > this can't happen. > Ok. I can add a note and throw this into clk-fixes.
Quoting Marek Vasut (2018-12-14 16:55:19) > In case the upstream clock are not set, which can happen in case the > VC5 has no valid upstream clock, the $src variable is used uninited > by regmap_update_bits(). Check for this condition and return -EINVAL > in such case. > > Note that in case the VC5 has no valid upstream clock, the VC5 can > not operate correctly. That is a hardware property of the VC5. The > internal oscilator present in some VC5 models is also considered > upstream clock. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > Cc: Alexey Firago <alexey_firago@mentor.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Stephen Boyd <sboyd@kernel.org> > Cc: linux-renesas-soc@vger.kernel.org > --- Applied to clk-fixes
diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c index 5b393e711e94..b10801506518 100644 --- a/drivers/clk/clk-versaclock5.c +++ b/drivers/clk/clk-versaclock5.c @@ -262,8 +262,10 @@ static int vc5_mux_set_parent(struct clk_hw *hw, u8 index) if (vc5->clk_mux_ins == VC5_MUX_IN_XIN) src = VC5_PRIM_SRC_SHDN_EN_XTAL; - if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN) + else if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN) src = VC5_PRIM_SRC_SHDN_EN_CLKIN; + else + return -EINVAL; } return regmap_update_bits(vc5->regmap, VC5_PRIM_SRC_SHDN, mask, src);
In case the upstream clock are not set, which can happen in case the VC5 has no valid upstream clock, the $src variable is used uninited by regmap_update_bits(). Check for this condition and return -EINVAL in such case. Note that in case the VC5 has no valid upstream clock, the VC5 can not operate correctly. That is a hardware property of the VC5. The internal oscilator present in some VC5 models is also considered upstream clock. Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> Cc: Alexey Firago <alexey_firago@mentor.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Stephen Boyd <sboyd@kernel.org> Cc: linux-renesas-soc@vger.kernel.org --- NOTE: This is an updated version of: https://patchwork.kernel.org/patch/10731699/ --- drivers/clk/clk-versaclock5.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)