stable.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RFC 1/2] acpi: typec: ucsi: Introduce a ->poll_cci method
       [not found] <20250206184327.16308-1-boddah8794@gmail.com>
@ 2025-02-06 18:43 ` Fedor Pchelkin
  2025-02-07  0:38   ` Dmitry Baryshkov
  2025-02-13 13:56   ` Heikki Krogerus
  2025-02-06 18:43 ` [PATCH RFC 2/2] usb: typec: ucsi: increase timeout for PPM reset operations Fedor Pchelkin
  1 sibling, 2 replies; 10+ messages in thread
From: Fedor Pchelkin @ 2025-02-06 18:43 UTC (permalink / raw)
  To: Heikki Krogerus, Christian A. Ehrhardt
  Cc: Fedor Pchelkin, Greg Kroah-Hartman, Dmitry Baryshkov,
	Benson Leung, Jameson Thies, Saranya Gopal, linux-usb,
	linux-kernel, Mark Pearson, stable

From: "Christian A. Ehrhardt" <lk@c--e.de>

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, i.e.
leading to the following spurious assertion splat:

WARNING: CPU: 3 PID: 76 at drivers/usb/typec/ucsi/ucsi.c:1388 ucsi_reset_ppm+0x1b4/0x1c0 [typec_ucsi]
CPU: 3 UID: 0 PID: 76 Comm: kworker/3:0 Not tainted 6.12.11-200.fc41.x86_64 #1
Hardware name: LENOVO 21D0/LNVNB161216, BIOS J6CN45WW 03/17/2023
Workqueue: events_long ucsi_init_work [typec_ucsi]
RIP: 0010:ucsi_reset_ppm+0x1b4/0x1c0 [typec_ucsi]
Call Trace:
 <TASK>
 ucsi_init_work+0x3c/0xac0 [typec_ucsi]
 process_one_work+0x179/0x330
 worker_thread+0x252/0x390
 kthread+0xd2/0x100
 ret_from_fork+0x34/0x50
 ret_from_fork_asm+0x1a/0x30
 </TASK>

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")
Cc: stable@vger.kernel.org
Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
Tested-by: Fedor Pchelkin <boddah8794@gmail.com>
Signed-off-by: Fedor Pchelkin <boddah8794@gmail.com>
---
Add the explicit WARNING splat and slightly increase the length of text
lines in the changelog.
Original patch: https://lore.kernel.org/linux-usb/Z2Cf1AI8CXao5ZAn@cae.in-ulm.de/

 drivers/usb/typec/ucsi/ucsi.c           | 10 +++++-----
 drivers/usb/typec/ucsi/ucsi.h           |  2 ++
 drivers/usb/typec/ucsi/ucsi_acpi.c      | 21 ++++++++++++++-------
 drivers/usb/typec/ucsi/ucsi_ccg.c       |  1 +
 drivers/usb/typec/ucsi/ucsi_glink.c     |  1 +
 drivers/usb/typec/ucsi/ucsi_stm32g0.c   |  1 +
 drivers/usb/typec/ucsi/ucsi_yoga_c630.c |  1 +
 7 files changed, 25 insertions(+), 12 deletions(-)

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 82735eb34f0e..28780acc4af2 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -62,6 +62,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
@@ -76,6 +77,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 740171f24ef9..4b1668733a4b 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 fed39d458090..8af79101a2fc 100644
--- a/drivers/usb/typec/ucsi/ucsi_glink.c
+++ b/drivers/usb/typec/ucsi/ucsi_glink.c
@@ -206,6 +206,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 4cae85c0dc12..d33e3f2dd1d8 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)
 static 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,
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH RFC 2/2] usb: typec: ucsi: increase timeout for PPM reset operations
       [not found] <20250206184327.16308-1-boddah8794@gmail.com>
  2025-02-06 18:43 ` [PATCH RFC 1/2] acpi: typec: ucsi: Introduce a ->poll_cci method Fedor Pchelkin
@ 2025-02-06 18:43 ` Fedor Pchelkin
  2025-02-13 13:58   ` Heikki Krogerus
  2025-02-18 16:57   ` Mark Pearson
  1 sibling, 2 replies; 10+ messages in thread
From: Fedor Pchelkin @ 2025-02-06 18:43 UTC (permalink / raw)
  To: Heikki Krogerus, Christian A. Ehrhardt
  Cc: Fedor Pchelkin, Greg Kroah-Hartman, Dmitry Baryshkov,
	Benson Leung, Jameson Thies, Saranya Gopal, linux-usb,
	linux-kernel, Mark Pearson, stable

It is observed that on some systems an initial PPM reset during the boot
phase can trigger a timeout:

[    6.482546] ucsi_acpi USBC000:00: failed to reset PPM!
[    6.482551] ucsi_acpi USBC000:00: error -ETIMEDOUT: PPM init failed

