[PATCH 1/2] ARM: kirkwood: fix a not initialized variable in the sound subsystem

[PATCH 1/2] ARM: kirkwood: fix a not initialized variable in the sound subsystem

[Jean-Francois Moine]

In the function kirkwood_set_rate, in case of a non dco supported rate
and no external clock, the clock source was set to an undefined value.
This patch just displays a message without changing the clock source.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 sound/soc/kirkwood/kirkwood-i2s.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index c74c890..afca1ec 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -118,6 +118,9 @@ static void kirkwood_set_rate(struct snd_soc_dai *dai,
 		clk_set_rate(priv->extclk, 256 * rate);
 
 		clks_ctrl = KIRKWOOD_MCLK_SOURCE_EXTCLK;
+	} else {
+		dev_err(dai->dev, "%s: no clock\n", __func__);
+		return;
 	}
 	writel(clks_ctrl, priv->io + KIRKWOOD_CLOCKS_CTRL);
 }
-- 
1.7.10.4

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCH 1/2] ARM: kirkwood: fix a not initialized variable in the sound subsystem

[Sebastian Hesselbarth]

On 03/26/2013 07:05 PM, Jean-Francois Moine wrote:
> In the function kirkwood_set_rate, in case of a non dco supported rate
> and no external clock, the clock source was set to an undefined value.
> This patch just displays a message without changing the clock source.
>
> Signed-off-by: Jean-Francois Moine<moinejf@free.fr>
> ---
>   sound/soc/kirkwood/kirkwood-i2s.c |    3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
> index c74c890..afca1ec 100644
> --- a/sound/soc/kirkwood/kirkwood-i2s.c
> +++ b/sound/soc/kirkwood/kirkwood-i2s.c
> @@ -118,6 +118,9 @@ static void kirkwood_set_rate(struct snd_soc_dai *dai,
>   		clk_set_rate(priv->extclk, 256 * rate);
>
>   		clks_ctrl = KIRKWOOD_MCLK_SOURCE_EXTCLK;
> +	} else {
> +		dev_err(dai->dev, "%s: no clock\n", __func__);
> +		return;
>   	}
>   	writel(clks_ctrl, priv->io + KIRKWOOD_CLOCKS_CTRL);
>   }

NACK.

Having no clock at all should be catched during _probe. Moreover,
not having the internal clock enabled will lead to system hang due to
clock gating. You should rather pass an optional (DT-only) extclk phandle
on the second clocks property.

Sebastian

P.S. as this is alsa stuff you should have some alsa maintainers on your
Cc list. Please run ./scripts/get_maintainer.pl on your patches next time.
It will give you a list of people you have to to Cc.

[PATCH 1/2] ARM: kirkwood: fix a not initialized variable in the sound subsystem

[Jean-Francois Moine]

On Tue, 26 Mar 2013 20:41:40 +0100
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:

> On 03/26/2013 07:05 PM, Jean-Francois Moine wrote:
> > In the function kirkwood_set_rate, in case of a non dco supported rate
> > and no external clock, the clock source was set to an undefined value.
> > This patch just displays a message without changing the clock source.
> >
> > Signed-off-by: Jean-Francois Moine<moinejf@free.fr>
> > ---
> >   sound/soc/kirkwood/kirkwood-i2s.c |    3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
> > index c74c890..afca1ec 100644
> > --- a/sound/soc/kirkwood/kirkwood-i2s.c
> > +++ b/sound/soc/kirkwood/kirkwood-i2s.c
> > @@ -118,6 +118,9 @@ static void kirkwood_set_rate(struct snd_soc_dai *dai,
> >   		clk_set_rate(priv->extclk, 256 * rate);
> >
> >   		clks_ctrl = KIRKWOOD_MCLK_SOURCE_EXTCLK;
> > +	} else {
> > +		dev_err(dai->dev, "%s: no clock\n", __func__);
> > +		return;
> >   	}
> >   	writel(clks_ctrl, priv->io + KIRKWOOD_CLOCKS_CTRL);
> >   }
> 
> NACK.
> 
> Having no clock at all should be catched during _probe. Moreover,
> not having the internal clock enabled will lead to system hang due to
> clock gating. You should rather pass an optional (DT-only) extclk phandle
> on the second clocks property.
> 
> Sebastian
> 
> P.S. as this is alsa stuff you should have some alsa maintainers on your
> Cc list. Please run ./scripts/get_maintainer.pl on your patches next time.
> It will give you a list of people you have to to Cc.

It is a compilation error: the variable clks_ctrl is not initialized.

The sequence has been created by the commit 363589bf110aa0352a2031
   "ASoC: kirkwood-i2s: add support for external clock rates"
Russell is in the Cc list.

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCH 1/2] ARM: kirkwood: fix a not initialized variable in the sound subsystem

[Sebastian Hesselbarth]

