| Message ID | 20240313-qcom-ucsi-fixes-v1-1-74d90cb48a00@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: ucsi: fix several issues manifesting on Qualcomm platforms | expand |
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
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);
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> --- drivers/usb/typec/ucsi/ucsi.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)