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

Herve Codina
2024-03-04T16:49:36Z

Hi Rob,

On Mon, 4 Mar 2024 09:22:02 -0600
Rob Herring <robh@kernel.org> wrote:

...

> > > @@ -853,6 +854,14 @@ static void free_overlay_changeset(struct
> > > overlay_changeset *ovcs)
> > >  {
> > >   int i;
> > >
> > > + /*
> > > + * Wait for any ongoing device link removals before removing some of
> > > + * nodes. Drop the global lock while waiting
> > > + */
> > > + mutex_unlock(&of_mutex);
> > > + device_link_wait_removal();
> > > + mutex_lock(&of_mutex);
> >
> > I'm still not convinced we need to drop the lock. What happens if someone else
> > grabs the lock while we are in device_link_wait_removal()? Can we guarantee that
> > we can't screw things badly?
>
> It is also just ugly because it's the callers of
> free_overlay_changeset() that hold the lock and now we're releasing it
> behind their back.
>
> As device_link_wait_removal() is called before we touch anything, can't
> it be called before we take the lock? And do we need to call it if
> applying the overlay fails?
>

Indeed, having device_link_wait_removal() is not needed when applying the
overlay fails.

I can call device_link_wait_removal() from the caller of_overlay_remove()
but not before the lock is taken.
We need to call it between __of_changeset_revert_notify() and
free_overlay_changeset() and so, the lock is taken.

This lead to the following sequence:
--- 8< ---
int of_overlay_remove(int *ovcs_id)
{
...
mutex_lock(&of_mutex);
...

ret = __of_changeset_revert_notify(&ovcs->cset);
...

ret_tmp = overlay_notify(ovcs, OF_OVERLAY_POST_REMOVE);
...

mutex_unlock(&of_mutex);
device_link_wait_removal();
mutex_lock(&of_mutex);

free_overlay_changeset(ovcs);
...
mutex_unlock(&of_mutex);
...
}
--- 8< ---

In this sequence, the question is:
Do we need to release the mutex lock while device_link_wait_removal() is
called ?

Best regards,
Hervé

--
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering