[11/11] ASoC: wm8994: Mark expected switch fall-through
diff mbox series

Message ID 8e3df79b1dca92d49fd9d5c97391d31814ab40f9.1533151956.git.gustavo@embeddedor.com
State Accepted
Commit 2cea1542859bc812f1ec51ea71c06e927e5b922e
Headers show
Series
  • ASoC: codecs: Mark expected switch fall-throughs
Related show

Commit Message

Gustavo A. R. Silva Aug. 1, 2018, 7:56 p.m. UTC
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(+)

Comments

Valdis Kl ē tnieks Aug. 3, 2018, 4:26 p.m. UTC | #1
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?
Gustavo A. R. Silva Aug. 3, 2018, 4:41 p.m. UTC | #2
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
Mark Brown Aug. 3, 2018, 4:45 p.m. UTC | #3
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.
Gustavo A. R. Silva Aug. 3, 2018, 4:56 p.m. UTC | #4
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
Valdis Kl ē tnieks Aug. 3, 2018, 5:55 p.m. UTC | #5
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.
Gustavo A. R. Silva Aug. 3, 2018, 6:24 p.m. UTC | #6
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
Mark Brown Aug. 6, 2018, 11:34 a.m. UTC | #7
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.
Gustavo A. R. Silva Aug. 6, 2018, 11:48 a.m. UTC | #8
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
Mark Brown Aug. 6, 2018, 11:52 a.m. UTC | #9
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.
Gustavo A. R. Silva Aug. 6, 2018, 11:59 a.m. UTC | #10
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

Patch
diff mbox series

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;