linux-kernel - Re: [PATCH 3/3 v2] usb: typec: tcpm: Stay in BIST mode till hardreset or unattached

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: <2c9e0f1a-3071-8fe7-54d1-6ce670268197@roeck-us.net>
Date:   Wed, 15 Jul 2020 09:32:33 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Badhri Jagan Sridharan <badhri@...gle.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        reg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3 v2] usb: typec: tcpm: Stay in BIST mode till hardreset
 or unattached

On 7/14/20 4:12 PM, Badhri Jagan Sridharan wrote:
> Port starts to toggle when transitioning to unattached state.
> This is incorrect while in BIST mode.
> 
> 6.4.3.1 BIST Carrier Mode
> Upon receipt of a BIST Message, with a BIST Carrier Mode BIST Data Object,
> the UUT Shall send out a continuous string of BMC encoded alternating "1"s
> and “0”s. The UUT Shall exit the Continuous BIST Mode within
> tBISTContMode of this Continuous BIST Mode being enabled(see
> Section 6.6.7.2).
> 
> 6.4.3.2 BIST Test Data
> Upon receipt of a BIST Message, with a BIST Test Data BIST Data Object,
> the UUT Shall return a GoodCRC Message and Shall enter a test mode in which
> it sends no further Messages except for GoodCRC Messages in response to
> received Messages. See Section 5.9.2 for the definition of the Test Data
> Frame. The test Shall be ended by sending Hard Reset Signaling to reset the
> UUT.
> 
> Signed-off-by: Badhri Jagan Sridharan <badhri@...gle.com>
> ---
> Version history:
> Changes since V1:
> -  None
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 8 ++++++--
>  include/linux/usb/pd.h        | 1 +
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 379fcab9dbd973..245cfe80948502 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -3559,6 +3559,8 @@ static void run_state_machine(struct tcpm_port *port)
>  		switch (BDO_MODE_MASK(port->bist_request)) {
>  		case BDO_MODE_CARRIER2:
>  			tcpm_pd_transmit(port, TCPC_TX_BIST_MODE_2, NULL);
> +			tcpm_set_state(port, unattached_state(port),
> +				       PD_T_BIST_CONT_MODE);

One line should now be sufficient.

>  			break;
>  		case BDO_MODE_TESTDATA:
>  			if (port->tcpc->set_bist_data) {
> @@ -3569,8 +3571,6 @@ static void run_state_machine(struct tcpm_port *port)
>  		default:
>  			break;
>  		}
> -		/* Always switch to unattached state */
> -		tcpm_set_state(port, unattached_state(port), 0);
>  		break;
>  	case GET_STATUS_SEND:
>  		tcpm_pd_send_control(port, PD_CTRL_GET_STATUS);
> @@ -3960,6 +3960,10 @@ static void _tcpm_pd_vbus_off(struct tcpm_port *port)
>  static void _tcpm_pd_hard_reset(struct tcpm_port *port)
>  {
>  	tcpm_log_force(port, "Received hard reset");
> +	if (port->bist_request ==  BDO_MODE_TESTDATA &&

Nit: Extra space after "=="

Also, I think this now fits into one line (line length limit is 100).

> +	    port->tcpc->set_bist_data)
> +		port->tcpc->set_bist_data(port->tcpc, false);
> +
>  	/*
>  	 * If we keep receiving hard reset requests, executing the hard reset
>  	 * must have failed. Revert to error recovery if that happens.
> diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
> index a665d7f211424d..b420d8d613cd23 100644
> --- a/include/linux/usb/pd.h
> +++ b/include/linux/usb/pd.h
> @@ -483,4 +483,5 @@ 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	60 /* 30 - 60 ms */

Maybe a bit less to ensure that it is disabled within 60 ms. If we use
the maximum, we may end up having it enabled for more than 60 ms, which
would violate the specification and may tick some picky compliance test
system.

Thanks,
Guenter

>  #endif /* __LINUX_USB_PD_H */
>