Re: [PATCH 2/2] of: overlay: Synchronize of_overlay_remove() with the devlink removals

[Nuno Sá]

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á