Re: [PATCH] of/overlay: add of overlay notifications

[Rob Herring]

On Wed, Mar 2, 2016 at 12:49 PM, atull <atull@xxxxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, 2 Mar 2016, Rob Herring wrote:
>
>> On Fri, Feb 26, 2016 at 3:44 PM, Alan Tull <atull@xxxxxxxxxxxxxxxxxxxxx> wrote:
>> > This patch add of overlay notifications.
>> >
>> > When DT overlays are being added, some drivers/subsystems
>> > need to see device tree overlays before the changes go into
>> > the live tree.

[...]

>> > @@ -389,6 +431,8 @@ int of_overlay_create(struct device_node *tree)
>> >         /* add to the tail of the overlay list */
>> >         list_add_tail(&ov->node, &ov_list);
>> >
>> > +       of_overlay_notify(ov, OF_OVERLAY_POST_APPLY);
>> > +
>> >         mutex_unlock(&of_mutex);
>>
>> This means that any post-apply handlers can't make any calls that take
>> the mutex. Maybe that is fine? On the flip side, maybe we want to
>> prevent any changes while the notifiers are called.
>>
>> The other calls could have similar issues. We should make sure we are
>> consistent.
>>
>
> Currently it's consistent - all 4 notifiers hold the mutex,
> so all 4 prevent dt changes.  That's not super bad, but it's
> different from the reconfig notifiers and I could see some
> possible usefulness in allowing changes.
>
> I don't see any harm in unlocking for pre-apply, post-apply,
> or post-remove.  But for pre-remove, unlocking would allow
> notifier code to make changes to the overlay's changeset
> that is about to be removed.  Is that something we need to
> be super worried about preventing?
>
> For pre-apply, unlocking would allow changes to either the
> live tree or the overlay. That's ok since the next thing
> that will happen after the notifier is that the changeset
> will be built from the overlay, so any changes made to the
> overlay would make it into the changeset before it is
> applied.  Post-apply and post-remove are also ok to
> be unlocked.
>
> So if the pre-remove case is ok, I could release the mutex
> for all 4 cases in v3.

The only one I can see it being useful to make changes would be
pre-apply. The complication there is you would have to move the mutex.
I don't think the mutex really needs to be held until before
of_overlay_apply, but I'm not completely sure. For now, I think just
leave things as they are now.

Rob