[PATCH] clk: vc5: Abort clock configuration without upstream clock

[PATCH] clk: vc5: Abort clock configuration without upstream clock

[Marek Vasut]

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(-)

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);
-- 
2.18.0

Re: [PATCH] clk: vc5: Abort clock configuration without upstream clock

[Laurent Pinchart]

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);

-- 
Regards,

Laurent Pinchart

Re: [PATCH] clk: vc5: Abort clock configuration without upstream clock

[Marek Vasut]

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).

-- 
Best regards,
Marek Vasut

Re: [PATCH] clk: vc5: Abort clock configuration without upstream clock

[Stephen Boyd]

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?

Re: [PATCH] clk: vc5: Abort clock configuration without upstream clock

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] clk: vc5: Abort clock configuration without upstream clock

[Stephen Boyd]

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.

Re: [PATCH] clk: vc5: Abort clock configuration without upstream clock

[Stephen Boyd]

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