Index | Thread | Search

From:
YASUOKA Masahiko <yasuoka@openbsd.org>
Subject:
Re: diff: trigger acpiac_refresh when acpibat notification
To:
jca@wxcvbn.org
Cc:
jcs@jcs.org, tech@openbsd.org
Date:
Mon, 02 Dec 2024 09:29:02 +0900

Download raw body.

Thread
Hi,

On Fri, 29 Nov 2024 18:26:32 +0100
Jeremie Courreges-Anglas <jca@wxcvbn.org> wrote:
> On Fri, Nov 29, 2024 at 12:19:22AM +0900, YASUOKA Masahiko wrote:
>> Let me ping this diff.
>> The diff becomes more important when we have hw.perfpolicy for me.
>> 
>> ok? comments?

The same workaround was committed on June 24th by mglocker@

https://marc.info/?l=openbsd-cvs&m=171924457208939&w=2
or https://github.com/openbsd/src/commit/6a17c0b7bcb83b86d761af696b661c135483e739:
|Some machines send AC change notifications to acpibat(4).  Forward this
|notification to acpiac(4), so that the AC status can be reflected correctly
|to programs like apm(8).
|
|This for example fixes the AC status on the Microsoft Surface Go 4.

I didn't notice this commit.  So my diff is not needed anymore.  I'm
sorry for disturbing you.

> Seems like a pragmatic approach, but please see comments inline.
> 
>> On Thu, 17 Aug 2023 16:12:07 +0900 (JST)
>> YASUOKA Masahiko <yasuoka@openbsd.org> wrote:
>> > Hi,
>> > 
>> > Update the AC status when the battery notification is happened.
>> > Because the AC status notification doesn't happen on some machines.
>> > My vaio actually has this problem.
>> > 
>> > Also Linux is doing the same thing
>> > 
>> > https://github.com/torvalds/linux/blob/v6.4/drivers/acpi/ac.c#L165-L183
>> 
>> Index: sys/dev/acpi/acpiac.c
>> ===================================================================
>> RCS file: /cvs/src/sys/dev/acpi/acpiac.c,v
>> diff -u -p -r1.36 acpiac.c
>> --- sys/dev/acpi/acpiac.c	6 Apr 2022 18:59:27 -0000	1.36
>> +++ sys/dev/acpi/acpiac.c	28 Nov 2024 15:17:44 -0000
>> @@ -140,6 +140,8 @@ acpiac_getpsr(struct acpiac_softc *sc)
>>  	return (0);
>>  }
>>  
>> +static int acpiac_notify_triggered = 0;
>> +
>>  int
>>  acpiac_notify(struct aml_node *node, int notify_type, void *arg)
>>  {
>> @@ -148,6 +150,8 @@ acpiac_notify(struct aml_node *node, int
>>  	dnprintf(10, "acpiac_notify: %.2x %s\n", notify_type,
>>  	    DEVNAME(sc));
>>  
>> +	acpiac_notify_triggered = 1;
>> +
>>  	switch (notify_type) {
>>  	case 0x00:
>>  	case 0x01:
> 
> Here's the full function body,
> ---
> int
> acpiac_notify(struct aml_node *node, int notify_type, void *arg)
> {
>         struct acpiac_softc *sc = arg;
> 
>         dnprintf(10, "acpiac_notify: %.2x %s\n", notify_type,
>             DEVNAME(sc));
> 
>         switch (notify_type) {
>         case 0x00:
>         case 0x01:
>         case 0x81:
>                 /*
>                  * XXX some sony vaio's use the wrong notify type
>                  * work around it by honoring it as a 0x80
>                  */
>                 /* FALLTHROUGH */
>         case 0x80:
>                 acpiac_refresh(sc);
>                 acpi_record_event(sc->sc_acpi, APM_POWER_CHANGE);
>                 dnprintf(10, "A/C status: %d\n", sc->sc_ac_stat);
>                 break;
>         }
>         return (0);
> }
> ---

I checked if acpiac_notify() happens. On my recent vaio, any
notification never happens.

> Maybe your Vaio uses yet another notification type?  Did you try a
> kernel built with ACPI_DEBUG?
> 
>> @@ -164,4 +168,22 @@ acpiac_notify(struct aml_node *node, int
>>  		break;
>>  	}
>>  	return (0);
>> +}
>> +
>> +void
>> +acpiac_battery_notify(void)
>> +{
>> +	struct acpi_softc *sc = acpi_softc;
>> +	struct acpi_ac *ac;
>> +
>> +	if (acpiac_notify_triggered)
>> +		return;
>> +	/*
>> +	 * On some machines (vaio VJPK23 at least) AC status notifications
>> +	 * are not triggered.  Update the AC status when battery notifications.
> 
> s/when/on/ ?

Thanks,

>> +	 */
>> +	SLIST_FOREACH(ac, &sc->sc_ac, aac_link) {
>> +		acpiac_refresh(ac->aac_softc);
>> +		acpi_record_event(sc, APM_POWER_CHANGE);
>> +	}
>>  }
>> Index: sys/dev/acpi/acpibat.c
>> ===================================================================
>> RCS file: /cvs/src/sys/dev/acpi/acpibat.c,v
>> diff -u -p -r1.72 acpibat.c
>> --- sys/dev/acpi/acpibat.c	5 Aug 2024 18:37:29 -0000	1.72
>> +++ sys/dev/acpi/acpibat.c	28 Nov 2024 15:17:44 -0000
>> @@ -540,5 +540,7 @@ acpibat_notify(struct aml_node *node, in
>>  	acpibat_refresh(sc);
>>  	acpi_record_event(sc->sc_acpi, APM_POWER_CHANGE);
>>  
>> +	acpiac_battery_notify();
> 
> I'd suggest moving the acpiac_battery_notify() call between
> acpibat_refresh() and acpi_record_event().  Thay way you don't need to
> send an extra APM_POWER_CHANGE event from the acpiac_battery_notify()
> function.
> 
>> +
>>  	return (0);
>>  }
>> Index: sys/dev/acpi/acpidev.h
>> ===================================================================
>> RCS file: /cvs/src/sys/dev/acpi/acpidev.h,v
>> diff -u -p -r1.45 acpidev.h
>> --- sys/dev/acpi/acpidev.h	6 Aug 2024 17:38:56 -0000	1.45
>> +++ sys/dev/acpi/acpidev.h	28 Nov 2024 15:17:44 -0000
>> @@ -307,6 +307,8 @@ struct acpiac_softc {
>>  	struct ksensordev	sc_sensdev;
>>  };
>>  
>> +void acpiac_battery_notify(void);
>> +
>>  struct acpibat_softc {
>>  	struct device		sc_dev;
>>  
>> 
> 
> -- 
> jca
>