[alsa-devel] [PATCH] ASoC: kirkwood-i2s: fix a compilation warning
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 moinejf@free.fr --- 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 broonie@kernel.org 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 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. ---------------- end message ---------------
On Mon, Jul 15, 2013 at 08:37:56PM +0200, Jean-Francois Moine wrote:
Mark Brown broonie@kernel.org 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.
participants (3)
-
Jean-Francois Moine -
Mark Brown -
Russell King - ARM Linux