Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 31 Jul 2023 14:16:22 +0300
From:   Heikki Krogerus <heikki.krogerus@linux.intel.com>
To:     RD Babiera <rdbabiera@google.com>
Cc:     linux@roeck-us.net, gregkh@linuxfoundation.org, kyletso@google.com,
        badhri@google.com, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH v1] usb: typec: tcpm: set initial svdm version based on
 pd revision
Message-ID: <ZMeYBvD+I1U8HNfF@kuha.fi.intel.com>
References: <20230726020810.1408299-1-rdbabiera@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230726020810.1408299-1-rdbabiera@google.com>
Precedence: bulk

Hi,

I'm sorry to keep you waiting.

On Wed, Jul 26, 2023 at 02:08:07AM +0000, RD Babiera wrote:
> When sending Discover Identity messages to a Port Partner that uses Power
> Delivery v2 and SVDM v1, we currently send PD v2 messages with SVDM v2.0,
> expecting the port partner to respond with its highest supported SVDM
> version as stated in Section 6.4.4.2.3 in the Power Delivery v3
> specification. However, sending SVDM v2 to some Power Delivery v2 port
> partners results in a NAK whereas sending SVDM v1 does not.
> 
> NAK messages can be handled by the initiator (PD v3 section 6.4.4.2.5.1),
> and one solution could be to resend Discover Identity on a lower SVDM
> version if possible. But, Section 6.4.4.3 of PD v2 states that "A NAK
> response Should be taken as an indication not to retry that particular
> Command."
> 
> Instead, we can set the SVDM version to the maximum one supported by the
> negotiated PD revision. When operating in PD v2, this obeys Section
> 6.4.4.2.3, which states the SVDM field "Shall be set to zero to indicate
> Version 1.0." In PD v3, the SVDM field "Shall be set to 01b to indicate
> Version 2.0."

This makes sense to me, but couple of nitpicks below.

> Fixes: c34e85fa69b9 ("usb: typec: tcpm: Send DISCOVER_IDENTITY from dedicated work")
> Cc: stable@vger.kernel.org
> Signed-off-by: RD Babiera <rdbabiera@google.com>
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 35 +++++++++++++++++++++++++++++++----
>  1 file changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 829d75ebab42..5b0a428fcf5d 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -3928,6 +3928,31 @@ static enum typec_cc_status tcpm_pwr_opmode_to_rp(enum typec_pwr_opmode opmode)
>  	}
>  }
>  
> +static void tcpm_set_initial_svdm_version(struct tcpm_port *port)
> +{
> +	switch (port->negotiated_rev) {
> +	case PD_REV30:
> +		break;
> +	/*
> +	 * 6.4.4.2.3 Structured VDM Version
> +	 * 2.0 states "At this time, there is only one version (1.0) defined.
> +	 * This field Shall be set to zero to indicate Version 1.0."
> +	 * 3.0 states "This field Shall be set to 01b to indicate Version 2.0."
> +	 * To ensure that we follow the Power Delivery revision we are currently
> +	 * operating on, downgrade the SVDM version to the highest one supported
> +	 * by the Power Delivery revision.
> +	 */
> +	case PD_REV20:
> +		typec_partner_set_svdm_version(port->partner,
> +					       SVDM_VER_1_0);

One line is enough.

> +		break;
> +	default:
> +		typec_partner_set_svdm_version(port->partner,
> +					       SVDM_VER_1_0);

Ditto.

> +		break;
> +	}
> +}
> +
>  static void run_state_machine(struct tcpm_port *port)
>  {
>  	int ret;
> @@ -4165,9 +4190,10 @@ static void run_state_machine(struct tcpm_port *port)
>  		 * For now, this driver only supports SOP for DISCOVER_IDENTITY, thus using
>  		 * port->explicit_contract to decide whether to send the command.
>  		 */
> -		if (port->explicit_contract)
> +		if (port->explicit_contract) {
> +			tcpm_set_initial_svdm_version(port);
>  			mod_send_discover_delayed_work(port, 0);
> -		else
> +		} else
>  			port->send_discover = false;

The else statement needs to be wrapped in curly brackets in this case
since the if statement had them.

>  		/*
> @@ -4455,9 +4481,10 @@ static void run_state_machine(struct tcpm_port *port)
>  		 * For now, this driver only supports SOP for DISCOVER_IDENTITY, thus using
>  		 * port->explicit_contract.
>  		 */
> -		if (port->explicit_contract)
> +		if (port->explicit_contract) {
> +			tcpm_set_initial_svdm_version(port);
>  			mod_send_discover_delayed_work(port, 0);
> -		else
> +		} else
>  			port->send_discover = false;

Ditto.

thanks,

-- 
heikki