linux-kernel - Re: Re: [PATCH 1/3] power: supply: axp20x_usb

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 for Android: free password hash cracker in your pocket

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

Message-ID: <2dkeq46flco2xukggii3ljlw2zo7wjwuiutha7jlxozmf63swd@xnirwnfvvvcv>
Date: Mon, 22 Jan 2024 19:52:44 -0500
From: Aren <aren@...cevolution.org>
To: Chen-Yu Tsai <wens@...e.org>
Cc: linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Aidan MacDonald <aidanmacdonald.0x0@...il.com>, Ondřej Jirman <megi@....cz>, 
	Sebastian Reichel <sre@...nel.org>, Hans de Goede <j.w.r.degoede@...il.com>
Subject: Re: Re: [PATCH 1/3] power: supply: axp20x_usb_power: add input
 current limit

On Sun, Jan 21, 2024 at 11:25:04PM +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> +CC Hans de Goede since the AXP288 is similar to the later AXPs in the USB
> power section.
> 
> On Sun, Jan 21, 2024 at 9:54 AM Aren Moynihan <aren@...cevolution.org> wrote:
> >
> > Add properties for setting the maximum current that will be drawn from
> > the usb connection.
> >
> > These changes don't apply to all axp20x chips, so we need to add new
> > power_desc and power supply property objects for axp813 specifically.
> > These are copied from the axp22x variants that were used before, with
> > extra fields added.
> >
> > Also add a dev field to the axp20x_usb_power struct, so we can use
> > dev_dbg and dev_err in more places.
> 
> I think this patch highlights some errors in the driver.
> 
> 1. We are likely misusing POWER_SUPPLY_PROP_CURRENT_MAX. Based on the
>    ABI docs, this is supposed to be a read only property. The correct
>    way would be to use POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT as you did.

I think so, yes. I was hesitant to touch that because I wasn't sure if
current_max was relied on anywhere. I'll do some grepping to confirm
that it'll be safe.

> 2. For AXP288 and AXP803/AXP813/AXP818, we are setting the current limit
>    for when BC detection finishes in register 0x30, not the actual active
>    limit in register 0x35. This should be fixed.
> 
> 3. BC detection race condition
> 
> Could you maybe respin the patches to address them separately instead?

Sure, that sounds like a better way to split it up.

 - Aren

> Thanks
> ChenYu