* [PATCH v3] ACPI: battery: prevent sysfs_add_battery re-entry on rapid events
[not found] <20250910142653.313360-1-luogf2025@163.com>
@ 2025-09-12 15:48 ` GuangFei Luo
2025-09-12 15:57 ` Greg KH
2025-09-13 7:19 ` [PATCH v4] " GuangFei Luo
` (3 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: GuangFei Luo @ 2025-09-12 15:48 UTC (permalink / raw)
To: Rafael J . Wysocki
Cc: Len Brown, linux-acpi, linux-kernel, stable, Sebastian Reichel,
linux-pm, Dan Carpenter, luogf2025, kernel test robot
v3:
- Modified the earlier approach: since sysfs_add_battery() is invoked
from multiple places, the most reliable way is to add the lock inside
the function itself.
- sysfs_remove_battery() had a similar race issue in the past, which was
fixed by adding a lock as well. Reference:
https://lore.kernel.org/all/9c921c22a7f33397a6774d7fa076db9b6a0fd669
.1312318300.git.len.brown@intel.com/
v2:
- Fix missing mutex_unlock in acpi_battery_update()
(Reported-by: kernel test robot)
v1:
When removing and reinserting the laptop battery, ACPI can trigger
two notifications in quick succession:
- ACPI_BATTERY_NOTIFY_STATUS (0x80)
- ACPI_BATTERY_NOTIFY_INFO (0x81)
Both notifications call acpi_battery_update(). Because the events
happen very close in time, sysfs_add_battery() can be re-entered
before battery->bat is set, causing a duplicate sysfs entry error.
This patch ensures that sysfs_add_battery() is not re-entered
when battery->bat is already non-NULL, preventing the duplicate
sysfs creation and stabilizing battery hotplug handling.
[ 476.117945] sysfs: cannot create duplicate filename '/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0A:00/power_supply/BAT1'
[ 476.118896] CPU: 1 UID: 0 PID: 185 Comm: kworker/1:4 Kdump: loaded Not tainted 6.12.38+deb13-amd64 #1 Debian 6.12.38-1
[ 476.118903] Hardware name: Gateway NV44 /SJV40-MV , BIOS V1.3121 04/08/2009
[ 476.118906] Workqueue: kacpi_notify acpi_os_execute_deferred
[ 476.118917] Call Trace:
[ 476.118922] <TASK>
[ 476.118929] dump_stack_lvl+0x5d/0x80
[ 476.118938] sysfs_warn_dup.cold+0x17/0x23
[ 476.118943] sysfs_create_dir_ns+0xce/0xe0
[ 476.118952] kobject_add_internal+0xba/0x250
[ 476.118959] kobject_add+0x96/0xc0
[ 476.118964] ? get_device_parent+0xde/0x1e0
[ 476.118970] device_add+0xe2/0x870
[ 476.118975] __power_supply_register.part.0+0x20f/0x3f0
[ 476.118981] ? wake_up_q+0x4e/0x90
[ 476.118990] sysfs_add_battery+0xa4/0x1d0 [battery]
[ 476.118998] acpi_battery_update+0x19e/0x290 [battery]
[ 476.119002] acpi_battery_notify+0x50/0x120 [battery]
[ 476.119006] acpi_ev_notify_dispatch+0x49/0x70
[ 476.119012] acpi_os_execute_deferred+0x1a/0x30
[ 476.119015] process_one_work+0x177/0x330
[ 476.119022] worker_thread+0x251/0x390
[ 476.119026] ? __pfx_worker_thread+0x10/0x10
[ 476.119030] kthread+0xd2/0x100
[ 476.119033] ? __pfx_kthread+0x10/0x10
[ 476.119035] ret_from_fork+0x34/0x50
[ 476.119040] ? __pfx_kthread+0x10/0x10
[ 476.119042] ret_from_fork_asm+0x1a/0x30
[ 476.119049] </TASK>
[ 476.142552] kobject: kobject_add_internal failed for BAT1 with -EEXIST, don't try to register things with the same name in the same directory.
[ 476.415022] ata1.00: unexpected _GTF length (8)
[ 476.428076] sd 0:0:0:0: [sda] Starting disk
[ 476.835035] ata1.00: unexpected _GTF length (8)
[ 476.839720] ata1.00: configured for UDMA/133
[ 491.328831] sysfs: cannot create duplicate filename '/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0A:00/power_supply/BAT1'
[ 491.329720] CPU: 1 UID: 0 PID: 185 Comm: kworker/1:4 Kdump: loaded Not tainted 6.12.38+deb13-amd64 #1 Debian 6.12.38-1
[ 491.329727] Hardware name: Gateway NV44 /SJV40-MV , BIOS V1.3121 04/08/2009
[ 491.329731] Workqueue: kacpi_notify acpi_os_execute_deferred
[ 491.329741] Call Trace:
[ 491.329745] <TASK>
[ 491.329751] dump_stack_lvl+0x5d/0x80
[ 491.329758] sysfs_warn_dup.cold+0x17/0x23
[ 491.329762] sysfs_create_dir_ns+0xce/0xe0
[ 491.329770] kobject_add_internal+0xba/0x250
[ 491.329775] kobject_add+0x96/0xc0
[ 491.329779] ? get_device_parent+0xde/0x1e0
[ 491.329784] device_add+0xe2/0x870
[ 491.329790] __power_supply_register.part.0+0x20f/0x3f0
[ 491.329797] sysfs_add_battery+0xa4/0x1d0 [battery]
[ 491.329805] acpi_battery_update+0x19e/0x290 [battery]
[ 491.329809] acpi_battery_notify+0x50/0x120 [battery]
[ 491.329812] acpi_ev_notify_dispatch+0x49/0x70
[ 491.329817] acpi_os_execute_deferred+0x1a/0x30
[ 491.329820] process_one_work+0x177/0x330
[ 491.329826] worker_thread+0x251/0x390
[ 491.329830] ? __pfx_worker_thread+0x10/0x10
[ 491.329833] kthread+0xd2/0x100
[ 491.329836] ? __pfx_kthread+0x10/0x10
[ 491.329838] ret_from_fork+0x34/0x50
[ 491.329842] ? __pfx_kthread+0x10/0x10
[ 491.329844] ret_from_fork_asm+0x1a/0x30
[ 491.329850] </TASK>
[ 491.329855] kobject: kobject_add_internal failed for BAT1 with -EEXIST, don't try to register things with the same name in the same directory.
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/r/202509101620.yI0HZ5gT-lkp@intel.com/
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202509101620.yI0HZ5gT-lkp@intel.com/
Fixes: 508df92d1f8d ("ACPI: battery: register power_supply subdevice only when battery is present")
Signed-off-by: GuangFei Luo <luogf2025@163.com>
Cc: stable@vger.kernel.org
---
drivers/acpi/battery.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 6905b56bf3e4..f6d4a8b39a9c 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -850,6 +850,12 @@ static void __exit battery_hook_exit(void)
static int sysfs_add_battery(struct acpi_battery *battery)
{
+ mutex_lock(&battery->sysfs_lock);
+ if (battery->bat) {
+ mutex_unlock(&battery->sysfs_lock);
+ return 0;
+ }
+
struct power_supply_config psy_cfg = {
.drv_data = battery,
.attr_grp = acpi_battery_groups,
@@ -896,9 +902,11 @@ static int sysfs_add_battery(struct acpi_battery *battery)
int result = PTR_ERR(battery->bat);
battery->bat = NULL;
+ mutex_unlock(&battery->sysfs_lock);
return result;
}
battery_hook_add_battery(battery);
+ mutex_unlock(&battery->sysfs_lock);
return 0;
}
@@ -1026,11 +1034,9 @@ static int acpi_battery_update(struct acpi_battery *battery, bool resume)
return result;
acpi_battery_quirks(battery);
- if (!battery->bat) {
- result = sysfs_add_battery(battery);
- if (result)
- return result;
- }
+ result = sysfs_add_battery(battery);
+ if (result)
+ return result;
/*
* Wakeup the system if battery is critical low
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v3] ACPI: battery: prevent sysfs_add_battery re-entry on rapid events
2025-09-12 15:48 ` [PATCH v3] ACPI: battery: prevent sysfs_add_battery re-entry on rapid events GuangFei Luo
@ 2025-09-12 15:57 ` Greg KH
2025-09-12 16:25 ` GuangFei Luo
0 siblings, 1 reply; 18+ messages in thread
From: Greg KH @ 2025-09-12 15:57 UTC (permalink / raw)
To: GuangFei Luo
Cc: Rafael J . Wysocki, Len Brown, linux-acpi, linux-kernel, stable,
Sebastian Reichel, linux-pm, Dan Carpenter, kernel test robot
On Fri, Sep 12, 2025 at 11:48:59PM +0800, GuangFei Luo wrote:
> v3:
> - Modified the earlier approach: since sysfs_add_battery() is invoked
> from multiple places, the most reliable way is to add the lock inside
> the function itself.
> - sysfs_remove_battery() had a similar race issue in the past, which was
> fixed by adding a lock as well. Reference:
> https://lore.kernel.org/all/9c921c22a7f33397a6774d7fa076db9b6a0fd669
> .1312318300.git.len.brown@intel.com/
>
> v2:
> - Fix missing mutex_unlock in acpi_battery_update()
> (Reported-by: kernel test robot)
>
> v1:
> When removing and reinserting the laptop battery, ACPI can trigger
> two notifications in quick succession:
>
> - ACPI_BATTERY_NOTIFY_STATUS (0x80)
> - ACPI_BATTERY_NOTIFY_INFO (0x81)
Changelog should go below the --- line.
> static int sysfs_add_battery(struct acpi_battery *battery)
> {
> + mutex_lock(&battery->sysfs_lock);
Why not just use a guard() here? That would make the logic elsewhere
much simpler.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH v3] ACPI: battery: prevent sysfs_add_battery re-entry on rapid events
2025-09-12 15:57 ` Greg KH
@ 2025-09-12 16:25 ` GuangFei Luo
2025-09-13 6:05 ` Greg KH
0 siblings, 1 reply; 18+ messages in thread
From: GuangFei Luo @ 2025-09-12 16:25 UTC (permalink / raw)
To: gregkh
Cc: rafael, dan.carpenter, lenb, linux-acpi, linux-kernel, linux-pm,
lkp, luogf2025, sre, stable
Hi Greg,
Thanks for your review and suggestion.
I've updated sysfs_add_battery() to address your comment.
The locking is now applied explicitly inside the function
to prevent re-entry issues, while keeping the function
self-contained for all call sites.
Patch version: v3
Thanks,
GuangFei
v3:
- Modified the earlier approach: since sysfs_add_battery() is invoked
from multiple places, the most reliable way is to add the lock inside
the function itself.
- sysfs_remove_battery() had a similar race issue in the past, which was
fixed by adding a lock as well. Reference:
https://lore.kernel.org/all/9c921c22a7f33397a6774d7fa076db9b6a0fd669
.1312318300.git.len.brown@intel.com/
v2:
- Fix missing mutex_unlock in acpi_battery_update()
(Reported-by: kernel test robot)
v1:
When removing and reinserting the laptop battery, ACPI can trigger
two notifications in quick succession:
- ACPI_BATTERY_NOTIFY_STATUS (0x80)
- ACPI_BATTERY_NOTIFY_INFO (0x81)
Both notifications call acpi_battery_update(). Because the events
happen very close in time, sysfs_add_battery() can be re-entered
before battery->bat is set, causing a duplicate sysfs entry error.
This patch ensures that sysfs_add_battery() is not re-entered
when battery->bat is already non-NULL, preventing the duplicate
sysfs creation and stabilizing battery hotplug handling.
[ 476.117945] sysfs: cannot create duplicate filename '/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0A:00/power_supply/BAT1'
[ 476.118896] CPU: 1 UID: 0 PID: 185 Comm: kworker/1:4 Kdump: loaded Not tainted 6.12.38+deb13-amd64 #1 Debian 6.12.38-1
[ 476.118903] Hardware name: Gateway NV44 /SJV40-MV , BIOS V1.3121 04/08/2009
[ 476.118906] Workqueue: kacpi_notify acpi_os_execute_deferred
[ 476.118917] Call Trace:
[ 476.118922] <TASK>
[ 476.118929] dump_stack_lvl+0x5d/0x80
[ 476.118938] sysfs_warn_dup.cold+0x17/0x23
[ 476.118943] sysfs_create_dir_ns+0xce/0xe0
[ 476.118952] kobject_add_internal+0xba/0x250
[ 476.118959] kobject_add+0x96/0xc0
[ 476.118964] ? get_device_parent+0xde/0x1e0
[ 476.118970] device_add+0xe2/0x870
[ 476.118975] __power_supply_register.part.0+0x20f/0x3f0
[ 476.118981] ? wake_up_q+0x4e/0x90
[ 476.118990] sysfs_add_battery+0xa4/0x1d0 [battery]
[ 476.118998] acpi_battery_update+0x19e/0x290 [battery]
[ 476.119002] acpi_battery_notify+0x50/0x120 [battery]
[ 476.119006] acpi_ev_notify_dispatch+0x49/0x70
[ 476.119012] acpi_os_execute_deferred+0x1a/0x30
[ 476.119015] process_one_work+0x177/0x330
[ 476.119022] worker_thread+0x251/0x390
[ 476.119026] ? __pfx_worker_thread+0x10/0x10
[ 476.119030] kthread+0xd2/0x100
[ 476.119033] ? __pfx_kthread+0x10/0x10
[ 476.119035] ret_from_fork+0x34/0x50
[ 476.119040] ? __pfx_kthread+0x10/0x10
[ 476.119042] ret_from_fork_asm+0x1a/0x30
[ 476.119049] </TASK>
[ 476.142552] kobject: kobject_add_internal failed for BAT1 with -EEXIST, don't try to register things with the same name in the same directory.
[ 476.415022] ata1.00: unexpected _GTF length (8)
[ 476.428076] sd 0:0:0:0: [sda] Starting disk
[ 476.835035] ata1.00: unexpected _GTF length (8)
[ 476.839720] ata1.00: configured for UDMA/133
[ 491.328831] sysfs: cannot create duplicate filename '/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0A:00/power_supply/BAT1'
[ 491.329720] CPU: 1 UID: 0 PID: 185 Comm: kworker/1:4 Kdump: loaded Not tainted 6.12.38+deb13-amd64 #1 Debian 6.12.38-1
[ 491.329727] Hardware name: Gateway NV44 /SJV40-MV , BIOS V1.3121 04/08/2009
[ 491.329731] Workqueue: kacpi_notify acpi_os_execute_deferred
[ 491.329741] Call Trace:
[ 491.329745] <TASK>
[ 491.329751] dump_stack_lvl+0x5d/0x80
[ 491.329758] sysfs_warn_dup.cold+0x17/0x23
[ 491.329762] sysfs_create_dir_ns+0xce/0xe0
[ 491.329770] kobject_add_internal+0xba/0x250
[ 491.329775] kobject_add+0x96/0xc0
[ 491.329779] ? get_device_parent+0xde/0x1e0
[ 491.329784] device_add+0xe2/0x870
[ 491.329790] __power_supply_register.part.0+0x20f/0x3f0
[ 491.329797] sysfs_add_battery+0xa4/0x1d0 [battery]
[ 491.329805] acpi_battery_update+0x19e/0x290 [battery]
[ 491.329809] acpi_battery_notify+0x50/0x120 [battery]
[ 491.329812] acpi_ev_notify_dispatch+0x49/0x70
[ 491.329817] acpi_os_execute_deferred+0x1a/0x30
[ 491.329820] process_one_work+0x177/0x330
[ 491.329826] worker_thread+0x251/0x390
[ 491.329830] ? __pfx_worker_thread+0x10/0x10
[ 491.329833] kthread+0xd2/0x100
[ 491.329836] ? __pfx_kthread+0x10/0x10
[ 491.329838] ret_from_fork+0x34/0x50
[ 491.329842] ? __pfx_kthread+0x10/0x10
[ 491.329844] ret_from_fork_asm+0x1a/0x30
[ 491.329850] </TASK>
[ 491.329855] kobject: kobject_add_internal failed for BAT1 with -EEXIST, don't try to register things with the same name in the same directory.
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/r/202509101620.yI0HZ5gT-lkp@intel.com/
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202509101620.yI0HZ5gT-lkp@intel.com/
Fixes: 508df92d1f8d ("ACPI: battery: register power_supply subdevice only when battery is present")
Signed-off-by: GuangFei Luo <luogf2025@163.com>
Cc: stable@vger.kernel.org
---
drivers/acpi/battery.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 6905b56bf3e4..f6d4a8b39a9c 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -850,6 +850,12 @@ static void __exit battery_hook_exit(void)
static int sysfs_add_battery(struct acpi_battery *battery)
{
+ mutex_lock(&battery->sysfs_lock);
+ if (battery->bat) {
+ mutex_unlock(&battery->sysfs_lock);
+ return 0;
+ }
+
struct power_supply_config psy_cfg = {
.drv_data = battery,
.attr_grp = acpi_battery_groups,
@@ -896,9 +902,11 @@ static int sysfs_add_battery(struct acpi_battery *battery)
int result = PTR_ERR(battery->bat);
battery->bat = NULL;
+ mutex_unlock(&battery->sysfs_lock);
return result;
}
battery_hook_add_battery(battery);
+ mutex_unlock(&battery->sysfs_lock);
return 0;
}
@@ -1026,11 +1034,9 @@ static int acpi_battery_update(struct acpi_battery *battery, bool resume)
return result;
acpi_battery_quirks(battery);
- if (!battery->bat) {
- result = sysfs_add_battery(battery);
- if (result)
- return result;
- }
+ result = sysfs_add_battery(battery);
+ if (result)
+ return result;
/*
* Wakeup the system if battery is critical low
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v3] ACPI: battery: prevent sysfs_add_battery re-entry on rapid events
2025-09-12 16:25 ` GuangFei Luo
@ 2025-09-13 6:05 ` Greg KH
0 siblings, 0 replies; 18+ messages in thread
From: Greg KH @ 2025-09-13 6:05 UTC (permalink / raw)
To: GuangFei Luo
Cc: rafael, dan.carpenter, lenb, linux-acpi, linux-kernel, linux-pm,
lkp, sre, stable
On Sat, Sep 13, 2025 at 12:25:16AM +0800, GuangFei Luo wrote:
> Hi Greg,
>
> Thanks for your review and suggestion.
>
> I've updated sysfs_add_battery() to address your comment.
> The locking is now applied explicitly inside the function
> to prevent re-entry issues, while keeping the function
> self-contained for all call sites.
>
> Patch version: v3
>
> Thanks,
> GuangFei
>
> v3:
> - Modified the earlier approach: since sysfs_add_battery() is invoked
> from multiple places, the most reliable way is to add the lock inside
> the function itself.
> - sysfs_remove_battery() had a similar race issue in the past, which was
> fixed by adding a lock as well. Reference:
> https://lore.kernel.org/all/9c921c22a7f33397a6774d7fa076db9b6a0fd669
> .1312318300.git.len.brown@intel.com/
>
> v2:
> - Fix missing mutex_unlock in acpi_battery_update()
> (Reported-by: kernel test robot)
>
> v1:
> When removing and reinserting the laptop battery, ACPI can trigger
> two notifications in quick succession:
Note, none of the above should be here in the changelog body, it should
be below the --- line.
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 6905b56bf3e4..f6d4a8b39a9c 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -850,6 +850,12 @@ static void __exit battery_hook_exit(void)
>
> static int sysfs_add_battery(struct acpi_battery *battery)
> {
> + mutex_lock(&battery->sysfs_lock);
Again, can you use guard() to make this logic simpler? That would turn
this into a much smaller patch.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4] ACPI: battery: prevent sysfs_add_battery re-entry on rapid events
[not found] <20250910142653.313360-1-luogf2025@163.com>
2025-09-12 15:48 ` [PATCH v3] ACPI: battery: prevent sysfs_add_battery re-entry on rapid events GuangFei Luo
@ 2025-09-13 7:19 ` GuangFei Luo
2025-09-13 7:33 ` Greg KH
2025-09-13 7:56 ` [PATCH v5] " GuangFei Luo
` (2 subsequent siblings)
4 siblings, 1 reply; 18+ messages in thread
From: GuangFei Luo @ 2025-09-13 7:19 UTC (permalink / raw)
To: rafael, gregkh
Cc: dan.carpenter, lenb, linux-acpi, linux-kernel, linux-pm, lkp,
luogf2025, sre, stable
v4:
- Uses guard(mutex) for battery->sysfs_lock in sysfs_add_battery().
- Since sysfs_add_battery() now handles the battery->bat check with
proper locking,the extra if (!battery->bat) check at the call site
has become redundant.
v3:
- Modified the earlier approach: since sysfs_add_battery() is invoked
from multiple places, the most reliable way is to add the lock inside
the function itself.
- sysfs_remove_battery() had a similar race issue in the past, which was
fixed by adding a lock as well. Reference:
https://lore.kernel.org/all/9c921c22a7f33397a6774d7fa076db9b6a0fd669
.1312318300.git.len.brown@intel.com/
v2:
- Fix missing mutex_unlock in acpi_battery_update()
(Reported-by: kernel test robot)
v1:
When removing and reinserting the laptop battery, ACPI can trigger
two notifications in quick succession:
- ACPI_BATTERY_NOTIFY_STATUS (0x80)
- ACPI_BATTERY_NOTIFY_INFO (0x81)
Both notifications call acpi_battery_update(). Because the events
happen very close in time, sysfs_add_battery() can be re-entered
before battery->bat is set, causing a duplicate sysfs entry error.
This patch ensures that sysfs_add_battery() is not re-entered
when battery->bat is already non-NULL, preventing the duplicate
sysfs creation and stabilizing battery hotplug handling.
[ 476.117945] sysfs: cannot create duplicate filename '/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0A:00/power_supply/BAT1'
[ 476.118896] CPU: 1 UID: 0 PID: 185 Comm: kworker/1:4 Kdump: loaded Not tainted 6.12.38+deb13-amd64 #1 Debian 6.12.38-1
[ 476.118903] Hardware name: Gateway NV44 /SJV40-MV , BIOS V1.3121 04/08/2009
[ 476.118906] Workqueue: kacpi_notify acpi_os_execute_deferred
[ 476.118917] Call Trace:
[ 476.118922] <TASK>
[ 476.118929] dump_stack_lvl+0x5d/0x80
[ 476.118938] sysfs_warn_dup.cold+0x17/0x23
[ 476.118943] sysfs_create_dir_ns+0xce/0xe0
[ 476.118952] kobject_add_internal+0xba/0x250
[ 476.118959] kobject_add+0x96/0xc0
[ 476.118964] ? get_device_parent+0xde/0x1e0
[ 476.118970] device_add+0xe2/0x870
[ 476.118975] __power_supply_register.part.0+0x20f/0x3f0
[ 476.118981] ? wake_up_q+0x4e/0x90
[ 476.118990] sysfs_add_battery+0xa4/0x1d0 [battery]
[ 476.118998] acpi_battery_update+0x19e/0x290 [battery]
[ 476.119002] acpi_battery_notify+0x50/0x120 [battery]
[ 476.119006] acpi_ev_notify_dispatch+0x49/0x70
[ 476.119012] acpi_os_execute_deferred+0x1a/0x30
[ 476.119015] process_one_work+0x177/0x330
[ 476.119022] worker_thread+0x251/0x390
[ 476.119026] ? __pfx_worker_thread+0x10/0x10
[ 476.119030] kthread+0xd2/0x100
[ 476.119033] ? __pfx_kthread+0x10/0x10
[ 476.119035] ret_from_fork+0x34/0x50
[ 476.119040] ? __pfx_kthread+0x10/0x10
[ 476.119042] ret_from_fork_asm+0x1a/0x30
[ 476.119049] </TASK>
[ 476.142552] kobject: kobject_add_internal failed for BAT1 with -EEXIST, don't try to register things with the same name in the same directory.
[ 476.415022] ata1.00: unexpected _GTF length (8)
[ 476.428076] sd 0:0:0:0: [sda] Starting disk
[ 476.835035] ata1.00: unexpected _GTF length (8)
[ 476.839720] ata1.00: configured for UDMA/133
[ 491.328831] sysfs: cannot create duplicate filename '/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0A:00/power_supply/BAT1'
[ 491.329720] CPU: 1 UID: 0 PID: 185 Comm: kworker/1:4 Kdump: loaded Not tainted 6.12.38+deb13-amd64 #1 Debian 6.12.38-1
[ 491.329727] Hardware name: Gateway NV44 /SJV40-MV , BIOS V1.3121 04/08/2009
[ 491.329731] Workqueue: kacpi_notify acpi_os_execute_deferred
[ 491.329741] Call Trace:
[ 491.329745] <TASK>
[ 491.329751] dump_stack_lvl+0x5d/0x80
[ 491.329758] sysfs_warn_dup.cold+0x17/0x23
[ 491.329762] sysfs_create_dir_ns+0xce/0xe0
[ 491.329770] kobject_add_internal+0xba/0x250
[ 491.329775] kobject_add+0x96/0xc0
[ 491.329779] ? get_device_parent+0xde/0x1e0
[ 491.329784] device_add+0xe2/0x870
[ 491.329790] __power_supply_register.part.0+0x20f/0x3f0
[ 491.329797] sysfs_add_battery+0xa4/0x1d0 [battery]
[ 491.329805] acpi_battery_update+0x19e/0x290 [battery]
[ 491.329809] acpi_battery_notify+0x50/0x120 [battery]
[ 491.329812] acpi_ev_notify_dispatch+0x49/0x70
[ 491.329817] acpi_os_execute_deferred+0x1a/0x30
[ 491.329820] process_one_work+0x177/0x330
[ 491.329826] worker_thread+0x251/0x390
[ 491.329830] ? __pfx_worker_thread+0x10/0x10
[ 491.329833] kthread+0xd2/0x100
[ 491.329836] ? __pfx_kthread+0x10/0x10
[ 491.329838] ret_from_fork+0x34/0x50
[ 491.329842] ? __pfx_kthread+0x10/0x10
[ 491.329844] ret_from_fork_asm+0x1a/0x30
[ 491.329850] </TASK>
[ 491.329855] kobject: kobject_add_internal failed for BAT1 with -EEXIST, don't try to register things with the same name in the same directory.
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/r/202509101620.yI0HZ5gT-lkp@intel.com/
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202509101620.yI0HZ5gT-lkp@intel.com/
Fixes: 508df92d1f8d ("ACPI: battery: register power_supply subdevice only when battery is present")
Signed-off-by: GuangFei Luo <luogf2025@163.com>
Cc: stable@vger.kernel.org
---
drivers/acpi/battery.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 6905b56bf3e4..20d68f3e881f 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -850,6 +850,10 @@ static void __exit battery_hook_exit(void)
static int sysfs_add_battery(struct acpi_battery *battery)
{
+ guard(mutex)(&battery->sysfs_lock);
+ if (battery->bat)
+ return 0;
+
struct power_supply_config psy_cfg = {
.drv_data = battery,
.attr_grp = acpi_battery_groups,
@@ -1026,11 +1030,9 @@ static int acpi_battery_update(struct acpi_battery *battery, bool resume)
return result;
acpi_battery_quirks(battery);
- if (!battery->bat) {
- result = sysfs_add_battery(battery);
- if (result)
- return result;
- }
+ result = sysfs_add_battery(battery);
+ if (result)
+ return result;
/*
* Wakeup the system if battery is critical low
@@ -1112,12 +1114,12 @@ static int battery_notify(struct notifier_block *nb,
result = acpi_battery_get_info(battery);
if (result)
return result;
-
- result = sysfs_add_battery(battery);
- if (result)
- return result;
}
+ result = sysfs_add_battery(battery);
+ if (result)
+ return result;
+
acpi_battery_init_alarm(battery);
acpi_battery_get_state(battery);
break;
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4] ACPI: battery: prevent sysfs_add_battery re-entry on rapid events
2025-09-13 7:19 ` [PATCH v4] " GuangFei Luo
@ 2025-09-13 7:33 ` Greg KH
0 siblings, 0 replies; 18+ messages in thread
From: Greg KH @ 2025-09-13 7:33 UTC (permalink / raw)
To: GuangFei Luo
Cc: rafael, dan.carpenter, lenb, linux-acpi, linux-kernel, linux-pm,
lkp, sre, stable
On Sat, Sep 13, 2025 at 03:19:25PM +0800, GuangFei Luo wrote:
> v4:
> - Uses guard(mutex) for battery->sysfs_lock in sysfs_add_battery().
> - Since sysfs_add_battery() now handles the battery->bat check with
> proper locking,the extra if (!battery->bat) check at the call site
> has become redundant.
>
> v3:
> - Modified the earlier approach: since sysfs_add_battery() is invoked
> from multiple places, the most reliable way is to add the lock inside
> the function itself.
> - sysfs_remove_battery() had a similar race issue in the past, which was
> fixed by adding a lock as well. Reference:
> https://lore.kernel.org/all/9c921c22a7f33397a6774d7fa076db9b6a0fd669
> .1312318300.git.len.brown@intel.com/
>
> v2:
> - Fix missing mutex_unlock in acpi_battery_update()
> (Reported-by: kernel test robot)
>
> v1:
> When removing and reinserting the laptop battery, ACPI can trigger
> two notifications in quick succession:
Again, all of this goes below the --- line, not above it.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v5] ACPI: battery: prevent sysfs_add_battery re-entry on rapid events
[not found] <20250910142653.313360-1-luogf2025@163.com>
2025-09-12 15:48 ` [PATCH v3] ACPI: battery: prevent sysfs_add_battery re-entry on rapid events GuangFei Luo
2025-09-13 7:19 ` [PATCH v4] " GuangFei Luo
@ 2025-09-13 7:56 ` GuangFei Luo
2025-09-14 11:12 ` [PATCH v6] " GuangFei Luo
2025-09-23 16:18 ` GuangFei Luo
4 siblings, 0 replies; 18+ messages in thread
From: GuangFei Luo @ 2025-09-13 7:56 UTC (permalink / raw)
To: rafael, gregkh
Cc: dan.carpenter, lenb, linux-acpi, linux-kernel, linux-pm, lkp,
luogf2025, sre, stable
When removing and reinserting the laptop battery, ACPI can trigger
two notifications in quick succession:
- ACPI_BATTERY_NOTIFY_STATUS (0x80)
- ACPI_BATTERY_NOTIFY_INFO (0x81)
Both notifications call acpi_battery_update(). Because the events
happen very close in time, sysfs_add_battery() can be re-entered
before battery->bat is set, causing a duplicate sysfs entry error.
This patch ensures that sysfs_add_battery() is not re-entered
when battery->bat is already non-NULL, preventing the duplicate
sysfs creation and stabilizing battery hotplug handling.
[ 476.117945] sysfs: cannot create duplicate filename '/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0A:00/power_supply/BAT1'
[ 476.118896] CPU: 1 UID: 0 PID: 185 Comm: kworker/1:4 Kdump: loaded Not tainted 6.12.38+deb13-amd64 #1 Debian 6.12.38-1
[ 476.118903] Hardware name: Gateway NV44 /SJV40-MV , BIOS V1.3121 04/08/2009
[ 476.118906] Workqueue: kacpi_notify acpi_os_execute_deferred
[ 476.118917] Call Trace:
[ 476.118922] <TASK>
[ 476.118929] dump_stack_lvl+0x5d/0x80
[ 476.118938] sysfs_warn_dup.cold+0x17/0x23
[ 476.118943] sysfs_create_dir_ns+0xce/0xe0
[ 476.118952] kobject_add_internal+0xba/0x250
[ 476.118959] kobject_add+0x96/0xc0
[ 476.118964] ? get_device_parent+0xde/0x1e0
[ 476.118970] device_add+0xe2/0x870
[ 476.118975] __power_supply_register.part.0+0x20f/0x3f0
[ 476.118981] ? wake_up_q+0x4e/0x90
[ 476.118990] sysfs_add_battery+0xa4/0x1d0 [battery]
[ 476.118998] acpi_battery_update+0x19e/0x290 [battery]
[ 476.119002] acpi_battery_notify+0x50/0x120 [battery]
[ 476.119006] acpi_ev_notify_dispatch+0x49/0x70
[ 476.119012] acpi_os_execute_deferred+0x1a/0x30
[ 476.119015] process_one_work+0x177/0x330
[ 476.119022] worker_thread+0x251/0x390
[ 476.119026] ? __pfx_worker_thread+0x10/0x10
[ 476.119030] kthread+0xd2/0x100
[ 476.119033] ? __pfx_kthread+0x10/0x10
[ 476.119035] ret_from_fork+0x34/0x50
[ 476.119040] ? __pfx_kthread+0x10/0x10
[ 476.119042] ret_from_fork_asm+0x1a/0x30
[ 476.119049] </TASK>
[ 476.142552] kobject: kobject_add_internal failed for BAT1 with -EEXIST, don't try to register things with the same name in the same directory.
[ 476.415022] ata1.00: unexpected _GTF length (8)
[ 476.428076] sd 0:0:0:0: [sda] Starting disk
[ 476.835035] ata1.00: unexpected _GTF length (8)
[ 476.839720] ata1.00: configured for UDMA/133
[ 491.328831] sysfs: cannot create duplicate filename '/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0A:00/power_supply/BAT1'
[ 491.329720] CPU: 1 UID: 0 PID: 185 Comm: kworker/1:4 Kdump: loaded Not tainted 6.12.38+deb13-amd64 #1 Debian 6.12.38-1
[ 491.329727] Hardware name: Gateway NV44 /SJV40-MV , BIOS V1.3121 04/08/2009
[ 491.329731] Workqueue: kacpi_notify acpi_os_execute_deferred
[ 491.329741] Call Trace:
[ 491.329745] <TASK>
[ 491.329751] dump_stack_lvl+0x5d/0x80
[ 491.329758] sysfs_warn_dup.cold+0x17/0x23
[ 491.329762] sysfs_create_dir_ns+0xce/0xe0
[ 491.329770] kobject_add_internal+0xba/0x250
[ 491.329775] kobject_add+0x96/0xc0
[ 491.329779] ? get_device_parent+0xde/0x1e0
[ 491.329784] device_add+0xe2/0x870
[ 491.329790] __power_supply_register.part.0+0x20f/0x3f0
[ 491.329797] sysfs_add_battery+0xa4/0x1d0 [battery]
[ 491.329805] acpi_battery_update+0x19e/0x290 [battery]
[ 491.329809] acpi_battery_notify+0x50/0x120 [battery]
[ 491.329812] acpi_ev_notify_dispatch+0x49/0x70
[ 491.329817] acpi_os_execute_deferred+0x1a/0x30
[ 491.329820] process_one_work+0x177/0x330
[ 491.329826] worker_thread+0x251/0x390
[ 491.329830] ? __pfx_worker_thread+0x10/0x10
[ 491.329833] kthread+0xd2/0x100
[ 491.329836] ? __pfx_kthread+0x10/0x10
[ 491.329838] ret_from_fork+0x34/0x50
[ 491.329842] ? __pfx_kthread+0x10/0x10
[ 491.329844] ret_from_fork_asm+0x1a/0x30
[ 491.329850] </TASK>
[ 491.329855] kobject: kobject_add_internal failed for BAT1 with -EEXIST, don't try to register things with the same name in the same directory.
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/r/202509101620.yI0HZ5gT-lkp@intel.com/
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202509101620.yI0HZ5gT-lkp@intel.com/
Fixes: 508df92d1f8d ("ACPI: battery: register power_supply subdevice only when battery is present")
Signed-off-by: GuangFei Luo <luogf2025@163.com>
Cc: stable@vger.kernel.org
---
v5:
- Move changelog above the '---' line as per submission guidelines.
v4:
- Uses guard(mutex) for battery->sysfs_lock in sysfs_add_battery().
- Since sysfs_add_battery() now handles the battery->bat check with
proper locking,the extra if (!battery->bat) check at the call site
has become redundant.
v3:
- Modified the earlier approach: since sysfs_add_battery() is invoked
from multiple places, the most reliable way is to add the lock inside
the function itself.
- sysfs_remove_battery() had a similar race issue in the past, which was
fixed by adding a lock as well. Reference:
https://lore.kernel.org/all/9c921c22a7f33397a6774d7fa076db9b6a0fd669
.1312318300.git.len.brown@intel.com/
v2:
- Fix missing mutex_unlock in acpi_battery_update()
(Reported-by: kernel test robot)
v1:
- Initial patch to handle race when hotplugging battery, preventing
duplicate sysfs entries.
---
drivers/acpi/battery.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 6905b56bf3e4..20d68f3e881f 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -850,6 +850,10 @@ static void __exit battery_hook_exit(void)
static int sysfs_add_battery(struct acpi_battery *battery)
{
+ guard(mutex)(&battery->sysfs_lock);
+ if (battery->bat)
+ return 0;
+
struct power_supply_config psy_cfg = {
.drv_data = battery,
.attr_grp = acpi_battery_groups,
@@ -1026,11 +1030,9 @@ static int acpi_battery_update(struct acpi_battery *battery, bool resume)
return result;
acpi_battery_quirks(battery);
- if (!battery->bat) {
- result = sysfs_add_battery(battery);
- if (result)
- return result;
- }
+ result = sysfs_add_battery(battery);
+ if (result)
+ return result;
/*
* Wakeup the system if battery is critical low
@@ -1112,12 +1114,12 @@ static int battery_notify(struct notifier_block *nb,
result = acpi_battery_get_info(battery);
if (result)
return result;
-
- result = sysfs_add_battery(battery);
- if (result)
- return result;
}
+ result = sysfs_add_battery(battery);
+ if (result)
+ return result;
+
acpi_battery_init_alarm(battery);
acpi_battery_get_state(battery);
break;
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH v6] ACPI: battery: prevent sysfs_add_battery re-entry on rapid events
[not found] <20250910142653.313360-1-luogf2025@163.com>
` (2 preceding siblings ...)
2025-09-13 7:56 ` [PATCH v5] " GuangFei Luo
@ 2025-09-14 11:12 ` GuangFei Luo
2025-09-23 2:38 ` GuangFei Luo
2025-09-23 16:18 ` GuangFei Luo
4 siblings, 1 reply; 18+ messages in thread
From: GuangFei Luo @ 2025-09-14 11:12 UTC (permalink / raw)
To: rafael
Cc: michal.wilczynski, gregkh, dan.carpenter, lenb, linux-acpi,
linux-kernel, linux-pm, lkp, luogf2025, sre, stable
When removing and reinserting the laptop battery, ACPI can trigger
two notifications in quick succession:
- ACPI_BATTERY_NOTIFY_STATUS (0x80)
- ACPI_BATTERY_NOTIFY_INFO (0x81)
Both notifications call acpi_battery_update(). Because the events
happen very close in time, sysfs_add_battery() can be re-entered
before battery->bat is set, causing a duplicate sysfs entry error.
This patch ensures that sysfs_add_battery() is not re-entered
when battery->bat is already non-NULL, preventing the duplicate
sysfs creation and stabilizing battery hotplug handling.
[ 476.117945] sysfs: cannot create duplicate filename '/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0A:00/power_supply/BAT1'
[ 476.118896] CPU: 1 UID: 0 PID: 185 Comm: kworker/1:4 Kdump: loaded Not tainted 6.12.38+deb13-amd64 #1 Debian 6.12.38-1
[ 476.118903] Hardware name: Gateway NV44 /SJV40-MV , BIOS V1.3121 04/08/2009
[ 476.118906] Workqueue: kacpi_notify acpi_os_execute_deferred
[ 476.118917] Call Trace:
[ 476.118922] <TASK>
[ 476.118929] dump_stack_lvl+0x5d/0x80
[ 476.118938] sysfs_warn_dup.cold+0x17/0x23
[ 476.118943] sysfs_create_dir_ns+0xce/0xe0
[ 476.118952] kobject_add_internal+0xba/0x250
[ 476.118959] kobject_add+0x96/0xc0
[ 476.118964] ? get_device_parent+0xde/0x1e0
[ 476.118970] device_add+0xe2/0x870
[ 476.118975] __power_supply_register.part.0+0x20f/0x3f0
[ 476.118981] ? wake_up_q+0x4e/0x90
[ 476.118990] sysfs_add_battery+0xa4/0x1d0 [battery]
[ 476.118998] acpi_battery_update+0x19e/0x290 [battery]
[ 476.119002] acpi_battery_notify+0x50/0x120 [battery]
[ 476.119006] acpi_ev_notify_dispatch+0x49/0x70
[ 476.119012] acpi_os_execute_deferred+0x1a/0x30
[ 476.119015] process_one_work+0x177/0x330
[ 476.119022] worker_thread+0x251/0x390
[ 476.119026] ? __pfx_worker_thread+0x10/0x10
[ 476.119030] kthread+0xd2/0x100
[ 476.119033] ? __pfx_kthread+0x10/0x10
[ 476.119035] ret_from_fork+0x34/0x50
[ 476.119040] ? __pfx_kthread+0x10/0x10
[ 476.119042] ret_from_fork_asm+0x1a/0x30
[ 476.119049] </TASK>
[ 476.142552] kobject: kobject_add_internal failed for BAT1 with -EEXIST, don't try to register things with the same name in the same directory.
[ 476.415022] ata1.00: unexpected _GTF length (8)
[ 476.428076] sd 0:0:0:0: [sda] Starting disk
[ 476.835035] ata1.00: unexpected _GTF length (8)
[ 476.839720] ata1.00: configured for UDMA/133
[ 491.328831] sysfs: cannot create duplicate filename '/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0A:00/power_supply/BAT1'
[ 491.329720] CPU: 1 UID: 0 PID: 185 Comm: kworker/1:4 Kdump: loaded Not tainted 6.12.38+deb13-amd64 #1 Debian 6.12.38-1
[ 491.329727] Hardware name: Gateway NV44 /SJV40-MV , BIOS V1.3121 04/08/2009
[ 491.329731] Workqueue: kacpi_notify acpi_os_execute_deferred
[ 491.329741] Call Trace:
[ 491.329745] <TASK>
[ 491.329751] dump_stack_lvl+0x5d/0x80
[ 491.329758] sysfs_warn_dup.cold+0x17/0x23
[ 491.329762] sysfs_create_dir_ns+0xce/0xe0
[ 491.329770] kobject_add_internal+0xba/0x250
[ 491.329775] kobject_add+0x96/0xc0
[ 491.329779] ? get_device_parent+0xde/0x1e0
[ 491.329784] device_add+0xe2/0x870
[ 491.329790] __power_supply_register.part.0+0x20f/0x3f0
[ 491.329797] sysfs_add_battery+0xa4/0x1d0 [battery]
[ 491.329805] acpi_battery_update+0x19e/0x290 [battery]
[ 491.329809] acpi_battery_notify+0x50/0x120 [battery]
[ 491.329812] acpi_ev_notify_dispatch+0x49/0x70
[ 491.329817] acpi_os_execute_deferred+0x1a/0x30
[ 491.329820] process_one_work+0x177/0x330
[ 491.329826] worker_thread+0x251/0x390
[ 491.329830] ? __pfx_worker_thread+0x10/0x10
[ 491.329833] kthread+0xd2/0x100
[ 491.329836] ? __pfx_kthread+0x10/0x10
[ 491.329838] ret_from_fork+0x34/0x50
[ 491.329842] ? __pfx_kthread+0x10/0x10
[ 491.329844] ret_from_fork_asm+0x1a/0x30
[ 491.329850] </TASK>
[ 491.329855] kobject: kobject_add_internal failed for BAT1 with -EEXIST, don't try to register things with the same name in the same directory.
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/r/202509101620.yI0HZ5gT-lkp@intel.com/
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202509101620.yI0HZ5gT-lkp@intel.com/
Fixes: 10666251554c ("ACPI: battery: Install Notify() handler directly")
Signed-off-by: GuangFei Luo <luogf2025@163.com>
Cc: stable@vger.kernel.org
---
v6:
- Update Fixes tag: point to commit 10666251554c ("ACPI: battery: Install
Notify() handler directly"), which introduced the sysfs_add_battery()
re-entry issue when acpi_battery_notify is registered via
acpi_dev_install_notify_handler(). The problem does not occur with
acpi_bus_register_driver().
v5:
- Move changelog above the '---' line as per submission guidelines.
v4:
- Uses guard(mutex) for battery->sysfs_lock in sysfs_add_battery().
- Since sysfs_add_battery() now handles the battery->bat check with
proper locking,the extra if (!battery->bat) check at the call site
has become redundant.
v3:
- Modified the earlier approach: since sysfs_add_battery() is invoked
from multiple places, the most reliable way is to add the lock inside
the function itself.
- sysfs_remove_battery() had a similar race issue in the past, which was
fixed by adding a lock as well. Reference:
https://lore.kernel.org/all/9c921c22a7f33397a6774d7fa076db9b6a0fd669
.1312318300.git.len.brown@intel.com/
v2:
- Fix missing mutex_unlock in acpi_battery_update()
(Reported-by: kernel test robot)
v1:
- Initial patch to handle race when hotplugging battery, preventing
duplicate sysfs entries.
---
drivers/acpi/battery.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 6905b56bf3e4..20d68f3e881f 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -850,6 +850,10 @@ static void __exit battery_hook_exit(void)
static int sysfs_add_battery(struct acpi_battery *battery)
{
+ guard(mutex)(&battery->sysfs_lock);
+ if (battery->bat)
+ return 0;
+
struct power_supply_config psy_cfg = {
.drv_data = battery,
.attr_grp = acpi_battery_groups,
@@ -1026,11 +1030,9 @@ static int acpi_battery_update(struct acpi_battery *battery, bool resume)
return result;
acpi_battery_quirks(battery);
- if (!battery->bat) {
- result = sysfs_add_battery(battery);
- if (result)
- return result;
- }
+ result = sysfs_add_battery(battery);
+ if (result)
+ return result;
/*
* Wakeup the system if battery is critical low
@@ -1112,12 +1114,12 @@ static int battery_notify(struct notifier_block *nb,
result = acpi_battery_get_info(battery);
if (result)
return result;
-
- result = sysfs_add_battery(battery);
- if (result)
- return result;
}
+ result = sysfs_add_battery(battery);
+ if (result)
+ return result;
+
acpi_battery_init_alarm(battery);
acpi_battery_get_state(battery);
break;
--
2.43.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v6] ACPI: battery: prevent sysfs_add_battery re-entry on rapid events
2025-09-14 11:12 ` [PATCH v6] " GuangFei Luo
@ 2025-09-23 2:38 ` GuangFei Luo
2025-09-23 11:48 ` Rafael J. Wysocki
0 siblings, 1 reply; 18+ messages in thread
From: GuangFei Luo @ 2025-09-23 2:38 UTC (permalink / raw)
To: rafael
Cc: dan.carpenter, lenb, linux-acpi, linux-kernel, linux-pm, lkp, sre,
stable, Guofeng Luo
From: Guofeng Luo <luogf2025@163.com>
>On Thu, Sep 18, 2025 at 3:56 PM GuangFei Luo <luogf2025@163.com> wrote:
>>
>> When removing and reinserting the laptop battery, ACPI can trigger
>> two notifications in quick succession:
>> - ACPI_BATTERY_NOTIFY_STATUS (0x80)
>> - ACPI_BATTERY_NOTIFY_INFO (0x81)
>>
>> Both notifications call acpi_battery_update(). Because the events
>> happen very close in time, sysfs_add_battery() can be re-entered
>> before battery->bat is set, causing a duplicate sysfs entry error.
>>
>> When the ACPI battery driver uses
>> acpi_dev_install_notify_handler() to register acpi_battery_notify,
>> the callback may be triggered twice in a very short period of time.
>>
>> This patch ensures that sysfs_add_battery() is not re-entered
>> when battery->bat is already non-NULL, preventing the duplicate
>> sysfs creation and stabilizing battery hotplug handling.
>>
>> [ 476.117945] sysfs: cannot create duplicate filename '/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0A:00/power_supply/BAT1'
>> [ 476.118896] CPU: 1 UID: 0 PID: 185 Comm: kworker/1:4 Kdump: loaded Not tainted 6.12.38+deb13-amd64 #1 Debian 6.12.38-1
>> [ 476.118903] Hardware name: Gateway NV44 /SJV40-MV , BIOS V1.3121 04/08/2009
>> [ 476.118906] Workqueue: kacpi_notify acpi_os_execute_deferred
>> [ 476.118917] Call Trace:
>> [ 476.118922] <TASK>
>> [ 476.118929] dump_stack_lvl+0x5d/0x80
>> [ 476.118938] sysfs_warn_dup.cold+0x17/0x23
>> [ 476.118943] sysfs_create_dir_ns+0xce/0xe0
>> [ 476.118952] kobject_add_internal+0xba/0x250
>> [ 476.118959] kobject_add+0x96/0xc0
>> [ 476.118964] ? get_device_parent+0xde/0x1e0
>> [ 476.118970] device_add+0xe2/0x870
>> [ 476.118975] __power_supply_register.part.0+0x20f/0x3f0
>> [ 476.118981] ? wake_up_q+0x4e/0x90
>> [ 476.118990] sysfs_add_battery+0xa4/0x1d0 [battery]
>> [ 476.118998] acpi_battery_update+0x19e/0x290 [battery]
>> [ 476.119002] acpi_battery_notify+0x50/0x120 [battery]
>> [ 476.119006] acpi_ev_notify_dispatch+0x49/0x70
>> [ 476.119012] acpi_os_execute_deferred+0x1a/0x30
>> [ 476.119015] process_one_work+0x177/0x330
>> [ 476.119022] worker_thread+0x251/0x390
>> [ 476.119026] ? __pfx_worker_thread+0x10/0x10
>> [ 476.119030] kthread+0xd2/0x100
>> [ 476.119033] ? __pfx_kthread+0x10/0x10
>> [ 476.119035] ret_from_fork+0x34/0x50
>> [ 476.119040] ? __pfx_kthread+0x10/0x10
>> [ 476.119042] ret_from_fork_asm+0x1a/0x30
>> [ 476.119049] </TASK>
>> [ 476.142552] kobject: kobject_add_internal failed for BAT1 with -EEXIST, don't try to register things with the same name in the same directory.
>> [ 476.415022] ata1.00: unexpected _GTF length (8)
>> [ 476.428076] sd 0:0:0:0: [sda] Starting disk
>> [ 476.835035] ata1.00: unexpected _GTF length (8)
>> [ 476.839720] ata1.00: configured for UDMA/133
>> [ 491.328831] sysfs: cannot create duplicate filename '/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0A:00/power_supply/BAT1'
>> [ 491.329720] CPU: 1 UID: 0 PID: 185 Comm: kworker/1:4 Kdump: loaded Not tainted 6.12.38+deb13-amd64 #1 Debian 6.12.38-1
>> [ 491.329727] Hardware name: Gateway NV44 /SJV40-MV , BIOS V1.3121 04/08/2009
>> [ 491.329731] Workqueue: kacpi_notify acpi_os_execute_deferred
>> [ 491.329741] Call Trace:
>> [ 491.329745] <TASK>
>> [ 491.329751] dump_stack_lvl+0x5d/0x80
>> [ 491.329758] sysfs_warn_dup.cold+0x17/0x23
>> [ 491.329762] sysfs_create_dir_ns+0xce/0xe0
>> [ 491.329770] kobject_add_internal+0xba/0x250
>> [ 491.329775] kobject_add+0x96/0xc0
>> [ 491.329779] ? get_device_parent+0xde/0x1e0
>> [ 491.329784] device_add+0xe2/0x870
>> [ 491.329790] __power_supply_register.part.0+0x20f/0x3f0
>> [ 491.329797] sysfs_add_battery+0xa4/0x1d0 [battery]
>> [ 491.329805] acpi_battery_update+0x19e/0x290 [battery]
>> [ 491.329809] acpi_battery_notify+0x50/0x120 [battery]
>> [ 491.329812] acpi_ev_notify_dispatch+0x49/0x70
>> [ 491.329817] acpi_os_execute_deferred+0x1a/0x30
>> [ 491.329820] process_one_work+0x177/0x330
>> [ 491.329826] worker_thread+0x251/0x390
>> [ 491.329830] ? __pfx_worker_thread+0x10/0x10
>> [ 491.329833] kthread+0xd2/0x100
>> [ 491.329836] ? __pfx_kthread+0x10/0x10
>> [ 491.329838] ret_from_fork+0x34/0x50
>> [ 491.329842] ? __pfx_kthread+0x10/0x10
>> [ 491.329844] ret_from_fork_asm+0x1a/0x30
>> [ 491.329850] </TASK>
>> [ 491.329855] kobject: kobject_add_internal failed for BAT1 with -EEXIST, don't try to register things with the same name in the same directory.
>>
>> Fixes: 10666251554c ("ACPI: battery: Install Notify() handler directly")
>> Signed-off-by: GuangFei Luo <luogf2025@163.com>
>> Cc: stable@vger.kernel.org
>> ---
>> v6:
>> - Update Fixes tag: point to commit 10666251554c ("ACPI: battery: Install
>> Notify() handler directly"), which introduced the sysfs_add_battery()
>> re-entry issue when acpi_battery_notify is registered via
>> acpi_dev_install_notify_handler(). The problem does not occur with
>> acpi_bus_register_driver().
>>
>> v5:
>> - Move changelog above the '---' line as per submission guidelines.
>>
>> v4:
>> - Uses guard(mutex) for battery->sysfs_lock in sysfs_add_battery().
>> - Since sysfs_add_battery() now handles the battery->bat check with
>> proper locking,the extra if (!battery->bat) check at the call site
>> has become redundant.
>>
>> v3:
>> - Modified the earlier approach: since sysfs_add_battery() is invoked
>> from multiple places, the most reliable way is to add the lock inside
>> the function itself.
>> - sysfs_remove_battery() had a similar race issue in the past, which was
>> fixed by adding a lock as well. Reference:
>> https://lore.kernel.org/all/9c921c22a7f33397a6774d7fa076db9b6a0fd669
>> .1312318300.git.len.brown@intel.com/
>>
>> v2:
>> - Fix missing mutex_unlock in acpi_battery_update()
>> (Reported-by: kernel test robot)
>>
>> v1:
>> - Initial patch to handle race when hotplugging battery, preventing
>> duplicate sysfs entries.
>> ---
>> drivers/acpi/battery.c | 20 +++++++++++---------
>> 1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>> index 6905b56bf3e4..20d68f3e881f 100644
>> --- a/drivers/acpi/battery.c
>> +++ b/drivers/acpi/battery.c
>> @@ -850,6 +850,10 @@ static void __exit battery_hook_exit(void)
>>
>> static int sysfs_add_battery(struct acpi_battery *battery)
>> {
>> + guard(mutex)(&battery->sysfs_lock);
>> + if (battery->bat)
>> + return 0;
>> +
>> struct power_supply_config psy_cfg = {
>> .drv_data = battery,
>> .attr_grp = acpi_battery_groups,
>> @@ -1026,11 +1030,9 @@ static int acpi_battery_update(struct acpi_battery *battery, bool resume)
>> return result;
>> acpi_battery_quirks(battery);
>>
>> - if (!battery->bat) {
>> - result = sysfs_add_battery(battery);
>> - if (result)
>> - return result;
>> - }
>> + result = sysfs_add_battery(battery);
>> + if (result)
>> + return result;
>>
>> /*
>> * Wakeup the system if battery is critical low
>> @@ -1112,12 +1114,12 @@ static int battery_notify(struct notifier_block *nb,
>> result = acpi_battery_get_info(battery);
>> if (result)
>> return result;
>> -
>> - result = sysfs_add_battery(battery);
>> - if (result)
>> - return result;
>> }
>>
>> + result = sysfs_add_battery(battery);
>> + if (result)
>> + return result;
>> +
>
>Why is this change necessary?
Hi Rafael,
In the previous code:
if (battery->bat) {
acpi_battery_refresh(battery);
} else {
result = acpi_battery_get_info(battery);
if (result)
return result;
result = sysfs_add_battery(battery);
if (result)
return result;
}
the `if (!battery->bat)` check was done at the call site. However,
this check is not atomic: two threads can both see `battery->bat == NULL`
and then call `sysfs_add_battery()` concurrently, leading to duplicate
sysfs registration and `-EEXIST` errors.
By moving the check and mutex into `sysfs_add_battery()`, the race is
handled atomically. All call sites can now invoke it unconditionally,
and the function itself will no-op if the battery is already registered.
This avoids duplicated `if (!battery->bat)` logic and ensures consistent
protection everywhere.
Thanks,
GuangFei
>
>> acpi_battery_init_alarm(battery);
>> acpi_battery_get_state(battery);
>> break;
>> --
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v6] ACPI: battery: prevent sysfs_add_battery re-entry on rapid events
2025-09-23 2:38 ` GuangFei Luo
@ 2025-09-23 11:48 ` Rafael J. Wysocki
2025-09-23 16:13 ` GuangFei Luo
0 siblings, 1 reply; 18+ messages in thread
From: Rafael J. Wysocki @ 2025-09-23 11:48 UTC (permalink / raw)
To: GuangFei Luo
Cc: rafael, dan.carpenter, lenb, linux-acpi, linux-kernel, linux-pm,
lkp, sre, stable
On Tue, Sep 23, 2025 at 4:39 AM GuangFei Luo <luogf2025@163.com> wrote:
>
> From: Guofeng Luo <luogf2025@163.com>
>
> >On Thu, Sep 18, 2025 at 3:56 PM GuangFei Luo <luogf2025@163.com> wrote:
> >>
> >> When removing and reinserting the laptop battery, ACPI can trigger
> >> two notifications in quick succession:
> >> - ACPI_BATTERY_NOTIFY_STATUS (0x80)
> >> - ACPI_BATTERY_NOTIFY_INFO (0x81)
> >>
> >> Both notifications call acpi_battery_update(). Because the events
> >> happen very close in time, sysfs_add_battery() can be re-entered
> >> before battery->bat is set, causing a duplicate sysfs entry error.
> >>
> >> When the ACPI battery driver uses
> >> acpi_dev_install_notify_handler() to register acpi_battery_notify,
> >> the callback may be triggered twice in a very short period of time.
> >>
> >> This patch ensures that sysfs_add_battery() is not re-entered
> >> when battery->bat is already non-NULL, preventing the duplicate
> >> sysfs creation and stabilizing battery hotplug handling.
> >>
> >> [ 476.117945] sysfs: cannot create duplicate filename '/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0A:00/power_supply/BAT1'
> >> [ 476.118896] CPU: 1 UID: 0 PID: 185 Comm: kworker/1:4 Kdump: loaded Not tainted 6.12.38+deb13-amd64 #1 Debian 6.12.38-1
> >> [ 476.118903] Hardware name: Gateway NV44 /SJV40-MV , BIOS V1.3121 04/08/2009
> >> [ 476.118906] Workqueue: kacpi_notify acpi_os_execute_deferred
> >> [ 476.118917] Call Trace:
> >> [ 476.118922] <TASK>
> >> [ 476.118929] dump_stack_lvl+0x5d/0x80
> >> [ 476.118938] sysfs_warn_dup.cold+0x17/0x23
> >> [ 476.118943] sysfs_create_dir_ns+0xce/0xe0
> >> [ 476.118952] kobject_add_internal+0xba/0x250
> >> [ 476.118959] kobject_add+0x96/0xc0
> >> [ 476.118964] ? get_device_parent+0xde/0x1e0
> >> [ 476.118970] device_add+0xe2/0x870
> >> [ 476.118975] __power_supply_register.part.0+0x20f/0x3f0
> >> [ 476.118981] ? wake_up_q+0x4e/0x90
> >> [ 476.118990] sysfs_add_battery+0xa4/0x1d0 [battery]
> >> [ 476.118998] acpi_battery_update+0x19e/0x290 [battery]
> >> [ 476.119002] acpi_battery_notify+0x50/0x120 [battery]
> >> [ 476.119006] acpi_ev_notify_dispatch+0x49/0x70
> >> [ 476.119012] acpi_os_execute_deferred+0x1a/0x30
> >> [ 476.119015] process_one_work+0x177/0x330
> >> [ 476.119022] worker_thread+0x251/0x390
> >> [ 476.119026] ? __pfx_worker_thread+0x10/0x10
> >> [ 476.119030] kthread+0xd2/0x100
> >> [ 476.119033] ? __pfx_kthread+0x10/0x10
> >> [ 476.119035] ret_from_fork+0x34/0x50
> >> [ 476.119040] ? __pfx_kthread+0x10/0x10
> >> [ 476.119042] ret_from_fork_asm+0x1a/0x30
> >> [ 476.119049] </TASK>
> >> [ 476.142552] kobject: kobject_add_internal failed for BAT1 with -EEXIST, don't try to register things with the same name in the same directory.
> >> [ 476.415022] ata1.00: unexpected _GTF length (8)
> >> [ 476.428076] sd 0:0:0:0: [sda] Starting disk
> >> [ 476.835035] ata1.00: unexpected _GTF length (8)
> >> [ 476.839720] ata1.00: configured for UDMA/133
> >> [ 491.328831] sysfs: cannot create duplicate filename '/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0A:00/power_supply/BAT1'
> >> [ 491.329720] CPU: 1 UID: 0 PID: 185 Comm: kworker/1:4 Kdump: loaded Not tainted 6.12.38+deb13-amd64 #1 Debian 6.12.38-1
> >> [ 491.329727] Hardware name: Gateway NV44 /SJV40-MV , BIOS V1.3121 04/08/2009
> >> [ 491.329731] Workqueue: kacpi_notify acpi_os_execute_deferred
> >> [ 491.329741] Call Trace:
> >> [ 491.329745] <TASK>
> >> [ 491.329751] dump_stack_lvl+0x5d/0x80
> >> [ 491.329758] sysfs_warn_dup.cold+0x17/0x23
> >> [ 491.329762] sysfs_create_dir_ns+0xce/0xe0
> >> [ 491.329770] kobject_add_internal+0xba/0x250
> >> [ 491.329775] kobject_add+0x96/0xc0
> >> [ 491.329779] ? get_device_parent+0xde/0x1e0
> >> [ 491.329784] device_add+0xe2/0x870
> >> [ 491.329790] __power_supply_register.part.0+0x20f/0x3f0
> >> [ 491.329797] sysfs_add_battery+0xa4/0x1d0 [battery]
> >> [ 491.329805] acpi_battery_update+0x19e/0x290 [battery]
> >> [ 491.329809] acpi_battery_notify+0x50/0x120 [battery]
> >> [ 491.329812] acpi_ev_notify_dispatch+0x49/0x70
> >> [ 491.329817] acpi_os_execute_deferred+0x1a/0x30
> >> [ 491.329820] process_one_work+0x177/0x330
> >> [ 491.329826] worker_thread+0x251/0x390
> >> [ 491.329830] ? __pfx_worker_thread+0x10/0x10
> >> [ 491.329833] kthread+0xd2/0x100
> >> [ 491.329836] ? __pfx_kthread+0x10/0x10
> >> [ 491.329838] ret_from_fork+0x34/0x50
> >> [ 491.329842] ? __pfx_kthread+0x10/0x10
> >> [ 491.329844] ret_from_fork_asm+0x1a/0x30
> >> [ 491.329850] </TASK>
> >> [ 491.329855] kobject: kobject_add_internal failed for BAT1 with -EEXIST, don't try to register things with the same name in the same directory.
> >>
> >> Fixes: 10666251554c ("ACPI: battery: Install Notify() handler directly")
> >> Signed-off-by: GuangFei Luo <luogf2025@163.com>
> >> Cc: stable@vger.kernel.org
> >> ---
> >> v6:
> >> - Update Fixes tag: point to commit 10666251554c ("ACPI: battery: Install
> >> Notify() handler directly"), which introduced the sysfs_add_battery()
> >> re-entry issue when acpi_battery_notify is registered via
> >> acpi_dev_install_notify_handler(). The problem does not occur with
> >> acpi_bus_register_driver().
> >>
> >> v5:
> >> - Move changelog above the '---' line as per submission guidelines.
> >>
> >> v4:
> >> - Uses guard(mutex) for battery->sysfs_lock in sysfs_add_battery().
> >> - Since sysfs_add_battery() now handles the battery->bat check with
> >> proper locking,the extra if (!battery->bat) check at the call site
> >> has become redundant.
> >>
> >> v3:
> >> - Modified the earlier approach: since sysfs_add_battery() is invoked
> >> from multiple places, the most reliable way is to add the lock inside
> >> the function itself.
> >> - sysfs_remove_battery() had a similar race issue in the past, which was
> >> fixed by adding a lock as well. Reference:
> >> https://lore.kernel.org/all/9c921c22a7f33397a6774d7fa076db9b6a0fd669
> >> .1312318300.git.len.brown@intel.com/
> >>
> >> v2:
> >> - Fix missing mutex_unlock in acpi_battery_update()
> >> (Reported-by: kernel test robot)
> >>
> >> v1:
> >> - Initial patch to handle race when hotplugging battery, preventing
> >> duplicate sysfs entries.
> >> ---
> >> drivers/acpi/battery.c | 20 +++++++++++---------
> >> 1 file changed, 11 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> >> index 6905b56bf3e4..20d68f3e881f 100644
> >> --- a/drivers/acpi/battery.c
> >> +++ b/drivers/acpi/battery.c
> >> @@ -850,6 +850,10 @@ static void __exit battery_hook_exit(void)
> >>
> >> static int sysfs_add_battery(struct acpi_battery *battery)
> >> {
> >> + guard(mutex)(&battery->sysfs_lock);
> >> + if (battery->bat)
> >> + return 0;
> >> +
> >> struct power_supply_config psy_cfg = {
> >> .drv_data = battery,
> >> .attr_grp = acpi_battery_groups,
> >> @@ -1026,11 +1030,9 @@ static int acpi_battery_update(struct acpi_battery *battery, bool resume)
> >> return result;
> >> acpi_battery_quirks(battery);
> >>
> >> - if (!battery->bat) {
> >> - result = sysfs_add_battery(battery);
> >> - if (result)
> >> - return result;
> >> - }
> >> + result = sysfs_add_battery(battery);
> >> + if (result)
> >> + return result;
> >>
> >> /*
> >> * Wakeup the system if battery is critical low
> >> @@ -1112,12 +1114,12 @@ static int battery_notify(struct notifier_block *nb,
> >> result = acpi_battery_get_info(battery);
> >> if (result)
> >> return result;
> >> -
> >> - result = sysfs_add_battery(battery);
> >> - if (result)
> >> - return result;
> >> }
> >>
> >> + result = sysfs_add_battery(battery);
> >> + if (result)
> >> + return result;
> >> +
> >
> >Why is this change necessary?
> Hi Rafael,
>
> In the previous code:
>
> if (battery->bat) {
> acpi_battery_refresh(battery);
> } else {
> result = acpi_battery_get_info(battery);
> if (result)
> return result;
>
> result = sysfs_add_battery(battery);
> if (result)
> return result;
> }
>
> the `if (!battery->bat)` check was done at the call site. However,
> this check is not atomic: two threads can both see `battery->bat == NULL`
> and then call `sysfs_add_battery()` concurrently, leading to duplicate
> sysfs registration and `-EEXIST` errors.
>
> By moving the check and mutex into `sysfs_add_battery()`, the race is
> handled atomically. All call sites can now invoke it unconditionally,
> and the function itself will no-op if the battery is already registered.
> This avoids duplicated `if (!battery->bat)` logic and ensures consistent
> protection everywhere.
Since the lock is acquired in sysfs_add_battery(), the race is
addressed regardless of the change in question. The only difference
is when the previous battery->bat check passes (that is, battery->bat
is not NULL to start with), and if battery->bat becomes NULL between
the two checks, the entire section of the code checking battery->bat
needs to go under the lock.
I think that all battery->bat accesses need to be done under the lock
now, don't they?
^ permalink raw reply [flat|nested] 18+ messages in thread* Re:[PATCH v6] ACPI: battery: prevent sysfs_add_battery re-entry on rapid events
2025-09-23 11:48 ` Rafael J. Wysocki
@ 2025-09-23 16:13 ` GuangFei Luo
2025-09-23 17:12 ` [PATCH " Rafael J. Wysocki
0 siblings, 1 reply; 18+ messages in thread
From: GuangFei Luo @ 2025-09-23 16:13 UTC (permalink / raw)
To: rafael
Cc: michal.wilczynski, dan.carpenter, lenb, linux-acpi, linux-kernel,
linux-pm, lkp, sre, stable
The functions battery_hook_add_battery(), battery_hook_remove_battery(),
and sysfs_remove_battery() already acquire locks, so their internal
accesses are safe.
acpi_battery_refresh() does check battery->bat, but its child
functions (sysfs_add_battery() and sysfs_remove_battery()) already
handle locking.
In acpi_battery_notify(), battery->bat has no lock. However, the
check of battery->bat is at the very end of the function. During
earlier calls, battery->bat has already been protected by locks, so
re-entry will not cause issues.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6] ACPI: battery: prevent sysfs_add_battery re-entry on rapid events
2025-09-23 16:13 ` GuangFei Luo
@ 2025-09-23 17:12 ` Rafael J. Wysocki
2025-09-23 19:10 ` Rafael J. Wysocki
0 siblings, 1 reply; 18+ messages in thread
From: Rafael J. Wysocki @ 2025-09-23 17:12 UTC (permalink / raw)
To: GuangFei Luo
Cc: rafael, michal.wilczynski, dan.carpenter, lenb, linux-acpi,
linux-kernel, linux-pm, lkp, sre, stable
On Tue, Sep 23, 2025 at 6:14 PM GuangFei Luo <luogf2025@163.com> wrote:
>
> The functions battery_hook_add_battery(), battery_hook_remove_battery(),
> and sysfs_remove_battery() already acquire locks, so their internal
> accesses are safe.
In fact, there are two locks in use, battery->sysfs_lock and
hook_mutex. The latter is used for managing hooks and the former is
only used by sysfs_remove_battery(), so it only prevents that function
from racing with another instance of itself.
I would suggest using battery->sysfs_lock for protecting battery->bat
in general.
> acpi_battery_refresh() does check battery->bat, but its child
> functions (sysfs_add_battery() and sysfs_remove_battery()) already
> handle locking.
What locking? Before the $subject patch, sysfs_add_battery() doesn't
do any locking at all AFAICS.
> In acpi_battery_notify(), battery->bat has no lock. However, the
> check of battery->bat is at the very end of the function. During
> earlier calls, battery->bat has already been protected by locks, so
> re-entry will not cause issues.
All of the battery->bat checks and the code depending on them need to
go under the same lock. I'd use battery->sysfs_lock for this as
already mentioned above.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v6] ACPI: battery: prevent sysfs_add_battery re-entry on rapid events
2025-09-23 17:12 ` [PATCH " Rafael J. Wysocki
@ 2025-09-23 19:10 ` Rafael J. Wysocki
2025-09-23 22:38 ` GuangFei Luo
0 siblings, 1 reply; 18+ messages in thread
From: Rafael J. Wysocki @ 2025-09-23 19:10 UTC (permalink / raw)
To: GuangFei Luo; +Cc: dan.carpenter, linux-acpi, linux-kernel, linux-pm
On Tuesday, September 23, 2025 7:12:03 PM CEST Rafael J. Wysocki wrote:
> On Tue, Sep 23, 2025 at 6:14 PM GuangFei Luo <luogf2025@163.com> wrote:
> >
> > The functions battery_hook_add_battery(), battery_hook_remove_battery(),
> > and sysfs_remove_battery() already acquire locks, so their internal
> > accesses are safe.
>
> In fact, there are two locks in use, battery->sysfs_lock and
> hook_mutex. The latter is used for managing hooks and the former is
> only used by sysfs_remove_battery(), so it only prevents that function
> from racing with another instance of itself.
>
> I would suggest using battery->sysfs_lock for protecting battery->bat
> in general.
>
> > acpi_battery_refresh() does check battery->bat, but its child
> > functions (sysfs_add_battery() and sysfs_remove_battery()) already
> > handle locking.
>
> What locking? Before the $subject patch, sysfs_add_battery() doesn't
> do any locking at all AFAICS.
>
> > In acpi_battery_notify(), battery->bat has no lock. However, the
> > check of battery->bat is at the very end of the function. During
> > earlier calls, battery->bat has already been protected by locks, so
> > re-entry will not cause issues.
>
> All of the battery->bat checks and the code depending on them need to
> go under the same lock. I'd use battery->sysfs_lock for this as
> already mentioned above.
So my (untested) version of this fix is appended.
Note that it explicitly prevents acpi_battery_notify() from racing with
addition/removal, PM notifications, and resume.
---
drivers/acpi/battery.c | 36 +++++++++++++++++++++++-------------
1 file changed, 23 insertions(+), 13 deletions(-)
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -92,7 +92,7 @@ enum {
struct acpi_battery {
struct mutex lock;
- struct mutex sysfs_lock;
+ struct mutex update_lock;
struct power_supply *bat;
struct power_supply_desc bat_desc;
struct acpi_device *device;
@@ -904,15 +904,12 @@ static int sysfs_add_battery(struct acpi
static void sysfs_remove_battery(struct acpi_battery *battery)
{
- mutex_lock(&battery->sysfs_lock);
- if (!battery->bat) {
- mutex_unlock(&battery->sysfs_lock);
+ if (!battery->bat)
return;
- }
+
battery_hook_remove_battery(battery);
power_supply_unregister(battery->bat);
battery->bat = NULL;
- mutex_unlock(&battery->sysfs_lock);
}
static void find_battery(const struct dmi_header *dm, void *private)
@@ -1072,6 +1069,9 @@ static void acpi_battery_notify(acpi_han
if (!battery)
return;
+
+ guard(mutex)(&battery->update_lock);
+
old = battery->bat;
/*
* On Acer Aspire V5-573G notifications are sometimes triggered too
@@ -1094,21 +1094,22 @@ static void acpi_battery_notify(acpi_han
}
static int battery_notify(struct notifier_block *nb,
- unsigned long mode, void *_unused)
+ unsigned long mode, void *_unused)
{
struct acpi_battery *battery = container_of(nb, struct acpi_battery,
pm_nb);
- int result;
- switch (mode) {
- case PM_POST_HIBERNATION:
- case PM_POST_SUSPEND:
+ if (mode == PM_POST_SUSPEND || mode == PM_POST_HIBERNATION) {
+ guard(mutex)(&battery->update_lock);
+
if (!acpi_battery_present(battery))
return 0;
if (battery->bat) {
acpi_battery_refresh(battery);
} else {
+ int result;
+
result = acpi_battery_get_info(battery);
if (result)
return result;
@@ -1120,7 +1121,6 @@ static int battery_notify(struct notifie
acpi_battery_init_alarm(battery);
acpi_battery_get_state(battery);
- break;
}
return 0;
@@ -1198,6 +1198,8 @@ static int acpi_battery_update_retry(str
{
int retry, ret;
+ guard(mutex)(&battery->update_lock);
+
for (retry = 5; retry; retry--) {
ret = acpi_battery_update(battery, false);
if (!ret)
@@ -1230,7 +1232,7 @@ static int acpi_battery_add(struct acpi_
if (result)
return result;
- result = devm_mutex_init(&device->dev, &battery->sysfs_lock);
+ result = devm_mutex_init(&device->dev, &battery->update_lock);
if (result)
return result;
@@ -1262,6 +1264,8 @@ fail_pm:
device_init_wakeup(&device->dev, 0);
unregister_pm_notifier(&battery->pm_nb);
fail:
+ guard(mutex)(&battery->update_lock);
+
sysfs_remove_battery(battery);
return result;
@@ -1281,6 +1285,9 @@ static void acpi_battery_remove(struct a
device_init_wakeup(&device->dev, 0);
unregister_pm_notifier(&battery->pm_nb);
+
+ guard(mutex)(&battery->update_lock);
+
sysfs_remove_battery(battery);
}
@@ -1297,6 +1304,9 @@ static int acpi_battery_resume(struct de
return -EINVAL;
battery->update_time = 0;
+
+ guard(mutex)(&battery->update_lock);
+
acpi_battery_update(battery, true);
return 0;
}
^ permalink raw reply [flat|nested] 18+ messages in thread* Re:[PATCH v6] ACPI: battery: prevent sysfs_add_battery re-entry on rapid events
2025-09-23 19:10 ` Rafael J. Wysocki
@ 2025-09-23 22:38 ` GuangFei Luo
2025-09-24 10:40 ` [PATCH " Rafael J. Wysocki
0 siblings, 1 reply; 18+ messages in thread
From: GuangFei Luo @ 2025-09-23 22:38 UTC (permalink / raw)
To: rafael; +Cc: dan.carpenter, linux-acpi, linux-kernel, linux-pm, luogf2025
> On Tuesday, September 23, 2025 7:12:03 PM CEST Rafael J. Wysocki wrote:
> > On Tue, Sep 23, 2025 at 6:14 PM GuangFei Luo <luogf2025@163.com> wrote:
> > >
> > > The functions battery_hook_add_battery(), battery_hook_remove_battery(),
> > > and sysfs_remove_battery() already acquire locks, so their internal
> > > accesses are safe.
> >
> > In fact, there are two locks in use, battery->sysfs_lock and
> > hook_mutex. The latter is used for managing hooks and the former is
> > only used by sysfs_remove_battery(), so it only prevents that function
> > from racing with another instance of itself.
> >
> > I would suggest using battery->sysfs_lock for protecting battery->bat
> > in general.
> >
> > > acpi_battery_refresh() does check battery->bat, but its child
> > > functions (sysfs_add_battery() and sysfs_remove_battery()) already
> > > handle locking.
> >
> > What locking? Before the $subject patch, sysfs_add_battery() doesn't
> > do any locking at all AFAICS.
> >
> > > In acpi_battery_notify(), battery->bat has no lock. However, the
> > > check of battery->bat is at the very end of the function. During
> > > earlier calls, battery->bat has already been protected by locks, so
> > > re-entry will not cause issues.
> >
> > All of the battery->bat checks and the code depending on them need to
> > go under the same lock. I'd use battery->sysfs_lock for this as
> > already mentioned above.
>
> So my (untested) version of this fix is appended.
>
> Note that it explicitly prevents acpi_battery_notify() from racing with
> addition/removal, PM notifications, and resume.
>
> ---
> drivers/acpi/battery.c | 36 +++++++++++++++++++++++-------------
> 1 file changed, 23 insertions(+), 13 deletions(-)
>
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -92,7 +92,7 @@ enum {
>
> struct acpi_battery {
> struct mutex lock;
> - struct mutex sysfs_lock;
> + struct mutex update_lock;
> struct power_supply *bat;
> struct power_supply_desc bat_desc;
> struct acpi_device *device;
> @@ -904,15 +904,12 @@ static int sysfs_add_battery(struct acpi
>
> static void sysfs_remove_battery(struct acpi_battery *battery)
> {
> - mutex_lock(&battery->sysfs_lock);
> - if (!battery->bat) {
> - mutex_unlock(&battery->sysfs_lock);
> + if (!battery->bat)
> return;
> - }
> +
> battery_hook_remove_battery(battery);
> power_supply_unregister(battery->bat);
> battery->bat = NULL;
> - mutex_unlock(&battery->sysfs_lock);
> }
>
> static void find_battery(const struct dmi_header *dm, void *private)
> @@ -1072,6 +1069,9 @@ static void acpi_battery_notify(acpi_han
>
> if (!battery)
> return;
> +
> + guard(mutex)(&battery->update_lock);
> +
> old = battery->bat;
> /*
> * On Acer Aspire V5-573G notifications are sometimes triggered too
> @@ -1094,21 +1094,22 @@ static void acpi_battery_notify(acpi_han
> }
>
> static int battery_notify(struct notifier_block *nb,
> - unsigned long mode, void *_unused)
> + unsigned long mode, void *_unused)
> {
> struct acpi_battery *battery = container_of(nb, struct acpi_battery,
> pm_nb);
> - int result;
>
> - switch (mode) {
> - case PM_POST_HIBERNATION:
> - case PM_POST_SUSPEND:
> + if (mode == PM_POST_SUSPEND || mode == PM_POST_HIBERNATION) {
> + guard(mutex)(&battery->update_lock);
> +
> if (!acpi_battery_present(battery))
> return 0;
>
> if (battery->bat) {
> acpi_battery_refresh(battery);
> } else {
> + int result;
> +
> result = acpi_battery_get_info(battery);
> if (result)
> return result;
> @@ -1120,7 +1121,6 @@ static int battery_notify(struct notifie
>
> acpi_battery_init_alarm(battery);
> acpi_battery_get_state(battery);
> - break;
> }
>
> return 0;
> @@ -1198,6 +1198,8 @@ static int acpi_battery_update_retry(str
> {
> int retry, ret;
>
> + guard(mutex)(&battery->update_lock);
> +
> for (retry = 5; retry; retry--) {
> ret = acpi_battery_update(battery, false);
> if (!ret)
> @@ -1230,7 +1232,7 @@ static int acpi_battery_add(struct acpi_
> if (result)
> return result;
>
> - result = devm_mutex_init(&device->dev, &battery->sysfs_lock);
> + result = devm_mutex_init(&device->dev, &battery->update_lock);
> if (result)
> return result;
>
> @@ -1262,6 +1264,8 @@ fail_pm:
> device_init_wakeup(&device->dev, 0);
> unregister_pm_notifier(&battery->pm_nb);
> fail:
> + guard(mutex)(&battery->update_lock);
> +
> sysfs_remove_battery(battery);
>
> return result;
> @@ -1281,6 +1285,9 @@ static void acpi_battery_remove(struct a
>
> device_init_wakeup(&device->dev, 0);
> unregister_pm_notifier(&battery->pm_nb);
> +
> + guard(mutex)(&battery->update_lock);
> +
> sysfs_remove_battery(battery);
> }
>
> @@ -1297,6 +1304,9 @@ static int acpi_battery_resume(struct de
> return -EINVAL;
>
> battery->update_time = 0;
> +
> + guard(mutex)(&battery->update_lock);
> +
> acpi_battery_update(battery, true);
> return 0;
> }
Thanks for the detailed explanation and the updated version of the fix.
I will test your suggested changes on my platform.
After verification, I will send a v7 based on your suggestion.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v6] ACPI: battery: prevent sysfs_add_battery re-entry on rapid events
2025-09-23 22:38 ` GuangFei Luo
@ 2025-09-24 10:40 ` Rafael J. Wysocki
2025-09-24 14:10 ` GuangFei Luo
0 siblings, 1 reply; 18+ messages in thread
From: Rafael J. Wysocki @ 2025-09-24 10:40 UTC (permalink / raw)
To: GuangFei Luo; +Cc: rafael, dan.carpenter, linux-acpi, linux-kernel, linux-pm
On Wed, Sep 24, 2025 at 12:38 AM GuangFei Luo <luogf2025@163.com> wrote:
>
> > On Tuesday, September 23, 2025 7:12:03 PM CEST Rafael J. Wysocki wrote:
> > > On Tue, Sep 23, 2025 at 6:14 PM GuangFei Luo <luogf2025@163.com> wrote:
> > > >
> > > > The functions battery_hook_add_battery(), battery_hook_remove_battery(),
> > > > and sysfs_remove_battery() already acquire locks, so their internal
> > > > accesses are safe.
> > >
> > > In fact, there are two locks in use, battery->sysfs_lock and
> > > hook_mutex. The latter is used for managing hooks and the former is
> > > only used by sysfs_remove_battery(), so it only prevents that function
> > > from racing with another instance of itself.
> > >
> > > I would suggest using battery->sysfs_lock for protecting battery->bat
> > > in general.
> > >
> > > > acpi_battery_refresh() does check battery->bat, but its child
> > > > functions (sysfs_add_battery() and sysfs_remove_battery()) already
> > > > handle locking.
> > >
> > > What locking? Before the $subject patch, sysfs_add_battery() doesn't
> > > do any locking at all AFAICS.
> > >
> > > > In acpi_battery_notify(), battery->bat has no lock. However, the
> > > > check of battery->bat is at the very end of the function. During
> > > > earlier calls, battery->bat has already been protected by locks, so
> > > > re-entry will not cause issues.
> > >
> > > All of the battery->bat checks and the code depending on them need to
> > > go under the same lock. I'd use battery->sysfs_lock for this as
> > > already mentioned above.
> >
> > So my (untested) version of this fix is appended.
> >
> > Note that it explicitly prevents acpi_battery_notify() from racing with
> > addition/removal, PM notifications, and resume.
> >
> > ---
> > drivers/acpi/battery.c | 36 +++++++++++++++++++++++-------------
> > 1 file changed, 23 insertions(+), 13 deletions(-)
> >
> > --- a/drivers/acpi/battery.c
> > +++ b/drivers/acpi/battery.c
> > @@ -92,7 +92,7 @@ enum {
> >
> > struct acpi_battery {
> > struct mutex lock;
> > - struct mutex sysfs_lock;
> > + struct mutex update_lock;
> > struct power_supply *bat;
> > struct power_supply_desc bat_desc;
> > struct acpi_device *device;
> > @@ -904,15 +904,12 @@ static int sysfs_add_battery(struct acpi
> >
> > static void sysfs_remove_battery(struct acpi_battery *battery)
> > {
> > - mutex_lock(&battery->sysfs_lock);
> > - if (!battery->bat) {
> > - mutex_unlock(&battery->sysfs_lock);
> > + if (!battery->bat)
> > return;
> > - }
> > +
> > battery_hook_remove_battery(battery);
> > power_supply_unregister(battery->bat);
> > battery->bat = NULL;
> > - mutex_unlock(&battery->sysfs_lock);
> > }
> >
> > static void find_battery(const struct dmi_header *dm, void *private)
> > @@ -1072,6 +1069,9 @@ static void acpi_battery_notify(acpi_han
> >
> > if (!battery)
> > return;
> > +
> > + guard(mutex)(&battery->update_lock);
> > +
> > old = battery->bat;
> > /*
> > * On Acer Aspire V5-573G notifications are sometimes triggered too
> > @@ -1094,21 +1094,22 @@ static void acpi_battery_notify(acpi_han
> > }
> >
> > static int battery_notify(struct notifier_block *nb,
> > - unsigned long mode, void *_unused)
> > + unsigned long mode, void *_unused)
> > {
> > struct acpi_battery *battery = container_of(nb, struct acpi_battery,
> > pm_nb);
> > - int result;
> >
> > - switch (mode) {
> > - case PM_POST_HIBERNATION:
> > - case PM_POST_SUSPEND:
> > + if (mode == PM_POST_SUSPEND || mode == PM_POST_HIBERNATION) {
> > + guard(mutex)(&battery->update_lock);
> > +
> > if (!acpi_battery_present(battery))
> > return 0;
> >
> > if (battery->bat) {
> > acpi_battery_refresh(battery);
> > } else {
> > + int result;
> > +
> > result = acpi_battery_get_info(battery);
> > if (result)
> > return result;
> > @@ -1120,7 +1121,6 @@ static int battery_notify(struct notifie
> >
> > acpi_battery_init_alarm(battery);
> > acpi_battery_get_state(battery);
> > - break;
> > }
> >
> > return 0;
> > @@ -1198,6 +1198,8 @@ static int acpi_battery_update_retry(str
> > {
> > int retry, ret;
> >
> > + guard(mutex)(&battery->update_lock);
> > +
> > for (retry = 5; retry; retry--) {
> > ret = acpi_battery_update(battery, false);
> > if (!ret)
> > @@ -1230,7 +1232,7 @@ static int acpi_battery_add(struct acpi_
> > if (result)
> > return result;
> >
> > - result = devm_mutex_init(&device->dev, &battery->sysfs_lock);
> > + result = devm_mutex_init(&device->dev, &battery->update_lock);
> > if (result)
> > return result;
> >
> > @@ -1262,6 +1264,8 @@ fail_pm:
> > device_init_wakeup(&device->dev, 0);
> > unregister_pm_notifier(&battery->pm_nb);
> > fail:
> > + guard(mutex)(&battery->update_lock);
> > +
> > sysfs_remove_battery(battery);
> >
> > return result;
> > @@ -1281,6 +1285,9 @@ static void acpi_battery_remove(struct a
> >
> > device_init_wakeup(&device->dev, 0);
> > unregister_pm_notifier(&battery->pm_nb);
> > +
> > + guard(mutex)(&battery->update_lock);
> > +
> > sysfs_remove_battery(battery);
> > }
> >
> > @@ -1297,6 +1304,9 @@ static int acpi_battery_resume(struct de
> > return -EINVAL;
> >
> > battery->update_time = 0;
> > +
> > + guard(mutex)(&battery->update_lock);
> > +
> > acpi_battery_update(battery, true);
> > return 0;
> > }
>
> Thanks for the detailed explanation and the updated version of the fix.
>
> I will test your suggested changes on my platform.
> After verification, I will send a v7 based on your suggestion.
Please just verify and I'll add a changelog and subject to the patch
and submit it.
Thanks!
^ permalink raw reply [flat|nested] 18+ messages in thread* Re:[PATCH v6] ACPI: battery: prevent sysfs_add_battery re-entry on rapid events
2025-09-24 10:40 ` [PATCH " Rafael J. Wysocki
@ 2025-09-24 14:10 ` GuangFei Luo
2025-09-24 17:22 ` [PATCH " Rafael J. Wysocki
0 siblings, 1 reply; 18+ messages in thread
From: GuangFei Luo @ 2025-09-24 14:10 UTC (permalink / raw)
To: rafael; +Cc: dan.carpenter, linux-acpi, linux-kernel, linux-pm, luogf2025
> On Wed, Sep 24, 2025 at 12:38 AM GuangFei Luo <luogf2025@163.com> wrote:
> >
> > > On Tuesday, September 23, 2025 7:12:03 PM CEST Rafael J. Wysocki wrote:
> > > > On Tue, Sep 23, 2025 at 6:14 PM GuangFei Luo <luogf2025@163.com> wrote:
> > > > >
> > > > > The functions battery_hook_add_battery(), battery_hook_remove_battery(),
> > > > > and sysfs_remove_battery() already acquire locks, so their internal
> > > > > accesses are safe.
> > > >
> > > > In fact, there are two locks in use, battery->sysfs_lock and
> > > > hook_mutex. The latter is used for managing hooks and the former is
> > > > only used by sysfs_remove_battery(), so it only prevents that function
> > > > from racing with another instance of itself.
> > > >
> > > > I would suggest using battery->sysfs_lock for protecting battery->bat
> > > > in general.
> > > >
> > > > > acpi_battery_refresh() does check battery->bat, but its child
> > > > > functions (sysfs_add_battery() and sysfs_remove_battery()) already
> > > > > handle locking.
> > > >
> > > > What locking? Before the $subject patch, sysfs_add_battery() doesn't
> > > > do any locking at all AFAICS.
> > > >
> > > > > In acpi_battery_notify(), battery->bat has no lock. However, the
> > > > > check of battery->bat is at the very end of the function. During
> > > > > earlier calls, battery->bat has already been protected by locks, so
> > > > > re-entry will not cause issues.
> > > >
> > > > All of the battery->bat checks and the code depending on them need to
> > > > go under the same lock. I'd use battery->sysfs_lock for this as
> > > > already mentioned above.
> > >
> > > So my (untested) version of this fix is appended.
> > >
> > > Note that it explicitly prevents acpi_battery_notify() from racing with
> > > addition/removal, PM notifications, and resume.
> > >
> > > ---
> > > drivers/acpi/battery.c | 36 +++++++++++++++++++++++-------------
> > > 1 file changed, 23 insertions(+), 13 deletions(-)
> > >
> > > --- a/drivers/acpi/battery.c
> > > +++ b/drivers/acpi/battery.c
> > > @@ -92,7 +92,7 @@ enum {
> > >
> > > struct acpi_battery {
> > > struct mutex lock;
> > > - struct mutex sysfs_lock;
> > > + struct mutex update_lock;
> > > struct power_supply *bat;
> > > struct power_supply_desc bat_desc;
> > > struct acpi_device *device;
> > > @@ -904,15 +904,12 @@ static int sysfs_add_battery(struct acpi
> > >
> > > static void sysfs_remove_battery(struct acpi_battery *battery)
> > > {
> > > - mutex_lock(&battery->sysfs_lock);
> > > - if (!battery->bat) {
> > > - mutex_unlock(&battery->sysfs_lock);
> > > + if (!battery->bat)
> > > return;
> > > - }
> > > +
> > > battery_hook_remove_battery(battery);
> > > power_supply_unregister(battery->bat);
> > > battery->bat = NULL;
> > > - mutex_unlock(&battery->sysfs_lock);
> > > }
> > >
> > > static void find_battery(const struct dmi_header *dm, void *private)
> > > @@ -1072,6 +1069,9 @@ static void acpi_battery_notify(acpi_han
> > >
> > > if (!battery)
> > > return;
> > > +
> > > + guard(mutex)(&battery->update_lock);
> > > +
> > > old = battery->bat;
> > > /*
> > > * On Acer Aspire V5-573G notifications are sometimes triggered too
> > > @@ -1094,21 +1094,22 @@ static void acpi_battery_notify(acpi_han
> > > }
> > >
> > > static int battery_notify(struct notifier_block *nb,
> > > - unsigned long mode, void *_unused)
> > > + unsigned long mode, void *_unused)
> > > {
> > > struct acpi_battery *battery = container_of(nb, struct acpi_battery,
> > > pm_nb);
> > > - int result;
> > >
> > > - switch (mode) {
> > > - case PM_POST_HIBERNATION:
> > > - case PM_POST_SUSPEND:
> > > + if (mode == PM_POST_SUSPEND || mode == PM_POST_HIBERNATION) {
> > > + guard(mutex)(&battery->update_lock);
> > > +
> > > if (!acpi_battery_present(battery))
> > > return 0;
> > >
> > > if (battery->bat) {
> > > acpi_battery_refresh(battery);
> > > } else {
> > > + int result;
> > > +
> > > result = acpi_battery_get_info(battery);
> > > if (result)
> > > return result;
> > > @@ -1120,7 +1121,6 @@ static int battery_notify(struct notifie
> > >
> > > acpi_battery_init_alarm(battery);
> > > acpi_battery_get_state(battery);
> > > - break;
> > > }
> > >
> > > return 0;
> > > @@ -1198,6 +1198,8 @@ static int acpi_battery_update_retry(str
> > > {
> > > int retry, ret;
> > >
> > > + guard(mutex)(&battery->update_lock);
> > > +
> > > for (retry = 5; retry; retry--) {
> > > ret = acpi_battery_update(battery, false);
> > > if (!ret)
> > > @@ -1230,7 +1232,7 @@ static int acpi_battery_add(struct acpi_
> > > if (result)
> > > return result;
> > >
> > > - result = devm_mutex_init(&device->dev, &battery->sysfs_lock);
> > > + result = devm_mutex_init(&device->dev, &battery->update_lock);
> > > if (result)
> > > return result;
> > >
> > > @@ -1262,6 +1264,8 @@ fail_pm:
> > > device_init_wakeup(&device->dev, 0);
> > > unregister_pm_notifier(&battery->pm_nb);
> > > fail:
> > > + guard(mutex)(&battery->update_lock);
> > > +
> > > sysfs_remove_battery(battery);
> > >
> > > return result;
> > > @@ -1281,6 +1285,9 @@ static void acpi_battery_remove(struct a
> > >
> > > device_init_wakeup(&device->dev, 0);
> > > unregister_pm_notifier(&battery->pm_nb);
> > > +
> > > + guard(mutex)(&battery->update_lock);
> > > +
> > > sysfs_remove_battery(battery);
> > > }
> > >
> > > @@ -1297,6 +1304,9 @@ static int acpi_battery_resume(struct de
> > > return -EINVAL;
> > >
> > > battery->update_time = 0;
> > > +
> > > + guard(mutex)(&battery->update_lock);
> > > +
> > > acpi_battery_update(battery, true);
> > > return 0;
> > > }
> >
> > Thanks for the detailed explanation and the updated version of the fix.
> >
> > I will test your suggested changes on my platform.
> > After verification, I will send a v7 based on your suggestion.
>
> Please just verify and I'll add a changelog and subject to the patch
> and submit it.
>
> Thanks!
I have tested your updated patch on my laptop with battery hot-plug scenarios.
Everything looks normal and I did not observe any issues.
Thanks!
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v6] ACPI: battery: prevent sysfs_add_battery re-entry on rapid events
2025-09-24 14:10 ` GuangFei Luo
@ 2025-09-24 17:22 ` Rafael J. Wysocki
0 siblings, 0 replies; 18+ messages in thread
From: Rafael J. Wysocki @ 2025-09-24 17:22 UTC (permalink / raw)
To: GuangFei Luo; +Cc: rafael, dan.carpenter, linux-acpi, linux-kernel, linux-pm
On Wed, Sep 24, 2025 at 4:11 PM GuangFei Luo <luogf2025@163.com> wrote:
>
> > On Wed, Sep 24, 2025 at 12:38 AM GuangFei Luo <luogf2025@163.com> wrote:
> > >
> > > > On Tuesday, September 23, 2025 7:12:03 PM CEST Rafael J. Wysocki wrote:
> > > > > On Tue, Sep 23, 2025 at 6:14 PM GuangFei Luo <luogf2025@163.com> wrote:
> > > > > >
> > > > > > The functions battery_hook_add_battery(), battery_hook_remove_battery(),
> > > > > > and sysfs_remove_battery() already acquire locks, so their internal
> > > > > > accesses are safe.
> > > > >
> > > > > In fact, there are two locks in use, battery->sysfs_lock and
> > > > > hook_mutex. The latter is used for managing hooks and the former is
> > > > > only used by sysfs_remove_battery(), so it only prevents that function
> > > > > from racing with another instance of itself.
> > > > >
> > > > > I would suggest using battery->sysfs_lock for protecting battery->bat
> > > > > in general.
> > > > >
> > > > > > acpi_battery_refresh() does check battery->bat, but its child
> > > > > > functions (sysfs_add_battery() and sysfs_remove_battery()) already
> > > > > > handle locking.
> > > > >
> > > > > What locking? Before the $subject patch, sysfs_add_battery() doesn't
> > > > > do any locking at all AFAICS.
> > > > >
> > > > > > In acpi_battery_notify(), battery->bat has no lock. However, the
> > > > > > check of battery->bat is at the very end of the function. During
> > > > > > earlier calls, battery->bat has already been protected by locks, so
> > > > > > re-entry will not cause issues.
> > > > >
> > > > > All of the battery->bat checks and the code depending on them need to
> > > > > go under the same lock. I'd use battery->sysfs_lock for this as
> > > > > already mentioned above.
> > > >
> > > > So my (untested) version of this fix is appended.
> > > >
> > > > Note that it explicitly prevents acpi_battery_notify() from racing with
> > > > addition/removal, PM notifications, and resume.
> > > >
> > > > ---
> > > > drivers/acpi/battery.c | 36 +++++++++++++++++++++++-------------
> > > > 1 file changed, 23 insertions(+), 13 deletions(-)
> > > >
> > > > --- a/drivers/acpi/battery.c
> > > > +++ b/drivers/acpi/battery.c
> > > > @@ -92,7 +92,7 @@ enum {
> > > >
> > > > struct acpi_battery {
> > > > struct mutex lock;
> > > > - struct mutex sysfs_lock;
> > > > + struct mutex update_lock;
> > > > struct power_supply *bat;
> > > > struct power_supply_desc bat_desc;
> > > > struct acpi_device *device;
> > > > @@ -904,15 +904,12 @@ static int sysfs_add_battery(struct acpi
> > > >
> > > > static void sysfs_remove_battery(struct acpi_battery *battery)
> > > > {
> > > > - mutex_lock(&battery->sysfs_lock);
> > > > - if (!battery->bat) {
> > > > - mutex_unlock(&battery->sysfs_lock);
> > > > + if (!battery->bat)
> > > > return;
> > > > - }
> > > > +
> > > > battery_hook_remove_battery(battery);
> > > > power_supply_unregister(battery->bat);
> > > > battery->bat = NULL;
> > > > - mutex_unlock(&battery->sysfs_lock);
> > > > }
> > > >
> > > > static void find_battery(const struct dmi_header *dm, void *private)
> > > > @@ -1072,6 +1069,9 @@ static void acpi_battery_notify(acpi_han
> > > >
> > > > if (!battery)
> > > > return;
> > > > +
> > > > + guard(mutex)(&battery->update_lock);
> > > > +
> > > > old = battery->bat;
> > > > /*
> > > > * On Acer Aspire V5-573G notifications are sometimes triggered too
> > > > @@ -1094,21 +1094,22 @@ static void acpi_battery_notify(acpi_han
> > > > }
> > > >
> > > > static int battery_notify(struct notifier_block *nb,
> > > > - unsigned long mode, void *_unused)
> > > > + unsigned long mode, void *_unused)
> > > > {
> > > > struct acpi_battery *battery = container_of(nb, struct acpi_battery,
> > > > pm_nb);
> > > > - int result;
> > > >
> > > > - switch (mode) {
> > > > - case PM_POST_HIBERNATION:
> > > > - case PM_POST_SUSPEND:
> > > > + if (mode == PM_POST_SUSPEND || mode == PM_POST_HIBERNATION) {
> > > > + guard(mutex)(&battery->update_lock);
> > > > +
> > > > if (!acpi_battery_present(battery))
> > > > return 0;
> > > >
> > > > if (battery->bat) {
> > > > acpi_battery_refresh(battery);
> > > > } else {
> > > > + int result;
> > > > +
> > > > result = acpi_battery_get_info(battery);
> > > > if (result)
> > > > return result;
> > > > @@ -1120,7 +1121,6 @@ static int battery_notify(struct notifie
> > > >
> > > > acpi_battery_init_alarm(battery);
> > > > acpi_battery_get_state(battery);
> > > > - break;
> > > > }
> > > >
> > > > return 0;
> > > > @@ -1198,6 +1198,8 @@ static int acpi_battery_update_retry(str
> > > > {
> > > > int retry, ret;
> > > >
> > > > + guard(mutex)(&battery->update_lock);
> > > > +
> > > > for (retry = 5; retry; retry--) {
> > > > ret = acpi_battery_update(battery, false);
> > > > if (!ret)
> > > > @@ -1230,7 +1232,7 @@ static int acpi_battery_add(struct acpi_
> > > > if (result)
> > > > return result;
> > > >
> > > > - result = devm_mutex_init(&device->dev, &battery->sysfs_lock);
> > > > + result = devm_mutex_init(&device->dev, &battery->update_lock);
> > > > if (result)
> > > > return result;
> > > >
> > > > @@ -1262,6 +1264,8 @@ fail_pm:
> > > > device_init_wakeup(&device->dev, 0);
> > > > unregister_pm_notifier(&battery->pm_nb);
> > > > fail:
> > > > + guard(mutex)(&battery->update_lock);
> > > > +
> > > > sysfs_remove_battery(battery);
> > > >
> > > > return result;
> > > > @@ -1281,6 +1285,9 @@ static void acpi_battery_remove(struct a
> > > >
> > > > device_init_wakeup(&device->dev, 0);
> > > > unregister_pm_notifier(&battery->pm_nb);
> > > > +
> > > > + guard(mutex)(&battery->update_lock);
> > > > +
> > > > sysfs_remove_battery(battery);
> > > > }
> > > >
> > > > @@ -1297,6 +1304,9 @@ static int acpi_battery_resume(struct de
> > > > return -EINVAL;
> > > >
> > > > battery->update_time = 0;
> > > > +
> > > > + guard(mutex)(&battery->update_lock);
> > > > +
> > > > acpi_battery_update(battery, true);
> > > > return 0;
> > > > }
> > >
> > > Thanks for the detailed explanation and the updated version of the fix.
> > >
> > > I will test your suggested changes on my platform.
> > > After verification, I will send a v7 based on your suggestion.
> >
> > Please just verify and I'll add a changelog and subject to the patch
> > and submit it.
> >
> > Thanks!
>
> I have tested your updated patch on my laptop with battery hot-plug scenarios.
> Everything looks normal and I did not observe any issues.
Thanks for the confirmation!
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re:[PATCH v6] ACPI: battery: prevent sysfs_add_battery re-entry on rapid events
[not found] <20250910142653.313360-1-luogf2025@163.com>
` (3 preceding siblings ...)
2025-09-14 11:12 ` [PATCH v6] " GuangFei Luo
@ 2025-09-23 16:18 ` GuangFei Luo
4 siblings, 0 replies; 18+ messages in thread
From: GuangFei Luo @ 2025-09-23 16:18 UTC (permalink / raw)
To: rafael
Cc: michal.wilczynski, dan.carpenter, lenb, linux-acpi, linux-kernel,
linux-pm, lkp, sre, stable
Yes, in my tests with battery hot-plug, I observed that
acpi_battery_notify() can be triggered multiple times in quick
succession. Only adding a lock inside acpi_battery_notify()
is sufficient.
The functions battery_hook_add_battery(), battery_hook_remove_battery(),
and sysfs_remove_battery() already acquire locks, so their internal
accesses are safe.
acpi_battery_refresh() does check battery->bat, but its child
functions (sysfs_add_battery() and sysfs_remove_battery()) already
handle locking.
In acpi_battery_notify(), battery->bat has no lock. However, the
check of battery->bat is at the very end of the function. During
earlier calls, battery->bat has already been protected by locks, so
re-entry will not cause issues.
^ permalink raw reply [flat|nested] 18+ messages in thread