| Message ID | 20240116224041.220740-2-lk@c--e.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | UCSI fixes | expand |
On Tue, Jan 16, 2024 at 11:40:39PM +0100, Christian A. Ehrhardt wrote: > Calling ->sync_write must be done while holding the PPM lock as the > mailbox logic does not support concurrent commands. > > Thus protect the only call to ucsi_acknowledge_connector_change > with the PPM lock as it calls ->sync_write. All other calls to > ->sync_write already happen under the PPM lock. > > Signed-off-by: Christian A. Ehrhardt <lk@c--e.de> > --- > NOTE: This is not a theoretical issue. I've seen problems resulting > from the missing lock on real hardware. What commit id does this fix? Should it be cc: stable? thanks, greg k-h
On Wed, Jan 17, 2024 at 06:44:40AM +0100, Greg Kroah-Hartman wrote: > On Tue, Jan 16, 2024 at 11:40:39PM +0100, Christian A. Ehrhardt wrote: > > Calling ->sync_write must be done while holding the PPM lock as the > > mailbox logic does not support concurrent commands. > > > > Thus protect the only call to ucsi_acknowledge_connector_change > > with the PPM lock as it calls ->sync_write. All other calls to > > ->sync_write already happen under the PPM lock. > > > > Signed-off-by: Christian A. Ehrhardt <lk@c--e.de> > > --- > > NOTE: This is not a theoretical issue. I've seen problems resulting > > from the missing lock on real hardware. > > What commit id does this fix? It's hard to tell (due to rewrites, logic and API changes). After digging a bit more I think it is at least a theoretical issues since the introduction of partner tasks. I'll wait a bit for additional feedback and fix this and other issues noticed by your patch bot (sorry for those) in the next iteration. > Should it be cc: stable? Not sure. The race is triggered much more ofter after the quirk added in patch 3/3, so this may not be a practical issue before that. I'll add the tag in the next iteration, though. thanks Christian
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index 61b64558f96c..8f9dff993b3d 100644 --- a/drivers/usb/typec/ucsi/ucsi.c +++ b/drivers/usb/typec/ucsi/ucsi.c @@ -935,7 +935,9 @@ static void ucsi_handle_connector_change(struct work_struct *work) clear_bit(EVENT_PENDING, &con->ucsi->flags); + mutex_lock(&ucsi->ppm_lock); ret = ucsi_acknowledge_connector_change(ucsi); + mutex_unlock(&ucsi->ppm_lock); if (ret) dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__, ret);
Calling ->sync_write must be done while holding the PPM lock as the mailbox logic does not support concurrent commands. Thus protect the only call to ucsi_acknowledge_connector_change with the PPM lock as it calls ->sync_write. All other calls to ->sync_write already happen under the PPM lock. Signed-off-by: Christian A. Ehrhardt <lk@c--e.de> --- NOTE: This is not a theoretical issue. I've seen problems resulting from the missing lock on real hardware. drivers/usb/typec/ucsi/ucsi.c | 2 ++ 1 file changed, 2 insertions(+)