| Message ID | 8e3df79b1dca92d49fd9d5c97391d31814ab40f9.1533151956.git.gustavo@embeddedor.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2cea1542859bc812f1ec51ea71c06e927e5b922e |
| Headers | show |
| Series | ASoC: codecs: Mark expected switch fall-throughs | expand |
On Wed, 01 Aug 2018 14:56:16 -0500, "Gustavo A. R. Silva" said: > diff --git a/sound/soc/codecs/wm8994.c b/sound/soc/codecs/wm8994.c > index 7fdfdf3..62f8c5b 100644 > --- a/sound/soc/codecs/wm8994.c > +++ b/sound/soc/codecs/wm8994.c > @@ -2432,6 +2432,7 @@ static int wm8994_set_dai_sysclk(struct snd_soc_dai *dai, > snd_soc_component_update_bits(component, WM8994_POWER_MANAGEMENT_2, > WM8994_OPCLK_ENA, 0); > } > + /* fall through */ > > default: > return -EINVAL; Wait, what? This looks like the sort of bug -Wimplicit-fallthrough is supposed to catch. Unless for 'case WM8994_SYSCLK_OPCLK:' we actually do want to do a whole bunch of snd_soc_component_update_bits() calls and then return -EINVAL whether or not that case succeeded?
On 08/03/2018 11:26 AM, valdis.kletnieks@vt.edu wrote: > On Wed, 01 Aug 2018 14:56:16 -0500, "Gustavo A. R. Silva" said: > >> diff --git a/sound/soc/codecs/wm8994.c b/sound/soc/codecs/wm8994.c >> index 7fdfdf3..62f8c5b 100644 >> --- a/sound/soc/codecs/wm8994.c >> +++ b/sound/soc/codecs/wm8994.c >> @@ -2432,6 +2432,7 @@ static int wm8994_set_dai_sysclk(struct snd_soc_dai *dai, >> snd_soc_component_update_bits(component, WM8994_POWER_MANAGEMENT_2, >> WM8994_OPCLK_ENA, 0); >> } >> + /* fall through */ >> >> default: >> return -EINVAL; > > Wait, what? This looks like the sort of bug -Wimplicit-fallthrough is supposed > to catch. Unless for 'case WM8994_SYSCLK_OPCLK:' we actually do want to do a > whole bunch of snd_soc_component_update_bits() calls and then return -EINVAL > whether or not that case succeeded? > > Yeah, it seems like a bug. Can someone confirm this? Notice that this code has been there since 2010. Thanks Valdis for pointing this out. -- Gustavo
On Fri, Aug 03, 2018 at 11:41:39AM -0500, Gustavo A. R. Silva wrote: > On 08/03/2018 11:26 AM, valdis.kletnieks@vt.edu wrote: > > On Wed, 01 Aug 2018 14:56:16 -0500, "Gustavo A. R. Silva" said: > > Wait, what? This looks like the sort of bug -Wimplicit-fallthrough is supposed > > to catch. Unless for 'case WM8994_SYSCLK_OPCLK:' we actually do want to do a > > whole bunch of snd_soc_component_update_bits() calls and then return -EINVAL > > whether or not that case succeeded? > Yeah, it seems like a bug. Can someone confirm this? > Notice that this code has been there since 2010. Basically nobody ever uses OPCLK so I'd be susprised if anyone ever noticed.
On 08/03/2018 11:45 AM, Mark Brown wrote: > On Fri, Aug 03, 2018 at 11:41:39AM -0500, Gustavo A. R. Silva wrote: >> On 08/03/2018 11:26 AM, valdis.kletnieks@vt.edu wrote: >>> On Wed, 01 Aug 2018 14:56:16 -0500, "Gustavo A. R. Silva" said: > >>> Wait, what? This looks like the sort of bug -Wimplicit-fallthrough is supposed >>> to catch. Unless for 'case WM8994_SYSCLK_OPCLK:' we actually do want to do a >>> whole bunch of snd_soc_component_update_bits() calls and then return -EINVAL >>> whether or not that case succeeded? > >> Yeah, it seems like a bug. Can someone confirm this? > >> Notice that this code has been there since 2010. > > Basically nobody ever uses OPCLK so I'd be susprised if anyone ever > noticed. > I see. I wonder what's the best approach in this case. Should that code be removed instead of 'fixed'? Thanks -- Gustavo
On Fri, 03 Aug 2018 11:56:12 -0500, "Gustavo A. R. Silva" said: > On 08/03/2018 11:45 AM, Mark Brown wrote: > > Basically nobody ever uses OPCLK so I'd be susprised if anyone ever > > noticed. I wonder if nobody uses it because any attempts to do so get an error? :) > I see. I wonder what's the best approach in this case. Should that code be > removed instead of 'fixed'? I'm thinking that's a spot that needs a 'break;' added. Of course, my understanding of that code is limited to looking at the first 10 patches and the fall-through hit something reasonable, and then the last one did a fall-through to an error return.
On 08/03/2018 12:55 PM, valdis.kletnieks@vt.edu wrote: > On Fri, 03 Aug 2018 11:56:12 -0500, "Gustavo A. R. Silva" said: >> On 08/03/2018 11:45 AM, Mark Brown wrote: >>> Basically nobody ever uses OPCLK so I'd be susprised if anyone ever >>> noticed. > > I wonder if nobody uses it because any attempts to do so get an error? :) > >> I see. I wonder what's the best approach in this case. Should that code be >> removed instead of 'fixed'? > > I'm thinking that's a spot that needs a 'break;' added. > Yep. And I think the patch to fix this should be tagged for stable. So, it seems we have two options: 1. Revert commit 2cea1542859bc812f1ec51ea71c06e927e5b922e and then apply a fix with the break statement. or 2. Apply a fix with the break statement and then backport the fix to stable. am I correct? Thanks -- Gustavo
On Fri, Aug 03, 2018 at 01:55:18PM -0400, valdis.kletnieks@vt.edu wrote: > On Fri, 03 Aug 2018 11:56:12 -0500, "Gustavo A. R. Silva" said: > > On 08/03/2018 11:45 AM, Mark Brown wrote: > > > Basically nobody ever uses OPCLK so I'd be susprised if anyone ever > > > noticed. > I wonder if nobody uses it because any attempts to do so get an error? :) No, nobody ever physically wires up an OPCLK. If one was there it'd be unavoidable. > > I see. I wonder what's the best approach in this case. Should that code be > > removed instead of 'fixed'? > I'm thinking that's a spot that needs a 'break;' added. Yes.
On 08/06/2018 06:34 AM, Mark Brown wrote: > On Fri, Aug 03, 2018 at 01:55:18PM -0400, valdis.kletnieks@vt.edu wrote: >> On Fri, 03 Aug 2018 11:56:12 -0500, "Gustavo A. R. Silva" said: >>> On 08/03/2018 11:45 AM, Mark Brown wrote: >>>> Basically nobody ever uses OPCLK so I'd be susprised if anyone ever >>>> noticed. > >> I wonder if nobody uses it because any attempts to do so get an error? :) > > No, nobody ever physically wires up an OPCLK. If one was there it'd be > unavoidable. > >>> I see. I wonder what's the best approach in this case. Should that code be >>> removed instead of 'fixed'? > >> I'm thinking that's a spot that needs a 'break;' added. > > Yes. > OK. I'll send a patch for this. But I wonder what's the best approach in this case, so the fix can also be applied to stable. Thanks -- Gustavo
On Mon, Aug 06, 2018 at 06:48:13AM -0500, Gustavo A. R. Silva wrote: Please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to. > OK. I'll send a patch for this. But I wonder what's the best approach in this case, > so the fix can also be applied to stable. Just send a patch, a fixup can be done when the backport fails.
On 08/06/2018 06:52 AM, Mark Brown wrote: > On Mon, Aug 06, 2018 at 06:48:13AM -0500, Gustavo A. R. Silva wrote: > > Please fix your mail client to word wrap within paragraphs at something > substantially less than 80 columns. Doing this makes your messages much > easier to read and reply to. > Sure thing. >> OK. I'll send a patch for this. But I wonder what's the best approach in this case, >> so the fix can also be applied to stable. > > Just send a patch, a fixup can be done when the backport fails. > I got it. I'll send the patch and I'll take care of the backport when the time comes. Thanks -- Gustavo
diff --git a/sound/soc/codecs/wm8994.c b/sound/soc/codecs/wm8994.c index 7fdfdf3..62f8c5b 100644 --- a/sound/soc/codecs/wm8994.c +++ b/sound/soc/codecs/wm8994.c @@ -2432,6 +2432,7 @@ static int wm8994_set_dai_sysclk(struct snd_soc_dai *dai, snd_soc_component_update_bits(component, WM8994_POWER_MANAGEMENT_2, WM8994_OPCLK_ENA, 0); } + /* fall through */ default: return -EINVAL;
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. Addresses-Coverity-ID: 115050 ("Missing break in switch") Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> --- sound/soc/codecs/wm8994.c | 1 + 1 file changed, 1 insertion(+)