[PATCH v1] usb: typec: tcpm: restrict SNK_WAIT_CAPABILITIES_TIMEOUT transitions to non self-powered devices

[PATCH v1] usb: typec: tcpm: restrict SNK_WAIT_CAPABILITIES_TIMEOUT transitions to non self-powered devices

[Amit Sunil Dhamne]

PD3.1 spec ("8.3.3.3.3 PE_SNK_Wait_for_Capabilities State") mandates
that the policy engine perform a hard reset when SinkWaitCapTimer
expires. Instead the code explicitly does a GET_SOURCE_CAP when the
timer expires as part of SNK_WAIT_CAPABILITIES_TIMEOUT. Due to this the
following compliance test failures are reported by the compliance tester
(added excerpts from the PD Test Spec):

* COMMON.PROC.PD.2#1:
  The Tester receives a Get_Source_Cap Message from the UUT. This
  message is valid except the following conditions: [COMMON.PROC.PD.2#1]
    a. The check fails if the UUT sends this message before the Tester
       has established an Explicit Contract
    ...

* TEST.PD.PROT.SNK.4:
  ...
  4. The check fails if the UUT does not send a Hard Reset between
    tTypeCSinkWaitCap min and max. [TEST.PD.PROT.SNK.4#1] The delay is
    between the VBUS present vSafe5V min and the time of the first bit
    of Preamble of the Hard Reset sent by the UUT.

For the purpose of interoperability, restrict the quirk introduced in
https://lore.kernel.org/all/20240523171806.223727-1-sebastian.reichel@collabora.com/
to only non self-powered devices as battery powered devices will not
have the issue mentioned in that commit.

Cc: stable@vger.kernel.org
Fixes: 122968f8dda8 ("usb: typec: tcpm: avoid resets for missing source capability messages")
Reported-by: Badhri Jagan Sridharan <badhri@google.com>
Closes: https://lore.kernel.org/all/CAPTae5LAwsVugb0dxuKLHFqncjeZeJ785nkY4Jfd+M-tCjHSnQ@mail.gmail.com/
Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
Reviewed-by: Badhri Jagan Sridharan <badhri@google.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index d6f2412381cf..c8f467d24fbb 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -4515,7 +4515,8 @@ static inline enum tcpm_state hard_reset_state(struct tcpm_port *port)
 		return ERROR_RECOVERY;
 	if (port->pwr_role == TYPEC_SOURCE)
 		return SRC_UNATTACHED;
-	if (port->state == SNK_WAIT_CAPABILITIES_TIMEOUT)
+	if (port->state == SNK_WAIT_CAPABILITIES ||
+	    port->state == SNK_WAIT_CAPABILITIES_TIMEOUT)
 		return SNK_READY;
 	return SNK_UNATTACHED;
 }
@@ -5043,8 +5044,11 @@ 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, SNK_WAIT_CAPABILITIES_TIMEOUT,
-				       PD_T_SINK_WAIT_CAP);
+			if (!port->self_powered)
+				upcoming_state = SNK_WAIT_CAPABILITIES_TIMEOUT;
+			else
+				upcoming_state = hard_reset_state(port);
+			tcpm_set_state(port, upcoming_state, PD_T_SINK_WAIT_CAP);
 		}
 		break;
 	case SNK_WAIT_CAPABILITIES_TIMEOUT:

base-commit: c6d9e43954bfa7415a1e9efdb2806ec1d8a8afc8
-- 
2.47.0.105.g07ac214952-goog

Re: [PATCH v1] usb: typec: tcpm: restrict SNK_WAIT_CAPABILITIES_TIMEOUT transitions to non self-powered devices

[Xu Yang]

