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

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

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!

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

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!