| 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
| ||
|
Message-ID: <CAPTae5KZjWEOxSG+57GvdxYkzbJucbo7SB5NuhR65gGU+37s0g@mail.gmail.com>
Date: Tue, 1 Dec 2020 20:10:03 -0800
From: Badhri Jagan Sridharan <badhri@...gle.com>
To: Guenter Roeck <linux@...ck-us.net>
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>
Subject: Re: [PATCH v1 1/3] usb: typec: tcpm: Introduce vsafe0v for vbus
On Tue, Dec 1, 2020 at 5:15 AM Guenter Roeck <linux@...ck-us.net> wrote:
>
> On Mon, Nov 30, 2020 at 05:32:44PM -0800, Badhri Jagan Sridharan wrote:
> > TCPM at present lacks the notion of VSAFE0V. There
> > are three vbus threshold levels that are critical to track:
> > a. vSafe5V - VBUS “5 volts” as defined by the USB
> > PD specification.
> > b. vSinkDisconnect - Threshold used for transition from
> > Attached.SNK to Unattached.SNK.
> > c. vSafe0V - VBUS “0 volts” as defined by the USB
> > PD specification.
> >
> > Tracking vSafe0V is crucial for entry into Try.SNK and
> > Attached.SRC and turning vbus back on by the source in
> > response to hard reset.
> >
> > From "4.5.2.2.8.2 Exiting from AttachWait.SRC State" section
> > in the Type-C spec:
> >
> > "The port shall transition to Attached.SRC when VBUS is at
> > vSafe0V and the SRC.Rd state is detected on exactly one of
> > the CC1 or CC2 pins for at least tCCDebounce."
> >
> > "A DRP that strongly prefers the Sink role may optionally
> > transition to Try.SNK instead of Attached.SRC when VBUS
> > is at vSafe0V and the SRC.Rd state is detected on exactly
> > one of the CC1 or CC2 pins for at least tCCDebounce."
> >
> > From "7.1.5 Response to Hard Resets" section in the PD spec:
> >
> > "After establishing the vSafe0V voltage condition on VBUS,
> > the Source Shall wait tSrcRecover before re-applying VCONN
> > and restoring VBUS to vSafe5V."
> >
> > vbus_present in the TCPM code tracks vSafe5V(vbus_present is true)
> > and vSinkDisconnect(vbus_present is false).
> >
> > This change adds is_vbus_vsafe0v callback which when set makes
> > TCPM query for vSafe0V voltage level when needed.
> >
> > Since not all TCPC controllers might have the capability
> > to report vSafe0V, TCPM assumes that vSafe0V is same as
> > vSinkDisconnect when is_vbus_vsafe0v callback is not set.
> > This allows TCPM to continue to support controllers which don't
> > have the support for reporting vSafe0V.
> >
> > Introducing vSafe0V helps fix the failure reported at
> > "Step 15. CVS verifies PUT remains in AttachWait.SRC for 500ms"
> > of "TD 4.7.2 Try. SNK DRP Connect DRP Test" of
> > "Universal Serial Bus Type-C (USB Type-C) Functional Test
> > Specification Chapters 4 and 5". Here the compliance tester
> > intentionally maintains vbus at greater than vSafe0V and expects
> > the Product under test to stay in AttachWait.SRC till vbus drops
> > to vSafe0V.
> >
> > Signed-off-by: Badhri Jagan Sridharan <badhri@...gle.com>
> > ---
> > drivers/usb/typec/tcpm/tcpm.c | 63 +++++++++++++++++++++++++++++------
> > include/linux/usb/tcpm.h | 6 ++++
> > 2 files changed, 58 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > index 3bbc1f10af49..10a065eef73e 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -258,7 +258,19 @@ struct tcpm_port {
> > bool attached;
> > bool connected;
> > enum typec_port_type port_type;
> > +
> > + /*
> > + * Set to true when vbus is greater than VSAFE5V min.
> > + * Set to false when vbus falls below vSinkDisconnect max threshold.
> > + */
> > bool vbus_present;
> > +
> > + /*
> > + * Set to true when vbus is less than VSAFE0V max.
> > + * Set to false when vbus is greater than VSAFE0V max.
> > + */
> > + bool vbus_vsafe0v;
> > +
> > bool vbus_never_low;
> > bool vbus_source;
> > bool vbus_charge;
> > @@ -3094,7 +3106,7 @@ static void run_state_machine(struct tcpm_port *port)
> > else if (tcpm_port_is_audio(port))
> > tcpm_set_state(port, AUDIO_ACC_ATTACHED,
> > PD_T_CC_DEBOUNCE);
> > - else if (tcpm_port_is_source(port))
> > + else if (tcpm_port_is_source(port) && port->vbus_vsafe0v)
> > tcpm_set_state(port,
> > tcpm_try_snk(port) ? SNK_TRY
> > : SRC_ATTACHED,
> > @@ -4097,6 +4109,12 @@ static void _tcpm_pd_vbus_on(struct tcpm_port *port)
> > {
> > tcpm_log_force(port, "VBUS on");
> > port->vbus_present = true;
> > + /*
> > + * When vbus_present is true i.e. Voltage at VBUS is greater than VSAFE5V implicitly
> > + * states that vbus is not at VSAFE0V, hence clear the vbus_vsafe0v flag here.
> > + */
> > + port->vbus_vsafe0v = false;
> > +
> > switch (port->state) {
> > case SNK_TRANSITION_SINK_VBUS:
> > port->explicit_contract = true;
> > @@ -4186,16 +4204,8 @@ static void _tcpm_pd_vbus_off(struct tcpm_port *port)
> > case SNK_HARD_RESET_SINK_OFF:
> > tcpm_set_state(port, SNK_HARD_RESET_WAIT_VBUS, 0);
> > break;
> > - case SRC_HARD_RESET_VBUS_OFF:
> > - /*
> > - * After establishing the vSafe0V voltage condition on VBUS, the Source Shall wait
> > - * tSrcRecover before re-applying VCONN and restoring VBUS to vSafe5V.
> > - */
> > - tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
> > - break;
> > case HARD_RESET_SEND:
> > break;
> > -
> > case SNK_TRY:
> > /* Do nothing, waiting for timeout */
> > break;
> > @@ -4266,6 +4276,28 @@ static void _tcpm_pd_vbus_off(struct tcpm_port *port)
> > }
> > }
> >
> > +static void _tcpm_pd_vbus_vsafe0v(struct tcpm_port *port)
> > +{
> > + tcpm_log_force(port, "VBUS VSAFE0V");
> > + port->vbus_vsafe0v = true;
> > + switch (port->state) {
> > + case SRC_HARD_RESET_VBUS_OFF:
> > + /*
> > + * After establishing the vSafe0V voltage condition on VBUS, the Source Shall wait
> > + * tSrcRecover before re-applying VCONN and restoring VBUS to vSafe5V.
> > + */
> > + tcpm_set_state(port, SRC_HARD_RESET_VBUS_ON, PD_T_SRC_RECOVER);
> > + break;
> > + case SRC_ATTACH_WAIT:
> > + if (tcpm_port_is_source(port))
> > + tcpm_set_state(port, tcpm_try_snk(port) ? SNK_TRY : SRC_ATTACHED,
> > + PD_T_CC_DEBOUNCE);
> > + break;
> > + default:
> > + break;
> > + }
> > +}
> > +
> > static void _tcpm_pd_hard_reset(struct tcpm_port *port)
> > {
> > tcpm_log_force(port, "Received hard reset");
> > @@ -4301,10 +4333,19 @@ static void tcpm_pd_event_handler(struct kthread_work *work)
> > bool vbus;
> >
> > vbus = port->tcpc->get_vbus(port->tcpc);
> > - if (vbus)
> > + if (vbus) {
> > _tcpm_pd_vbus_on(port);
> > - else
> > + } else {
> > _tcpm_pd_vbus_off(port);
> > + /*
> > + * When TCPC does not support detecting vsafe0v voltage level,
> > + * treat vbus absent as vsafe0v. Else invoke is_vbus_vsafe0v
> > + * to see if vbus has discharge to VSAFE0V.
> > + */
> > + if (!port->tcpc->is_vbus_vsafe0v ||
> > + port->tcpc->is_vbus_vsafe0v(port->tcpc) > 0)
> > + _tcpm_pd_vbus_vsafe0v(port);
> > + }
> > }
> > if (events & TCPM_CC_EVENT) {
> > enum typec_cc_status cc1, cc2;
> > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> > index e68aaa12886f..615d4532c028 100644
> > --- a/include/linux/usb/tcpm.h
> > +++ b/include/linux/usb/tcpm.h
> > @@ -98,6 +98,11 @@ enum tcpm_transmit_type {
> > * will be turned on. requested_vbus_voltage is set to 0 when vbus
> > * is going to disappear knowingly i.e. during PR_SWAP and
> > * HARD_RESET etc.
> > + * @is_vbus_vsafe0v:
> > + * Optional; TCPCI spec based TCPC implementations are expected to
> > + * detect VSAFE0V voltage level at vbus. When detection of VSAFE0V
> > + * is supported by TCPC, set this callback for TCPM to query
> > + * whether vbus is at VSAFE0V when needed.
>
> Return values ?
Fixing in v2.
>
> > */
> > struct tcpc_dev {
> > struct fwnode_handle *fwnode;
> > @@ -128,6 +133,7 @@ struct tcpc_dev {
> > int (*enable_auto_vbus_discharge)(struct tcpc_dev *dev, bool enable);
> > int (*set_auto_vbus_discharge_threshold)(struct tcpc_dev *dev, enum typec_pwr_opmode mode,
> > bool pps_active, u32 requested_vbus_voltage);
> > + int (*is_vbus_vsafe0v)(struct tcpc_dev *dev);
>
> I don't really see the point of returning an int here. If an error is
> returned, it should be checked. However, that is not currently the case;
> an error is treated as "false" return. As such, the function may as well
> return true/false (and false if an error is observed).
Sure. Changing the callback return to boolean in v2.
>
> > };
> >
> > struct tcpm_port;
> > --
> > 2.29.2.454.gaff20da3a2-goog
> >
Powered by blists - more mailing lists