linux-kernel - Re: [PATCH v2 6/9] power: supply: max77693: Add USB extcon detection for enabling charging

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9eedeaa3-ceda-427a-80d8-de6b59eb1f4d@gmail.com>
Date: Sat, 20 Jul 2024 22:59:50 +0200
From: Artur Weber <aweber.kernel@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
 Chanwoo Choi <cw00.choi@...sung.com>
Cc: Sebastian Reichel <sre@...nel.org>, Rob Herring <robh@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Lee Jones <lee@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Alim Akhtar <alim.akhtar@...sung.com>, linux-pm@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-samsung-soc@...r.kernel.org,
 ~postmarketos/upstreaming@...ts.sr.ht, Henrik Grimler <henrik@...mler.se>,
 Wolfgang Wiedmeyer <wolfgit@...dmeyer.de>,
 Denis 'GNUtoo' Carikli <GNUtoo@...erdimension.org>
Subject: Re: [PATCH v2 6/9] power: supply: max77693: Add USB extcon detection
 for enabling charging

On 15.07.2024 14:55, Artur Weber wrote:
> Add a device tree property, "maxim,usb-connector", that can be used to
> specify a USB connector to use to detect whether a charging cable has
> been plugged in/out, and enable/disable charging accordingly.
> 
> To accommodate this, also add an internal pointer to the CHARGER regulator,
> which is used to enable/disable charging and set the input current limit
> (which will be done in subsequent commits).
> 
> The extcon listener/worker implementation is inspired by the rt5033_charger
> driver.
> 
> Signed-off-by: Artur Weber <aweber.kernel@...il.com>
> ...> diff --git a/drivers/power/supply/max77693_charger.c 
b/drivers/power/supply/max77693_charger.c
> index 0ddaa03669a2..2dc273dd96ee 100644
> --- a/drivers/power/supply/max77693_charger.c
> +++ b/drivers/power/supply/max77693_charger.c
> ...
> +static int max77693_set_charging(struct max77693_charger *chg, bool enable)
> +{
> +	int is_enabled;
> +	int ret = 0;
> +
> +	is_enabled = regulator_is_enabled(chg->regu);
> +	if (is_enabled < 0)
> +		return is_enabled;
> +
> +	if (enable && !is_enabled)
> +		ret = regulator_enable(chg->regu);
> +	else if (!enable && is_enabled)
> +		ret = regulator_disable(chg->regu);
> +
> +	return ret;
> +}

Nevermind, the regulator-based approach simply doesn't work here, 
because we pretty frequently end up in a situation like this:

[   20.162612] ------------[ cut here ]------------
[   20.162618] WARNING: CPU: 0 PID: 29 at drivers/regulator/core.c:3015 
_regulator_disable+0x140/0x1a0
[   20.162645] unbalanced disables for CHARGER
[   20.162649] Modules linked in: fuse brcmfmac_wcc hci_uart btintel 
btbcm bluetooth snd_soc_midas_wm1811 st_accel_i2c st_sensors_i2c 
st_accel_spi st_accel brcmfmac ecdh_generic st_sensors st_sensors_spi 
ecc libaes brcmutil cfg80211 rfkill exynos_adc yamaha_yas530 
industrialio_triggered_buffer kfifo_buf exynos_rng s5p_sss cm3323 
industrialio
[   20.162737] CPU: 0 PID: 29 Comm: kworker/0:2 Tainted: G        W 
     6.10.0-postmarketos-exynos4 #82
[   20.162747] Hardware name: Samsung Exynos (Flattened Device Tree)
[   20.162754] Workqueue: events max77693_charger_extcon_work
[   20.162770] Call trace:
[   20.162778]  unwind_backtrace from show_stack+0x10/0x14
[   20.162804]  show_stack from dump_stack_lvl+0x50/0x64
[   20.162824]  dump_stack_lvl from __warn+0x94/0xc0
[   20.162838]  __warn from warn_slowpath_fmt+0x120/0x1b4
[   20.162855]  warn_slowpath_fmt from _regulator_disable+0x140/0x1a0
[   20.162873]  _regulator_disable from regulator_disable+0x34/0x6c
[   20.162890]  regulator_disable from 
max77693_charger_extcon_work+0x1e4/0x268
[   20.162907]  max77693_charger_extcon_work from 
process_one_work+0x154/0x2dc
[   20.162925]  process_one_work from worker_thread+0x250/0x438
[   20.162941]  worker_thread from kthread+0x110/0x12c
[   20.162958]  kthread from ret_from_fork+0x14/0x28
[   20.162970] Exception stack(0xc18edfb0 to 0xc18edff8)
[   20.162979] dfa0:                                     00000000 
00000000 00000000 00000000
[   20.162987] dfc0: 00000000 00000000 00000000 00000000 00000000 
00000000 00000000 00000000
[   20.162994] dfe0: 00000000 00000000 00000000 00000000 00000013 00000000
[   20.162999] ---[ end trace 0000000000000000 ]---
[   20.163007] max77693-charger max77693-charger: failed to set charging 
(-5)

This can be reproduced by booting the device in with no cable plugged 
in, then plugging in an OTG cable. It prints the warning on connect and 
disconnect. More importantly, this prevents a switch to/from OTG mode in 
the scenarios that print a warning. (I've also encountered some issues 
with similar warnings being printed on unsuspend, but wasn't able to 
reproduce them; I'm willing to assume they were related to plugging in 
an OTG cable as the wakeup trigger.)

As far as I understand it, this is because regulator_is_enabled checks 
for the hardware state, not for the in-software regulator enable count, 
and the charging bits are flipped on by default (presumably by the 
bootloader). I thought regulator-boot-on would handle this, but clearly 
it doesn't...

Best regards
Artur