Re: [PATCH 1/7] usb: typec: ucsi: fix race condition in connection change ACK'ing

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

Hi Dmitry,

On Wed, Mar 13, 2024 at 05:54:11AM +0200, Dmitry Baryshkov wrote:
> The code to handle connection change events contains a race: there is an
> open window for notifications to arrive between clearing EVENT_PENDING
> bit and sending the ACK_CC_CI command to acknowledge the connection
> change. This is mostly not an issue, but on Qualcomm platforms when the
> PPM receives ACK_CC_CI with the ConnectorChange bit set if there is no
> pending reported Connector Change, it responds with the CommandCompleted
> + NotSupported notifications, completely breaking UCSI state machine.
> 
> Fix this by reading out CCI after ACK_CC_CI and scheduling the work if
> there is a connector change reported.
 
> Fixes: bdc62f2bae8f ("usb: typec: ucsi: Simplified registration and I/O API")
> Cc: stable@vger.kernel.org
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

UCSI specification quite clearly states that the PPM must wait until
OPM has acknowledged the notification before sending the next
notification, so this looks like a workaround for Qualcomm specific
issue. Ideally it would have been isolated - now this is done on
every platform.

I'm a little bit uncomfortable with the unconditional reading of the
CCI field. On most systems reading the field will clear it completely.
Hopefully that will not cause more problems.

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

> ---
>  drivers/usb/typec/ucsi/ucsi.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index cf52cb34d285..4abb752c6806 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -61,12 +61,28 @@ static int ucsi_acknowledge_command(struct ucsi *ucsi)
>  
>  static int ucsi_acknowledge_connector_change(struct ucsi *ucsi)
>  {
> +	unsigned int con_num;
>  	u64 ctrl;
> +	u32 cci;
> +	int ret;
>  
>  	ctrl = UCSI_ACK_CC_CI;
>  	ctrl |= UCSI_ACK_CONNECTOR_CHANGE;
>  
> -	return ucsi->ops->sync_write(ucsi, UCSI_CONTROL, &ctrl, sizeof(ctrl));
> +	ret = ucsi->ops->sync_write(ucsi, UCSI_CONTROL, &ctrl, sizeof(ctrl));
> +	if (ret)
> +		return ret;
> +
> +	clear_bit(EVENT_PENDING, &ucsi->flags);
> +	ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
> +	if (ret)
> +		return ret;
> +
> +	con_num = UCSI_CCI_CONNECTOR(cci);
> +	if (con_num)
> +		ucsi_connector_change(ucsi, con_num);
> +
> +	return 0;
>  }
>  
>  static int ucsi_exec_command(struct ucsi *ucsi, u64 command);
> @@ -1215,8 +1231,6 @@ static void ucsi_handle_connector_change(struct work_struct *work)
>  	if (con->status.change & UCSI_CONSTAT_CAM_CHANGE)
>  		ucsi_partner_task(con, ucsi_check_altmodes, 1, 0);
>  
> -	clear_bit(EVENT_PENDING, &con->ucsi->flags);
> -
>  	mutex_lock(&ucsi->ppm_lock);
>  	ret = ucsi_acknowledge_connector_change(ucsi);
>  	mutex_unlock(&ucsi->ppm_lock);
> 
> -- 
> 2.39.2

-- 
heikki