On Tue, 2024-02-27 at 16:24 +0100, Herve Codina wrote: > Hi Saravana, Luca, Nuno, > > On Tue, 20 Feb 2024 16:37:05 -0800 > Saravana Kannan <saravanak@xxxxxxxxxx> wrote: > > ... > > > > > > > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c > > > index a9a292d6d59b..5c5f808b163e 100644 > > > --- a/drivers/of/overlay.c > > > +++ b/drivers/of/overlay.c > > > @@ -1202,6 +1202,12 @@ int of_overlay_remove(int *ovcs_id) > > > goto out; > > > } > > > > > > + /* > > > + * Wait for any ongoing device link removals before removing some of > > > + * nodes > > > + */ > > > + device_link_wait_removal(); > > > + > > > > Nuno in his patch[1] had this "wait" happen inside > > __of_changeset_entry_destroy(). Which seems to be necessary to not hit > > the issue that Luca reported[2] in this patch series. Is there any > > problem with doing that? > > > > Luca for some reason did a unlock/lock(of_mutex) in his test patch and > > I don't think that's necessary. > > I think the unlock/lock in Luca's case and so in Nuno's case is needed. > > I do the device_link_wait_removal() wihout having the of_mutex locked. > > Now, suppose I do the device_link_wait_removal() call with the of_mutex locked. > The following flow is allowed and a deadlock is present. > > of_overlay_remove() > lock(of_mutex) > device_link_wait_removal() > > And, from the workqueue jobs execution: > ... > device_put() > some_driver->remove() > of_overlay_remove() <--- The job will never end. > It is waiting for of_mutex. > Deadlock > We may need some input from Saravana (and others) on this. I might be missing something but can a put_device() lead into a driver remove callback? Driver code is not device code and put_device() leads to device_release() which will either call the device ->release(), ->type->release() or the class ->dev_release(). And, IMO, calling of_overlay_remove() or something like that (like something that would lead to unbinding a device from it's driver) in a device release callback would be at the very least very questionable. Typically, what you see in there is of_node_put() and things like kfree() of the device itself or any other data. The driver remove callback should be called when unbinding the device from it's drivers and devlinks should also be removed after device_unbind_cleanup() (i.e, after the driver remove callback). Having said the above, the driver core has lots of subtleties so, again, I can be missing something. But at this point I'm still not seeing any deadlock... - Nuno Sá
