| 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: <CAPTae5JJ6sRshFxqWroQPSctJogbS_bQcnCCDBXFgxL5B6N6mQ@mail.gmail.com>
Date: Tue, 4 Aug 2020 17:18:07 -0700
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>, --validate@...gle.com
Subject: Re: [PATCH v1] tcpm: Honour pSnkStdby requirement during negotiation
Hi Guenter,
Your concern is very valid ! I was myself pondering at the
significance and meaning of
current_limit, supply_voltage. I will pen down what I understood from
reading the code.
If it makes sense, I will add the documentation to the change as well.
In TOT code i.e. without the patch in discussion, current_limit and
supply_voltage gets
in three places.
1. tcpm_set_current_limit --------------> Actual current_limit
/voltage requested to be set
in the hardware.
2. tcpm_pd_build_request --------------> current_limit /voltage
requested to the port partner.
Not yet
accepted by port partner
3. case PD_CTRL_ACCEPT: ----------> PPS path: new
current_limit/voltage to be set in hw &
already
accepted by the port partner.
switch (port->state) {
case SNK_NEGOTIATE_CAPABILITIES:
port->pps_data.active = false;
tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
break;
case SNK_NEGOTIATE_PPS_CAPABILITIES:
port->pps_data.active = true;
port->supply_voltage = port->pps_data.out_volt;
port->current_limit = port->pps_data.op_curr;
tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
PPS path safely differentiates between new current_limit &
supply_voltage requested
to the port-partner and current_limit & supply_voltage requested to
be set in the
hardware by caching the new requested values to partner in
port->pps_data.op_curr/_volt (in tcpm_pd_select_pps_apdo) and updates
port->supply_voltage, port->current_limit only when the port-partner responds
with an accept packet (as pointed in the above code stub).
However, when NOT IN PPS, TCPM code updates port->current_limit &
port->supply_voltage directly in tcpm_pd_build_request where the partner can
still REJECT the request and port->current_limit & port->supply_voltage
might go out of sync. So, there is already a need to introduce new variables to
cache the requested values to port partners.
Now coming to addressing pSnkStdby, Once the partner accepts the new request,
PD spec wants the sink to limit the power consumption to pSnkStdby (2.5W) till
source sends the PS_RDY. i.e. while TCPM waits in SNK_TRANSITION_SINK,
the sink's power consumption has to be limited to 2.5W. Sink can enforce or draw
new partner accepted current & voltage at the hardware level only
after PS_RDY is received from the port-partner.
With the patch that I am submitting, I am hoping to cleanly
differentiate between the
current limit/voltage requested to be set in hardware(
tcpm_set_current_limit) Vs
the current limit/voltage requested to the port partner.
current_limit, supply_voltage -> Requested to be set in hardware.
req_current_limit, req_supply_voltage -> Newly requested value to the
port partner.
Perhaps doing the following alleviates current_limit/supply_voltage ambiguity:
1. renaming current_limit, supply_voltage to hw_current_limit, hw_supply_voltage
2. making a comment saying hw_current_limit, hw_supply_voltage should only
be set in tcpm_set_current_limit.
3. Instead of calling requested values to port-partner as
req_current_limit/req_supply_voltage, maybe partner_req_current_limit,
partner_req_supply_voltage is a better name.
4. Adding a comment to state significance of hw_current_limit,
hw_supply_voltage,
partner_req_current_limit, partner_req_supply_voltage in tcpm struct.
Let me know if it makes sense I will update the patch (may be even
split if it makes
sense) and resend.
Thanks,
Badhri
On Tue, Aug 4, 2020 at 9:59 AM Guenter Roeck <linux@...ck-us.net> wrote:
>
> On 8/3/20 11:51 PM, Badhri Jagan Sridharan wrote:
> >>From PD Spec:
> > The Sink Shall transition to Sink Standby before a positive or
> > negative voltage transition of VBUS. During Sink Standby
> > the Sink Shall reduce its power draw to pSnkStdby. This allows
> > the Source to manage the voltage transition as well as
> > supply sufficient operating current to the Sink to maintain PD
> > operation during the transition. The Sink Shall
> > complete this transition to Sink Standby within tSnkStdby
> > after evaluating the Accept Message from the Source. The
> > transition when returning to Sink operation from Sink Standby
> > Shall be completed within tSnkNewPower. The
> > pSnkStdby requirement Shall only apply if the Sink power draw
> > is higher than this level.
> >
> > The above requirement needs to be met to prevent hard resets
> > from port partner.
> >
> > Introducing psnkstdby_after_accept flag to accommodate systems
> > where the input current limit loops are not fast enough to meet
> > tSnkStby(15 msec).
> >
> > When not set, port requests PD_P_SNK_STDBY upon entering SNK_DISCOVERY and
> > the actual currrent limit after RX of PD_CTRL_PSRDY for PD link,
> > SNK_READY for non-pd link.
> >
> > When set, port requests CC advertisement based current limit during
> > SNK_DISCOVERY, current gets limited to PD_P_SNK_STDBY during
> > SNK_TRANSITION_SINK, PD based current limit gets set after RX of
> > PD_CTRL_PSRDY. However, in this case it has to be made sure that the
> > tSnkStdby (15 msec) is met.
> >
> > Signed-off-by: Badhri Jagan Sridharan <badhri@...gle.com>
> > ---
> > drivers/usb/typec/tcpm/tcpm.c | 52 +++++++++++++++++++++++++++--------
> > include/linux/usb/pd.h | 5 +++-
> > 2 files changed, 44 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > index 3ef37202ee37..e46da41940b9 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -293,9 +293,12 @@ struct tcpm_port {
> > unsigned int operating_snk_mw;
> > bool update_sink_caps;
> >
> > - /* Requested current / voltage */
> > + /* Set current / voltage */
> > u32 current_limit;
> > u32 supply_voltage;
> > + /* current / voltage requested to partner */
> > + u32 req_current_limit;
> > + u32 req_supply_voltage;
>
> I don't see a clear delineation where, when, and why supply_voltage vs. req_supply_voltage
> and current_limit vs. req_current_limit is used. Maybe someone else does and can give a
> Reviewed-by: tag, but for my part I'll have to spend some time trying to understand
> what each variable and its use now means. Overall this suggests that the code may now be
> a bit fragile. If those two sets of variables are now indeed needed, I think it would help
> to have a detailed explanation for each use. This would help reducing the probability
> of errors if the code has to be touched again in the future.
>
> >
> > /* Used to export TA voltage and current */
> > struct power_supply *psy;
> > @@ -323,13 +326,27 @@ struct tcpm_port {
> > struct pd_mode_data mode_data;
> > struct typec_altmode *partner_altmode[ALTMODE_DISCOVERY_MAX];
> > struct typec_altmode *port_altmode[ALTMODE_DISCOVERY_MAX];
> > -
> > /* Deadline in jiffies to exit src_try_wait state */
> > unsigned long max_wait;
> >
> > /* port belongs to a self powered device */
> > bool self_powered;
> >
> > + /*
> > + * Honour psnkstdby after accept is received.
> > + * However, in this case it has to be made sure that the tSnkStdby (15
> > + * msec) is met.
> > + *
> > + * When not set, port requests PD_P_SNK_STDBY_5V upon entering SNK_DISCOVERY and
> > + * the actual currrent limit after RX of PD_CTRL_PSRDY for PD link,
> > + * SNK_READY for non-pd link.
> > + *
> > + * When set, port requests CC advertisement based current limit during
> > + * SNK_DISCOVERY, current gets limited to PD_P_SNK_STDBY_5V during SNK_TRANSITION_SINK,
> > + * PD based current limit gets set after RX of PD_CTRL_PSRDY.
> > + */
> > + bool psnkstdby_after_accept;
> > +
> > #ifdef CONFIG_DEBUG_FS
> > struct dentry *dentry;
> > struct mutex logbuffer_lock; /* log buffer access lock */
> > @@ -1787,9 +1804,8 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
> > switch (port->state) {
> > case SNK_TRANSITION_SINK:
> > if (port->vbus_present) {
> > - tcpm_set_current_limit(port,
> > - port->current_limit,
> > - port->supply_voltage);
> > + tcpm_set_current_limit(port, port->req_current_limit,
> > + port->req_supply_voltage);
> > port->explicit_contract = true;
> > tcpm_set_state(port, SNK_READY, 0);
> > } else {
> > @@ -1861,8 +1877,8 @@ static void tcpm_pd_ctrl_request(struct tcpm_port *port,
> > break;
> > case SNK_NEGOTIATE_PPS_CAPABILITIES:
> > port->pps_data.active = true;
> > - port->supply_voltage = port->pps_data.out_volt;
> > - port->current_limit = port->pps_data.op_curr;
> > + port->req_supply_voltage = port->pps_data.out_volt;
> > + port->req_current_limit = port->pps_data.op_curr;
> > tcpm_set_state(port, SNK_TRANSITION_SINK, 0);
> > break;
> > case SOFT_RESET_SEND:
> > @@ -2463,8 +2479,8 @@ static int tcpm_pd_build_request(struct tcpm_port *port, u32 *rdo)
> > flags & RDO_CAP_MISMATCH ? " [mismatch]" : "");
> > }
> >
> > - port->current_limit = ma;
> > - port->supply_voltage = mv;
> > + port->req_current_limit = ma;
> > + port->req_supply_voltage = mv;
> >
> > return 0;
> > }
> > @@ -3235,9 +3251,11 @@ static void run_state_machine(struct tcpm_port *port)
> > break;
> > case SNK_DISCOVERY:
> > if (port->vbus_present) {
> > - tcpm_set_current_limit(port,
> > - tcpm_get_current_limit(port),
> > - 5000);
> > + if (port->psnkstdby_after_accept || tcpm_get_current_limit(port) <=
> > + PD_P_SNK_STDBY_5V)
> > + tcpm_set_current_limit(port, tcpm_get_current_limit(port), 5000);
> > + else
> > + tcpm_set_current_limit(port, PD_P_SNK_STDBY_5V, 5000);
> > tcpm_set_charge(port, true);
> > tcpm_set_state(port, SNK_WAIT_CAPABILITIES, 0);
> > break;
> > @@ -3318,6 +3336,10 @@ static void run_state_machine(struct tcpm_port *port)
> > }
> > break;
> > case SNK_TRANSITION_SINK:
> > + if (port->psnkstdby_after_accept)
> > + tcpm_set_current_limit(port, tcpm_get_current_limit(port) >
> > + PD_P_SNK_STDBY_5V ? PD_P_SNK_STDBY_5V :
> > + tcpm_get_current_limit(port), 5000);
> > case SNK_TRANSITION_SINK_VBUS:
> > tcpm_set_state(port, hard_reset_state(port),
> > PD_T_PS_TRANSITION);
> > @@ -3331,6 +3353,10 @@ static void run_state_machine(struct tcpm_port *port)
> > port->pwr_opmode = TYPEC_PWR_MODE_PD;
> > }
> >
> > + /* Set current limit for NON-PD link when psnkstdby_after_accept is not set*/
> > + if (!port->pd_capable && !port->psnkstdby_after_accept)
> > + tcpm_set_current_limit(port, tcpm_get_current_limit(port), 5000);
> > +
> > tcpm_swap_complete(port, 0);
> > tcpm_typec_connect(port);
> > tcpm_check_send_discover(port);
> > @@ -4513,6 +4539,8 @@ static int tcpm_fw_get_caps(struct tcpm_port *port,
> > port->typec_caps.type = ret;
> > port->port_type = port->typec_caps.type;
> >
> > + port->psnkstdby_after_accept = fwnode_property_read_bool(fwnode, "psnkstdby-after-accept");
> > +
>
> I think this will need to be documented.
>
> Guenter
>
> > if (port->port_type == TYPEC_PORT_SNK)
> > goto sink;
> >
> > diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
> > index b6c233e79bd4..6bd70f77045e 100644
> > --- a/include/linux/usb/pd.h
> > +++ b/include/linux/usb/pd.h
> > @@ -483,5 +483,8 @@ static inline unsigned int rdo_max_power(u32 rdo)
> > #define PD_N_CAPS_COUNT (PD_T_NO_RESPONSE / PD_T_SEND_SOURCE_CAP)
> > #define PD_N_HARD_RESET_COUNT 2
> >
> > -#define PD_T_BIST_CONT_MODE 50 /* 30 - 60 ms */
> > +#define PD_T_BIST_CONT_MODE 50 /* 30 - 60 ms */
> > +
> > +#define PD_P_SNK_STDBY_5V 500 /* 2500 mw - 500mA @ 5V */
> > +
> > #endif /* __LINUX_USB_PD_H */
> >
>
Powered by blists - more mailing lists