Hi,
On Sat, Aug 20, 2022 at 08:40:52PM +0200, Greg Kroah-Hartman wrote:
On Fri, Aug 19, 2022 at 06:32:43PM +0200, Takashi Iwai wrote:No, I don't think that's better. Right now I would prefer that we play
Hi,Adding Heikki to the thread...
we've got multiple reports about 5.19 kernel starting crashing after
some time, and this turned out to be triggered by ucsi_acpi driver.
The details are found in:
https://bugzilla.suse.com/show_bug.cgi?id=1202386
The culprit seems to be the commit 87d0e2f41b8c
usb: typec: ucsi: add a common function ucsi_unregister_connectors()
This commit looks as if it were a harmless cleanup, but this failed inI'll be glad to revert this now, unless Heikki thinks:
a subtle way. Namely, in the error scenario, the driver gets an error
at ucsi_register_altmodes(), and goes to the error handling to release
the resources. Through this refactoring, the release part was unified
to a funciton ucsi_unregister_connectors(). And there, it has a NULL
check of con->wq, and it bails out the loop if it's NULL.
Meanwhile, ucsi_register_port() itself still calls destroy_workqueue()
and clear con->wq at its error path. This ended up in the leftover
power supply device with the uninitialized / cleared device.
It was confirmed that the problem could be avoided by a simple
revert.
I guess another fix could be removing the part clearing con->wq, i.e.that is any better?
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1192,11 +1192,6 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
out_unlock:
mutex_unlock(&con->lock);
- if (ret && con->wq) {
- destroy_workqueue(con->wq);
- con->wq = NULL;
- }
-
return ret;
}
... but it's totally untested and I'm not entirely sure whether it's
better.
it safe and revert.
The conditions are different in the two places where the ports are
unregistered in this driver. Therefore I don't think it makes sense
to use a function like ucsi_unregister_connectors() that tries to
cover both cases. It will always be a little bit fragile.
Instead we could introduce a function that can be used to remove a
single port. That would leave the handling of the conditions to the
callers of the function, but it would still remove the boilerplate.
That would be much safer IMO.
But to fix this problem, I think we should revert.
thanks,