[PATCH] platform/x86: acer-wmi: Mark expected switch fall-throughs

[PATCH] platform/x86: acer-wmi: Mark expected switch fall-throughs

[Gustavo A. R. Silva]

In preparation to enabling -Wimplicit-fallthrough, mark switch
cases where we are expecting to fall through.

This patch fixes the following warnings:

drivers/platform/x86/acer-wmi.c: In function ‘set_u32’:
drivers/platform/x86/acer-wmi.c:1378:33: warning: this statement may fall through [-Wimplicit-fallthrough=]
    if (cap == ACER_CAP_WIRELESS ||
                                 ^
drivers/platform/x86/acer-wmi.c:1386:3: note: here
   case ACER_WMID:
   ^~~~
drivers/platform/x86/acer-wmi.c:1393:12: warning: this statement may fall through [-Wimplicit-fallthrough=]
    else if (wmi_has_guid(WMID_GUID2))
            ^
drivers/platform/x86/acer-wmi.c:1395:3: note: here
   default:
   ^~~~~~~
drivers/platform/x86/acer-wmi.c: In function ‘get_u32’:
drivers/platform/x86/acer-wmi.c:1340:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
   if (cap == ACER_CAP_MAILLED) {
      ^
drivers/platform/x86/acer-wmi.c:1344:2: note: here
  case ACER_WMID:
  ^~~~
drivers/platform/x86/acer-wmi.c: In function ‘WMID_get_u32’:
drivers/platform/x86/acer-wmi.c:1013:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
   if (quirks->mailled == 1) {
      ^
drivers/platform/x86/acer-wmi.c:1018:2: note: here
  default:
  ^~~~~~~

Warning level 3 was used: -Wimplicit-fallthrough=3

This patch is part of the ongoing efforts to enable
-Wimplicit-fallthrough.

Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/platform/x86/acer-wmi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index fcfeadd1301f..bd87f9037f95 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -1015,6 +1015,7 @@ static acpi_status WMID_get_u32(u32 *value, u32 cap)
 			*value = tmp & 0x1;
 			return 0;
 		}
+		/* fall through */
 	default:
 		return AE_ERROR;
 	}
@@ -1341,6 +1342,7 @@ static acpi_status get_u32(u32 *value, u32 cap)
 			status = AMW0_get_u32(value, cap);
 			break;
 		}
+		/* fall through */
 	case ACER_WMID:
 		status = WMID_get_u32(value, cap);
 		break;
@@ -1383,6 +1385,7 @@ static acpi_status set_u32(u32 value, u32 cap)
 
 				return AMW0_set_u32(value, cap);
 			}
+			/* fall through */
 		case ACER_WMID:
 			return WMID_set_u32(value, cap);
 		case ACER_WMID_v2:
@@ -1392,6 +1395,7 @@ static acpi_status set_u32(u32 value, u32 cap)
 				return wmid_v2_set_u32(value, cap);
 			else if (wmi_has_guid(WMID_GUID2))
 				return WMID_set_u32(value, cap);
+			/* fall through */
 		default:
 			return AE_BAD_PARAMETER;
 		}
-- 
2.21.0

Re: [PATCH] platform/x86: acer-wmi: Mark expected switch fall-throughs

[Darren Hart]

On Wed, May 08, 2019 at 11:49:34AM -0500, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch
> cases where we are expecting to fall through.
> 
> This patch fixes the following warnings:
> 
> drivers/platform/x86/acer-wmi.c: In function ‘set_u32’:
> drivers/platform/x86/acer-wmi.c:1378:33: warning: this statement may fall through [-Wimplicit-fallthrough=]
>     if (cap == ACER_CAP_WIRELESS ||
>                                  ^
> drivers/platform/x86/acer-wmi.c:1386:3: note: here
>    case ACER_WMID:
>    ^~~~
> drivers/platform/x86/acer-wmi.c:1393:12: warning: this statement may fall through [-Wimplicit-fallthrough=]
>     else if (wmi_has_guid(WMID_GUID2))
>             ^
> drivers/platform/x86/acer-wmi.c:1395:3: note: here
>    default:
>    ^~~~~~~
> drivers/platform/x86/acer-wmi.c: In function ‘get_u32’:
> drivers/platform/x86/acer-wmi.c:1340:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
>    if (cap == ACER_CAP_MAILLED) {
>       ^
> drivers/platform/x86/acer-wmi.c:1344:2: note: here
>   case ACER_WMID:
>   ^~~~
> drivers/platform/x86/acer-wmi.c: In function ‘WMID_get_u32’:
> drivers/platform/x86/acer-wmi.c:1013:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
>    if (quirks->mailled == 1) {
>       ^
> drivers/platform/x86/acer-wmi.c:1018:2: note: here
>   default:
>   ^~~~~~~
> 
> Warning level 3 was used: -Wimplicit-fallthrough=3
> 
> This patch is part of the ongoing efforts to enable
> -Wimplicit-fallthrough.
> 
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
>  drivers/platform/x86/acer-wmi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index fcfeadd1301f..bd87f9037f95 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -1015,6 +1015,7 @@ static acpi_status WMID_get_u32(u32 *value, u32 cap)
>  			*value = tmp & 0x1;
>  			return 0;
>  		}
> +		/* fall through */
>  	default:
>  		return AE_ERROR;
>  	}
> @@ -1341,6 +1342,7 @@ static acpi_status get_u32(u32 *value, u32 cap)
>  			status = AMW0_get_u32(value, cap);
>  			break;
>  		}
> +		/* fall through */

