Received-SPF: pass (google.com: domain of linux-kernel+bounces-5370-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 19 Dec 2023 16:26:53 +0200
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
To: RD Babiera <rdbabiera@google.com>
Cc: linux@roeck-us.net, gregkh@linuxfoundation.org,
	linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
	badhri@google.com, bryan.odonoghue@linaro.org, agross@kernel.org,
	andersson@kernel.org, konrad.dybcio@linaro.org
Subject: Re: [PATCH v2 09/12] usb: typec: tcpm: add state machine support for
 SRC_VDM_IDENTITY_REQUEST
Message-ID: <ZYGoLTm2VyU8cWfD@kuha.fi.intel.com>
References: <20231214230850.379863-14-rdbabiera@google.com>
 <20231214230850.379863-23-rdbabiera@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231214230850.379863-23-rdbabiera@google.com>

On Thu, Dec 14, 2023 at 11:08:53PM +0000, RD Babiera wrote:
> Add SRC_VDM_IDENTITY_REQUEST state which first enters after SRC_STARTUP.
> The state sends Discover Identity on SOP' and transitions to
> SRC_SEND_CAPABILITIES. SRC_SEND_CAPABILITIES will transition back into
> SRC_VDM_IDENTITY_REQUEST instead of retrying immediately.
> 
> Signed-off-by: RD Babiera <rdbabiera@google.com>
> ---

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

>  drivers/usb/typec/tcpm/tcpm.c | 49 ++++++++++++++++++++++++++++++-----
>  1 file changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 5924e359e14d..e21bc2eea3fc 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -146,7 +146,9 @@
>  	S(PORT_RESET_WAIT_OFF),			\
>  						\
>  	S(AMS_START),				\
> -	S(CHUNK_NOT_SUPP)
> +	S(CHUNK_NOT_SUPP),			\
> +						\
> +	S(SRC_VDM_IDENTITY_REQUEST)
>  
>  #define FOREACH_AMS(S)				\
>  	S(NONE_AMS),				\
> @@ -1956,6 +1958,7 @@ static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
>  					ret = tcpm_ams_start(port, VCONN_SWAP);
>  					if (!ret)
>  						return 0;
> +					/* Cannot perform Vconn swap */
>  					port->upcoming_state = INVALID_STATE;
>  					port->send_discover_prime = false;
>  				}
> @@ -1987,6 +1990,16 @@ static int tcpm_pd_svdm(struct tcpm_port *port, struct typec_altmode *adev,
>  				 * the svdm_version for the cable moving forward.
>  				 */
>  				svdm_consume_identity_sop_prime(port, p, cnt);
> +
> +				/*
> +				 * If received in SRC_VDM_IDENTITY_REQUEST, continue
> +				 * to SRC_SEND_CAPABILITIES
> +				 */
> +				if (port->state == SRC_VDM_IDENTITY_REQUEST) {
> +					tcpm_set_state(port, SRC_SEND_CAPABILITIES, 0);
> +					return 0;
> +				}
> +
>  				*response_tx_sop_type = TCPC_TX_SOP;
>  				response[0] = VDO(USB_SID_PD, 1,
>  						  typec_get_negotiated_svdm_version(typec),
> @@ -2281,7 +2294,8 @@ static void vdm_run_state_machine(struct tcpm_port *port)
>  		 * if there's traffic or we're not in PDO ready state don't send
>  		 * a VDM.
>  		 */
> -		if (port->state != SRC_READY && port->state != SNK_READY) {
> +		if (port->state != SRC_READY && port->state != SNK_READY &&
> +		    port->state != SRC_VDM_IDENTITY_REQUEST) {
>  			port->vdm_sm_running = false;
>  			break;
>  		}
> @@ -2357,13 +2371,22 @@ static void vdm_run_state_machine(struct tcpm_port *port)
>  			tcpm_ams_finish(port);
>  		break;
>  	case VDM_STATE_ERR_SEND:
> +		/*
> +		 * When sending Discover Identity to SOP' before establishing an
> +		 * explicit contract, do not retry. Instead, weave sending
> +		 * Source_Capabilities over SOP and Discover Identity over SOP'.
> +		 */
> +		if (port->state == SRC_VDM_IDENTITY_REQUEST) {
> +			tcpm_ams_finish(port);
> +			port->vdm_state = VDM_STATE_DONE;
> +			tcpm_set_state(port, SRC_SEND_CAPABILITIES, 0);
>  		/*
>  		 * A partner which does not support USB PD will not reply,
>  		 * so this is not a fatal error. At the same time, some
>  		 * devices may not return GoodCRC under some circumstances,
>  		 * so we need to retry.
>  		 */
> -		if (port->vdm_retries < 3) {
> +		} else if (port->vdm_retries < 3) {
>  			tcpm_log(port, "VDM Tx error, retry");
>  			port->vdm_retries++;
>  			port->vdm_state = VDM_STATE_READY;
> @@ -4477,8 +4500,12 @@ static void run_state_machine(struct tcpm_port *port)
>  		}
>  		ret = tcpm_pd_send_source_caps(port);
>  		if (ret < 0) {
> -			tcpm_set_state(port, SRC_SEND_CAPABILITIES,
> -				       PD_T_SEND_SOURCE_CAP);
> +			if (tcpm_can_communicate_sop_prime(port) &&
> +			    IS_ERR_OR_NULL(port->cable))
> +				tcpm_set_state(port, SRC_VDM_IDENTITY_REQUEST, 0);
> +			else
> +				tcpm_set_state(port, SRC_SEND_CAPABILITIES,
> +					       PD_T_SEND_SOURCE_CAP);
>  		} else {
>  			/*
>  			 * Per standard, we should clear the reset counter here.
> @@ -5393,6 +5420,15 @@ static void run_state_machine(struct tcpm_port *port)
>  		tcpm_pd_send_control(port, PD_CTRL_NOT_SUPP, TCPC_TX_SOP);
>  		tcpm_set_state(port, port->pwr_role == TYPEC_SOURCE ? SRC_READY : SNK_READY, 0);
>  		break;
> +
> +	/* Cable states */
> +	case SRC_VDM_IDENTITY_REQUEST:
> +		port->send_discover_prime = true;
> +		port->tx_sop_type = TCPC_TX_SOP_PRIME;
> +		mod_send_discover_delayed_work(port, 0);
> +		port->upcoming_state = SRC_SEND_CAPABILITIES;
> +		break;
> +
>  	default:
>  		WARN(1, "Unexpected port state %d\n", port->state);
>  		break;
> @@ -6118,7 +6154,8 @@ static void tcpm_send_discover_work(struct kthread_work *work)
>  	}
>  
>  	/* Retry if the port is not idle */
> -	if ((port->state != SRC_READY && port->state != SNK_READY) || port->vdm_sm_running) {
> +	if ((port->state != SRC_READY && port->state != SNK_READY &&
> +	     port->state != SRC_VDM_IDENTITY_REQUEST) || port->vdm_sm_running) {
>  		mod_send_discover_delayed_work(port, SEND_DISCOVER_RETRY_MS);
>  		goto unlock;
>  	}
> -- 
> 2.43.0.472.g3155946c3a-goog

-- 
heikki