Re: [PATCH v4 1/2] platform/x86: Add Uniwill laptop driver

[Armin Wolf <W_Armin@gmx.de>]

Am 03.10.25 um 01:36 schrieb Nathan Chancellor:

> Hi Armin,
>
> On Thu, Oct 02, 2025 at 08:41:19PM +0200, Armin Wolf wrote:
>> i think this is a problem inside the clang compiler. I did not encounter this warning when
>> build for x86-64 using gcc.
> Clang is actually saving you from yourself, it is a bug in GCC that it
> does not warn for this:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91951

Oh my, i didn't expect that. Thank you for explaining this issue to to me,
it seems that i still have much to learn.

Thanks,
Armin Wolf

>>> vim +1243 drivers/platform/x86/uniwill/uniwill-acpi.c
>>>
>>>     1235	
>>>     1236	static int uniwill_notifier_call(struct notifier_block *nb, unsigned long action, void *dummy)
>>>     1237	{
>>>     1238		struct uniwill_data *data = container_of(nb, struct uniwill_data, nb);
>>>     1239		struct uniwill_battery_entry *entry;
>>>     1240	
>>>     1241		switch (action) {
>>>     1242		case UNIWILL_OSD_BATTERY_ALERT:
>>>> 1243			guard(mutex)(&data->battery_lock);
> mutex_unlock() will be called on &data->battery_lock even when the
> default case is taken, as demonstrated by the following test case.
>
>>>     1244			list_for_each_entry(entry, &data->batteries, head) {
>>>     1245				power_supply_changed(entry->battery);
>>>     1246			}
>>>     1247	
>>>     1248			return NOTIFY_OK;
>>>     1249		default:
>>>     1250			guard(mutex)(&data->input_lock);
>>>     1251			sparse_keymap_report_event(data->input_device, action, 1, true);
>>>     1252	
>>>     1253			return NOTIFY_OK;
>>>     1254		}
>>>     1255	}
>>>     1256	
>>>
> $ cat test.c
> #include <stdio.h>
>
> void cleanup_1(int *a) { printf("+ %s(%p)\n", __func__, a); }
> void cleanup_2(int *a) { printf("+ %s(%p)\n", __func__, a); }
> void cleanup_3(int *a) { printf("+ %s(%p)\n", __func__, a); }
>
> void no_scopes(int a)
> {
>      printf("%s(%d)\n", __func__, a);
>      switch (a) {
>      case 1:
>          int case_1 __attribute__((cleanup(cleanup_1)));
>          return;
>      case 2:
>          int case_2 __attribute__((cleanup(cleanup_2)));
>          return;
>      default:
>          int case_default __attribute__((cleanup(cleanup_3)));
>          return;
>      }
> }
>
> void with_scopes(int a)
> {
>      printf("%s(%d)\n", __func__, a);
>      switch (a) {
>      case 1: {
>          int case_1 __attribute__((cleanup(cleanup_1)));
>          return;
>      }
>      case 2: {
>          int case_2 __attribute__((cleanup(cleanup_2)));
>          return;
>      }
>      default: {
>          int case_default __attribute__((cleanup(cleanup_3)));
>          return;
>      }
>      }
> }
>
> int main(void)
> {
>      no_scopes(1); printf("\n");
>      no_scopes(2); printf("\n");
>      no_scopes(3); printf("\n");
>
>      with_scopes(1); printf("\n");
>      with_scopes(2); printf("\n");
>      with_scopes(3);
> }
>
> $ gcc -O2 test.c
>
> $ ./a.out
> no_scopes(1)
> + cleanup_1(0x7ffea3450c0c)
>
> no_scopes(2)
> + cleanup_2(0x7ffea3450c10)
> + cleanup_1(0x7ffea3450c0c)
>
> no_scopes(3)
> + cleanup_3(0x7ffea3450c14)
> + cleanup_2(0x7ffea3450c10)
> + cleanup_1(0x7ffea3450c0c)
>
> with_scopes(1)
> + cleanup_1(0x7ffea3450c14)
>
> with_scopes(2)
> + cleanup_2(0x7ffea3450c14)
>
> with_scopes(3)
> + cleanup_3(0x7ffea3450c14)
>
> $ clang -O2 test.c
> test.c:12:9: warning: label followed by a declaration is a C23 extension [-Wc23-extensions]
>     12 |         int case_1 __attribute__((cleanup(cleanup_1)));
>        |         ^
> test.c:15:9: warning: label followed by a declaration is a C23 extension [-Wc23-extensions]
>     15 |         int case_2 __attribute__((cleanup(cleanup_2)));
>        |         ^
> test.c:18:9: warning: label followed by a declaration is a C23 extension [-Wc23-extensions]
>     18 |         int case_default __attribute__((cleanup(cleanup_3)));
>        |         ^
> test.c:17:5: error: cannot jump from switch statement to this case label
>     17 |     default:
>        |     ^
> test.c:15:13: note: jump bypasses initialization of variable with __attribute__((cleanup))
>     15 |         int case_2 __attribute__((cleanup(cleanup_2)));
>        |             ^
> test.c:12:13: note: jump bypasses initialization of variable with __attribute__((cleanup))
>     12 |         int case_1 __attribute__((cleanup(cleanup_1)));
>        |             ^
> test.c:14:5: error: cannot jump from switch statement to this case label
>     14 |     case 2:
>        |     ^
> test.c:12:13: note: jump bypasses initialization of variable with __attribute__((cleanup))
>     12 |         int case_1 __attribute__((cleanup(cleanup_1)));
>        |             ^
> 3 warnings and 2 errors generated.
>
> https://godbolt.org/z/1Tx7Gj1xf
>
> I would add the scoping to the case labels or use scoped_guard() to
> avoid this, which would also avoid the instances of -Wc23-extensions.
>
> Cheers,
> Nathan
>