linux-kernel - Re: [PATCH 2/2] typec: tcpm: Add option to maintain current limit at Vsafe5V

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] [day] [month] [year] [list]

Message-ID: <CAPTae5KOoNHwh1n8SLuE642uSYq_zJtXUZ3j8ob9VeE+9DXbEw@mail.gmail.com>
Date:   Fri, 28 Sep 2018 15:44:44 -0700
From:   Badhri Jagan Sridharan <badhri@...gle.com>
To:     jackp@...eaurora.org
Cc:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        USB <linux-usb@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Guenter Roeck <linux@...ck-us.net>
Subject: Re: [PATCH 2/2] typec: tcpm: Add option to maintain current limit at Vsafe5V

On Fri, Sep 14, 2018 at 11:42 AM Jack Pham <jackp@...eaurora.org> wrote:
>
> Hi Badhri,
>
> On Thu, Sep 13, 2018 at 04:38:10PM -0700, Badhri Jagan Sridharan wrote:
> > On Thu, Sep 13, 2018 at 10:07 AM Jack Pham <jackp@...eaurora.org> wrote:
> > > On Thu, Sep 13, 2018 at 6:43 AM Badhri Jagan Sridharan
> > > > Yeah thats for the source. But for sink, Say if the source isnt PD, then,
> > > > sink initiated hard resets happen during the connection. Sink would hard reset
> > > > couple of times before deeming that the partner is non PD. When connected
> > > > to Type-A ports/non-pd partner, vbus is not going to likely drop so there isnt
> > > > a reason to setcharge to false or drop the input current limit. Do you agree ?
> > >
> > > Sure that makes sense. In this case I wonder if TCPM even needs to call
> > > set_charge(false) considering it does not yet know if the partner is PD
> > > capable or not. For sure, if the partner is PD capable and contract had
> > > been previously established, we'd definitely need to set_current_limit()
> > > to default levels and/or turn off charging.
> > >
> > > But in the case of hard reset attempts to try to determine if the source
> > > will send its capabilities (thereby being PD capable), wouldn't the
> > > initial default current limits still be in place? I think this is the
> > > point you're trying to make, that there is no need to disrupt charging
> > > if a hard reset is not going to cause VBUS to reset.
> >
> > Yes that's right ! I wasnt sure how to put that in words. Thanks for
> > doing that !
> > You do concur right ?
>
> Yes.
>
> > > To me it sounds like what you're trying to do makes sense only if you
> > > can make a run-time determination of a partner's PD capability, and not
> > > based on a config option.
> >
> > Yes this should be possible. port->pd_capable already has that info.
> >
> > To sum it up:
> > if the partner is pd capable, set charge to false in SNK_HARD_RESET_SINK_OFF and
> > readjust current limits to default in SNK_HARD_RESET_SINK_ON and turn
> > on charging.
> >
> > If partner is not pd capable, do not turn off charging nor adjust
> > current limit as host port is
> > not going to respond to hard reset.
> >
> > Does it make sense ?
>
> Right, although the "not pd capable" path could also mean the partner is
> PD capable but it is not determined yet. For example: if a sink is
> connected to a PD source, established a contract, and the sink reboots
> and has to initialize TCPM again. Assuming the sink never disconnected
> from CC when rebooting, the source won't automatically re-send its
> Source Capabilities as it will still be in its previous state(*). The
> sink TCPM however would send a hard reset in order to try to
> (re)establish a contract, so here is where this path overlaps with the
> not-pd-capable case. In this case I think it might be proper to readjust
> current limits to default also unless it was already done earlier--I see
> it is done in tcpm_reset_port() and in SNK_DISCOVERY, so you might be
> covered here.

Yes I verified this.

>
> The question I have is whether you still need to consider calling
> set_charge(false) for this example.


>
> (*) Just thought of one more thing, what if the previous contract was
> negotiated at greater than 5V VBUS? How does the sink TCPM handle
> setting charging parameters after a reboot but prior to hard reset?


Rebooting the device is an interesting case ! TCPM is unique in its own
way as it neither a platform_driver nor a device_driver. Simply its the state
machine of the type-c port's device. I see that I could register to the
shutdown callback in the lower level driver which registers the port
by calling tcpm_register_port and handle the shutdown operation by calling
tcpm_unregister_port and subsequently opening the ports but dont know
how to achiever this at the TCPM level.

However, this reboot case is not directly related to the HARD_RESET
state as this would have to be handled for all the sub states. Isn't that the
case ?

Updating the patchset to do the following:
"if the partner is pd capable, set charge to false in
SNK_HARD_RESET_SINK_OFF and
readjust current limits to default in SNK_HARD_RESET_SINK_ON and turn
on charging.

If partner is not pd capable, do not turn off charging nor adjust
current limit as host port is
not going to respond to hard reset."

>
> Jack
> --
> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project