Linux-Clk Archive on lore.kernel.org
 help / Atom feed
* [PATCH] clk: vc5: Abort clock configuration without upstream clock
@ 2018-12-15  0:55 Marek Vasut
  2018-12-16  7:19 ` Laurent Pinchart
  2019-01-09 18:54 ` Stephen Boyd
  0 siblings, 2 replies; 7+ messages in thread
From: Marek Vasut @ 2018-12-15  0:55 UTC (permalink / raw)
  To: linux-clk
  Cc: linux-renesas-soc, Marek Vasut, Alexey Firago, Laurent Pinchart,
	Stephen Boyd

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] clk: vc5: Abort clock configuration without upstream clock
  2018-12-15  0:55 [PATCH] clk: vc5: Abort clock configuration without upstream clock Marek Vasut
@ 2018-12-16  7:19 ` Laurent Pinchart
  2018-12-16 17:14   ` Marek Vasut
  2019-01-09 18:54 ` Stephen Boyd
  1 sibling, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2018-12-16  7:19 UTC (permalink / raw)
  To: Marek Vasut
  Cc: linux-clk, linux-renesas-soc, Marek Vasut, Alexey Firago, Stephen Boyd

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




^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] clk: vc5: Abort clock configuration without upstream clock
  2018-12-16  7:19 ` Laurent Pinchart
@ 2018-12-16 17:14   ` Marek Vasut
  2018-12-28 22:30     ` Stephen Boyd
  0 siblings, 1 reply; 7+ messages in thread
From: Marek Vasut @ 2018-12-16 17:14 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: linux-clk, linux-renesas-soc, Marek Vasut, Alexey Firago, Stephen Boyd

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] clk: vc5: Abort clock configuration without upstream clock
  2018-12-16 17:14   ` Marek Vasut
@ 2018-12-28 22:30     ` Stephen Boyd
  2018-12-29 16:34       ` Laurent Pinchart
  0 siblings, 1 reply; 7+ messages in thread
From: Stephen Boyd @ 2018-12-28 22:30 UTC (permalink / raw)
  To: Laurent Pinchart, Marek Vasut
  Cc: linux-clk, linux-renesas-soc, Marek Vasut, Alexey Firago

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?


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] clk: vc5: Abort clock configuration without upstream clock
  2018-12-28 22:30     ` Stephen Boyd
@ 2018-12-29 16:34       ` Laurent Pinchart
  2019-01-09 18:49         ` Stephen Boyd
  0 siblings, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2018-12-29 16:34 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Marek Vasut, linux-clk, linux-renesas-soc, Marek Vasut, Alexey Firago

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




^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] clk: vc5: Abort clock configuration without upstream clock
  2018-12-29 16:34       ` Laurent Pinchart
@ 2019-01-09 18:49         ` Stephen Boyd
  0 siblings, 0 replies; 7+ messages in thread
From: Stephen Boyd @ 2019-01-09 18:49 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Marek Vasut, linux-clk, linux-renesas-soc, Marek Vasut, Alexey Firago

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.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] clk: vc5: Abort clock configuration without upstream clock
  2018-12-15  0:55 [PATCH] clk: vc5: Abort clock configuration without upstream clock Marek Vasut
  2018-12-16  7:19 ` Laurent Pinchart
@ 2019-01-09 18:54 ` Stephen Boyd
  1 sibling, 0 replies; 7+ messages in thread
From: Stephen Boyd @ 2019-01-09 18:54 UTC (permalink / raw)
  To: Marek Vasut, linux-clk
  Cc: linux-renesas-soc, Marek Vasut, Alexey Firago, Laurent Pinchart

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-15  0:55 [PATCH] clk: vc5: Abort clock configuration without upstream clock Marek Vasut
2018-12-16  7:19 ` Laurent Pinchart
2018-12-16 17:14   ` Marek Vasut
2018-12-28 22:30     ` Stephen Boyd
2018-12-29 16:34       ` Laurent Pinchart
2019-01-09 18:49         ` Stephen Boyd
2019-01-09 18:54 ` Stephen Boyd

Linux-Clk Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-clk/0 linux-clk/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-clk linux-clk/ https://lore.kernel.org/linux-clk \
		linux-clk@vger.kernel.org linux-clk@archiver.kernel.org
	public-inbox-index linux-clk


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-clk


AGPL code for this site: git clone https://public-inbox.org/ public-inbox