On 03/26/2013 09:05 PM, Jean-Francois Moine wrote:
> On Tue, 26 Mar 2013 20:41:40 +0100
> Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>  wrote:
>
>> On 03/26/2013 07:05 PM, Jean-Francois Moine wrote:
>>> In the function kirkwood_set_rate, in case of a non dco supported rate
>>> and no external clock, the clock source was set to an undefined value.
>>> This patch just displays a message without changing the clock source.
>>>
>>> Signed-off-by: Jean-Francois Moine<moinejf@free.fr>
>>> ---
>>>    sound/soc/kirkwood/kirkwood-i2s.c |    3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
>>> index c74c890..afca1ec 100644
>>> --- a/sound/soc/kirkwood/kirkwood-i2s.c
>>> +++ b/sound/soc/kirkwood/kirkwood-i2s.c
>>> @@ -118,6 +118,9 @@ static void kirkwood_set_rate(struct snd_soc_dai *dai,
>>>    		clk_set_rate(priv->extclk, 256 * rate);
>>>
>>>    		clks_ctrl = KIRKWOOD_MCLK_SOURCE_EXTCLK;
>>> +	} else {
>>> +		dev_err(dai->dev, "%s: no clock\n", __func__);
>>> +		return;
>>>    	}
>>>    	writel(clks_ctrl, priv->io + KIRKWOOD_CLOCKS_CTRL);
>>>    }
>>
>> NACK.
>>
>> Having no clock at all should be catched during _probe. Moreover,
>> not having the internal clock enabled will lead to system hang due to
>> clock gating. You should rather pass an optional (DT-only) extclk phandle
>> on the second clocks property.
>>
>> Sebastian
>>
>> P.S. as this is alsa stuff you should have some alsa maintainers on your
>> Cc list. Please run ./scripts/get_maintainer.pl on your patches next time.
>> It will give you a list of people you have to to Cc.
>
> It is a compilation error: the variable clks_ctrl is not initialized.

If it is about non-initialized clks_ctrl, I suggest to remove it
completely and clone the writel to both if and else branch.

> The sequence has been created by the commit 363589bf110aa0352a2031
>     "ASoC: kirkwood-i2s: add support for external clock rates"
> Russell is in the Cc list.

Russell was the commit author and also should be added because of his
ARM maintainer status. But kirkwood-i2s is alsa and needs some sound
maintainers to be Cc'd:

$ ./scripts/get_maintainer.pl -f sound/soc/kirkwood/kirkwood-i2s.c
Liam Girdwood <lgirdwood@gmail.com> (supporter:SOUND - SOC LAYER...)
Mark Brown <broonie@opensource.wolfsonmicro.com> (supporter:SOUND - SOC 
LAYER...,commit_signer:9/11=82%)
Jaroslav Kysela <perex@perex.cz> (maintainer:SOUND)
Takashi Iwai <tiwai@suse.de> (maintainer:SOUND)
Russell King <rmk+kernel@arm.linux.org.uk> (commit_signer:6/11=55%)
Greg Kroah-Hartman <gregkh@linuxfoundation.org> (commit_signer:2/11=18%)
Andrew Lumm <andrew@lunn.ch> (commit_signer:2/11=18%)
Thierry Reding <thierry.reding@avionic-design.de> (commit_signer:1/11=9%)
alsa-devel at alsa-project.org (moderated list:SOUND - SOC LAYER...)
linux-kernel at vger.kernel.org (open list)

Sebastian

[PATCH 1/2] ARM: kirkwood: fix a not initialized variable in the sound subsystem

[Sebastian Hesselbarth]

On 03/26/2013 09:05 PM, Jean-Francois Moine wrote:
> On Tue, 26 Mar 2013 20:41:40 +0100
> Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>  wrote:
>
>> On 03/26/2013 07:05 PM, Jean-Francois Moine wrote:
>>> In the function kirkwood_set_rate, in case of a non dco supported rate
>>> and no external clock, the clock source was set to an undefined value.
>>> This patch just displays a message without changing the clock source.
>>>
>>> Signed-off-by: Jean-Francois Moine<moinejf@free.fr>
>>> ---
>>>    sound/soc/kirkwood/kirkwood-i2s.c |    3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
>>> index c74c890..afca1ec 100644
>>> --- a/sound/soc/kirkwood/kirkwood-i2s.c
>>> +++ b/sound/soc/kirkwood/kirkwood-i2s.c
>>> @@ -118,6 +118,9 @@ static void kirkwood_set_rate(struct snd_soc_dai *dai,
>>>    		clk_set_rate(priv->extclk, 256 * rate);
>>>
>>>    		clks_ctrl = KIRKWOOD_MCLK_SOURCE_EXTCLK;
>>> +	} else {
>>> +		dev_err(dai->dev, "%s: no clock\n", __func__);
>>> +		return;
>>>    	}
>>>    	writel(clks_ctrl, priv->io + KIRKWOOD_CLOCKS_CTRL);
>>>    }
>>
>> NACK.
>>
>> Having no clock at all should be catched during _probe. Moreover,
>> not having the internal clock enabled will lead to system hang due to
>> clock gating. You should rather pass an optional (DT-only) extclk phandle
>> on the second clocks property.

