| Message ID | 20240320073927.1641788-6-lk@c--e.de |
|---|---|
| State | New |
| Headers | show |
| Series | Fix various races in UCSI | expand |
On Wed, Mar 20, 2024 at 08:39:26AM +0100, Christian A. Ehrhardt wrote: > Check the UCSI_CCI_RESET_COMPLETE complete flag before starting > another reset. Use a UCSI_SET_NOTIFICATION_ENABLE command to clear > the flag if it is set. > > Signed-off-by: Christian A. Ehrhardt <lk@c--e.de> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/usb/typec/ucsi/ucsi.c | 36 ++++++++++++++++++++++++++++++++++- > 1 file changed, 35 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > index 63f340dbd867..85e507df7fa8 100644 > --- a/drivers/usb/typec/ucsi/ucsi.c > +++ b/drivers/usb/typec/ucsi/ucsi.c > @@ -1264,13 +1264,47 @@ static int ucsi_reset_connector(struct ucsi_connector *con, bool hard) > > static int ucsi_reset_ppm(struct ucsi *ucsi) > { > - u64 command = UCSI_PPM_RESET; > + u64 command; > unsigned long tmo; > u32 cci; > int ret; > > mutex_lock(&ucsi->ppm_lock); > > + ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci)); > + if (ret < 0) > + goto out; > + > + /* > + * If UCSI_CCI_RESET_COMPLETE is already set we must clear > + * the flag before we start another reset. Send a > + * UCSI_SET_NOTIFICATION_ENABLE command to achieve this. > + * Ignore a timeout and try the reset anyway if this fails. > + */ > + if (cci & UCSI_CCI_RESET_COMPLETE) { > + command = UCSI_SET_NOTIFICATION_ENABLE; > + ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL, &command, > + sizeof(command)); > + if (ret < 0) > + goto out; > + > + tmo = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS); > + do { > + ret = ucsi->ops->read(ucsi, UCSI_CCI, > + &cci, sizeof(cci)); > + if (ret < 0) > + goto out; > + if (cci & UCSI_CCI_COMMAND_COMPLETE) > + break; > + if (time_is_before_jiffies(tmo)) > + break; > + msleep(20); > + } while (1); > + > + WARN_ON(cci & UCSI_CCI_RESET_COMPLETE); > + } > + > + command = UCSI_PPM_RESET; > ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL, &command, > sizeof(command)); > if (ret < 0) > -- > 2.40.1
On Wed, Mar 20, 2024 at 08:39:26AM +0100, Christian A. Ehrhardt wrote: >Check the UCSI_CCI_RESET_COMPLETE complete flag before starting >another reset. Use a UCSI_SET_NOTIFICATION_ENABLE command to clear >the flag if it is set. Hi Christian, It looks like kernelci is able to trigger the warning added by this commit: [ 15.988528] WARNING: CPU: 0 PID: 8 at drivers/usb/typec/ucsi/ucsi.c:1377 ucsi_reset_ppm+0x1af/0x1c0 [typec_ucsi] [ 15.999924] Modules linked in: snd_sof_pci_intel_cnl snd_sof_intel_hda_generic snd_sof_intel_hda_common snd_soc_hdac_hda snd_sof_intel_hda_mlink snd_sof_intel_hda snd_sof_pci snd_sof_xtensa_dsp snd_sof snd_sof_utils snd_soc_acpi_intel_match snd_soc_acpi snd_soc_acpi_intel_sdca_quirks snd_soc_sdca snd_soc_avs snd_soc_hda_codec snd_hda_ext_core btusb btrtl snd_soc_core intel_pmc_core_pltdrv iTCO_wdt intel_ish_ipc snd_compress btintel btbcm intel_pmc_bxt intel_pmc_core btmtk ucsi_acpi bluetooth gsmi wilco_charger wilco_ec_telem typec_ucsi intel_vsec pmt_telemetry roles pmt_class i2c_hid_acpi iwlmvm x86_pkg_temp_thermal snd_pcm_dmaengine iwlwifi watchdog i2c_hid intel_ishtp idma64 rtc_wilco_ec wilco_ec_debugfs uvcvideo processor_thermal_device_pci_legacy videobuf2_vmalloc elan_i2c intel_soc_dts_iosf videobuf2_memops uvc videobuf2_v4l2 videobuf2_common processor_thermal_device processor_thermal_wt_hint processor_thermal_rfim processor_thermal_wt_req processor_thermal_power_floor processor_thermal_mbox int3403_thermal [ 16.000010] int340x_thermal_zone typec memconsole_coreboot memconsole vpd_sysfs wilco_ec pinctrl_cannonlake intel_vbtn int3400_thermal wilco_ec_events chromeos_pstore coreboot_table acpi_thermal_rel [ 16.120093] CPU: 0 UID: 0 PID: 8 Comm: kworker/0:0 Not tainted 6.13.0-rc2 #1 [ 16.127972] Hardware name: Dell Inc. Arcada/Arcada, BIOS Google_Arcada.12200.103.0 07/29/2020 Linux debian-bookworm-amd64 6.13.0-rc2 #1 SMP PREEMPT_DYNAMIC Su[ 16.137499] Workqueue: events_long ucsi_init_work [typec_ucsi] [ 16.150229] RIP: 0010:ucsi_reset_ppm+0x1af/0x1c0 [typec_ucsi] [ 16.156654] Code: 44 24 04 a9 00 00 00 08 0f 85 36 ff ff ff 4c 89 74 24 10 48 8b 05 b1 fe f1 e1 49 39 c4 79 8d bb 92 ff ff ff e9 1b ff ff ff 90 <0f> 0b 90 e9 4c ff ff ff e8 74 72 f9 df 0f 1f 40 00 90 90 90 90 90 [ 16.177649] RSP: 0018:ffffa62bc0053dd0 EFLAGS: 00010206 [ 16.189709] RAX: 0000000008000000 RBX: 0000000000000000 RCX: 0000000000000002 [ 16.203899] RDX: 00000000fffba9c0 RSI: ffffa62bc0053dd4 RDI: ffff91f601610200 [ 16.211860] RBP: ffff91f601610200 R08: 0000000000000400 R09: 0000000000000001 [ 16.219831] R10: 0000000000000001 R11: 00000000000ee6b2 R12: 00000000fffba9be [ 16.227801] R13: ffff91f6016102c0 R14: ffff91f60006f605 R15: ffff91f6001b8000 [ 16.235770] FS: 0000000000000000(0000) GS:ffff91f764400000(0000) knlGS:0000000000000000 [ 16.244810] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 16.251226] CR2: 00007f5bcc93b030 CR3: 000000022542e001 CR4: 00000000003706f0 [ 16.259195] Call Trace: [ 16.261923] <TASK> [ 16.264261] ? __warn+0x84/0x130 [ 16.267862] ? ucsi_reset_ppm+0x1af/0x1c0 [typec_ucsi] [ 16.273600] ? report_bug+0x164/0x190 [ 16.277679] ? handle_bug+0x54/0x90 [ 16.281572] ? exc_invalid_op+0x17/0x70 [ 16.285853] ? asm_exc_invalid_op+0x1a/0x20 [ 16.290523] ? ucsi_reset_ppm+0x1af/0x1c0 [typec_ucsi] [ 16.296261] ucsi_init_work+0xaa/0xb20 [typec_ucsi] [ 16.301709] process_one_work+0x160/0x390 [ 16.306185] worker_thread+0x2a0/0x3b0 [ 16.310359] ? __pfx_worker_thread+0x10/0x10 [ 16.315126] kthread+0xc8/0xf0 [ 16.318533] ? __pfx_kthread+0x10/0x10 [ 16.322706] ret_from_fork+0x2c/0x50 [ 16.326687] ? __pfx_kthread+0x10/0x10 [ 16.330869] ret_from_fork_asm+0x1a/0x30 [ 16.335238] </TASK>
Hi, [ CC: Saranya Gopal <saranya.gopal@intel.com> ] On Sun, Dec 15, 2024 at 01:34:12PM -0500, Sasha Levin wrote: > On Wed, Mar 20, 2024 at 08:39:26AM +0100, Christian A. Ehrhardt wrote: > > Check the UCSI_CCI_RESET_COMPLETE complete flag before starting > > another reset. Use a UCSI_SET_NOTIFICATION_ENABLE command to clear > > the flag if it is set. > > Hi Christian, > > It looks like kernelci is able to trigger the warning added by this > commit: > > [ 15.988528] WARNING: CPU: 0 PID: 8 at drivers/usb/typec/ucsi/ucsi.c:1377 ucsi_reset_ppm+0x1af/0x1c0 [typec_ucsi] > [ ... ] I think I can see what's going on. First of all: The warning is harmless and UCSI will still work as expected (but there is an additional delay during init). The trigger for the warning is likely this commit (reviewed by me, so ...): | commit fa48d7e81624efdf398b990a9049e9cd75a5aead | Author: Saranya Gopal <saranya.gopal@intel.com> | Date: Fri Aug 30 14:13:42 2024 +0530 | | usb: typec: ucsi: Do not call ACPI _DSM method for UCSI read | operations The (compile tested) diff below should fix it and I can turn this into a proper patch but I lost access to test hardware with UCSI, thus this would need a "Tested-by:" from someone else before it can be included. Maybe Saranya can do this? Best regards Christian commit b44ba223cd840e6dbab6c7f69da6203c7a8ba570 Author: Christian A. Ehrhardt <lk@c--e.de> Date: Mon Dec 16 21:52:46 2024 +0100 acpi: typec: ucsi: Introduce a ->poll_cci method For the ACPI backend of UCSI the UCSI "registers" are just a memory copy of the register values in an opregion. The ACPI implementation in the BIOS ensures that the opregion contents are synced to the embedded controller and it ensures that the registers (in particular CCI) are synced back to the opregion on notifications. While there is an ACPI call that syncs the actual registers to the opregion there is rarely a need to do this and on some ACPI implementations it actually breaks in various interesting ways. The only reason to force a sync from the embedded controller is to poll CCI while notifications are disabled. Only the ucsi core knows if this is the case and guessing based on the current command is suboptimal. Thus introduce a ->poll_cci() method that works like ->read_cci() with an additional forced sync and document that this should be used when polling with notifications disabled. For all other backends that presumably don't have this issue use the same implementation for both methods. Fixes: fa48d7e81624 ("usb: typec: ucsi: Do not call ACPI _DSM method for UCSI read operations") Signed-off-by: Christian A. Ehrhardt <lk@c--e.de> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index fcf499cc9458..0fe1476f4c29 100644 --- a/drivers/usb/typec/ucsi/ucsi.c +++ b/drivers/usb/typec/ucsi/ucsi.c @@ -1346,7 +1346,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi) mutex_lock(&ucsi->ppm_lock); - ret = ucsi->ops->read_cci(ucsi, &cci); + ret = ucsi->ops->poll_cci(ucsi, &cci); if (ret < 0) goto out; @@ -1364,7 +1364,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi) tmo = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS); do { - ret = ucsi->ops->read_cci(ucsi, &cci); + ret = ucsi->ops->poll_cci(ucsi, &cci); if (ret < 0) goto out; if (cci & UCSI_CCI_COMMAND_COMPLETE) @@ -1393,7 +1393,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi) /* Give the PPM time to process a reset before reading CCI */ msleep(20); - ret = ucsi->ops->read_cci(ucsi, &cci); + ret = ucsi->ops->poll_cci(ucsi, &cci); if (ret) goto out; @@ -1929,8 +1929,8 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops) struct ucsi *ucsi; if (!ops || - !ops->read_version || !ops->read_cci || !ops->read_message_in || - !ops->sync_control || !ops->async_control) + !ops->read_version || !ops->read_cci || !ops->poll_cci || + !ops->read_message_in || !ops->sync_control || !ops->async_control) return ERR_PTR(-EINVAL); ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL); diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h index 5ff369c24a2f..e4c563da715f 100644 --- a/drivers/usb/typec/ucsi/ucsi.h +++ b/drivers/usb/typec/ucsi/ucsi.h @@ -61,6 +61,7 @@ struct dentry; * struct ucsi_operations - UCSI I/O operations * @read_version: Read implemented UCSI version * @read_cci: Read CCI register + * @poll_cci: Read CCI register while polling with notifications disabled * @read_message_in: Read message data from UCSI * @sync_control: Blocking control operation * @async_control: Non-blocking control operation @@ -75,6 +76,7 @@ struct dentry; struct ucsi_operations { int (*read_version)(struct ucsi *ucsi, u16 *version); int (*read_cci)(struct ucsi *ucsi, u32 *cci); + int (*poll_cci)(struct ucsi *ucsi, u32 *cci); int (*read_message_in)(struct ucsi *ucsi, void *val, size_t val_len); int (*sync_control)(struct ucsi *ucsi, u64 command); int (*async_control)(struct ucsi *ucsi, u64 command); diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c index 5c5515551963..ac1ebb5d9527 100644 --- a/drivers/usb/typec/ucsi/ucsi_acpi.c +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c @@ -59,19 +59,24 @@ static int ucsi_acpi_read_version(struct ucsi *ucsi, u16 *version) static int ucsi_acpi_read_cci(struct ucsi *ucsi, u32 *cci) { struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi); - int ret; - - if (UCSI_COMMAND(ua->cmd) == UCSI_PPM_RESET) { - ret = ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ); - if (ret) - return ret; - } memcpy(cci, ua->base + UCSI_CCI, sizeof(*cci)); return 0; } +static int ucsi_acpi_poll_cci(struct ucsi *ucsi, u32 *cci) +{ + struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi); + int ret; + + ret = ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ); + if (ret) + return ret; + + return ucsi_acpi_read_cci(ucsi, cci); +} + static int ucsi_acpi_read_message_in(struct ucsi *ucsi, void *val, size_t val_len) { struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi); @@ -94,6 +99,7 @@ static int ucsi_acpi_async_control(struct ucsi *ucsi, u64 command) static const struct ucsi_operations ucsi_acpi_ops = { .read_version = ucsi_acpi_read_version, .read_cci = ucsi_acpi_read_cci, + .poll_cci = ucsi_acpi_poll_cci, .read_message_in = ucsi_acpi_read_message_in, .sync_control = ucsi_sync_control_common, .async_control = ucsi_acpi_async_control @@ -142,6 +148,7 @@ static int ucsi_gram_sync_control(struct ucsi *ucsi, u64 command) static const struct ucsi_operations ucsi_gram_ops = { .read_version = ucsi_acpi_read_version, .read_cci = ucsi_acpi_read_cci, + .poll_cci = ucsi_acpi_poll_cci, .read_message_in = ucsi_gram_read_message_in, .sync_control = ucsi_gram_sync_control, .async_control = ucsi_acpi_async_control diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c index fcb8e61136cf..bb0dc2005c05 100644 --- a/drivers/usb/typec/ucsi/ucsi_ccg.c +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c @@ -664,6 +664,7 @@ static int ucsi_ccg_sync_control(struct ucsi *ucsi, u64 command) static const struct ucsi_operations ucsi_ccg_ops = { .read_version = ucsi_ccg_read_version, .read_cci = ucsi_ccg_read_cci, + .poll_cci = ucsi_ccg_read_cci, .read_message_in = ucsi_ccg_read_message_in, .sync_control = ucsi_ccg_sync_control, .async_control = ucsi_ccg_async_control, diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c index 90948cd6d297..a78e53480875 100644 --- a/drivers/usb/typec/ucsi/ucsi_glink.c +++ b/drivers/usb/typec/ucsi/ucsi_glink.c @@ -201,6 +201,7 @@ static void pmic_glink_ucsi_connector_status(struct ucsi_connector *con) static const struct ucsi_operations pmic_glink_ucsi_ops = { .read_version = pmic_glink_ucsi_read_version, .read_cci = pmic_glink_ucsi_read_cci, + .poll_cci = pmic_glink_ucsi_read_cci, .read_message_in = pmic_glink_ucsi_read_message_in, .sync_control = ucsi_sync_control_common, .async_control = pmic_glink_ucsi_async_control, diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c index 6923fad31d79..57ef7d83a412 100644 --- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c +++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c @@ -424,6 +424,7 @@ static irqreturn_t ucsi_stm32g0_irq_handler(int irq, void *data) static const struct ucsi_operations ucsi_stm32g0_ops = { .read_version = ucsi_stm32g0_read_version, .read_cci = ucsi_stm32g0_read_cci, + .poll_cci = ucsi_stm32g0_read_cci, .read_message_in = ucsi_stm32g0_read_message_in, .sync_control = ucsi_sync_control_common, .async_control = ucsi_stm32g0_async_control, diff --git a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c index f3a5e24ea84d..40e5da4fd2a4 100644 --- a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c +++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c @@ -74,6 +74,7 @@ static int yoga_c630_ucsi_async_control(struct ucsi *ucsi, u64 command) const struct ucsi_operations yoga_c630_ucsi_ops = { .read_version = yoga_c630_ucsi_read_version, .read_cci = yoga_c630_ucsi_read_cci, + .poll_cci = yoga_c630_ucsi_read_cci, .read_message_in = yoga_c630_ucsi_read_message_in, .sync_control = ucsi_sync_control_common, .async_control = yoga_c630_ucsi_async_control,
Hi Christian, > -----Original Message----- > From: Gopal, Saranya > Sent: Tuesday, December 17, 2024 9:55 AM > To: Christian A. Ehrhardt <lk@c--e.de>; Sasha Levin > <sashal@kernel.org> > Cc: linux-kernel@vger.kernel.org; Heikki Krogerus > <heikki.krogerus@linux.intel.com>; Greg Kroah-Hartman > <gregkh@linuxfoundation.org>; Prashant Malani > <pmalani@chromium.org>; Jameson Thies <jthies@google.com>; > Abhishek Pandit-Subedi <abhishekpandit@chromium.org>; Neil > Armstrong <neil.armstrong@linaro.org>; Uwe Kleine-König <u.kleine- > koenig@pengutronix.de>; Samuel Čavoj <samuel@cavoj.net>; linux- > usb@vger.kernel.org; Kenneth Crudup <kenny@panix.com> > Subject: RE: [PATCH 5/5] usb: typec: ucsi: Clear > UCSI_CCI_RESET_COMPLETE before reset > > Hi Christian, > > > -----Original Message----- > > From: Christian A. Ehrhardt <lk@c--e.de> > > Sent: Tuesday, December 17, 2024 3:17 AM > > To: Sasha Levin <sashal@kernel.org> > > Cc: linux-kernel@vger.kernel.org; Heikki Krogerus > > <heikki.krogerus@linux.intel.com>; Greg Kroah-Hartman > > <gregkh@linuxfoundation.org>; Prashant Malani > > <pmalani@chromium.org>; Jameson Thies <jthies@google.com>; > > Abhishek Pandit-Subedi <abhishekpandit@chromium.org>; Neil > > Armstrong <neil.armstrong@linaro.org>; Uwe Kleine-König > <u.kleine- > > koenig@pengutronix.de>; Samuel Čavoj <samuel@cavoj.net>; > linux- > > usb@vger.kernel.org; Kenneth Crudup <kenny@panix.com>; Gopal, > > Saranya <saranya.gopal@intel.com> > > Subject: Re: [PATCH 5/5] usb: typec: ucsi: Clear > > UCSI_CCI_RESET_COMPLETE before reset > > > > > > Hi, > > > > [ CC: Saranya Gopal <saranya.gopal@intel.com> ] > > > > On Sun, Dec 15, 2024 at 01:34:12PM -0500, Sasha Levin wrote: > > > On Wed, Mar 20, 2024 at 08:39:26AM +0100, Christian A. > Ehrhardt > > wrote: > > > > Check the UCSI_CCI_RESET_COMPLETE complete flag before > > starting > > > > another reset. Use a UCSI_SET_NOTIFICATION_ENABLE > > command to clear > > > > the flag if it is set. > > > > > > Hi Christian, > > > > > > It looks like kernelci is able to trigger the warning added by this > > > commit: > > > > > > [ 15.988528] WARNING: CPU: 0 PID: 8 at > > drivers/usb/typec/ucsi/ucsi.c:1377 ucsi_reset_ppm+0x1af/0x1c0 > > [typec_ucsi] > > > [ ... ] > > > > I think I can see what's going on. > > > > First of all: The warning is harmless and UCSI will still work as > > expected (but there is an additional delay during init). > > > > The trigger for the warning is likely this commit (reviewed by me, so > > ...): > > > > | commit fa48d7e81624efdf398b990a9049e9cd75a5aead > > | Author: Saranya Gopal <saranya.gopal@intel.com> > > | Date: Fri Aug 30 14:13:42 2024 +0530 > > | > > | usb: typec: ucsi: Do not call ACPI _DSM method for UCSI read > > | operations > > > > The (compile tested) diff below should fix it and I can turn this > > into a proper patch but I lost access to test hardware with UCSI, > > thus this would need a "Tested-by:" from someone else before it > can > > be included. Maybe Saranya can do this? > > I can get access to a couple of systems using UCSI and get this tested > tomorrow. I realized that all the systems I currently have need the forced sync and hence do not reproduce the reported issue. I believe this patch needs to be tested on Chromebooks that support UCSI. If so, it might take longer for me to get access to those systems considering the holiday season. Thanks, Saranya > > Thanks, > Saranya > > > > Best regards Christian > > > > > > commit b44ba223cd840e6dbab6c7f69da6203c7a8ba570 > > Author: Christian A. Ehrhardt <lk@c--e.de> > > Date: Mon Dec 16 21:52:46 2024 +0100 > > > > acpi: typec: ucsi: Introduce a ->poll_cci method > > > > For the ACPI backend of UCSI the UCSI "registers" are just > > a memory copy of the register values in an opregion. The ACPI > > implementation in the BIOS ensures that the opregion contents > > are synced to the embedded controller and it ensures that the > > registers (in particular CCI) are synced back to the opregion > > on notifications. While there is an ACPI call that syncs the > > actual registers to the opregion there is rarely a need to do > > this and on some ACPI implementations it actually breaks in > > various interesting ways. > > > > The only reason to force a sync from the embedded controller > > is to poll CCI while notifications are disabled. Only the > > ucsi core knows if this is the case and guessing based on > > the current command is suboptimal. > > > > Thus introduce a ->poll_cci() method that works like > > ->read_cci() with an additional forced sync and document that > > this should be used when polling with notifications disabled. > > For all other backends that presumably don't have this issue > > use the same implementation for both methods. > > > > Fixes: fa48d7e81624 ("usb: typec: ucsi: Do not call ACPI _DSM > > method for UCSI read operations") > > Signed-off-by: Christian A. Ehrhardt <lk@c--e.de> > > > > diff --git a/drivers/usb/typec/ucsi/ucsi.c > > b/drivers/usb/typec/ucsi/ucsi.c > > index fcf499cc9458..0fe1476f4c29 100644 > > --- a/drivers/usb/typec/ucsi/ucsi.c > > +++ b/drivers/usb/typec/ucsi/ucsi.c > > @@ -1346,7 +1346,7 @@ static int ucsi_reset_ppm(struct ucsi > *ucsi) > > > > mutex_lock(&ucsi->ppm_lock); > > > > - ret = ucsi->ops->read_cci(ucsi, &cci); > > + ret = ucsi->ops->poll_cci(ucsi, &cci); > > if (ret < 0) > > goto out; > > > > @@ -1364,7 +1364,7 @@ static int ucsi_reset_ppm(struct ucsi > *ucsi) > > > > tmo = jiffies + > > msecs_to_jiffies(UCSI_TIMEOUT_MS); > > do { > > - ret = ucsi->ops->read_cci(ucsi, &cci); > > + ret = ucsi->ops->poll_cci(ucsi, &cci); > > if (ret < 0) > > goto out; > > if (cci & UCSI_CCI_COMMAND_COMPLETE) > > @@ -1393,7 +1393,7 @@ static int ucsi_reset_ppm(struct ucsi > *ucsi) > > /* Give the PPM time to process a reset before > > reading CCI */ > > msleep(20); > > > > - ret = ucsi->ops->read_cci(ucsi, &cci); > > + ret = ucsi->ops->poll_cci(ucsi, &cci); > > if (ret) > > goto out; > > > > @@ -1929,8 +1929,8 @@ struct ucsi *ucsi_create(struct device > > *dev, const struct ucsi_operations *ops) > > struct ucsi *ucsi; > > > > if (!ops || > > - !ops->read_version || !ops->read_cci || !ops- > > >read_message_in || > > - !ops->sync_control || !ops->async_control) > > + !ops->read_version || !ops->read_cci || !ops->poll_cci || > > + !ops->read_message_in || !ops->sync_control || !ops- > > >async_control) > > return ERR_PTR(-EINVAL); > > > > ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL); > > diff --git a/drivers/usb/typec/ucsi/ucsi.h > > b/drivers/usb/typec/ucsi/ucsi.h > > index 5ff369c24a2f..e4c563da715f 100644 > > --- a/drivers/usb/typec/ucsi/ucsi.h > > +++ b/drivers/usb/typec/ucsi/ucsi.h > > @@ -61,6 +61,7 @@ struct dentry; > > * struct ucsi_operations - UCSI I/O operations > > * @read_version: Read implemented UCSI version > > * @read_cci: Read CCI register > > + * @poll_cci: Read CCI register while polling with notifications > > disabled > > * @read_message_in: Read message data from UCSI > > * @sync_control: Blocking control operation > > * @async_control: Non-blocking control operation > > @@ -75,6 +76,7 @@ struct dentry; > > struct ucsi_operations { > > int (*read_version)(struct ucsi *ucsi, u16 *version); > > int (*read_cci)(struct ucsi *ucsi, u32 *cci); > > + int (*poll_cci)(struct ucsi *ucsi, u32 *cci); > > int (*read_message_in)(struct ucsi *ucsi, void *val, size_t > > val_len); > > int (*sync_control)(struct ucsi *ucsi, u64 command); > > int (*async_control)(struct ucsi *ucsi, u64 command); > > diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c > > b/drivers/usb/typec/ucsi/ucsi_acpi.c > > index 5c5515551963..ac1ebb5d9527 100644 > > --- a/drivers/usb/typec/ucsi/ucsi_acpi.c > > +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c > > @@ -59,19 +59,24 @@ static int ucsi_acpi_read_version(struct > ucsi > > *ucsi, u16 *version) > > static int ucsi_acpi_read_cci(struct ucsi *ucsi, u32 *cci) > > { > > struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi); > > - int ret; > > - > > - if (UCSI_COMMAND(ua->cmd) == UCSI_PPM_RESET) { > > - ret = ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ); > > - if (ret) > > - return ret; > > - } > > > > memcpy(cci, ua->base + UCSI_CCI, sizeof(*cci)); > > > > return 0; > > } > > > > +static int ucsi_acpi_poll_cci(struct ucsi *ucsi, u32 *cci) > > +{ > > + struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi); > > + int ret; > > + > > + ret = ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ); > > + if (ret) > > + return ret; > > + > > + return ucsi_acpi_read_cci(ucsi, cci); > > +} > > + > > static int ucsi_acpi_read_message_in(struct ucsi *ucsi, void *val, > > size_t val_len) > > { > > struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi); > > @@ -94,6 +99,7 @@ static int ucsi_acpi_async_control(struct ucsi > > *ucsi, u64 command) > > static const struct ucsi_operations ucsi_acpi_ops = { > > .read_version = ucsi_acpi_read_version, > > .read_cci = ucsi_acpi_read_cci, > > + .poll_cci = ucsi_acpi_poll_cci, > > .read_message_in = ucsi_acpi_read_message_in, > > .sync_control = ucsi_sync_control_common, > > .async_control = ucsi_acpi_async_control > > @@ -142,6 +148,7 @@ static int ucsi_gram_sync_control(struct > ucsi > > *ucsi, u64 command) > > static const struct ucsi_operations ucsi_gram_ops = { > > .read_version = ucsi_acpi_read_version, > > .read_cci = ucsi_acpi_read_cci, > > + .poll_cci = ucsi_acpi_poll_cci, > > .read_message_in = ucsi_gram_read_message_in, > > .sync_control = ucsi_gram_sync_control, > > .async_control = ucsi_acpi_async_control > > diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c > > b/drivers/usb/typec/ucsi/ucsi_ccg.c > > index fcb8e61136cf..bb0dc2005c05 100644 > > --- a/drivers/usb/typec/ucsi/ucsi_ccg.c > > +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c > > @@ -664,6 +664,7 @@ static int ucsi_ccg_sync_control(struct ucsi > > *ucsi, u64 command) > > static const struct ucsi_operations ucsi_ccg_ops = { > > .read_version = ucsi_ccg_read_version, > > .read_cci = ucsi_ccg_read_cci, > > + .poll_cci = ucsi_ccg_read_cci, > > .read_message_in = ucsi_ccg_read_message_in, > > .sync_control = ucsi_ccg_sync_control, > > .async_control = ucsi_ccg_async_control, > > diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c > > b/drivers/usb/typec/ucsi/ucsi_glink.c > > index 90948cd6d297..a78e53480875 100644 > > --- a/drivers/usb/typec/ucsi/ucsi_glink.c > > +++ b/drivers/usb/typec/ucsi/ucsi_glink.c > > @@ -201,6 +201,7 @@ static void > > pmic_glink_ucsi_connector_status(struct ucsi_connector *con) > > static const struct ucsi_operations pmic_glink_ucsi_ops = { > > .read_version = pmic_glink_ucsi_read_version, > > .read_cci = pmic_glink_ucsi_read_cci, > > + .poll_cci = pmic_glink_ucsi_read_cci, > > .read_message_in = pmic_glink_ucsi_read_message_in, > > .sync_control = ucsi_sync_control_common, > > .async_control = pmic_glink_ucsi_async_control, > > diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c > > b/drivers/usb/typec/ucsi/ucsi_stm32g0.c > > index 6923fad31d79..57ef7d83a412 100644 > > --- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c > > +++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c > > @@ -424,6 +424,7 @@ static irqreturn_t > > ucsi_stm32g0_irq_handler(int irq, void *data) > > static const struct ucsi_operations ucsi_stm32g0_ops = { > > .read_version = ucsi_stm32g0_read_version, > > .read_cci = ucsi_stm32g0_read_cci, > > + .poll_cci = ucsi_stm32g0_read_cci, > > .read_message_in = ucsi_stm32g0_read_message_in, > > .sync_control = ucsi_sync_control_common, > > .async_control = ucsi_stm32g0_async_control, > > diff --git a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c > > b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c > > index f3a5e24ea84d..40e5da4fd2a4 100644 > > --- a/drivers/usb/typec/ucsi/ucsi_yoga_c630.c > > +++ b/drivers/usb/typec/ucsi/ucsi_yoga_c630.c > > @@ -74,6 +74,7 @@ static int > yoga_c630_ucsi_async_control(struct > > ucsi *ucsi, u64 command) > > const struct ucsi_operations yoga_c630_ucsi_ops = { > > .read_version = yoga_c630_ucsi_read_version, > > .read_cci = yoga_c630_ucsi_read_cci, > > + .poll_cci = yoga_c630_ucsi_read_cci, > > .read_message_in = yoga_c630_ucsi_read_message_in, > > .sync_control = ucsi_sync_control_common, > > .async_control = yoga_c630_ucsi_async_control,
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index 63f340dbd867..85e507df7fa8 100644 --- a/drivers/usb/typec/ucsi/ucsi.c +++ b/drivers/usb/typec/ucsi/ucsi.c @@ -1264,13 +1264,47 @@ static int ucsi_reset_connector(struct ucsi_connector *con, bool hard) static int ucsi_reset_ppm(struct ucsi *ucsi) { - u64 command = UCSI_PPM_RESET; + u64 command; unsigned long tmo; u32 cci; int ret; mutex_lock(&ucsi->ppm_lock); + ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci)); + if (ret < 0) + goto out; + + /* + * If UCSI_CCI_RESET_COMPLETE is already set we must clear + * the flag before we start another reset. Send a + * UCSI_SET_NOTIFICATION_ENABLE command to achieve this. + * Ignore a timeout and try the reset anyway if this fails. + */ + if (cci & UCSI_CCI_RESET_COMPLETE) { + command = UCSI_SET_NOTIFICATION_ENABLE; + ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL, &command, + sizeof(command)); + if (ret < 0) + goto out; + + tmo = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS); + do { + ret = ucsi->ops->read(ucsi, UCSI_CCI, + &cci, sizeof(cci)); + if (ret < 0) + goto out; + if (cci & UCSI_CCI_COMMAND_COMPLETE) + break; + if (time_is_before_jiffies(tmo)) + break; + msleep(20); + } while (1); + + WARN_ON(cci & UCSI_CCI_RESET_COMPLETE); + } + + command = UCSI_PPM_RESET; ret = ucsi->ops->async_write(ucsi, UCSI_CONTROL, &command, sizeof(command)); if (ret < 0)
Check the UCSI_CCI_RESET_COMPLETE complete flag before starting another reset. Use a UCSI_SET_NOTIFICATION_ENABLE command to clear the flag if it is set. Signed-off-by: Christian A. Ehrhardt <lk@c--e.de> --- drivers/usb/typec/ucsi/ucsi.c | 36 ++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-)