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