Hi,
I'm sending these as an RFC first because I'm not done testing.
I made a small modification to my original proposal (in the bug
report). Now also connection during suspend should be covered.
I would appreciate if you guys could test these again. If everything
works, and the bug is fixed, let me know, and I'll add your Tested-by
tags to the patches.
thanks,
Heikki Krogerus (2):
usb: typec: ucsi: Check the connection on resume
usb: typec: ucsi: acpi: Add PM hooks
drivers/usb/typec/ucsi/ucsi.c | 42 +++++++++++++++++++++---------
drivers/usb/typec/ucsi/ucsi_acpi.c | 15 +++++++++++
2 files changed, 44 insertions(+), 13 deletions(-)
--
2.35.1
This fixes an issue where the partner device is not unregistered
properly after resume if it's unplugged during suspend.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=210425
Fixes: a94ecde41f7e ("usb: typec: ucsi: ccg: enable runtime pm support")
Cc: <[email protected]>
Signed-off-by: Heikki Krogerus <[email protected]>
---
drivers/usb/typec/ucsi/ucsi.c | 42 ++++++++++++++++++++++++-----------
1 file changed, 29 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 7f2624f427241..33e1cee9dc184 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -183,16 +183,6 @@ int ucsi_send_command(struct ucsi *ucsi, u64 command,
}
EXPORT_SYMBOL_GPL(ucsi_send_command);
-int ucsi_resume(struct ucsi *ucsi)
-{
- u64 command;
-
- /* Restore UCSI notification enable mask after system resume */
- command = UCSI_SET_NOTIFICATION_ENABLE | ucsi->ntfy;
-
- return ucsi_send_command(ucsi, command, NULL, 0);
-}
-EXPORT_SYMBOL_GPL(ucsi_resume);
/* -------------------------------------------------------------------------- */
struct ucsi_work {
@@ -746,6 +736,7 @@ static void ucsi_partner_change(struct ucsi_connector *con)
static int ucsi_check_connection(struct ucsi_connector *con)
{
+ u8 prev_flags = con->status.flags;
u64 command;
int ret;
@@ -756,10 +747,13 @@ static int ucsi_check_connection(struct ucsi_connector *con)
return ret;
}
+ if (con->status.flags == prev_flags)
+ return 0;
+
if (con->status.flags & UCSI_CONSTAT_CONNECTED) {
- if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) ==
- UCSI_CONSTAT_PWR_OPMODE_PD)
- ucsi_partner_task(con, ucsi_check_altmodes, 30, 0);
+ ucsi_register_partner(con);
+ ucsi_pwr_opmode_change(con);
+ ucsi_partner_change(con);
} else {
ucsi_partner_change(con);
ucsi_port_psy_changed(con);
@@ -1280,6 +1274,28 @@ static int ucsi_init(struct ucsi *ucsi)
return ret;
}
+int ucsi_resume(struct ucsi *ucsi)
+{
+ struct ucsi_connector *con;
+ u64 command;
+ int ret;
+
+ /* Restore UCSI notification enable mask after system resume */
+ command = UCSI_SET_NOTIFICATION_ENABLE | ucsi->ntfy;
+ ret = ucsi_send_command(ucsi, command, NULL, 0);
+ if (ret < 0)
+ return ret;
+
+ for (con = ucsi->connector; con->port; con++) {
+ mutex_lock(&con->lock);
+ ucsi_check_connection(con);
+ mutex_unlock(&con->lock);
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(ucsi_resume);
+
static void ucsi_init_work(struct work_struct *work)
{
struct ucsi *ucsi = container_of(work, struct ucsi, work.work);
--
2.35.1
Some systems notify the driver separately after resume if
the state of the connection changed during suspend, but
there is no way we can rely on that.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=210425
Fixes: a94ecde41f7e ("usb: typec: ucsi: ccg: enable runtime pm support")
Cc: <[email protected]>
Signed-off-by: Heikki Krogerus <[email protected]>
---
drivers/usb/typec/ucsi/ucsi_acpi.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index 8873c1644a295..8c7008cc9942e 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -185,6 +185,20 @@ static int ucsi_acpi_remove(struct platform_device *pdev)
return 0;
}
+static int ucsi_acpi_suspend(struct device *dev)
+{
+ return 0;
+}
+
+static int ucsi_acpi_resume(struct device *dev)
+{
+ struct ucsi_acpi *ua = dev_get_drvdata(dev);
+
+ return ucsi_resume(ua->ucsi);
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(ucsi_acpi_pm_ops, ucsi_acpi_suspend, ucsi_acpi_resume);
+
static const struct acpi_device_id ucsi_acpi_match[] = {
{ "PNP0CA0", 0 },
{ },
@@ -194,6 +208,7 @@ MODULE_DEVICE_TABLE(acpi, ucsi_acpi_match);
static struct platform_driver ucsi_acpi_platform_driver = {
.driver = {
.name = "ucsi_acpi",
+ .pm = pm_ptr(&ucsi_acpi_pm_ops),
.acpi_match_table = ACPI_PTR(ucsi_acpi_match),
},
.probe = ucsi_acpi_probe,
--
2.35.1
Hi,
> Hi,
>
> I'm sending these as an RFC first because I'm not done testing.
>
> I made a small modification to my original proposal (in the bug
> report). Now also connection during suspend should be covered.
>
> I would appreciate if you guys could test these again. If everything
> works, and the bug is fixed, let me know, and I'll add your Tested-by
> tags to the patches.
>
> thanks,
>
> Heikki Krogerus (2):
> usb: typec: ucsi: Check the connection on resume
> usb: typec: ucsi: acpi: Add PM hooks
>
> drivers/usb/typec/ucsi/ucsi.c | 42 +++++++++++++++++++++---------
> drivers/usb/typec/ucsi/ucsi_acpi.c | 15 +++++++++++
> 2 files changed, 44 insertions(+), 13 deletions(-)
I have tested this patch on Lenovo T14 Gen2i with ac adapter and everything
looks good. I applied patch on linux 5.19.7.
Dear Heikki,
> I'm sending these as an RFC first because I'm not done testing.
>
> I made a small modification to my original proposal (in the bug
> report). Now also connection during suspend should be covered.
>
> I would appreciate if you guys could test these again. If
> everything works, and the bug is fixed, let me know, and I'll add
> your Tested-by tags to the patches.
>
Thanks so much for these changes—that's awesome! I have just finished
testing this against 5.19.7 (Arch Linux) with a Lenovo X1 (Gen 9).
I am very happy to see that, as far as I can tell, the issue
disappeared completely!
However, I am receiving additional warnings via `journalctl` that I
did not receive before; I have attached this trace as an additional
log file. Nothing in there seems critical and I can confirm that the
system continues to operate normally. I merely wanted to provide you
with this additional information in case it is of relevance.
Please let me know if there's anything else I can do here; I really
appreciate the time you spent on this!
All the best,
Bastian
On Thu, Sep 08, 2022 at 07:01:34AM +0200, Bastian Rieck wrote:
> Dear Heikki,
>
> > I'm sending these as an RFC first because I'm not done testing.
> >
> > I made a small modification to my original proposal (in the bug
> > report). Now also connection during suspend should be covered.
> >
> > I would appreciate if you guys could test these again. If
> > everything works, and the bug is fixed, let me know, and I'll add
> > your Tested-by tags to the patches.
> >
>
> Thanks so much for these changes—that's awesome! I have just finished
> testing this against 5.19.7 (Arch Linux) with a Lenovo X1 (Gen 9).
>
> I am very happy to see that, as far as I can tell, the issue
> disappeared completely!
>
> However, I am receiving additional warnings via `journalctl` that I
> did not receive before; I have attached this trace as an additional
> log file. Nothing in there seems critical and I can confirm that the
> system continues to operate normally. I merely wanted to provide you
> with this additional information in case it is of relevance.
>
> Please let me know if there's anything else I can do here; I really
> appreciate the time you spent on this!
Thank you for the report. That warning certainly needs to be sorted
out before I send the final versions. I'll try to reproduce that.
thanks,
--
heikki
Hi Bastian,
On Fri, Sep 09, 2022 at 09:22:50AM +0300, Heikki Krogerus wrote:
> On Thu, Sep 08, 2022 at 07:01:34AM +0200, Bastian Rieck wrote:
> > Dear Heikki,
> >
> > > I'm sending these as an RFC first because I'm not done testing.
> > >
> > > I made a small modification to my original proposal (in the bug
> > > report). Now also connection during suspend should be covered.
> > >
> > > I would appreciate if you guys could test these again. If
> > > everything works, and the bug is fixed, let me know, and I'll add
> > > your Tested-by tags to the patches.
> > >
> >
> > Thanks so much for these changes—that's awesome! I have just finished
> > testing this against 5.19.7 (Arch Linux) with a Lenovo X1 (Gen 9).
> >
> > I am very happy to see that, as far as I can tell, the issue
> > disappeared completely!
> >
> > However, I am receiving additional warnings via `journalctl` that I
> > did not receive before; I have attached this trace as an additional
> > log file. Nothing in there seems critical and I can confirm that the
> > system continues to operate normally. I merely wanted to provide you
> > with this additional information in case it is of relevance.
> >
> > Please let me know if there's anything else I can do here; I really
> > appreciate the time you spent on this!
>
> Thank you for the report. That warning certainly needs to be sorted
> out before I send the final versions. I'll try to reproduce that.
I'm not getting anywhere with this one. Could you provide me with the
trace output from both module and ucsi events?
To enable those events - assuming debugfs is mounted to
/sys/kernel/debug:
% echo 1 > /sys/kernel/debug/tracing/events/ucsi/enable
% echo 1 > /sys/kernel/debug/tracing/events/module/enable
Run the suspend resume cycle, and then dump the trace output to a
file:
% cat /sys/kernel/debug/tracing/trace > ucsi_trace
thanks,
--
heikki
Dear Heikki, dear all,
I am still unable to reproduce the previous error messages I
mentioned. In the meantime, the UCSI issues disappeared with 5.19.9
for me (!). I think this is related to a revert by Takashi Iwai [1]
in 5.19.8.
Given these data, would it be useful to test your patch against
5.19.9 or a more recent version?
Thanks for all your efforts in supporting us here!
All the best,
Bastian
[1] https://www.spinics.net/lists/linux-usb/msg230368.html
On Friday, September 16, 2022 12:46:47 PM CEST Heikki Krogerus
wrote:
> Hi Bastian,
>
> On Fri, Sep 09, 2022 at 09:22:50AM +0300, Heikki Krogerus wrote:
> > On Thu, Sep 08, 2022 at 07:01:34AM +0200, Bastian Rieck wrote:
> > > Dear Heikki,
> > >
> > > > I'm sending these as an RFC first because I'm not done
> > > > testing.
> > > >
> > > > I made a small modification to my original proposal (in the
> > > > bug
> > > > report). Now also connection during suspend should be
> > > > covered.
> > > >
> > > > I would appreciate if you guys could test these again. If
> > > > everything works, and the bug is fixed, let me know, and
> > > > I'll add
> > > > your Tested-by tags to the patches.
> > >
> > > Thanks so much for these changes—that's awesome! I have just
> > > finished testing this against 5.19.7 (Arch Linux) with a
> > > Lenovo X1 (Gen 9).
> > >
> > > I am very happy to see that, as far as I can tell, the issue
> > > disappeared completely!
> > >
> > > However, I am receiving additional warnings via `journalctl`
> > > that I did not receive before; I have attached this trace as
> > > an additional log file. Nothing in there seems critical and I
> > > can confirm that the system continues to operate normally. I
> > > merely wanted to provide you with this additional information
> > > in case it is of relevance.
> > >
> > > Please let me know if there's anything else I can do here; I
> > > really appreciate the time you spent on this!
> >
> > Thank you for the report. That warning certainly needs to be
> > sorted out before I send the final versions. I'll try to
> > reproduce that.
> I'm not getting anywhere with this one. Could you provide me with
> the trace output from both module and ucsi events?
>
> To enable those events - assuming debugfs is mounted to
> /sys/kernel/debug:
>
> % echo 1 > /sys/kernel/debug/tracing/events/ucsi/enable
> % echo 1 > /sys/kernel/debug/tracing/events/module/enable
>
> Run the suspend resume cycle, and then dump the trace output to a
> file:
>
> % cat /sys/kernel/debug/tracing/trace > ucsi_trace
>
> thanks,
On Fri, Oct 07, 2022 at 12:00:16PM +0200, Bastian Rieck wrote:
> Dear Heikki, dear all,
>
> I am still unable to reproduce the previous error messages I
> mentioned. In the meantime, the UCSI issues disappeared with 5.19.9
> for me (!). I think this is related to a revert by Takashi Iwai [1]
> in 5.19.8.
>
> Given these data, would it be useful to test your patch against
> 5.19.9 or a more recent version?
No need. I'm just about to resend these.
thanks,
--
heikki