Still, increasing the timeout value, albeit being the most straightforward
solution, eliminates the problem: the initial PPM reset may take up to
~8000-10000ms on some Lenovo laptops. When it is reset after the above
period of time (or even if ucsi_reset_ppm() is not called overall), UCSI
works as expected.

Moreover, if the ucsi_acpi module is loaded/unloaded manually after the
system has booted, reading the CCI values and resetting the PPM works
perfectly, without any timeout. Thus it's only a boot-time issue.

The reason for this behavior is not clear but it may be the consequence
of some tricks that the firmware performs or be an actual firmware bug.
As a workaround, increase the timeout to avoid failing the UCSI
initialization prematurely.

Fixes: b1b59e16075f ("usb: typec: ucsi: Increase command completion timeout value")
Cc: stable@vger.kernel.org
Signed-off-by: Fedor Pchelkin <boddah8794@gmail.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 0fe1476f4c29..7a56d3f840d7 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -25,7 +25,7 @@
  * difficult to estimate the time it takes for the system to process the command
  * before it is actually passed to the PPM.
  */
-#define UCSI_TIMEOUT_MS		5000
+#define UCSI_TIMEOUT_MS		10000
 
 /*
  * UCSI_SWAP_TIMEOUT_MS - Timeout for role swap requests
-- 
2.48.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH RFC 1/2] acpi: typec: ucsi: Introduce a ->poll_cci method
  2025-02-06 18:43 ` [PATCH RFC 1/2] acpi: typec: ucsi: Introduce a ->poll_cci method Fedor Pchelkin
@ 2025-02-07  0:38   ` Dmitry Baryshkov
  2025-02-07  6:51     ` Christian A. Ehrhardt
  2025-02-13 13:56   ` Heikki Krogerus
  1 sibling, 1 reply; 10+ messages in thread
From: Dmitry Baryshkov @ 2025-02-07  0:38 UTC (permalink / raw)
  To: Fedor Pchelkin
  Cc: Heikki Krogerus, Christian A. Ehrhardt, Greg Kroah-Hartman,
	Benson Leung, Jameson Thies, Saranya Gopal, linux-usb,
	linux-kernel, Mark Pearson, stable

On Thu, 6 Feb 2025 at 20:43, Fedor Pchelkin <boddah8794@gmail.com> wrote:
>
> From: "Christian A. Ehrhardt" <lk@c--e.de>
>
> 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, i.e.
> leading to the following spurious assertion splat:
>
> WARNING: CPU: 3 PID: 76 at drivers/usb/typec/ucsi/ucsi.c:1388 ucsi_reset_ppm+0x1b4/0x1c0 [typec_ucsi]
> CPU: 3 UID: 0 PID: 76 Comm: kworker/3:0 Not tainted 6.12.11-200.fc41.x86_64 #1
> Hardware name: LENOVO 21D0/LNVNB161216, BIOS J6CN45WW 03/17/2023
> Workqueue: events_long ucsi_init_work [typec_ucsi]
> RIP: 0010:ucsi_reset_ppm+0x1b4/0x1c0 [typec_ucsi]
> Call Trace:
>  <TASK>
>  ucsi_init_work+0x3c/0xac0 [typec_ucsi]
>  process_one_work+0x179/0x330
>  worker_thread+0x252/0x390
>  kthread+0xd2/0x100
>  ret_from_fork+0x34/0x50
>  ret_from_fork_asm+0x1a/0x30
>  </TASK>
>
> 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.

Should the ucsi_init() also use ->poll_cci instead of ->read_cci?
Although it's executed with notifications enabled, it looks as if it
might need the additional resync.

>
> Fixes: fa48d7e81624 ("usb: typec: ucsi: Do not call ACPI _DSM method for UCSI read operations")
> Cc: stable@vger.kernel.org
> Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
> Tested-by: Fedor Pchelkin <boddah8794@gmail.com>
> Signed-off-by: Fedor Pchelkin <boddah8794@gmail.com>
> ---



-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH RFC 1/2] acpi: typec: ucsi: Introduce a ->poll_cci method
  2025-02-07  0:38   ` Dmitry Baryshkov
@ 2025-02-07  6:51     ` Christian A. Ehrhardt
  0 siblings, 0 replies; 10+ messages in thread
From: Christian A. Ehrhardt @ 2025-02-07  6:51 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Fedor Pchelkin, Heikki Krogerus, Greg Kroah-Hartman, Benson Leung,
	Jameson Thies, Saranya Gopal, linux-usb, linux-kernel,
	Mark Pearson, stable


Hi,

On Fri, Feb 07, 2025 at 02:38:03AM +0200, Dmitry Baryshkov wrote:
> On Thu, 6 Feb 2025 at 20:43, Fedor Pchelkin <boddah8794@gmail.com> wrote:
> >
> > From: "Christian A. Ehrhardt" <lk@c--e.de>
> >
> > 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, i.e.
> > leading to the following spurious assertion splat:
> >
> > WARNING: CPU: 3 PID: 76 at drivers/usb/typec/ucsi/ucsi.c:1388 ucsi_reset_ppm+0x1b4/0x1c0 [typec_ucsi]
> > CPU: 3 UID: 0 PID: 76 Comm: kworker/3:0 Not tainted 6.12.11-200.fc41.x86_64 #1
> > Hardware name: LENOVO 21D0/LNVNB161216, BIOS J6CN45WW 03/17/2023
> > Workqueue: events_long ucsi_init_work [typec_ucsi]
> > RIP: 0010:ucsi_reset_ppm+0x1b4/0x1c0 [typec_ucsi]
> > Call Trace:
> >  <TASK>
> >  ucsi_init_work+0x3c/0xac0 [typec_ucsi]
> >  process_one_work+0x179/0x330
> >  worker_thread+0x252/0x390
> >  kthread+0xd2/0x100
> >  ret_from_fork+0x34/0x50
> >  ret_from_fork_asm+0x1a/0x30
> >  </TASK>
> >
> > 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.
> 
> Should the ucsi_init() also use ->poll_cci instead of ->read_cci?
> Although it's executed with notifications enabled, it looks as if it
> might need the additional resync.

I don't think it should be neccessary. The command completion event
for the ucsi_send_command just above should have synced already and
anything that happens after that ought to generate an event.

Best regards,
Christian


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH RFC 1/2] acpi: typec: ucsi: Introduce a ->poll_cci method
  2025-02-06 18:43 ` [PATCH RFC 1/2] acpi: typec: ucsi: Introduce a ->poll_cci method Fedor Pchelkin
  2025-02-07  0:38   ` Dmitry Baryshkov
@ 2025-02-13 13:56   ` Heikki Krogerus
  1 sibling, 0 replies; 10+ messages in thread
From: Heikki Krogerus @ 2025-02-13 13:56 UTC (permalink / raw)
  To: Fedor Pchelkin
  Cc: Christian A. Ehrhardt, Greg Kroah-Hartman, Dmitry Baryshkov,
	Benson Leung, Jameson Thies, Saranya Gopal, linux-usb,
	linux-kernel, Mark Pearson, stable

On Thu, Feb 06, 2025 at 09:43:14PM +0300, Fedor Pchelkin wrote:
> From: "Christian A. Ehrhardt" <lk@c--e.de>
> 
> 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, i.e.
> leading to the following spurious assertion splat:
> 
> WARNING: CPU: 3 PID: 76 at drivers/usb/typec/ucsi/ucsi.c:1388 ucsi_reset_ppm+0x1b4/0x1c0 [typec_ucsi]
> CPU: 3 UID: 0 PID: 76 Comm: kworker/3:0 Not tainted 6.12.11-200.fc41.x86_64 #1
> Hardware name: LENOVO 21D0/LNVNB161216, BIOS J6CN45WW 03/17/2023
> Workqueue: events_long ucsi_init_work [typec_ucsi]
> RIP: 0010:ucsi_reset_ppm+0x1b4/0x1c0 [typec_ucsi]
> Call Trace:
>  <TASK>
>  ucsi_init_work+0x3c/0xac0 [typec_ucsi]
>  process_one_work+0x179/0x330
>  worker_thread+0x252/0x390
>  kthread+0xd2/0x100
>  ret_from_fork+0x34/0x50
>  ret_from_fork_asm+0x1a/0x30
>  </TASK>
> 
> 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")
> Cc: stable@vger.kernel.org
> Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
> Tested-by: Fedor Pchelkin <boddah8794@gmail.com>
> Signed-off-by: Fedor Pchelkin <boddah8794@gmail.com>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> Add the explicit WARNING splat and slightly increase the length of text
> lines in the changelog.
> Original patch: https://lore.kernel.org/linux-usb/Z2Cf1AI8CXao5ZAn@cae.in-ulm.de/
> 
>  drivers/usb/typec/ucsi/ucsi.c           | 10 +++++-----
>  drivers/usb/typec/ucsi/ucsi.h           |  2 ++
>  drivers/usb/typec/ucsi/ucsi_acpi.c      | 21 ++++++++++++++-------
>  drivers/usb/typec/ucsi/ucsi_ccg.c       |  1 +
>  drivers/usb/typec/ucsi/ucsi_glink.c     |  1 +
>  drivers/usb/typec/ucsi/ucsi_stm32g0.c   |  1 +
>  drivers/usb/typec/ucsi/ucsi_yoga_c630.c |  1 +
>  7 files changed, 25 insertions(+), 12 deletions(-)
> 
> 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 82735eb34f0e..28780acc4af2 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -62,6 +62,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
> @@ -76,6 +77,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 740171f24ef9..4b1668733a4b 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 fed39d458090..8af79101a2fc 100644
> --- a/drivers/usb/typec/ucsi/ucsi_glink.c
> +++ b/drivers/usb/typec/ucsi/ucsi_glink.c
> @@ -206,6 +206,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 4cae85c0dc12..d33e3f2dd1d8 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)
>  static 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,
> -- 
> 2.48.1

-- 
heikki

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH RFC 2/2] usb: typec: ucsi: increase timeout for PPM reset operations
  2025-02-06 18:43 ` [PATCH RFC 2/2] usb: typec: ucsi: increase timeout for PPM reset operations Fedor Pchelkin
@ 2025-02-13 13:58   ` Heikki Krogerus
  2025-02-17 10:18     ` Fedor Pchelkin
  2025-02-18 16:57   ` Mark Pearson
  1 sibling, 1 reply; 10+ messages in thread
From: Heikki Krogerus @ 2025-02-13 13:58 UTC (permalink / raw)
  To: Fedor Pchelkin
  Cc: Christian A. Ehrhardt, Greg Kroah-Hartman, Dmitry Baryshkov,
	Benson Leung, Jameson Thies, Saranya Gopal, linux-usb,
	linux-kernel, Mark Pearson, stable

On Thu, Feb 06, 2025 at 09:43:15PM +0300, Fedor Pchelkin wrote:
> It is observed that on some systems an initial PPM reset during the boot
> phase can trigger a timeout:
> 
> [    6.482546] ucsi_acpi USBC000:00: failed to reset PPM!
> [    6.482551] ucsi_acpi USBC000:00: error -ETIMEDOUT: PPM init failed
> 
> Still, increasing the timeout value, albeit being the most straightforward
> solution, eliminates the problem: the initial PPM reset may take up to
> ~8000-10000ms on some Lenovo laptops. When it is reset after the above
> period of time (or even if ucsi_reset_ppm() is not called overall), UCSI
> works as expected.
> 
> Moreover, if the ucsi_acpi module is loaded/unloaded manually after the
> system has booted, reading the CCI values and resetting the PPM works
> perfectly, without any timeout. Thus it's only a boot-time issue.
> 
> The reason for this behavior is not clear but it may be the consequence
> of some tricks that the firmware performs or be an actual firmware bug.
> As a workaround, increase the timeout to avoid failing the UCSI
> initialization prematurely.
> 
> Fixes: b1b59e16075f ("usb: typec: ucsi: Increase command completion timeout value")
> Cc: stable@vger.kernel.org
> Signed-off-by: Fedor Pchelkin <boddah8794@gmail.com>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/ucsi/ucsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 0fe1476f4c29..7a56d3f840d7 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -25,7 +25,7 @@
>   * difficult to estimate the time it takes for the system to process the command
>   * before it is actually passed to the PPM.
>   */
> -#define UCSI_TIMEOUT_MS		5000
> +#define UCSI_TIMEOUT_MS		10000
>  
>  /*
>   * UCSI_SWAP_TIMEOUT_MS - Timeout for role swap requests
> -- 
> 2.48.1

-- 
heikki

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH RFC 2/2] usb: typec: ucsi: increase timeout for PPM reset operations
  2025-02-13 13:58   ` Heikki Krogerus
@ 2025-02-17 10:18     ` Fedor Pchelkin
  2025-02-17 10:32       ` Greg Kroah-Hartman
  0 siblings, 1 reply; 10+ messages in thread
From: Fedor Pchelkin @ 2025-02-17 10:18 UTC (permalink / raw)
  To: Heikki Krogerus, Greg Kroah-Hartman
  Cc: Christian A. Ehrhardt, Dmitry Baryshkov, Benson Leung,
	Jameson Thies, Saranya Gopal, linux-usb, linux-kernel,
	Mark Pearson, stable

On Thu, 13. Feb 15:58, Heikki Krogerus wrote:
> On Thu, Feb 06, 2025 at 09:43:15PM +0300, Fedor Pchelkin wrote:
> > It is observed that on some systems an initial PPM reset during the boot
> > phase can trigger a timeout:
> > 
> > [    6.482546] ucsi_acpi USBC000:00: failed to reset PPM!
> > [    6.482551] ucsi_acpi USBC000:00: error -ETIMEDOUT: PPM init failed
> > 
> > Still, increasing the timeout value, albeit being the most straightforward
> > solution, eliminates the problem: the initial PPM reset may take up to
> > ~8000-10000ms on some Lenovo laptops. When it is reset after the above
> > period of time (or even if ucsi_reset_ppm() is not called overall), UCSI
> > works as expected.
> > 
> > Moreover, if the ucsi_acpi module is loaded/unloaded manually after the
> > system has booted, reading the CCI values and resetting the PPM works
> > perfectly, without any timeout. Thus it's only a boot-time issue.
> > 
> > The reason for this behavior is not clear but it may be the consequence
> > of some tricks that the firmware performs or be an actual firmware bug.
> > As a workaround, increase the timeout to avoid failing the UCSI
> > initialization prematurely.
> > 
> > Fixes: b1b59e16075f ("usb: typec: ucsi: Increase command completion timeout value")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Fedor Pchelkin <boddah8794@gmail.com>
> 
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Thanks for review!

Should I respin the series or it can be taken as is despite being
initially tagged an RFC material?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH RFC 2/2] usb: typec: ucsi: increase timeout for PPM reset operations
  2025-02-17 10:18     ` Fedor Pchelkin
@ 2025-02-17 10:32       ` Greg Kroah-Hartman
  0 siblings, 0 replies; 10+ messages in thread
From: Greg Kroah-Hartman @ 2025-02-17 10:32 UTC (permalink / raw)
  To: Fedor Pchelkin
  Cc: Heikki Krogerus, Christian A. Ehrhardt, Dmitry Baryshkov,
	Benson Leung, Jameson Thies, Saranya Gopal, linux-usb,
	linux-kernel, Mark Pearson, stable

On Mon, Feb 17, 2025 at 01:18:25PM +0300, Fedor Pchelkin wrote:
> On Thu, 13. Feb 15:58, Heikki Krogerus wrote:
> > On Thu, Feb 06, 2025 at 09:43:15PM +0300, Fedor Pchelkin wrote:
> > > It is observed that on some systems an initial PPM reset during the boot
> > > phase can trigger a timeout:
> > > 
> > > [    6.482546] ucsi_acpi USBC000:00: failed to reset PPM!
> > > [    6.482551] ucsi_acpi USBC000:00: error -ETIMEDOUT: PPM init failed
> > > 
> > > Still, increasing the timeout value, albeit being the most straightforward
> > > solution, eliminates the problem: the initial PPM reset may take up to
> > > ~8000-10000ms on some Lenovo laptops. When it is reset after the above
> > > period of time (or even if ucsi_reset_ppm() is not called overall), UCSI
> > > works as expected.
> > > 
> > > Moreover, if the ucsi_acpi module is loaded/unloaded manually after the
> > > system has booted, reading the CCI values and resetting the PPM works
> > > perfectly, without any timeout. Thus it's only a boot-time issue.
> > > 
> > > The reason for this behavior is not clear but it may be the consequence
> > > of some tricks that the firmware performs or be an actual firmware bug.
> > > As a workaround, increase the timeout to avoid failing the UCSI
> > > initialization prematurely.
> > > 
> > > Fixes: b1b59e16075f ("usb: typec: ucsi: Increase command completion timeout value")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Fedor Pchelkin <boddah8794@gmail.com>
> > 
> > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> Thanks for review!
> 
> Should I respin the series or it can be taken as is despite being
> initially tagged an RFC material?

For obvious reasons, I can't take RFC patches as obviously you didn't
think they were worthy of being taken, hence you marking them that way
:)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH RFC 2/2] usb: typec: ucsi: increase timeout for PPM reset operations
  2025-02-06 18:43 ` [PATCH RFC 2/2] usb: typec: ucsi: increase timeout for PPM reset operations Fedor Pchelkin
  2025-02-13 13:58   ` Heikki Krogerus
@ 2025-02-18 16:57   ` Mark Pearson
  2025-02-18 19:53     ` Fedor Pchelkin
  1 sibling, 1 reply; 10+ messages in thread
From: Mark Pearson @ 2025-02-18 16:57 UTC (permalink / raw)
  To: Fedor Pchelkin, Heikki Krogerus, Christian A. Ehrhardt
  Cc: Greg KH, Dmitry Baryshkov, Benson Leung, Jameson Thies,
	Saranya Gopal, linux-usb, linux-kernel, stable

Hi  Fedor,

On Thu, Feb 6, 2025, at 1:43 PM, Fedor Pchelkin wrote:
> It is observed that on some systems an initial PPM reset during the boot
> phase can trigger a timeout:
>
> [    6.482546] ucsi_acpi USBC000:00: failed to reset PPM!
> [    6.482551] ucsi_acpi USBC000:00: error -ETIMEDOUT: PPM init failed
>
> Still, increasing the timeout value, albeit being the most straightforward
> solution, eliminates the problem: the initial PPM reset may take up to
> ~8000-10000ms on some Lenovo laptops. When it is reset after the above
> period of time (or even if ucsi_reset_ppm() is not called overall), UCSI
> works as expected.
>
> Moreover, if the ucsi_acpi module is loaded/unloaded manually after the
> system has booted, reading the CCI values and resetting the PPM works
> perfectly, without any timeout. Thus it's only a boot-time issue.
>
> The reason for this behavior is not clear but it may be the consequence
> of some tricks that the firmware performs or be an actual firmware bug.
> As a workaround, increase the timeout to avoid failing the UCSI
> initialization prematurely.
>

Could you let me know which Lenovo platform(s) you see the issue on?

I don't have any concerns with the patch below, but if the platform is in the Linux program I can reach out to the FW team and try to determine if there's an expected time needed (and how close we are to it).

Thanks

Mark

> Fixes: b1b59e16075f ("usb: typec: ucsi: Increase command completion 
> timeout value")
> Cc: stable@vger.kernel.org
> Signed-off-by: Fedor Pchelkin <boddah8794@gmail.com>
> ---
>  drivers/usb/typec/ucsi/ucsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c 
> b/drivers/usb/typec/ucsi/ucsi.c
> index 0fe1476f4c29..7a56d3f840d7 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -25,7 +25,7 @@
>   * difficult to estimate the time it takes for the system to process 
> the command
>   * before it is actually passed to the PPM.
>   */
> -#define UCSI_TIMEOUT_MS		5000
> +#define UCSI_TIMEOUT_MS		10000
> 
>  /*
>   * UCSI_SWAP_TIMEOUT_MS - Timeout for role swap requests
> -- 
> 2.48.1

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH RFC 2/2] usb: typec: ucsi: increase timeout for PPM reset operations
  2025-02-18 16:57   ` Mark Pearson
@ 2025-02-18 19:53     ` Fedor Pchelkin
  0 siblings, 0 replies; 10+ messages in thread
From: Fedor Pchelkin @ 2025-02-18 19:53 UTC (permalink / raw)
  To: Mark Pearson
  Cc: Heikki Krogerus, Christian A. Ehrhardt, Greg KH, Dmitry Baryshkov,
	Benson Leung, Jameson Thies, Saranya Gopal, linux-usb,
	linux-kernel, stable

On Tue, 18. Feb 11:57, Mark Pearson wrote:
> Hi  Fedor,
> 
> On Thu, Feb 6, 2025, at 1:43 PM, Fedor Pchelkin wrote:
> > It is observed that on some systems an initial PPM reset during the boot
> > phase can trigger a timeout:
> >
> > [    6.482546] ucsi_acpi USBC000:00: failed to reset PPM!
> > [    6.482551] ucsi_acpi USBC000:00: error -ETIMEDOUT: PPM init failed
> >
> > Still, increasing the timeout value, albeit being the most straightforward
> > solution, eliminates the problem: the initial PPM reset may take up to
> > ~8000-10000ms on some Lenovo laptops. When it is reset after the above
> > period of time (or even if ucsi_reset_ppm() is not called overall), UCSI
> > works as expected.
> >
> > Moreover, if the ucsi_acpi module is loaded/unloaded manually after the
> > system has booted, reading the CCI values and resetting the PPM works
> > perfectly, without any timeout. Thus it's only a boot-time issue.
> >
> > The reason for this behavior is not clear but it may be the consequence
> > of some tricks that the firmware performs or be an actual firmware bug.
> > As a workaround, increase the timeout to avoid failing the UCSI
> > initialization prematurely.
> >
> 
> Could you let me know which Lenovo platform(s) you see the issue on?
> 
> I don't have any concerns with the patch below, but if the platform is in
> the Linux program I can reach out to the FW team and try to determine if
> there's an expected time needed (and how close we are to it).

Yep, here. (also at the bottom of the [PATCH RFC 0/2] mail)

Please let me know if there's more info needed.

Machine:
  Type: Laptop System: LENOVO product: 21D0 v: ThinkBook 14 G4+ ARA
    serial: <superuser required>
  Mobo: LENOVO model: LNVNB161216 v: SDK0T76479 WIN
    serial: <superuser required> UEFI: LENOVO v: J6CN50WW date: 09/27/2024


Relevant part of the ACPI dump.

    Scope (\_SB)
    {
        Device (UBTC)
        {
            Name (_HID, EisaId ("USBC000"))  // _HID: Hardware ID
            Name (_CID, EisaId ("PNP0CA0"))  // _CID: Compatible ID
            Name (_UID, Zero)  // _UID: Unique ID
            Name (_DDN, "USB Type C")  // _DDN: DOS Device Name
            Name (_ADR, Zero)  // _ADR: Address
            Name (_DEP, Package (0x01)  // _DEP: Dependencies
            {
                \_SB.PCI0.LPC0.EC0
            })
            Method (_STA, 0, NotSerialized)  // _STA: Status
            {
                Return (0x0F)
            }

            Method (_PS0, 0, Serialized)  // _PS0: Power State 0
            {
                Sleep (0x07D0)
            }

            Method (_PS3, 0, Serialized)  // _PS3: Power State 3
            {
                Sleep (0x07D0)
            }

            Method (TPLD, 2, Serialized)
            {
                Name (PCKG, Package (0x01)
                {
                    Buffer (0x10){}
                })
                CreateField (DerefOf (PCKG [Zero]), Zero, 0x07, REV)
                REV = One
                CreateField (DerefOf (PCKG [Zero]), 0x40, One, VISI)
                VISI = Arg0
                CreateField (DerefOf (PCKG [Zero]), 0x57, 0x08, GPOS)
                GPOS = Arg1
                CreateField (DerefOf (PCKG [Zero]), 0x4A, 0x04, SHAP)
                SHAP = One
                CreateField (DerefOf (PCKG [Zero]), 0x20, 0x10, WID)
                WID = 0x08
                CreateField (DerefOf (PCKG [Zero]), 0x30, 0x10, HGT)
                HGT = 0x03
                Return (PCKG) /* \_SB_.UBTC.TPLD.PCKG */
            }

            Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
            {
                Name (RBUF, ResourceTemplate ()
                {
                    Memory32Fixed (ReadWrite,
                        0x7AF66000,         // Address Base
                        0x00001000,         // Address Length
                        )
                })
                Return (RBUF) /* \_SB_.UBTC._CRS.RBUF */
            }

            Device (HSP1)
            {
                Name (_ADR, One)  // _ADR: Address
                Name (_UPC, Package (0x04)  // _UPC: USB Port Capabilities
                {
                    0xFF, 
                    0x09, 
                    Zero, 
                    Zero
                })
                Method (_PLD, 0, NotSerialized)  // _PLD: Physical Location of Device
                {
                    Return (TPLD (One, 0x04))
                }
            }

            Device (HSP2)
            {
                Name (_ADR, One)  // _ADR: Address
                Name (_UPC, Package (0x04)  // _UPC: USB Port Capabilities
                {
                    0xFF, 
                    0x09, 
                    Zero, 
                    Zero
                })
                Method (_PLD, 0, NotSerialized)  // _PLD: Physical Location of Device
                {
                    Return (TPLD (One, 0x04))
                }
            }

            OperationRegion (USBC, SystemMemory, 0x7AF66000, 0x30)
            Field (USBC, ByteAcc, Lock, Preserve)
            {
                VER1,   8, 
                VER2,   8, 
                RSV1,   8, 
                RSV2,   8, 
                CCI0,   8, 
                CCI1,   8, 
                CCI2,   8, 
                CCI3,   8, 
                CTL0,   8, 
                CTL1,   8, 
                CTL2,   8, 
                CTL3,   8, 
                CTL4,   8, 
                CTL5,   8, 
                CTL6,   8, 
                CTL7,   8, 
                MGI0,   8, 
                MGI1,   8, 
                MGI2,   8, 
                MGI3,   8, 
                MGI4,   8, 
                MGI5,   8, 
                MGI6,   8, 
                MGI7,   8, 
                MGI8,   8, 
                MGI9,   8, 
                MGIA,   8, 
                MGIB,   8, 
                MGIC,   8, 
                MGID,   8, 
                MGIE,   8, 
                MGIF,   8, 
                MGO0,   8, 
                MGO1,   8, 
                MGO2,   8, 
                MGO3,   8, 
                MGO4,   8, 
                MGO5,   8, 
                MGO6,   8, 
                MGO7,   8, 
                MGO8,   8, 
                MGO9,   8, 
                MGOA,   8, 
                MGOB,   8, 
                MGOC,   8, 
                MGOD,   8, 
                MGOE,   8, 
                MGOF,   8
            }

            OperationRegion (DBG0, SystemIO, 0x80, One)
            Field (DBG0, ByteAcc, NoLock, Preserve)
            {
                IO80,   8
            }

            Method (NTFY, 0, Serialized)
            {
                ECRD ()
                Sleep (One)
                Notify (\_SB.UBTC, 0x80) // Status Change
            }

            Method (ECWR, 0, Serialized)
            {
                IO80 = 0xA0
                \_SB.PCI0.LPC0.EC0.ECWT (MGO0, RefOf (\_SB.PCI0.LPC0.EC0.MGO0))
                \_SB.PCI0.LPC0.EC0.ECWT (MGO1, RefOf (\_SB.PCI0.LPC0.EC0.MGO1))
                \_SB.PCI0.LPC0.EC0.ECWT (MGO2, RefOf (\_SB.PCI0.LPC0.EC0.MGO2))
                \_SB.PCI0.LPC0.EC0.ECWT (MGO3, RefOf (\_SB.PCI0.LPC0.EC0.MGO3))
                \_SB.PCI0.LPC0.EC0.ECWT (MGO4, RefOf (\_SB.PCI0.LPC0.EC0.MGO4))
                \_SB.PCI0.LPC0.EC0.ECWT (MGO5, RefOf (\_SB.PCI0.LPC0.EC0.MGO5))
                \_SB.PCI0.LPC0.EC0.ECWT (MGO6, RefOf (\_SB.PCI0.LPC0.EC0.MGO6))
                \_SB.PCI0.LPC0.EC0.ECWT (MGO7, RefOf (\_SB.PCI0.LPC0.EC0.MGO7))
                \_SB.PCI0.LPC0.EC0.ECWT (MGO8, RefOf (\_SB.PCI0.LPC0.EC0.MGO8))
                \_SB.PCI0.LPC0.EC0.ECWT (MGO9, RefOf (\_SB.PCI0.LPC0.EC0.MGO9))
                \_SB.PCI0.LPC0.EC0.ECWT (MGOA, RefOf (\_SB.PCI0.LPC0.EC0.MGOA))
                \_SB.PCI0.LPC0.EC0.ECWT (MGOB, RefOf (\_SB.PCI0.LPC0.EC0.MGOB))
                \_SB.PCI0.LPC0.EC0.ECWT (MGOC, RefOf (\_SB.PCI0.LPC0.EC0.MGOC))
                \_SB.PCI0.LPC0.EC0.ECWT (MGOD, RefOf (\_SB.PCI0.LPC0.EC0.MGOD))
                \_SB.PCI0.LPC0.EC0.ECWT (MGOE, RefOf (\_SB.PCI0.LPC0.EC0.MGOE))
                \_SB.PCI0.LPC0.EC0.ECWT (MGOF, RefOf (\_SB.PCI0.LPC0.EC0.MGOF))
                \_SB.PCI0.LPC0.EC0.ECWT (CTL0, RefOf (\_SB.PCI0.LPC0.EC0.CTL0))
                \_SB.PCI0.LPC0.EC0.ECWT (CTL1, RefOf (\_SB.PCI0.LPC0.EC0.CTL1))
                \_SB.PCI0.LPC0.EC0.ECWT (CTL2, RefOf (\_SB.PCI0.LPC0.EC0.CTL2))
                \_SB.PCI0.LPC0.EC0.ECWT (CTL3, RefOf (\_SB.PCI0.LPC0.EC0.CTL3))
                \_SB.PCI0.LPC0.EC0.ECWT (CTL4, RefOf (\_SB.PCI0.LPC0.EC0.CTL4))
                \_SB.PCI0.LPC0.EC0.ECWT (CTL5, RefOf (\_SB.PCI0.LPC0.EC0.CTL5))
                \_SB.PCI0.LPC0.EC0.ECWT (CTL6, RefOf (\_SB.PCI0.LPC0.EC0.CTL6))
                \_SB.PCI0.LPC0.EC0.ECWT (CTL7, RefOf (\_SB.PCI0.LPC0.EC0.CTL7))
                \_SB.PCI0.LPC0.EC0.ECWT (0xE0, RefOf (\_SB.PCI0.LPC0.EC0.USDC))
                IO80 = 0xA1
            }

            Method (ECRD, 0, Serialized)
            {
                IO80 = 0xA3
                MGI0 = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.MGI0))
                MGI1 = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.MGI1))
                MGI2 = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.MGI2))
                MGI3 = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.MGI3))
                MGI4 = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.MGI4))
                MGI5 = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.MGI5))
                MGI6 = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.MGI6))
                MGI7 = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.MGI7))
                MGI8 = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.MGI8))
                MGI9 = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.MGI9))
                MGIA = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.MGIA))
                MGIB = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.MGIB))
                MGIC = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.MGIC))
                MGID = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.MGID))
                MGIE = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.MGIE))
                MGIF = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.MGIF))
                VER1 = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.VER1))
                VER2 = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.VER2))
                RSV1 = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.RSV1))
                RSV2 = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.RSV2))
                CCI0 = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.CCI0))
                CCI1 = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.CCI1))
                CCI2 = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.CCI2))
                CCI3 = \_SB.PCI0.LPC0.EC0.ECRD (RefOf (\_SB.PCI0.LPC0.EC0.CCI3))
                \_SB.PCI0.LPC0.EC0.ECWT (0xE1, RefOf (\_SB.PCI0.LPC0.EC0.USGC))
                IO80 = 0xA4
            }

            Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
            {
                If ((Arg0 == ToUUID ("6f8398c2-7ca4-11e4-ad36-631042b5008f") /* Unknown UUID */))
                {
                    If ((ToInteger (Arg2) == Zero))
                    {
                        Return (Buffer (One)
                        {
                             0x0F                                             // .
                        })
                    }
                    ElseIf ((ToInteger (Arg2) == One))
                    {
                        ECWR ()
                    }
                    ElseIf ((ToInteger (Arg2) == 0x02))
                    {
                        ECRD ()
                    }
                    Else
                    {
                        Return (Zero)
                    }
                }

                Return (Zero)
            }
        }
    }

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2025-02-18 19:53 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20250206184327.16308-1-boddah8794@gmail.com>
2025-02-06 18:43 ` [PATCH RFC 1/2] acpi: typec: ucsi: Introduce a ->poll_cci method Fedor Pchelkin
2025-02-07  0:38   ` Dmitry Baryshkov
2025-02-07  6:51     ` Christian A. Ehrhardt
2025-02-13 13:56   ` Heikki Krogerus
2025-02-06 18:43 ` [PATCH RFC 2/2] usb: typec: ucsi: increase timeout for PPM reset operations Fedor Pchelkin
2025-02-13 13:58   ` Heikki Krogerus
2025-02-17 10:18     ` Fedor Pchelkin
2025-02-17 10:32       ` Greg Kroah-Hartman
2025-02-18 16:57   ` Mark Pearson
2025-02-18 19:53     ` Fedor Pchelkin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).