In the function kirkwood_set_rate, when the rate cannot be satisfied
by the internal nor by an external clock, the clock source in undefined:
warning: ‘clks_ctrl’ may be used uninitialized in this function
As the ALSA subsystem should never gives such a rate, this patch removes
the check of the external clock pointer.
Signed-off-by: Jean-Francois Moine <[email protected]>
---
sound/soc/kirkwood/kirkwood-i2s.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index 4c9dad3..8da5cdb 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -111,7 +111,7 @@ static void kirkwood_set_rate(struct snd_soc_dai *dai,
kirkwood_set_dco(priv->io, rate);
clks_ctrl = KIRKWOOD_MCLK_SOURCE_DCO;
- } else if (!IS_ERR(priv->extclk)) {
+ } else {
/* use optional external clk for other rates */
dev_dbg(dai->dev, "%s: extclk set rate = %lu -> %lu\n",
__func__, rate, 256 * rate);
On Mon, Jul 15, 2013 at 10:36:44AM +0200, Jean-Francois Moine wrote:
> In the function kirkwood_set_rate, when the rate cannot be satisfied
> by the internal nor by an external clock, the clock source in undefined:
> warning: ‘clks_ctrl’ may be used uninitialized in this function
> As the ALSA subsystem should never gives such a rate, this patch removes
> the check of the external clock pointer.
> - } else if (!IS_ERR(priv->extclk)) {
> + } else {
I'd really like to see an analysis explaining why this can never happen,
the driver explicitly supports running without extclk being provided.
Simply asserting that we should never get such a rate isn't really
enough detail...
On Mon, 15 Jul 2013 16:31:01 +0100
Mark Brown <[email protected]> wrote:
> On Mon, Jul 15, 2013 at 10:36:44AM +0200, Jean-Francois Moine wrote:
> > In the function kirkwood_set_rate, when the rate cannot be satisfied
> > by the internal nor by an external clock, the clock source in undefined:
>
> > warning: ‘clks_ctrl’ may be used uninitialized in this function
>
> > As the ALSA subsystem should never gives such a rate, this patch removes
> > the check of the external clock pointer.
>
> > - } else if (!IS_ERR(priv->extclk)) {
> > + } else {
>
> I'd really like to see an analysis explaining why this can never happen,
> the driver explicitly supports running without extclk being provided.
> Simply asserting that we should never get such a rate isn't really
> enough detail...
Russell explained this in the message below dated Wed, 27 Mar 2013
(http://www.spinics.net/lists/arm-kernel/msg233819.html)
--------------- Russell's message -----------------
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 <[email protected]> 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.
---------------- end message ---------------
--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
On Mon, Jul 15, 2013 at 08:37:56PM +0200, Jean-Francois Moine wrote:
> Mark Brown <[email protected]> wrote:
> > I'd really like to see an analysis explaining why this can never happen,
> > the driver explicitly supports running without extclk being provided.
> > Simply asserting that we should never get such a rate isn't really
> > enough detail...
> Russell explained this in the message below dated Wed, 27 Mar 2013
> (http://www.spinics.net/lists/arm-kernel/msg233819.html)
This is no good, the information needs to be in the commit message.
Right now the change just looks like a bug supported by wishful
thinking, you're not providing enough analysis and inspection of the
code suggests a bug.
> 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.
There's no obvious code that handles anything differently with extclk.
Indeed if you think about it for a minute you'll realise there's no way
the driver will ever use an extclk - set_rate() is badly implemented,
look at how other drivers select between clocks.
Fixing the driver so it can make use of an extclk would be more
useful...
On Mon, Jul 15, 2013 at 09:08:51PM +0100, Mark Brown wrote:
> There's no obvious code that handles anything differently with extclk.
> Indeed if you think about it for a minute you'll realise there's no way
> the driver will ever use an extclk - set_rate() is badly implemented,
> look at how other drivers select between clocks.
I suggest you go back and re-read the driver because it most certainly
does use extclk. What makes you think it won't?
On Mon, Jul 15, 2013 at 10:37:27PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jul 15, 2013 at 09:08:51PM +0100, Mark Brown wrote:
> > There's no obvious code that handles anything differently with extclk.
> > Indeed if you think about it for a minute you'll realise there's no way
> > the driver will ever use an extclk - set_rate() is badly implemented,
> > look at how other drivers select between clocks.
Actually looking some more it's not actually a normal set_rate() call
but rather something done transparently inside the driver - given that
the automatic source selection is OK.
> I suggest you go back and re-read the driver because it most certainly
> does use extclk. What makes you think it won't?
The only thing I can see which is pushing a constraint up the stack is
KIRKWOOD_I2S_RATES in the DAIs which only allows 44.1kHz, 48kHz and
96kHz, the rates for which the internal clock is used. The driver will
happily request the external clock and hold a reference to it but if
there's code there to tell the upper layers that extra rates are
supported I'm missing it and without that any attempt to use anything
else should be rejected by the stack.
Note that regular ALSA will do some sample rate conversions in software
automatically so if you're not testing with something like "aplay
-Dhw:0,0" that bypasses those then the hardware may not be running at
the same rate as the application.
On Mon, Jul 15, 2013 at 11:58:36PM +0100, Mark Brown wrote:
> The only thing I can see which is pushing a constraint up the stack is
> KIRKWOOD_I2S_RATES in the DAIs which only allows 44.1kHz, 48kHz and
> 96kHz, the rates for which the internal clock is used.
Take a closer look, because you are mistaken.
Particularly note how there are two struct snd_soc_dai_driver's, one
which gets used if we don't have an external clock, and the other
which does. They differ in their .rates initializer.
On Mon, Jul 15, 2013 at 11:58:36PM +0100, Mark Brown wrote:
> On Mon, Jul 15, 2013 at 10:37:27PM +0100, Russell King - ARM Linux wrote:
> > I suggest you go back and re-read the driver because it most certainly
> > does use extclk. What makes you think it won't?
> The only thing I can see which is pushing a constraint up the stack is
> KIRKWOOD_I2S_RATES in the DAIs which only allows 44.1kHz, 48kHz and
> 96kHz, the rates for which the internal clock is used. The driver will
Ugh, I just saw the dai_extclk stuff. That's probably also problematic
the other way - it'll break if someone decides to put a non-programmable
clock on there, or something that can't generate arbatrary frequencies
like a crystal plus divider. It's also not clear to me why the driver
ever uses the internal clocks if something external has been provided.
This does mean the change is safe, though - but it needs to be called
out in both the code and the changelog how this is happening.