On Wed, Oct 23, 2024 at 07:22:30PM -0700, Amit Sunil Dhamne wrote:
> PD3.1 spec ("8.3.3.3.3 PE_SNK_Wait_for_Capabilities State") mandates
> that the policy engine perform a hard reset when SinkWaitCapTimer
> expires. Instead the code explicitly does a GET_SOURCE_CAP when the
> timer expires as part of SNK_WAIT_CAPABILITIES_TIMEOUT. Due to this the
> following compliance test failures are reported by the compliance tester
> (added excerpts from the PD Test Spec):
> 
> * COMMON.PROC.PD.2#1:
>   The Tester receives a Get_Source_Cap Message from the UUT. This
>   message is valid except the following conditions: [COMMON.PROC.PD.2#1]
>     a. The check fails if the UUT sends this message before the Tester
>        has established an Explicit Contract
>     ...
> 
> * TEST.PD.PROT.SNK.4:
>   ...
>   4. The check fails if the UUT does not send a Hard Reset between
>     tTypeCSinkWaitCap min and max. [TEST.PD.PROT.SNK.4#1] The delay is
>     between the VBUS present vSafe5V min and the time of the first bit
>     of Preamble of the Hard Reset sent by the UUT.
> 
> For the purpose of interoperability, restrict the quirk introduced in
> https://lore.kernel.org/all/20240523171806.223727-1-sebastian.reichel@collabora.com/
> to only non self-powered devices as battery powered devices will not
> have the issue mentioned in that commit.
> 
> Cc: stable@vger.kernel.org
> Fixes: 122968f8dda8 ("usb: typec: tcpm: avoid resets for missing source capability messages")
> Reported-by: Badhri Jagan Sridharan <badhri@google.com>
> Closes: https://lore.kernel.org/all/CAPTae5LAwsVugb0dxuKLHFqncjeZeJ785nkY4Jfd+M-tCjHSnQ@mail.gmail.com/
> Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
> Reviewed-by: Badhri Jagan Sridharan <badhri@google.com>

Tested-by: Xu Yang <xu.yang_2@nxp.com>

Thanks,
Xu Yang

> ---
>  drivers/usb/typec/tcpm/tcpm.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index d6f2412381cf..c8f467d24fbb 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -4515,7 +4515,8 @@ static inline enum tcpm_state hard_reset_state(struct tcpm_port *port)
>  		return ERROR_RECOVERY;
>  	if (port->pwr_role == TYPEC_SOURCE)
>  		return SRC_UNATTACHED;
> -	if (port->state == SNK_WAIT_CAPABILITIES_TIMEOUT)
> +	if (port->state == SNK_WAIT_CAPABILITIES ||
> +	    port->state == SNK_WAIT_CAPABILITIES_TIMEOUT)
>  		return SNK_READY;
>  	return SNK_UNATTACHED;
>  }
> @@ -5043,8 +5044,11 @@ 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, SNK_WAIT_CAPABILITIES_TIMEOUT,
> -				       PD_T_SINK_WAIT_CAP);
> +			if (!port->self_powered)
> +				upcoming_state = SNK_WAIT_CAPABILITIES_TIMEOUT;
> +			else
> +				upcoming_state = hard_reset_state(port);
> +			tcpm_set_state(port, upcoming_state, PD_T_SINK_WAIT_CAP);
>  		}
>  		break;
>  	case SNK_WAIT_CAPABILITIES_TIMEOUT:
> 
> base-commit: c6d9e43954bfa7415a1e9efdb2806ec1d8a8afc8
> -- 
> 2.47.0.105.g07ac214952-goog
> 

Re: [PATCH v1] usb: typec: tcpm: restrict SNK_WAIT_CAPABILITIES_TIMEOUT transitions to non self-powered devices

[Heikki Krogerus]

On Wed, Oct 23, 2024 at 07:22:30PM -0700, Amit Sunil Dhamne wrote:
> PD3.1 spec ("8.3.3.3.3 PE_SNK_Wait_for_Capabilities State") mandates
> that the policy engine perform a hard reset when SinkWaitCapTimer
> expires. Instead the code explicitly does a GET_SOURCE_CAP when the
> timer expires as part of SNK_WAIT_CAPABILITIES_TIMEOUT. Due to this the
> following compliance test failures are reported by the compliance tester
> (added excerpts from the PD Test Spec):
> 
> * COMMON.PROC.PD.2#1:
>   The Tester receives a Get_Source_Cap Message from the UUT. This
>   message is valid except the following conditions: [COMMON.PROC.PD.2#1]
>     a. The check fails if the UUT sends this message before the Tester
>        has established an Explicit Contract
>     ...
> 
> * TEST.PD.PROT.SNK.4:
>   ...
>   4. The check fails if the UUT does not send a Hard Reset between
>     tTypeCSinkWaitCap min and max. [TEST.PD.PROT.SNK.4#1] The delay is
>     between the VBUS present vSafe5V min and the time of the first bit
>     of Preamble of the Hard Reset sent by the UUT.
> 
> For the purpose of interoperability, restrict the quirk introduced in
> https://lore.kernel.org/all/20240523171806.223727-1-sebastian.reichel@collabora.com/
> to only non self-powered devices as battery powered devices will not
> have the issue mentioned in that commit.
> 
> Cc: stable@vger.kernel.org
> Fixes: 122968f8dda8 ("usb: typec: tcpm: avoid resets for missing source capability messages")
> Reported-by: Badhri Jagan Sridharan <badhri@google.com>
> Closes: https://lore.kernel.org/all/CAPTae5LAwsVugb0dxuKLHFqncjeZeJ785nkY4Jfd+M-tCjHSnQ@mail.gmail.com/
> Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
> Reviewed-by: Badhri Jagan Sridharan <badhri@google.com>

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

> ---
>  drivers/usb/typec/tcpm/tcpm.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index d6f2412381cf..c8f467d24fbb 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -4515,7 +4515,8 @@ static inline enum tcpm_state hard_reset_state(struct tcpm_port *port)
>  		return ERROR_RECOVERY;
>  	if (port->pwr_role == TYPEC_SOURCE)
>  		return SRC_UNATTACHED;
> -	if (port->state == SNK_WAIT_CAPABILITIES_TIMEOUT)
> +	if (port->state == SNK_WAIT_CAPABILITIES ||
> +	    port->state == SNK_WAIT_CAPABILITIES_TIMEOUT)
>  		return SNK_READY;
>  	return SNK_UNATTACHED;
>  }
> @@ -5043,8 +5044,11 @@ 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, SNK_WAIT_CAPABILITIES_TIMEOUT,
> -				       PD_T_SINK_WAIT_CAP);
> +			if (!port->self_powered)
> +				upcoming_state = SNK_WAIT_CAPABILITIES_TIMEOUT;
> +			else
> +				upcoming_state = hard_reset_state(port);
> +			tcpm_set_state(port, upcoming_state, PD_T_SINK_WAIT_CAP);
>  		}
>  		break;
>  	case SNK_WAIT_CAPABILITIES_TIMEOUT:
> 
> base-commit: c6d9e43954bfa7415a1e9efdb2806ec1d8a8afc8
> -- 
> 2.47.0.105.g07ac214952-goog
> 

