Message-ID: <coagi34rszmagqfsixava2gkswmdbj5ezfkzbdxoo4yn6nzva3@3bb5a6egkpli>
Date: Tue, 28 Jan 2025 16:58:18 +0300
From: Fedor Pchelkin <boddah8794@...il.com>
To: "Christian A. Ehrhardt" <lk@...e.de>
Cc: Saranya Gopal <saranya.gopal@...el.com>, linux-kernel@...r.kernel.org, 
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Prashant Malani <pmalani@...omium.org>, Jameson Thies <jthies@...gle.com>, 
	Abhishek Pandit-Subedi <abhishekpandit@...omium.org>, Neil Armstrong <neil.armstrong@...aro.org>, 
	Uwe Kleine-König <u.kleine-koenig@...gutronix.de>, Samuel Čavoj <samuel@...oj.net>, 
	linux-usb@...r.kernel.org, Kenneth Crudup <kenny@...ix.com>, 
	Sasha Levin <sashal@...nel.org>
Subject: Re: [PATCH 5/5] usb: typec: ucsi: Clear UCSI_CCI_RESET_COMPLETE
 before reset

On Wed, 22. Jan 22:11, Christian A. Ehrhardt wrote:
> 
> Hi Fedor,
> 
> On Sun, Jan 19, 2025 at 04:23:21PM +0300, Fedor Pchelkin wrote:
> > Christian A. Ehrhardt wrote:
> > > 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@...e.de>
> > > Date:   Mon Dec 16 21:52:46 2024 +0100
> > > 
> > >     acpi: typec: ucsi: Introduce a ->poll_cci method
> > 
> > WARNING: CPU: 0 PID: 8 at drivers/usb/typec/ucsi/ucsi.c:1377 ucsi_reset_ppm+0x1af/0x1c0 [typec_ucsi]
> > is triggered on my laptop on roughly every system boot. When it's not,
> > there is a
> >   ucsi_acpi USBC000:00: error -ETIMEDOUT: PPM init failed
> > message observed in the log.
> > 
> > I've tried the above patch "acpi: typec: ucsi: Introduce a ->poll_cci
> > method" but the issue is still triggered [1].
> > 
> > Is there any useful info/logs I can provide you for further
> > investigation of the warning in question?
> > 
> > As the warning is quite reliably triggered on my system, I may help with
> > the testing of other patches.
> 
> Hard to say what might be going on. Some obvious questions to
> narrow it down, though:
> - Is this something new and UCSI worked before or has UCSI been broken
>   with older kernels as well (maybe with different or no error
>   messages).

Thanks for the feedback and sorry for the delay!

Yep, I've eventually checked this: as it stands, there's always been a
"-ETIMEDOUT: PPM init failed" observed on starting ucsi_init_work during
the boot. Back to v5.12 at least - the oldest kernel I've managed to boot
on this laptop.

On the other hand, the WARNING appears only after the commit fa48d7e81624
("usb: typec: ucsi: Do not call ACPI _DSM method for UCSI read operations").

> - If you get the warning but not the "PPM init failed" message,
>   does UCSI actually work? Try to plug something into the USB-C
>   ports and watch out for additional error messages (possibly after
>   a timeout). Do new files/devices show up in sysfs?

Well, it's interesting. When there is a WARNING and no "PPM init failed"
message, it works because ucsi_init() goes on. When there is a "PPM init
failed", UCSI doesn't actually initialize successfully and it doesn't work.

And I probably didn't pay attention to the "PPM init failed" messages
earlier because I'm not an active UCSI user, utilize Type-C port only for
the power supply (this always works and I guess the not working UCSI
doesn't affect this directly). On the opposite, the big WARNING during the
boot now became more visible :)


It appears like PPM is not ready yet for communication during the boot.

Increasing a timeout just to 2x eliminates the errors in my case:

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index fcf499cc9458..b1a4470214b6 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1362,7 +1362,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
                if (ret < 0)
                        goto out;
 
-               tmo = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS);
+               tmo = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS * 2);
                do {
                        ret = ucsi->ops->read_cci(ucsi, &cci);
                        if (ret < 0)
@@ -1382,7 +1382,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
        if (ret < 0)
                goto out;
 
-       tmo = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS);
+       tmo = jiffies + msecs_to_jiffies(UCSI_TIMEOUT_MS * 2);
 
        do {
                if (time_is_before_jiffies(tmo)) {


Turning the laptop several times on and off, I'd say the average time
taken for the initial reset takes around 8000ms:

[    2.568534] ucsi_acpi USBC000:00: enter ucsi_reset_ppm()
[   10.875710] ucsi_acpi USBC000:00: exit ucsi_reset_ppm(), ret 0

I see that UCSI_TIMEOUT_MS is already chosen to be a rather significant
value, much bigger than what the specs say. Maybe ucsi_init_work races
with something? Could this ever happen here? Or just a firmware/hardware
issue...

> - Printing the value of CCI at various stages of the init process
>   might help us to understand what's going on.

During ucsi_reset_ppm() in case of a timeout the reported value of CCI is
always zero and doesn't change on read/poll attempts. In case of the
WARNING it's always read as UCSI_CCI_RESET_COMPLETE thus it WARNs but
ucsi_reset_ppm() returns zero and the further initialization goes on
without any errors.

Is the usage of WARN macros justifiable here if it may potentially be
caused only by the firmware/hardware errors (well, at a quick glance) and
not an issue which can fixed at the kernel level? E.g. the timeout
situation here is not reported by WARN, but by simple printks..

Powered by blists - more mailing lists