linux-kernel - Re: [PATCH] usb: dwc3: fix build error when POWER

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: <20210314223513.tznvhuq7phxxvjfo@earth.universe>
Date:   Sun, 14 Mar 2021 23:35:13 +0100
From:   Sebastian Reichel <sre@...nel.org>
To:     Ray Chi <raychi@...gle.com>
Cc:     Greg KH <gregkh@...uxfoundation.org>, naresh.kamboju@...aro.org,
        Kyle Tso <kyletso@...gle.com>, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Badhri Jagan Sridharan <badhri@...gle.com>
Subject: Re: [PATCH] usb: dwc3: fix build error when POWER_SUPPLY is not
 enabled

Hi,

On Fri, Mar 12, 2021 at 09:57:56PM +0800, Ray Chi wrote:
> > While I'm fine with merging this after fixing up the subject, the
> > original patch for dwc3 [0] looks completly incorrect to me.
> >
> > First of all it uses wrong scale (power-supply uses uA, not mA),
> > so you are charging 1000x slower than expected. Then the patchset
> > introduces a new DT property to get the power-supply device, but
> > does not update the DT binding documentation and does not Cc the
> > DT binding maintainer.
> 
> Yes, it should use uA and send this information, and I will update a
> patch to fix it and add the DT binding documentation.

Considering your programming is off by a factor 1000 I wonder how
this patchset has been tested.

> > Next the property itself looks not very
> > smart to me. Usually one would use a device reference, not the
> > Linux device name.
> >
> > Finally all existing devices solve this by registering a usb
> > notifier from the charger, so why are you going the other way
> > around? This is going to break once you want to use one of the
> > existing chargers with dwc3.
> 
> Only the USB controller will know USB state/speed so that I think
> it is better to send this information from the USB side. For
> example: For USB high speed, charging current should be limited to
> 500 mA in configured state.  For USB super speed, charging current
> should be limited to 900 mA in configured state.

usb_register_notifier registers a callback to receive information
from the USB subsystem. Then power-supply drivers can query specific
info, e.g. call usb_phy_get_charger_current(). This is already being
done by some power-supply drivers, so using one of those
power-supply charger drivers in combination with dwc3 will now result
in potentially racy behaviour as far as I can see.

> > I suggest to drop/revert the whole patchset.

-- Sebastian

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)