Re: [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

Hi Yanik,

On Tue, Oct 22, 2024 at 05:28:51PM +0000, Yanik Fuchs wrote:
> Good Evening
> 
> Here is a Patch to resolve an issue with TCPM if Vbus was never low.
> Please consider that this is one of my first kernel pull requests, I may have missunderstood the process.

Welcome aboard :)

Thank you for the patch. Unfortunately it is not properly formatted.
As this is a patch, you can't really comment it like this here.
Instead you should put any additional comments...

> Freundliche Grüsse
> Best regards
> 
> 
> Yanik Fuchs
> 
> ---
> 
> >From 604b97b6394b5643394bc63d4ac691c098c99c40 Mon Sep 17 00:00:00 2001
> From: yfu <yanikfuchs@me.com>
> Date: Tue, 22 Oct 2024 18:23:18 +0200
> Subject: [PATCH] usb: typec: tcpm: Prevent Hard_Reset if Vbus was never low
> 
> Before this patch, tcpm went into SOFT_RESET state, if Vbus was never low
> resulting in Hard_Reset, if power supply does not support USB_PD Soft_Reset.
> 
> In order to prevent this, I remove the Vbus check completely, so that 
> we go as well into the SNK_WAIT_CAPABILITIES_TIMEOUT state. There, we send 
> PD_CTRL_GET_SOURCE_CAP which many power supply do support.
> (122968f8dda8 usb: typec: tcpm: avoid resets for missing source capability messages)
> 
> Additionally, I added SOFT_RESET (instead of Hard_Reset) as Fallback solution
> if we still not have gotten any capabilities. Hard_Reset is now only done, 
> if PD_CTRL_GET_SOURCE_CAP and SOFT_RESET fail to get capabilities.
> ---

... here after those three lines. The proper format, and the whole
development process is documented here:
https://docs.kernel.org/process/development-process.html

You have also not signed your patch with a Signed-off-by tag. The
importance of the signature in patches is explained in the fifth
section of the development process documentation, here:
https://docs.kernel.org/process/5.Posting.html

>  drivers/usb/typec/tcpm/tcpm.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index fc619478200f..c7a59c9f78ee 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -5038,14 +5038,8 @@ static void run_state_machine(struct tcpm_port *port)
>  		 * were already in a stable contract before this boot.
>  		 * Do this only once.
>  		 */
> -		if (port->vbus_never_low) {
> -			port->vbus_never_low = false;
> -			tcpm_set_state(port, SNK_SOFT_RESET,
> +		tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
>  				       PD_T_SINK_WAIT_CAP);

Here you should fix the alignment of the code so it matches the
parentheses. You can use the scripts/checkpatch.pl script, which is
part of the kernel source, to detect this kind of issues in your code
by supplying your patch to it.

> -		} else {
> -			tcpm_set_state(port, SNK_WAIT_CAPABILITIES_TIMEOUT,
> -				       PD_T_SINK_WAIT_CAP);
> -		}
>  		break;
>  	case SNK_WAIT_CAPABILITIES_TIMEOUT:
>  		/*
> @@ -5064,7 +5058,7 @@ static void run_state_machine(struct tcpm_port *port)
>  		 * 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);
> +			tcpm_set_state_cond(port, SNK_SOFT_RESET, 0);
>  		else
>  			tcpm_set_state(port, hard_reset_state(port), PD_T_SINK_WAIT_CAP);
>  		break;
> -- 
> 2.34.1

Otherwise the code looks very good to me, but I can't yet say if the
change is appropriate. Let's fix the patch format first.

Br,

-- 
heikki