This doesn't strike me as obviously the right thing to do here. If the interface
type is AMW0_V2, why is it the right thing to do to use WMID_get_u32 if the cap
isn't ACER_CAP_MAILLED?

>  	case ACER_WMID:
>  		status = WMID_get_u32(value, cap);
>  		break;
> @@ -1383,6 +1385,7 @@ static acpi_status set_u32(u32 value, u32 cap)
>  
>  				return AMW0_set_u32(value, cap);
>  			}
> +			/* fall through */

Similarly here.

Are we documenting intended behavior, or covering up a bug.

>  		case ACER_WMID:
>  			return WMID_set_u32(value, cap);

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH] platform/x86: acer-wmi: Mark expected switch fall-throughs

[Gustavo A. R. Silva]

Darren,

Please, see my comments below...

On 5/8/19 6:06 PM, Darren Hart wrote:
> On Wed, May 08, 2019 at 11:49:34AM -0500, Gustavo A. R. Silva wrote:
>> In preparation to enabling -Wimplicit-fallthrough, mark switch
>> cases where we are expecting to fall through.
>>
>> This patch fixes the following warnings:
>>
>> drivers/platform/x86/acer-wmi.c: In function ‘set_u32’:
>> drivers/platform/x86/acer-wmi.c:1378:33: warning: this statement may fall through [-Wimplicit-fallthrough=]
>>     if (cap == ACER_CAP_WIRELESS ||
>>                                  ^
>> drivers/platform/x86/acer-wmi.c:1386:3: note: here
>>    case ACER_WMID:
>>    ^~~~
>> drivers/platform/x86/acer-wmi.c:1393:12: warning: this statement may fall through [-Wimplicit-fallthrough=]
>>     else if (wmi_has_guid(WMID_GUID2))
>>             ^
>> drivers/platform/x86/acer-wmi.c:1395:3: note: here
>>    default:
>>    ^~~~~~~
>> drivers/platform/x86/acer-wmi.c: In function ‘get_u32’:
>> drivers/platform/x86/acer-wmi.c:1340:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
>>    if (cap == ACER_CAP_MAILLED) {
>>       ^
>> drivers/platform/x86/acer-wmi.c:1344:2: note: here
>>   case ACER_WMID:
>>   ^~~~
>> drivers/platform/x86/acer-wmi.c: In function ‘WMID_get_u32’:
>> drivers/platform/x86/acer-wmi.c:1013:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
>>    if (quirks->mailled == 1) {
>>       ^
>> drivers/platform/x86/acer-wmi.c:1018:2: note: here
>>   default:
>>   ^~~~~~~
>>
>> Warning level 3 was used: -Wimplicit-fallthrough=3
>>
>> This patch is part of the ongoing efforts to enable
>> -Wimplicit-fallthrough.
>>
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>> ---
>>  drivers/platform/x86/acer-wmi.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
>> index fcfeadd1301f..bd87f9037f95 100644
>> --- a/drivers/platform/x86/acer-wmi.c
>> +++ b/drivers/platform/x86/acer-wmi.c
>> @@ -1015,6 +1015,7 @@ static acpi_status WMID_get_u32(u32 *value, u32 cap)
>>  			*value = tmp & 0x1;
>>  			return 0;
>>  		}
>> +		/* fall through */
>>  	default:
>>  		return AE_ERROR;
>>  	}
>> @@ -1341,6 +1342,7 @@ static acpi_status get_u32(u32 *value, u32 cap)
>>  			status = AMW0_get_u32(value, cap);
>>  			break;
>>  		}
>> +		/* fall through */
> 
> This doesn't strike me as obviously the right thing to do here. If the interface
> type is AMW0_V2, why is it the right thing to do to use WMID_get_u32 if the cap
> isn't ACER_CAP_MAILLED?
> 
In commit commit 745a5d2126926808295742932d0e36d485efa485 case ACER_AMW0_V2 falls
through to case ACER_WMID deliberately in function set_u32(), without reporting
any error or warning. So, I thought it was fair to assume that the fall-through
is intentional in both functions get_u32() and set_u32(). Otherwise I would
expect to see a message indicating that interface ACER_AMW0_V2 is unavailable
in function set_u32().

This is also complemented by the following...