-- 
heikki

Re: [PATCH v1] usb: typec: tcpm: restrict SNK_WAIT_CAPABILITIES_TIMEOUT transitions to non self-powered devices

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3446 bytes --]

Hi,

On Wed, Oct 23, 2024 at 07:22:30PM -0700, Amit Sunil Dhamne wrote:
> PD3.1 spec ("8.3.3.3.3 PE_SNK_Wait_for_Capabilities State") mandates
> that the policy engine perform a hard reset when SinkWaitCapTimer
> expires. Instead the code explicitly does a GET_SOURCE_CAP when the
> timer expires as part of SNK_WAIT_CAPABILITIES_TIMEOUT. Due to this the
> following compliance test failures are reported by the compliance tester
> (added excerpts from the PD Test Spec):
> 
> * COMMON.PROC.PD.2#1:
>   The Tester receives a Get_Source_Cap Message from the UUT. This
>   message is valid except the following conditions: [COMMON.PROC.PD.2#1]
>     a. The check fails if the UUT sends this message before the Tester
>        has established an Explicit Contract
>     ...
> 
> * TEST.PD.PROT.SNK.4:
>   ...
>   4. The check fails if the UUT does not send a Hard Reset between
>     tTypeCSinkWaitCap min and max. [TEST.PD.PROT.SNK.4#1] The delay is
>     between the VBUS present vSafe5V min and the time of the first bit
>     of Preamble of the Hard Reset sent by the UUT.
> 
> For the purpose of interoperability, restrict the quirk introduced in
> https://lore.kernel.org/all/20240523171806.223727-1-sebastian.reichel@collabora.com/
> to only non self-powered devices as battery powered devices will not
> have the issue mentioned in that commit.
> 
> Cc: stable@vger.kernel.org
> Fixes: 122968f8dda8 ("usb: typec: tcpm: avoid resets for missing source capability messages")
> Reported-by: Badhri Jagan Sridharan <badhri@google.com>
> Closes: https://lore.kernel.org/all/CAPTae5LAwsVugb0dxuKLHFqncjeZeJ785nkY4Jfd+M-tCjHSnQ@mail.gmail.com/
> Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
> Reviewed-by: Badhri Jagan Sridharan <badhri@google.com>
> ---

I actually had this constrained to the !self_powered use-case
originally (before sending to the ML). Since I didn't see a good
reason for the extra check, I decided to keep the code simple :)

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

>  drivers/usb/typec/tcpm/tcpm.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index d6f2412381cf..c8f467d24fbb 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -4515,7 +4515,8 @@ static inline enum tcpm_state hard_reset_state(struct tcpm_port *port)
>  		return ERROR_RECOVERY;
>  	if (port->pwr_role == TYPEC_SOURCE)
>  		return SRC_UNATTACHED;
> -	if (port->state == SNK_WAIT_CAPABILITIES_TIMEOUT)
> +	if (port->state == SNK_WAIT_CAPABILITIES ||
> +	    port->state == SNK_WAIT_CAPABILITIES_TIMEOUT)
>  		return SNK_READY;
>  	return SNK_UNATTACHED;
>  }
> @@ -5043,8 +5044,11 @@ 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, SNK_WAIT_CAPABILITIES_TIMEOUT,
> -				       PD_T_SINK_WAIT_CAP);
> +			if (!port->self_powered)
> +				upcoming_state = SNK_WAIT_CAPABILITIES_TIMEOUT;
> +			else
> +				upcoming_state = hard_reset_state(port);
> +			tcpm_set_state(port, upcoming_state, PD_T_SINK_WAIT_CAP);
>  		}
>  		break;
>  	case SNK_WAIT_CAPABILITIES_TIMEOUT:
> 
> base-commit: c6d9e43954bfa7415a1e9efdb2806ec1d8a8afc8
> -- 
> 2.47.0.105.g07ac214952-goog
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]