Hello Robie,

Thank you for reviewing this!

I will update the test plan methodology to cover unattended upgrades
and/or any other application that makes use of the script.

1. > There are spurious development artifacts being added

I have looked through the debdiffs and couldn't find such, are you
referring to the "Maintainer" and "XSBC-Original-Maintainer" section
maybe?

2. > This does a glob expansion right here...

Yes this is intended behavior as USBC is not the only USB power type, it
is also the same value used before my changes.

3. >  Wouldn't that incorrectly identify the system as being on AC
power?

Yes you are correct! however this bug also exists in systemd. Systemd
also checks if any of the ports are sink first before looking at all the
USB power sources. The reason this works is because a USB-C port that is
in source more should NOT show up as online under
/sys/class/power_supply/* because only ports in sink more are supposed
to show their online status here (ports in source mode will show up in
the directory but will have their "online" state as 0 even if a device
is plugged in to them). The reason I wrote this patch is because some of
these source ports are mirroring their connection state inside the
"online" sysfs. Discrete GPUs are the biggest culprits where those with
USB-C ports are having their connection status mirrored inside
"/sys/class/power_supply/ucsi-*/online" although they are labeled as a
source device. Here is an upstream issue from systemd discussing such a
case: https://github.com/systemd/systemd/issues/21988

Also here is proof from my system that systemd does indeed only require
one port in sink mode to make USBC a valid power delivery option (my
system has 4 thunderbolt ports that can all be used for charging):

ghadi@XPS-17-9720 ~ » cat /sys/class/typec/*/power_role
[source] sink
[source] sink
source [sink]
source [sink]

ghadi@XPS-17-9720 ~ » cat /sys/class/power_supply/ucsi-*/online
0
0
0
1

ghadi@XPS-17-9720 ~ » SYSTEMD_LOG_LEVEL=debug systemd-ac-power 
BAT0: The battery status is 'Charging', assuming the battery is not used as a 
power source of this machine.
hidpp_battery_1: The power supply is a device battery, ignoring device.
AC: The power supply is currently online.
port0: The USB type-C port is in power source mode.
port1: The USB type-C port is in power source mode.
port2: The USB type-C port is in power sink mode.
port3: The USB type-C port is in power sink mode.
ucsi-source-psy-USBC000:001: The USB type-C device has at least one port in 
power sink mode.
ucsi-source-psy-USBC000:001: The power supply is currently offline.
port0: The USB type-C port is in power source mode.
port1: The USB type-C port is in power source mode.
port2: The USB type-C port is in power sink mode.
port3: The USB type-C port is in power sink mode.
ucsi-source-psy-USBC000:002: The USB type-C device has at least one port in 
power sink mode.
ucsi-source-psy-USBC000:002: The power supply is currently offline.
port0: The USB type-C port is in power source mode.
port1: The USB type-C port is in power source mode.
port2: The USB type-C port is in power sink mode.
port3: The USB type-C port is in power sink mode.
ucsi-source-psy-USBC000:003: The USB type-C device has at least one port in 
power sink mode.
ucsi-source-psy-USBC000:003: The power supply is currently offline.
port0: The USB type-C port is in power source mode.
port1: The USB type-C port is in power source mode.
port2: The USB type-C port is in power sink mode.
port3: The USB type-C port is in power sink mode.
ucsi-source-psy-USBC000:004: The USB type-C device has at least one port in 
power sink mode.
ucsi-source-psy-USBC000:004: The power supply is currently online.
Found at least one online non-battery power supply, system is running on AC.


You can see how all the ucsi devices are being labeled as having at
least one power sink device:

"ucsi-source-psy-USBC000:00*: The USB type-C device has at least one
port in power sink mode."

Although I only have two ports in power sink mode. Systemd is also
clearly able to identify which ports are in source mode and which are in
sink mode:

```
port0: The USB type-C port is in power source mode.
port1: The USB type-C port is in power source mode.
port2: The USB type-C port is in power sink mode.
port3: The USB type-C port is in power sink mode.

```
The only difference between my script and how systemd checks for the ports that 
are in sink mode, is that I check this behavior by accessing 
"/sys/class/typec/*" once and apply this rule for all ucsi devices, while 
systemd 
accesses"/sys/class/power_supply/ucsi-source-psy-USBC000:00*/device/typec/*" 
for each ucsi device.
However from my testing on two machines I noticed that these two directories 
are the same (minus port partner ports being in /sys/class/typec/*) and so I 
took the shortcut of only checking the status once.

I can revert this change and have it behave exactly as systemd does, in
case there might be scenarios where the two directories are not the
same.

Let me know what you think of the above proposition.


** Bug watch added: github.com/systemd/systemd/issues #21988
   https://github.com/systemd/systemd/issues/21988

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1980991

Title:
  /usr/sbin/on_ac_power incorrectly reporting ac power status

To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/powermgmt-base/+bug/1980991/+subscriptions


-- 
ubuntu-bugs mailing list
[email protected]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

Reply via email to