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@xxxxxxxxxxxxxxx
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
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@xxxxxxxxxxxxxxx>
> ---
> 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
