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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20251002233627.GA3978676@ax162>
Date: Thu, 2 Oct 2025 16:36:27 -0700
From: Nathan Chancellor <nathan@...nel.org>
To: Armin Wolf <W_Armin@....de>
Cc: kernel test robot <lkp@...el.com>, ilpo.jarvinen@...ux.intel.com,
	hdegoede@...hat.com, chumuzero@...il.com, corbet@....net,
	cs@...edo.de, wse@...edocomputers.com, ggo@...edocomputers.com,
	llvm@...ts.linux.dev, oe-kbuild-all@...ts.linux.dev,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	platform-driver-x86@...r.kernel.org, rdunlap@...radead.org,
	alok.a.tiwari@...cle.com, linux-leds@...r.kernel.org,
	lee@...nel.org, pobrn@...tonmail.com
Subject: Re: [PATCH v4 1/2] platform/x86: Add Uniwill laptop driver

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

> > 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