Received-SPF: pass (google.com: domain of linux-kernel+bounces-198969-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: Mon, 3 Jun 2024 13:13:01 +0300
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
To: Sebastian Reichel <sebastian.reichel@collabora.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-usb@vger.kernel.org, Guenter Roeck <linux@roeck-us.net>,
	linux-kernel@vger.kernel.org, kernel@collabora.com
Subject: Re: [PATCH v1 1/1] usb: typec: tcpm: avoid resets for missing source
 capability messages
Message-ID: <Zl2XLbOfAgYq6yWE@kuha.fi.intel.com>
References: <20240523171806.223727-1-sebastian.reichel@collabora.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240523171806.223727-1-sebastian.reichel@collabora.com>

On Thu, May 23, 2024 at 07:17:52PM +0200, Sebastian Reichel wrote:
> When the Linux Type-C controller drivers probe, they requests a soft
> reset, which should result in the source restarting to send Source
> Capability messages again independently of the previous state.
> Unfortunately some USB PD sources do not follow the specification and
> do not send them after a soft reset when they already negotiated a
> specific contract before. The current way (and what is described in the
> specificiation) to resolve this problem is triggering a hard reset.
> 
> But a hard reset is fatal on batteryless platforms powered via USB-C PD,
> since that removes VBUS for some time. Since this is triggered at boot
> time, the system will be stuck in a boot loop. Examples for platforms
> affected by this are the Radxa Rock 5B or the Libre Computer Renegade
> Elite ROC-RK3399-PC.
> 
> Instead of directly trying a hard reset when no Source Capability
> message is send by the USB-PD source automatically, this changes the
> state machine to try explicitly asking for the capabilities by sending
> a Get Source Capability control message.
> 
> For me this solves issues with 2 different USB-PD sources - a RAVPower
> powerbank and a Lemorele USB-C dock. Every other PD source I own
> follows the specification and automatically sends the Source Capability
> message after a soft reset, which works with or without this change.
> 
> I decided against making this extra step limited to devices not having
> the self_powered flag set, since I don't see any huge drawbacks in this
> approach and it keeps the logic simpler. The worst case scenario would
> be a power source, which is really stuck. In that case the hard reset
> is delayed by another 310ms.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

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

> ---
>  drivers/usb/typec/tcpm/tcpm.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 375bc84d14a2..bac6866617c8 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -57,6 +57,7 @@
>  	S(SNK_DISCOVERY_DEBOUNCE),		\
>  	S(SNK_DISCOVERY_DEBOUNCE_DONE),		\
>  	S(SNK_WAIT_CAPABILITIES),		\
> +	S(SNK_WAIT_CAPABILITIES_TIMEOUT),	\
>  	S(SNK_NEGOTIATE_CAPABILITIES),		\
>  	S(SNK_NEGOTIATE_PPS_CAPABILITIES),	\
>  	S(SNK_TRANSITION_SINK),			\
> @@ -3108,7 +3109,8 @@ static void tcpm_pd_data_request(struct tcpm_port *port,
>  						   PD_MSG_CTRL_REJECT :
>  						   PD_MSG_CTRL_NOT_SUPP,
>  						   NONE_AMS);
> -		} else if (port->state == SNK_WAIT_CAPABILITIES) {
> +		} else if (port->state == SNK_WAIT_CAPABILITIES ||
> +			   port->state == SNK_WAIT_CAPABILITIES_TIMEOUT) {
>  		/*
>  		 * This message may be received even if VBUS is not
>  		 * present. This is quite unexpected; see USB PD
> @@ -5039,10 +5041,31 @@ static void run_state_machine(struct tcpm_port *port)
>  			tcpm_set_state(port, SNK_SOFT_RESET,
>  				       PD_T_SINK_WAIT_CAP);
>  		} else {
> -			tcpm_set_state(port, hard_reset_state(port),
> +			tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
>  				       PD_T_SINK_WAIT_CAP);
>  		}
>  		break;
> +	case SNK_WAIT_CAPABILITIES_TIMEOUT:
> +		/*
> +		 * There are some USB PD sources in the field, which do not
> +		 * properly implement the specification and fail to start
> +		 * sending Source Capability messages after a soft reset. The
> +		 * specification suggests to do a hard reset when no Source
> +		 * capability message is received within PD_T_SINK_WAIT_CAP,
> +		 * but that might effectively kil the machine's power source.
> +		 *
> +		 * This slightly diverges from the specification and tries to
> +		 * recover from this by explicitly asking for the capabilities
> +		 * using the Get_Source_Cap control message before falling back
> +		 * to a hard reset. The control message should also be supported
> +		 * and handled by all USB PD source and dual role devices
> +		 * according to the specification.
> +		 */
> +		if (tcpm_pd_send_control(port, PD_CTRL_GET_SOURCE_CAP, TCPC_TX_SOP))
> +			tcpm_set_state_cond(port, hard_reset_state(port), 0);
> +		else
> +			tcpm_set_state(port, hard_reset_state(port), PD_T_SINK_WAIT_CAP);
> +		break;
>  	case SNK_NEGOTIATE_CAPABILITIES:
>  		port->pd_capable = true;
>  		tcpm_set_partner_usb_comm_capable(port,
> -- 
> 2.43.0

-- 
heikki