>>  	case ACER_WMID:
>>  		status = WMID_get_u32(value, cap);
>>  		break;
>> @@ -1383,6 +1385,7 @@ static acpi_status set_u32(u32 value, u32 cap)
>>  
>>  				return AMW0_set_u32(value, cap);
>>  			}
>> +			/* fall through */
> 
> Similarly here.
> 
> Are we documenting intended behavior, or covering up a bug.
> 

Commit 5c742b45dd5fbbb6cf74d3378341704f4b23c5e8 mentions that "This was fixed
in acer_acpi some time ago, but I forgot to port the patch over to acer-wmi
when it was merged." Notice that this driver (acer-wmi) is based on the
no-longer existing acer_acpi driver. But after googling for a while I could
found the fix the original author talks about:

https://repo.or.cz/acer_acpi.git/commitdiff/74c08a38875ffa9989c3100947650ac8a388c189

So, the fix is indeed similar and contains the same fall-throughs from case
ACER_AMW0_V2 to case ACER_WMID in both functions get_u32() and set_u32().

Thanks
--
Gustavo

Re: [PATCH] platform/x86: acer-wmi: Mark expected switch fall-throughs

[Andy Shevchenko]

On Thu, May 9, 2019 at 4:48 AM Gustavo A. R. Silva
<gustavo@embeddedor.com> wrote:

> Commit 5c742b45dd5fbbb6cf74d3378341704f4b23c5e8 mentions that "This was fixed
> in acer_acpi some time ago, but I forgot to port the patch over to acer-wmi
> when it was merged." Notice that this driver (acer-wmi) is based on the
> no-longer existing acer_acpi driver. But after googling for a while I could
> found the fix the original author talks about:
>
> https://repo.or.cz/acer_acpi.git/commitdiff/74c08a38875ffa9989c3100947650ac8a388c189
>
> So, the fix is indeed similar and contains the same fall-throughs from case
> ACER_AMW0_V2 to case ACER_WMID in both functions get_u32() and set_u32().

Pushed to my review and testing queue, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: acer-wmi: mark expected switch fall-throughs

[Darren Hart]

On Thu, Jul 05, 2018 at 03:42:21PM -0500, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
> 
> Warning level 2 was used: -Wimplicit-fallthrough=2
> 
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
>  drivers/platform/x86/acer-wmi.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
> index 8952173..114b028 100644
> --- a/drivers/platform/x86/acer-wmi.c
> +++ b/drivers/platform/x86/acer-wmi.c
> @@ -1018,6 +1018,7 @@ static acpi_status WMID_get_u32(u32 *value, u32 cap)
>  			*value = tmp & 0x1;
>  			return 0;
>  		}
> +		/* else: fall through */
>  	default:
>  		return AE_ERROR;
>  	}
> @@ -1344,6 +1345,7 @@ static acpi_status get_u32(u32 *value, u32 cap)
>  			status = AMW0_get_u32(value, cap);
>  			break;
>  		}
> +		/* else: fall through */
>  	case ACER_WMID:
>  		status = WMID_get_u32(value, cap);
>  		break;
> @@ -1386,6 +1388,7 @@ static acpi_status set_u32(u32 value, u32 cap)
>  
>  				return AMW0_set_u32(value, cap);
>  			}
> +			/* else: fall through */

I suspect you are correct, bu these last two weren't obviously intentional to
me. Has this seen any testing?

-- 
Darren Hart
VMware Open Source Technology Center

[PATCH] platform/x86: acer-wmi: mark expected switch fall-throughs

[Gustavo A. R. Silva]

In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.

Warning level 2 was used: -Wimplicit-fallthrough=2

Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/platform/x86/acer-wmi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c
index 8952173..114b028 100644
--- a/drivers/platform/x86/acer-wmi.c
+++ b/drivers/platform/x86/acer-wmi.c
@@ -1018,6 +1018,7 @@ static acpi_status WMID_get_u32(u32 *value, u32 cap)
 			*value = tmp & 0x1;
 			return 0;
 		}
+		/* else: fall through */
 	default:
 		return AE_ERROR;
 	}
@@ -1344,6 +1345,7 @@ static acpi_status get_u32(u32 *value, u32 cap)
 			status = AMW0_get_u32(value, cap);
 			break;
 		}
+		/* else: fall through */
 	case ACER_WMID:
 		status = WMID_get_u32(value, cap);
 		break;
@@ -1386,6 +1388,7 @@ static acpi_status set_u32(u32 value, u32 cap)
 
 				return AMW0_set_u32(value, cap);
 			}
+			/* else: fall through */
 		case ACER_WMID:
 			return WMID_set_u32(value, cap);
 		case ACER_WMID_v2:
@@ -1395,6 +1398,7 @@ static acpi_status set_u32(u32 value, u32 cap)
 				return wmid_v2_set_u32(value, cap);
 			else if (wmi_has_guid(WMID_GUID2))
 				return WMID_set_u32(value, cap);
+			/* else: fall through */
 		default:
 			return AE_BAD_PARAMETER;
 		}
-- 
2.7.4