linux-kernel - Re: [PATCH v1] usb: typec: tcpci: Prevent Sink disconnection before vPpsShutdown in SPR PPS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z4jsp4J6AX0X-uwX@kuha.fi.intel.com>
Date: Thu, 16 Jan 2025 13:25:27 +0200
From: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To: Kyle Tso <kyletso@...gle.com>
Cc: gregkh@...uxfoundation.org, andre.draszik@...aro.org,
	rdbabiera@...gle.com, m.felsch@...gutronix.de, xu.yang_2@....com,
	u.kleine-koenig@...libre.com, emanuele.ghidoli@...adex.com,
	badhri@...gle.com, amitsd@...gle.com, linux-kernel@...r.kernel.org,
	linux-usb@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v1] usb: typec: tcpci: Prevent Sink disconnection before
 vPpsShutdown in SPR PPS

On Tue, Jan 14, 2025 at 10:24:35PM +0800, Kyle Tso wrote:
> The Source can drop its output voltage to the minimum of the requested
> PPS APDO voltage range when it is in Current Limit Mode. If this voltage
> falls within the range of vPpsShutdown, the Source initiates a Hard
> Reset and discharges Vbus. However, currently the Sink may disconnect
> before the voltage reaches vPpsShutdown, leading to unexpected behavior.
> 
> Prevent premature disconnection by setting the Sink's disconnect
> threshold to the minimum vPpsShutdown value. Additionally, consider the
> voltage drop due to IR drop when calculating the appropriate threshold.
> This ensures a robust and reliable interaction between the Source and
> Sink during SPR PPS Current Limit Mode operation.
> 
> Fixes: 4288debeaa4e ("usb: typec: tcpci: Fix up sink disconnect thresholds for PD")
> Cc: stable@...r.kernel.org
> Signed-off-by: Kyle Tso <kyletso@...gle.com>

You've resend this, right? So is this v2 (or v1)?

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

> ---
>  drivers/usb/typec/tcpm/tcpci.c | 13 +++++++++----
>  drivers/usb/typec/tcpm/tcpm.c  |  8 +++++---
>  include/linux/usb/tcpm.h       |  3 ++-
>  3 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index 48762508cc86..19ab6647af70 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -27,6 +27,7 @@
>  #define	VPPS_NEW_MIN_PERCENT			95
>  #define	VPPS_VALID_MIN_MV			100
>  #define	VSINKDISCONNECT_PD_MIN_PERCENT		90
> +#define	VPPS_SHUTDOWN_MIN_PERCENT		85
>  
>  struct tcpci {
>  	struct device *dev;
> @@ -366,7 +367,8 @@ static int tcpci_enable_auto_vbus_discharge(struct tcpc_dev *dev, bool enable)
>  }
>  
>  static int tcpci_set_auto_vbus_discharge_threshold(struct tcpc_dev *dev, enum typec_pwr_opmode mode,
> -						   bool pps_active, u32 requested_vbus_voltage_mv)
> +						   bool pps_active, u32 requested_vbus_voltage_mv,
> +						   u32 apdo_min_voltage_mv)
>  {
>  	struct tcpci *tcpci = tcpc_to_tcpci(dev);
>  	unsigned int pwr_ctrl, threshold = 0;
> @@ -388,9 +390,12 @@ static int tcpci_set_auto_vbus_discharge_threshold(struct tcpc_dev *dev, enum ty
>  		threshold = AUTO_DISCHARGE_DEFAULT_THRESHOLD_MV;
>  	} else if (mode == TYPEC_PWR_MODE_PD) {
>  		if (pps_active)
> -			threshold = ((VPPS_NEW_MIN_PERCENT * requested_vbus_voltage_mv / 100) -
> -				     VSINKPD_MIN_IR_DROP_MV - VPPS_VALID_MIN_MV) *
> -				     VSINKDISCONNECT_PD_MIN_PERCENT / 100;
> +			/*
> +			 * To prevent disconnect when the source is in Current Limit Mode.
> +			 * Set the threshold to the lowest possible voltage vPpsShutdown (min)
> +			 */
> +			threshold = VPPS_SHUTDOWN_MIN_PERCENT * apdo_min_voltage_mv / 100 -
> +				    VSINKPD_MIN_IR_DROP_MV;
>  		else
>  			threshold = ((VSRC_NEW_MIN_PERCENT * requested_vbus_voltage_mv / 100) -
>  				     VSINKPD_MIN_IR_DROP_MV - VSRC_VALID_MIN_MV) *
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 460dbde9fe22..e4b85a09c3ae 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -2973,10 +2973,12 @@ static int tcpm_set_auto_vbus_discharge_threshold(struct tcpm_port *port,
>  		return 0;
>  
>  	ret = port->tcpc->set_auto_vbus_discharge_threshold(port->tcpc, mode, pps_active,
> -							    requested_vbus_voltage);
> +							    requested_vbus_voltage,
> +							    port->pps_data.min_volt);
>  	tcpm_log_force(port,
> -		       "set_auto_vbus_discharge_threshold mode:%d pps_active:%c vbus:%u ret:%d",
> -		       mode, pps_active ? 'y' : 'n', requested_vbus_voltage, ret);
> +		       "set_auto_vbus_discharge_threshold mode:%d pps_active:%c vbus:%u pps_apdo_min_volt:%u ret:%d",
> +		       mode, pps_active ? 'y' : 'n', requested_vbus_voltage,
> +		       port->pps_data.min_volt, ret);
>  
>  	return ret;
>  }
> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> index 061da9546a81..b22e659f81ba 100644
> --- a/include/linux/usb/tcpm.h
> +++ b/include/linux/usb/tcpm.h
> @@ -163,7 +163,8 @@ struct tcpc_dev {
>  	void (*frs_sourcing_vbus)(struct tcpc_dev *dev);
>  	int (*enable_auto_vbus_discharge)(struct tcpc_dev *dev, bool enable);
>  	int (*set_auto_vbus_discharge_threshold)(struct tcpc_dev *dev, enum typec_pwr_opmode mode,
> -						 bool pps_active, u32 requested_vbus_voltage);
> +						 bool pps_active, u32 requested_vbus_voltage,
> +						 u32 pps_apdo_min_voltage);
>  	bool (*is_vbus_vsafe0v)(struct tcpc_dev *dev);
>  	void (*set_partner_usb_comm_capable)(struct tcpc_dev *dev, bool enable);
>  	void (*check_contaminant)(struct tcpc_dev *dev);
> -- 
> 2.47.1.688.g23fc6f90ad-goog

-- 
heikki