Jean-Francois,

I had a close look at the code and your patch. From a driver
point-of-view kirkwood_set_rate should never been called with
an unsupported rate (see KIRKWOOD_I2S_RATES) when no extclk
is available. With extclk available, the else-if branch will
be taken on rates != KIRKWOOD_I2S_RATES. Actually, your added
else branch will never be taken at all.

I suggest (again) to remove clks_ctrl and move the writel inside
if and else-if branch to cure the compiler warning.

Sebastian

[PATCH 1/2] ARM: kirkwood: fix a not initialized variable in the sound subsystem

[Jean-Francois Moine]

On Tue, 26 Mar 2013 21:39:40 +0100
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:

> On 03/26/2013 09:05 PM, Jean-Francois Moine wrote:
> > On Tue, 26 Mar 2013 20:41:40 +0100
> > Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>  wrote:
> >
> >> On 03/26/2013 07:05 PM, Jean-Francois Moine wrote:
> >>> In the function kirkwood_set_rate, in case of a non dco supported rate
> >>> and no external clock, the clock source was set to an undefined value.
> >>> This patch just displays a message without changing the clock source.
> >>>
> >>> Signed-off-by: Jean-Francois Moine<moinejf@free.fr>
> >>> ---
> >>>    sound/soc/kirkwood/kirkwood-i2s.c |    3 +++
> >>>    1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
> >>> index c74c890..afca1ec 100644
> >>> --- a/sound/soc/kirkwood/kirkwood-i2s.c
> >>> +++ b/sound/soc/kirkwood/kirkwood-i2s.c
> >>> @@ -118,6 +118,9 @@ static void kirkwood_set_rate(struct snd_soc_dai *dai,
> >>>    		clk_set_rate(priv->extclk, 256 * rate);
> >>>
> >>>    		clks_ctrl = KIRKWOOD_MCLK_SOURCE_EXTCLK;
> >>> +	} else {
> >>> +		dev_err(dai->dev, "%s: no clock\n", __func__);
> >>> +		return;
> >>>    	}
> >>>    	writel(clks_ctrl, priv->io + KIRKWOOD_CLOCKS_CTRL);
> >>>    }
> >>
> >> NACK.
> >>
> >> Having no clock at all should be catched during _probe. Moreover,
> >> not having the internal clock enabled will lead to system hang due to
> >> clock gating. You should rather pass an optional (DT-only) extclk phandle
> >> on the second clocks property.
> 
> Jean-Francois,
> 
> I had a close look at the code and your patch. From a driver
> point-of-view kirkwood_set_rate should never been called with
> an unsupported rate (see KIRKWOOD_I2S_RATES) when no extclk
> is available. With extclk available, the else-if branch will
> be taken on rates != KIRKWOOD_I2S_RATES. Actually, your added
> else branch will never be taken at all.
> 
> I suggest (again) to remove clks_ctrl and move the writel inside
> if and else-if branch to cure the compiler warning.
> 
> Sebastian

That's what there was in the original patch from Rabeeh, but maybe it
is the opportunity to use WARN_ON. Russell?

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCH 1/2] ARM: kirkwood: fix a not initialized variable in the sound subsystem

[Russell King - ARM Linux]

On Wed, Mar 27, 2013 at 08:31:57AM +0100, Jean-Francois Moine wrote:
> On Tue, 26 Mar 2013 21:39:40 +0100
> Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:
> > I suggest (again) to remove clks_ctrl and move the writel inside
> > if and else-if branch to cure the compiler warning.
> > 
> > Sebastian
> 
> That's what there was in the original patch from Rabeeh, but maybe it
> is the opportunity to use WARN_ON. Russell?

Sebastian is correct in that such a path should _never_ be reached
because ALSA will reject anything but 44.1, 48 or 96kHz rates if we
don't have an extclk.

However, I disagree with Sebastian's solution - doing that is likely to
generate more code because gcc tends not to optimise:

	if (foo) {
		writel(some_value, register);
	} else {
		writel(some_other_value, register);
	}

very well.  It will generate two separate writel()s when in fact, it
could just do:

	if (foo) {
		val = some_value;
	} else {
		val = some_other_value;
	}
	writel(val, register);

One solution to this would be to just get rid of "if (!IS_ERR(priv->extclk))"
entirely, so that the "else" clause always assumes that if we hit that,
there will be an external clock.  If it does, the clk API gets to deal
with being passed an error pointer for a clock, and we switch to extclk
mode.  Either that'll cause a nice backtrace from the kernel (which is
good in this case) or audio will just not work.

Remember, though - if there isn't an extclk set, then we should never
get there in the first place.  If it makes people feel happier, also put
a WARN_ON(IS_ERR(priv->extclk)) there to guarantee a backtrace if it
happens.