Bug 215799 - apple-mfi-fastcharge causes automatic pm hibernation entry, when iPhone gets attached
Summary: apple-mfi-fastcharge causes automatic pm hibernation entry, when iPhone gets ...
Status: RESOLVED INVALID
Alias: None
Product: Drivers
Classification: Unclassified
Component: USB (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: Default virtual assignee for Drivers/USB
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-04-04 22:22 UTC by Manuel Ullmann
Modified: 2022-04-07 22:10 UTC (History)
2 users (show)

See Also:
Kernel Version: 5.17.1
Subsystem:
Regression: No
Bisected commit-id:


Attachments
dmesg log during a boot with an iPhone attached (11.04 KB, text/plain)
2022-04-04 22:22 UTC, Manuel Ullmann
Details
iPhone SE lsusb entry, while apple-mfi-fastcharge is unloaded (15.92 KB, text/plain)
2022-04-04 22:23 UTC, Manuel Ullmann
Details
correction of type of power source (1.07 KB, patch)
2022-04-05 09:42 UTC, Oliver Neukum
Details | Diff

Description Manuel Ullmann 2022-04-04 22:22:15 UTC
Created attachment 300695 [details]
dmesg log during a boot with an iPhone attached

=======================
Summary
=======================

If I attach an iPhone SE while the apple-mfi-fastcharge module is not blacklisted, the kernel will immediately hibernate. Loading the module without an attached iPhone does not have this effect. dmesg is not very helpful in this case, probably I need a higher debug level. I’ll attach it anyway with some context.

=======================
Steps to reproduce
=======================
1. Attach an iPhone SE (don’t have any others) via USB-A to Lightning cable while apple-mfi-fastcharge is either builtin, loaded, or can be loaded automatically, i.e. is not blacklisted.

=======================
Actual behaviour
=======================
The kernel automatically hibernates without user interaction.

=======================
Expected behaviour
=======================
The kernel continues normal operation.

=======================
Additional information
=======================
The cable has μUSB→Lightning and μUSB→USB C adapters attached. It works fine with an Android phone and with the iPhone SE without the apple-mfi-fastcharge module loaded, so it is not faulty.
Comment 1 Manuel Ullmann 2022-04-04 22:23:24 UTC
Created attachment 300696 [details]
iPhone SE lsusb entry, while apple-mfi-fastcharge is unloaded
Comment 2 Oliver Neukum 2022-04-05 08:26:29 UTC
Working theory:

You are misinterpreting the system's reaction. The hibernation is not done automatically by the kernel. Instead the kernel reports a charger as a battery. That battery, being a charger, reports a charge of zero. User space monitors this and does as it should if its only known power source is a battery at zero charge: it hibernates the system.
Comment 3 Oliver Neukum 2022-04-05 09:42:34 UTC
Created attachment 300701 [details]
correction of type of power source

Please test this patch
Comment 4 Manuel Ullmann 2022-04-06 01:56:33 UTC
(In reply to Oliver Neukum from comment #2)
> Working theory:
> 
> You are misinterpreting the system's reaction. The hibernation is not done
> automatically by the kernel. Instead the kernel reports a charger as a
> battery. That battery, being a charger, reports a charge of zero. User space
> monitors this and does as it should if its only known power source is a
> battery at zero charge: it hibernates the system.

Your theory is correct. The laptop-mode-tools auto-hibernation facility causes the hibernation. This happens even with the patch, because laptop-mode-tools fails to check, whether a battery has battery-like nodes or whether there are just chargers in /sys/class/power_supply. [1]

> Please test this patch

Thank you for looking into this and for the patch. laptop-mode-tools is in Bash, so I can send the PR myself. It would be nice, if it could be backported, since it fixes a bug, but laptop-mode-tools could also use a better check for battery nodes.

[1]: https://github.com/rickysarraf/laptop-mode-tools/blob/25496b668d597f947a7d65e7c2d4712e8f77692e/usr/sbin/laptop_mode#L486
Comment 5 Oliver Neukum 2022-04-06 08:47:37 UTC
(In reply to Manuel Ullmann from comment #4)

> 
> Thank you for looking into this and for the patch. laptop-mode-tools is in
> Bash, so I can send the PR myself. It would be nice, if it could be
> backported, since it fixes a bug, but laptop-mode-tools could also use a
> better check for battery nodes.

Hi,

just to make this clear. We seem to have two bugs. Do both need to be fixed to remedy this issue, or do we have two independent bugs?
Comment 6 Manuel Ullmann 2022-04-06 20:17:13 UTC
(In reply to Oliver Neukum from comment #5)
> just to make this clear. We seem to have two bugs. Do both need to be fixed
> to remedy this issue, or do we have two independent bugs?

They are partially dependent on each other, however the kernel can’t fix the issue alone, while userspace can by adding a safe guard/workaround.

Is it technically more correct to report the charger as USB or should it rather stay like it is? If the answer is yes, then the patch should be incorporated. Whether it is required depends on how the laptop-mode-tools developing community considers workarounds for buggy drivers.

Consider the following commit [1]:

diff --git a/usr/sbin/laptop_mode b/usr/sbin/laptop_mode
index 5e2ef72..c4387ab 100755
--- a/usr/sbin/laptop_mode
+++ b/usr/sbin/laptop_mode
@@ -488,7 +488,7 @@ lmt_load_config ()
 	BATTERY_NOT_DISCHARGING=0
 	for POWER_SUPPLY in /sys/class/power_supply/* ; do
 		if [ -f $POWER_SUPPLY/type ] ; then
-			SYSFS_POWER_SUPPLY=1
+			SYSFS_POWER_SUPPLY=$(( SYSFS_POWER_SUPPLY + 1 ))
 			if [ "$(cat $POWER_SUPPLY/type)" = "Mains" ]; then
 				log "VERBOSE" "Determining power state from $POWER_SUPPLY/online."
 				if [ "$(cat $POWER_SUPPLY/online)" = 1 ]; then
@@ -497,6 +497,14 @@ lmt_load_config ()
 				fi
                         elif [ "$(cat $POWER_SUPPLY/type)" = "Battery" ]; then
 				log "VERBOSE" "Determining power state from status of battery $POWER_SUPPLY."
+				#INFO: Because there are and will always be br0ken drivers
+				if [ ! -f $POWER_SUPPLY/status ]; then
+					log "ERR" "Your power_supply is lacking a status node"
+					log "ERR" "Its driver might be reporting a wrong type"
+					log "ERR" "Ignoring this battery"
+					SYSFS_POWER_SUPPLY=$(( SYSFS_POWER_SUPPLY - 1 ))
+					continue
+				fi
                                 #INFO: Because there are and will always be br0ken batteries
 				if [ "$(cat $POWER_SUPPLY/status)" != "Discharging" ]; then
 				        if [ "$(cat $POWER_SUPPLY/status)" = "Unknown" ]; then
@@ -512,11 +520,15 @@ lmt_load_config ()
 					ON_AC=0
 					break
 				fi
+			else
+			    log "VERBOSE" "Power_supply has unexpected type $(cat $POWER_SUPPLY/type)"
+			    log "VERBOSE" "Ignoring it"
+			    SYSFS_POWER_SUPPLY=$(( SYSFS_POWER_SUPPLY - 1 ))
 			fi
 		fi
 	done
 
-	if [ $SYSFS_POWER_SUPPLY = 1 ] ; then
+	if [ $SYSFS_POWER_SUPPLY -gt 0 ] ; then
 		# Already found it!
 		log "VERBOSE" "Not trying other options, already found a power supply."
 	elif [ $BATTERY_NOT_DISCHARGING = 1 ]; then
-- 
2.35.1

With the kernel patch, just the else case and increment/decrement would be required. Without it, the handling in the battery case is required. As I said, I would consider this a reasonable addition for robustness, but I don’t know, whether the laptop-mode-tools community shares that opinion.

Thank you again for pointing me in the right direction.

[1]: https://github.com/rickysarraf/laptop-mode-tools/pull/186/commits/9ddcb2e9f4665fa0b0620fc7eb397917d029fdfd
Comment 7 Bastien Nocera 2022-04-07 11:13:48 UTC
I replied about the problem on the linux-usb list and in https://github.com/rickysarraf/laptop-mode-tools/issues/183

This is most certainly a bug in the script, not a problem with the driver.
Comment 8 Manuel Ullmann 2022-04-07 18:09:29 UTC
(In reply to Bastien Nocera from comment #7)
> I replied about the problem on the linux-usb list and in
> https://github.com/rickysarraf/laptop-mode-tools/issues/183
> 
> This is most certainly a bug in the script, not a problem with the driver.

I tend to agree. But the remaining question is still, whether the driver should report the power_supply as battery, since it is not reporting any battery stats. It just allows to change the charging type (Fast|Trickle). This might be unexpected in other areas of userspace, so it might be reasonable to change it.
Comment 9 Bastien Nocera 2022-04-07 21:03:50 UTC
"battery" is the "least incorrect" power supply type we could use. From my reply to the list:
> POWER_SUPPLY_TYPE_USB also seems to only be used by USB ports that
> are used to charge the machine itself (so a "system" scope), like the
> USB port on a phone, not for devices connected through USB (of which
> there are a lot).

(In reply to Manuel Ullmann from comment #8)
> It just allows to change the charging type (Fast|Trickle).

This is one of the things that power_supply class devices can do. They don't have to report battery information if it's not available.

If you want to read the battery information of iOS devices, it's available in upower after at least one user has paired with the device.
Comment 10 Manuel Ullmann 2022-04-07 22:10:58 UTC
> (In reply to Manuel Ullmann from comment #8)
> > It just allows to change the charging type (Fast|Trickle).
> 
> This is one of the things that power_supply class devices can do. They don't
> have to report battery information if it's not available. Guess I have to try
> this out with my laptop for comparison.

And userspace has to handle that. I get your point. I can implement the scope handling, if I find any documentation on this.

> If you want to read the battery information of iOS devices, it's available
> in upower after at least one user has paired with the device.
Okay, thanks for the hint.

Changing this to invalid, since userspace has to handle that.

Note You need to log in before you can comment on or make changes to this bug.