linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] of/overlay: add of overlay notifications
@ 2016-02-26 21:44 Alan Tull
  2016-03-02  2:27 ` Rob Herring
  0 siblings, 1 reply; 8+ messages in thread
From: Alan Tull @ 2016-02-26 21:44 UTC (permalink / raw)
  To: Rob Herring, Pantelis Antoniou
  Cc: Frank Rowand, Grant Likely, devicetree, linux-kernel,
	Moritz Fischer, Pantelis Antoniou, Alan Tull, Dinh Nguyen,
	Alan Tull

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.

This is distinct from reconfig notifiers that are
post-apply or post-remove and which issue very granular
notifications without providing access to the context
of a whole overlay.

The following 4 notificatons are issued:
  OF_OVERLAY_PRE_APPLY
  OF_OVERLAY_POST_APPLY
  OF_OVERLAY_PRE_REMOVE
  OF_OVERLAY_POST_REMOVE

In the case of pre-apply notification, if the notifier
returns error, the overlay will be rejected.

This patch exports two functions for registering/unregistering
notifications:
  of_overlay_notifier_register(struct notifier_block *nb)
  of_overlay_notifier_unregister(struct notifier_block *nb)

The notification data includes pointers to the overlay
target and the overlay:

struct of_overlay_notify_data {
       struct device_node *overlay;
       struct device_node *target;
};

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
v2: add missing 'static inline' in of.h
---
 drivers/of/overlay.c |   47 ++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/of.h   |   25 +++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 8225081..a26f0ed 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -56,6 +56,41 @@ struct of_overlay {
 static int of_overlay_apply_one(struct of_overlay *ov,
 		struct device_node *target, const struct device_node *overlay);
 
+static BLOCKING_NOTIFIER_HEAD(of_overlay_chain);
+
+int of_overlay_notifier_register(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&of_overlay_chain, nb);
+}
+EXPORT_SYMBOL_GPL(of_overlay_notifier_register);
+
+int of_overlay_notifier_unregister(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&of_overlay_chain, nb);
+}
+EXPORT_SYMBOL_GPL(of_overlay_notifier_unregister);
+
+static int of_overlay_notify(struct of_overlay *ov,
+			     enum of_overlay_notify_action action)
+{
+	struct of_overlay_notify_data nd;
+	int i, ret;
+
+	for (i = 0; i < ov->count; i++) {
+		struct of_overlay_info *ovinfo = &ov->ovinfo_tab[i];
+
+		nd.target = ovinfo->target;
+		nd.overlay = ovinfo->overlay;
+
+		ret = blocking_notifier_call_chain(&of_overlay_chain,
+						   action, &nd);
+		if (ret)
+			return notifier_to_errno(ret);
+	}
+
+	return 0;
+}
+
 static int of_overlay_apply_single_property(struct of_overlay *ov,
 		struct device_node *target, struct property *prop)
 {
@@ -370,6 +405,13 @@ int of_overlay_create(struct device_node *tree)
 		goto err_free_idr;
 	}
 
+	err = of_overlay_notify(ov, OF_OVERLAY_PRE_APPLY);
+	if (err < 0) {
+		pr_err("%s: Pre-apply notifier failed (err=%d)\n",
+		       __func__, err);
+		goto err_free_idr;
+	}
+
 	/* apply the overlay */
 	err = of_overlay_apply(ov);
 	if (err) {
@@ -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);
 
 	return id;
@@ -509,9 +553,10 @@ int of_overlay_destroy(int id)
 		goto out;
 	}
 
-
+	of_overlay_notify(ov, OF_OVERLAY_PRE_REMOVE);
 	list_del(&ov->node);
 	__of_changeset_revert(&ov->cset);
+	of_overlay_notify(ov, OF_OVERLAY_POST_REMOVE);
 	of_free_overlay_info(ov);
 	idr_remove(&ov_idr, id);
 	of_changeset_destroy(&ov->cset);
diff --git a/include/linux/of.h b/include/linux/of.h
index dc6e396..b79e1c5 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1083,6 +1083,21 @@ int of_overlay_create(struct device_node *tree);
 int of_overlay_destroy(int id);
 int of_overlay_destroy_all(void);
 
+enum of_overlay_notify_action {
+	OF_OVERLAY_PRE_APPLY,
+	OF_OVERLAY_POST_APPLY,
+	OF_OVERLAY_PRE_REMOVE,
+	OF_OVERLAY_POST_REMOVE,
+};
+
+struct of_overlay_notify_data {
+	struct device_node *overlay;
+	struct device_node *target;
+};
+
+int of_overlay_notifier_register(struct notifier_block *nb);
+int of_overlay_notifier_unregister(struct notifier_block *nb);
+
 #else
 
 static inline int of_overlay_create(struct device_node *tree)
@@ -1100,6 +1115,16 @@ static inline int of_overlay_destroy_all(void)
 	return -ENOTSUPP;
 }
 
+static inline int of_overlay_notifier_register(struct notifier_block *nb)
+{
+	return 0;
+}
+
+static inline int of_overlay_notifier_unregister(struct notifier_block *nb)
+{
+	return 0;
+}
+
 #endif
 
 #endif /* _LINUX_OF_H */
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] of/overlay: add of overlay notifications
  2016-02-26 21:44 [PATCH] of/overlay: add of overlay notifications Alan Tull
@ 2016-03-02  2:27 ` Rob Herring
  2016-03-02 18:49   ` atull
  0 siblings, 1 reply; 8+ messages in thread
From: Rob Herring @ 2016-03-02  2:27 UTC (permalink / raw)
  To: Alan Tull
  Cc: Pantelis Antoniou, Frank Rowand, Grant Likely, devicetree,
	linux-kernel, Moritz Fischer, Pantelis Antoniou, Alan Tull,
	Dinh Nguyen

On Fri, Feb 26, 2016 at 3:44 PM, Alan Tull <atull@opensource.altera.com> 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.
>
> This is distinct from reconfig notifiers that are
> post-apply or post-remove and which issue very granular
> notifications without providing access to the context
> of a whole overlay.
>
> The following 4 notificatons are issued:
>   OF_OVERLAY_PRE_APPLY
>   OF_OVERLAY_POST_APPLY
>   OF_OVERLAY_PRE_REMOVE
>   OF_OVERLAY_POST_REMOVE
>
> In the case of pre-apply notification, if the notifier
> returns error, the overlay will be rejected.
>
> This patch exports two functions for registering/unregistering
> notifications:
>   of_overlay_notifier_register(struct notifier_block *nb)
>   of_overlay_notifier_unregister(struct notifier_block *nb)
>
> The notification data includes pointers to the overlay
> target and the overlay:
>
> struct of_overlay_notify_data {
>        struct device_node *overlay;
>        struct device_node *target;
> };
>
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> ---
> v2: add missing 'static inline' in of.h
> ---
>  drivers/of/overlay.c |   47 ++++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/of.h   |   25 +++++++++++++++++++++++++
>  2 files changed, 71 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 8225081..a26f0ed 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -56,6 +56,41 @@ struct of_overlay {
>  static int of_overlay_apply_one(struct of_overlay *ov,
>                 struct device_node *target, const struct device_node *overlay);
>
> +static BLOCKING_NOTIFIER_HEAD(of_overlay_chain);
> +
> +int of_overlay_notifier_register(struct notifier_block *nb)
> +{
> +       return blocking_notifier_chain_register(&of_overlay_chain, nb);
> +}
> +EXPORT_SYMBOL_GPL(of_overlay_notifier_register);
> +
> +int of_overlay_notifier_unregister(struct notifier_block *nb)
> +{
> +       return blocking_notifier_chain_unregister(&of_overlay_chain, nb);
> +}
> +EXPORT_SYMBOL_GPL(of_overlay_notifier_unregister);
> +
> +static int of_overlay_notify(struct of_overlay *ov,
> +                            enum of_overlay_notify_action action)
> +{
> +       struct of_overlay_notify_data nd;
> +       int i, ret;
> +
> +       for (i = 0; i < ov->count; i++) {
> +               struct of_overlay_info *ovinfo = &ov->ovinfo_tab[i];
> +
> +               nd.target = ovinfo->target;
> +               nd.overlay = ovinfo->overlay;
> +
> +               ret = blocking_notifier_call_chain(&of_overlay_chain,
> +                                                  action, &nd);
> +               if (ret)
> +                       return notifier_to_errno(ret);
> +       }
> +
> +       return 0;
> +}
> +
>  static int of_overlay_apply_single_property(struct of_overlay *ov,
>                 struct device_node *target, struct property *prop)
>  {
> @@ -370,6 +405,13 @@ int of_overlay_create(struct device_node *tree)
>                 goto err_free_idr;
>         }
>
> +       err = of_overlay_notify(ov, OF_OVERLAY_PRE_APPLY);
> +       if (err < 0) {
> +               pr_err("%s: Pre-apply notifier failed (err=%d)\n",
> +                      __func__, err);
> +               goto err_free_idr;
> +       }
> +
>         /* apply the overlay */
>         err = of_overlay_apply(ov);
>         if (err) {
> @@ -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.

>
>         return id;
> @@ -509,9 +553,10 @@ int of_overlay_destroy(int id)
>                 goto out;
>         }
>
> -
> +       of_overlay_notify(ov, OF_OVERLAY_PRE_REMOVE);
>         list_del(&ov->node);
>         __of_changeset_revert(&ov->cset);
> +       of_overlay_notify(ov, OF_OVERLAY_POST_REMOVE);
>         of_free_overlay_info(ov);
>         idr_remove(&ov_idr, id);
>         of_changeset_destroy(&ov->cset);
> diff --git a/include/linux/of.h b/include/linux/of.h
> index dc6e396..b79e1c5 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -1083,6 +1083,21 @@ int of_overlay_create(struct device_node *tree);
>  int of_overlay_destroy(int id);
>  int of_overlay_destroy_all(void);
>
> +enum of_overlay_notify_action {
> +       OF_OVERLAY_PRE_APPLY,
> +       OF_OVERLAY_POST_APPLY,
> +       OF_OVERLAY_PRE_REMOVE,
> +       OF_OVERLAY_POST_REMOVE,
> +};
> +
> +struct of_overlay_notify_data {
> +       struct device_node *overlay;
> +       struct device_node *target;
> +};

Both of these should be outside the ifdef. Otherwise, the !OF_OVERLAY
case will not compile.

> +
> +int of_overlay_notifier_register(struct notifier_block *nb);
> +int of_overlay_notifier_unregister(struct notifier_block *nb);
> +
>  #else
>
>  static inline int of_overlay_create(struct device_node *tree)
> @@ -1100,6 +1115,16 @@ static inline int of_overlay_destroy_all(void)
>         return -ENOTSUPP;
>  }
>
> +static inline int of_overlay_notifier_register(struct notifier_block *nb)
> +{
> +       return 0;
> +}
> +
> +static inline int of_overlay_notifier_unregister(struct notifier_block *nb)
> +{
> +       return 0;
> +}
> +
>  #endif
>
>  #endif /* _LINUX_OF_H */
> --
> 1.7.9.5
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] of/overlay: add of overlay notifications
  2016-03-02  2:27 ` Rob Herring
@ 2016-03-02 18:49   ` atull
  2016-03-03 14:24     ` Rob Herring
  0 siblings, 1 reply; 8+ messages in thread
From: atull @ 2016-03-02 18:49 UTC (permalink / raw)
  To: Rob Herring
  Cc: Pantelis Antoniou, Frank Rowand, Grant Likely, devicetree,
	linux-kernel, Moritz Fischer, Pantelis Antoniou, Alan Tull,
	Dinh Nguyen

On Wed, 2 Mar 2016, Rob Herring wrote:

> On Fri, Feb 26, 2016 at 3:44 PM, Alan Tull <atull@opensource.altera.com> 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.
> >
> > This is distinct from reconfig notifiers that are
> > post-apply or post-remove and which issue very granular
> > notifications without providing access to the context
> > of a whole overlay.
> >
> > The following 4 notificatons are issued:
> >   OF_OVERLAY_PRE_APPLY
> >   OF_OVERLAY_POST_APPLY
> >   OF_OVERLAY_PRE_REMOVE
> >   OF_OVERLAY_POST_REMOVE
> >
> > In the case of pre-apply notification, if the notifier
> > returns error, the overlay will be rejected.
> >
> > This patch exports two functions for registering/unregistering
> > notifications:
> >   of_overlay_notifier_register(struct notifier_block *nb)
> >   of_overlay_notifier_unregister(struct notifier_block *nb)
> >
> > The notification data includes pointers to the overlay
> > target and the overlay:
> >
> > struct of_overlay_notify_data {
> >        struct device_node *overlay;
> >        struct device_node *target;
> > };
> >
> > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> > ---
> > v2: add missing 'static inline' in of.h
> > ---
> >  drivers/of/overlay.c |   47 ++++++++++++++++++++++++++++++++++++++++++++++-
> >  include/linux/of.h   |   25 +++++++++++++++++++++++++
> >  2 files changed, 71 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> > index 8225081..a26f0ed 100644
> > --- a/drivers/of/overlay.c
> > +++ b/drivers/of/overlay.c
> > @@ -56,6 +56,41 @@ struct of_overlay {
> >  static int of_overlay_apply_one(struct of_overlay *ov,
> >                 struct device_node *target, const struct device_node *overlay);
> >
> > +static BLOCKING_NOTIFIER_HEAD(of_overlay_chain);
> > +
> > +int of_overlay_notifier_register(struct notifier_block *nb)
> > +{
> > +       return blocking_notifier_chain_register(&of_overlay_chain, nb);
> > +}
> > +EXPORT_SYMBOL_GPL(of_overlay_notifier_register);
> > +
> > +int of_overlay_notifier_unregister(struct notifier_block *nb)
> > +{
> > +       return blocking_notifier_chain_unregister(&of_overlay_chain, nb);
> > +}
> > +EXPORT_SYMBOL_GPL(of_overlay_notifier_unregister);
> > +
> > +static int of_overlay_notify(struct of_overlay *ov,
> > +                            enum of_overlay_notify_action action)
> > +{
> > +       struct of_overlay_notify_data nd;
> > +       int i, ret;
> > +
> > +       for (i = 0; i < ov->count; i++) {
> > +               struct of_overlay_info *ovinfo = &ov->ovinfo_tab[i];
> > +
> > +               nd.target = ovinfo->target;
> > +               nd.overlay = ovinfo->overlay;
> > +
> > +               ret = blocking_notifier_call_chain(&of_overlay_chain,
> > +                                                  action, &nd);
> > +               if (ret)
> > +                       return notifier_to_errno(ret);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  static int of_overlay_apply_single_property(struct of_overlay *ov,
> >                 struct device_node *target, struct property *prop)
> >  {
> > @@ -370,6 +405,13 @@ int of_overlay_create(struct device_node *tree)
> >                 goto err_free_idr;
> >         }
> >
> > +       err = of_overlay_notify(ov, OF_OVERLAY_PRE_APPLY);
> > +       if (err < 0) {
> > +               pr_err("%s: Pre-apply notifier failed (err=%d)\n",
> > +                      __func__, err);
> > +               goto err_free_idr;
> > +       }
> > +
> >         /* apply the overlay */
> >         err = of_overlay_apply(ov);
> >         if (err) {
> > @@ -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.

> >
> >         return id;
> > @@ -509,9 +553,10 @@ int of_overlay_destroy(int id)
> >                 goto out;
> >         }
> >
> > -
> > +       of_overlay_notify(ov, OF_OVERLAY_PRE_REMOVE);
> >         list_del(&ov->node);
> >         __of_changeset_revert(&ov->cset);
> > +       of_overlay_notify(ov, OF_OVERLAY_POST_REMOVE);
> >         of_free_overlay_info(ov);
> >         idr_remove(&ov_idr, id);
> >         of_changeset_destroy(&ov->cset);
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index dc6e396..b79e1c5 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -1083,6 +1083,21 @@ int of_overlay_create(struct device_node *tree);
> >  int of_overlay_destroy(int id);
> >  int of_overlay_destroy_all(void);
> >
> > +enum of_overlay_notify_action {
> > +       OF_OVERLAY_PRE_APPLY,
> > +       OF_OVERLAY_POST_APPLY,
> > +       OF_OVERLAY_PRE_REMOVE,
> > +       OF_OVERLAY_POST_REMOVE,
> > +};
> > +
> > +struct of_overlay_notify_data {
> > +       struct device_node *overlay;
> > +       struct device_node *target;
> > +};
> 
> Both of these should be outside the ifdef. Otherwise, the !OF_OVERLAY
> case will not compile.

OK

> 
> > +
> > +int of_overlay_notifier_register(struct notifier_block *nb);
> > +int of_overlay_notifier_unregister(struct notifier_block *nb);
> > +
> >  #else
> >
> >  static inline int of_overlay_create(struct device_node *tree)
> > @@ -1100,6 +1115,16 @@ static inline int of_overlay_destroy_all(void)
> >         return -ENOTSUPP;
> >  }
> >
> > +static inline int of_overlay_notifier_register(struct notifier_block *nb)
> > +{
> > +       return 0;
> > +}
> > +
> > +static inline int of_overlay_notifier_unregister(struct notifier_block *nb)
> > +{
> > +       return 0;
> > +}
> > +
> >  #endif
> >
> >  #endif /* _LINUX_OF_H */
> > --
> > 1.7.9.5
> >
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] of/overlay: add of overlay notifications
  2016-03-02 18:49   ` atull
@ 2016-03-03 14:24     ` Rob Herring
  2016-03-03 15:11       ` atull
  0 siblings, 1 reply; 8+ messages in thread
From: Rob Herring @ 2016-03-03 14:24 UTC (permalink / raw)
  To: atull
  Cc: Pantelis Antoniou, Frank Rowand, Grant Likely, devicetree,
	linux-kernel, Moritz Fischer, Pantelis Antoniou, Alan Tull,
	Dinh Nguyen

On Wed, Mar 2, 2016 at 12:49 PM, atull <atull@opensource.altera.com> wrote:
> On Wed, 2 Mar 2016, Rob Herring wrote:
>
>> On Fri, Feb 26, 2016 at 3:44 PM, Alan Tull <atull@opensource.altera.com> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] of/overlay: add of overlay notifications
  2016-03-03 14:24     ` Rob Herring
@ 2016-03-03 15:11       ` atull
  0 siblings, 0 replies; 8+ messages in thread
From: atull @ 2016-03-03 15:11 UTC (permalink / raw)
  To: Rob Herring
  Cc: Pantelis Antoniou, Frank Rowand, Grant Likely, devicetree,
	linux-kernel, Moritz Fischer, Pantelis Antoniou, Alan Tull,
	Dinh Nguyen

On Thu, 3 Mar 2016, Rob Herring wrote:

> On Wed, Mar 2, 2016 at 12:49 PM, atull <atull@opensource.altera.com> wrote:
> > On Wed, 2 Mar 2016, Rob Herring wrote:
> >
> >> On Fri, Feb 26, 2016 at 3:44 PM, Alan Tull <atull@opensource.altera.com> 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
> 

OK

Alan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] of/overlay: add of overlay notifications
  2016-02-25 23:36 Alan Tull
  2016-02-26  0:03 ` kbuild test robot
@ 2016-02-26  1:14 ` atull
  1 sibling, 0 replies; 8+ messages in thread
From: atull @ 2016-02-26  1:14 UTC (permalink / raw)
  To: Rob Herring, Pantelis Antoniou
  Cc: Frank Rowand, Grant Likely, devicetree, linux-kernel,
	Moritz Fischer, Pantelis Antoniou, Alan Tull, Dinh Nguyen

On Thu, 25 Feb 2016, Alan Tull 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.
> 
> This is distinct from reconfig notifiers that are
> post-apply or post-remove and which issue very granular
> notifications without providing access to the context
> of a whole overlay.
> 
> The following 4 notificatons are issued:
>   OF_OVERLAY_PRE_APPLY
>   OF_OVERLAY_POST_APPLY
>   OF_OVERLAY_PRE_REMOVE
>   OF_OVERLAY_POST_REMOVE
> 
> In the case of pre-apply notification, if the notifier
> returns error, the overlay will be rejected.
> 
> This patch exports two functions for registering/unregistering
> notifications:
>   of_overlay_notifier_register(struct notifier_block *nb)
>   of_overlay_notifier_unregister(struct notifier_block *nb)
> 
> The notification data includes pointers to the overlay
> target and the overlay:
> 
> struct of_overlay_notify_data {
>        struct device_node *overlay;
>        struct device_node *target;
> };
> 
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> ---
>  drivers/of/overlay.c |   47 ++++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/of.h   |   25 +++++++++++++++++++++++++
>  2 files changed, 71 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 8225081..a26f0ed 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -56,6 +56,41 @@ struct of_overlay {
>  static int of_overlay_apply_one(struct of_overlay *ov,
>  		struct device_node *target, const struct device_node *overlay);
>  
> +static BLOCKING_NOTIFIER_HEAD(of_overlay_chain);
> +
> +int of_overlay_notifier_register(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_register(&of_overlay_chain, nb);
> +}
> +EXPORT_SYMBOL_GPL(of_overlay_notifier_register);
> +
> +int of_overlay_notifier_unregister(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_unregister(&of_overlay_chain, nb);
> +}
> +EXPORT_SYMBOL_GPL(of_overlay_notifier_unregister);
> +
> +static int of_overlay_notify(struct of_overlay *ov,
> +			     enum of_overlay_notify_action action)
> +{
> +	struct of_overlay_notify_data nd;
> +	int i, ret;
> +
> +	for (i = 0; i < ov->count; i++) {
> +		struct of_overlay_info *ovinfo = &ov->ovinfo_tab[i];
> +
> +		nd.target = ovinfo->target;
> +		nd.overlay = ovinfo->overlay;
> +
> +		ret = blocking_notifier_call_chain(&of_overlay_chain,
> +						   action, &nd);
> +		if (ret)
> +			return notifier_to_errno(ret);
> +	}
> +
> +	return 0;
> +}
> +
>  static int of_overlay_apply_single_property(struct of_overlay *ov,
>  		struct device_node *target, struct property *prop)
>  {
> @@ -370,6 +405,13 @@ int of_overlay_create(struct device_node *tree)
>  		goto err_free_idr;
>  	}
>  
> +	err = of_overlay_notify(ov, OF_OVERLAY_PRE_APPLY);
> +	if (err < 0) {
> +		pr_err("%s: Pre-apply notifier failed (err=%d)\n",
> +		       __func__, err);
> +		goto err_free_idr;
> +	}
> +
>  	/* apply the overlay */
>  	err = of_overlay_apply(ov);
>  	if (err) {
> @@ -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);
>  
>  	return id;
> @@ -509,9 +553,10 @@ int of_overlay_destroy(int id)
>  		goto out;
>  	}
>  
> -
> +	of_overlay_notify(ov, OF_OVERLAY_PRE_REMOVE);
>  	list_del(&ov->node);
>  	__of_changeset_revert(&ov->cset);
> +	of_overlay_notify(ov, OF_OVERLAY_POST_REMOVE);
>  	of_free_overlay_info(ov);
>  	idr_remove(&ov_idr, id);
>  	of_changeset_destroy(&ov->cset);
> diff --git a/include/linux/of.h b/include/linux/of.h
> index dc6e396..e70a686 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -1083,6 +1083,21 @@ int of_overlay_create(struct device_node *tree);
>  int of_overlay_destroy(int id);
>  int of_overlay_destroy_all(void);
>  
> +enum of_overlay_notify_action {
> +	OF_OVERLAY_PRE_APPLY,
> +	OF_OVERLAY_POST_APPLY,
> +	OF_OVERLAY_PRE_REMOVE,
> +	OF_OVERLAY_POST_REMOVE,
> +};
> +
> +struct of_overlay_notify_data {
> +	struct device_node *overlay;
> +	struct device_node *target;
> +};
> +
> +int of_overlay_notifier_register(struct notifier_block *nb);
> +int of_overlay_notifier_unregister(struct notifier_block *nb);
> +
>  #else
>  
>  static inline int of_overlay_create(struct device_node *tree)
> @@ -1100,6 +1115,16 @@ static inline int of_overlay_destroy_all(void)
>  	return -ENOTSUPP;
>  }
>  
> +int of_overlay_notifier_register(struct notifier_block *nb)

Needs 'static inline'

> +{
> +	return 0;
> +}
> +
> +int of_overlay_notifier_unregister(struct notifier_block *nb)

Needs 'static inline'

Alan

> +{
> +	return 0;
> +}
> +
>  #endif
>  
>  #endif /* _LINUX_OF_H */
> -- 
> 1.7.9.5
> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] of/overlay: add of overlay notifications
  2016-02-25 23:36 Alan Tull
@ 2016-02-26  0:03 ` kbuild test robot
  2016-02-26  1:14 ` atull
  1 sibling, 0 replies; 8+ messages in thread
From: kbuild test robot @ 2016-02-26  0:03 UTC (permalink / raw)
  To: Alan Tull
  Cc: kbuild-all, Rob Herring, Pantelis Antoniou, Frank Rowand,
	Grant Likely, devicetree, linux-kernel, Moritz Fischer,
	Pantelis Antoniou, Alan Tull, Dinh Nguyen, Alan Tull

[-- Attachment #1: Type: text/plain, Size: 2454 bytes --]

Hi Alan,

[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.5-rc5 next-20160225]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Alan-Tull/of-overlay-add-of-overlay-notifications/20160226-074214
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux for-next
config: i386-tinyconfig (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   arch/x86/kernel/setup.o: In function `of_overlay_notifier_register':
>> setup.c:(.text+0x3): multiple definition of `of_overlay_notifier_register'
   arch/x86/kernel/irq.o:irq.c:(.text+0x1): first defined here
   arch/x86/kernel/setup.o: In function `of_overlay_notifier_unregister':
>> setup.c:(.text+0x6): multiple definition of `of_overlay_notifier_unregister'
   arch/x86/kernel/irq.o:irq.c:(.text+0x4): first defined here
   arch/x86/kernel/irqinit.o: In function `of_overlay_notifier_register':
   irqinit.c:(.text+0x0): multiple definition of `of_overlay_notifier_register'
   arch/x86/kernel/irq.o:irq.c:(.text+0x1): first defined here
   arch/x86/kernel/irqinit.o: In function `of_overlay_notifier_unregister':
   irqinit.c:(.text+0x3): multiple definition of `of_overlay_notifier_unregister'
   arch/x86/kernel/irq.o:irq.c:(.text+0x4): first defined here
   arch/x86/kernel/rtc.o: In function `of_overlay_notifier_register':
   rtc.c:(.text+0x43): multiple definition of `of_overlay_notifier_register'
   arch/x86/kernel/irq.o:irq.c:(.text+0x1): first defined here
   arch/x86/kernel/rtc.o: In function `of_overlay_notifier_unregister':
   rtc.c:(.text+0x46): multiple definition of `of_overlay_notifier_unregister'
   arch/x86/kernel/irq.o:irq.c:(.text+0x4): first defined here
   arch/x86/kernel/sysfb.o: In function `of_overlay_notifier_register':
   sysfb.c:(.text+0x0): multiple definition of `of_overlay_notifier_register'
   arch/x86/kernel/irq.o:irq.c:(.text+0x1): first defined here
   arch/x86/kernel/sysfb.o: In function `of_overlay_notifier_unregister':
   sysfb.c:(.text+0x3): multiple definition of `of_overlay_notifier_unregister'
   arch/x86/kernel/irq.o:irq.c:(.text+0x4): first defined here

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6205 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] of/overlay: add of overlay notifications
@ 2016-02-25 23:36 Alan Tull
  2016-02-26  0:03 ` kbuild test robot
  2016-02-26  1:14 ` atull
  0 siblings, 2 replies; 8+ messages in thread
From: Alan Tull @ 2016-02-25 23:36 UTC (permalink / raw)
  To: Rob Herring, Pantelis Antoniou
  Cc: Frank Rowand, Grant Likely, devicetree, linux-kernel,
	Moritz Fischer, Pantelis Antoniou, Alan Tull, Dinh Nguyen,
	Alan Tull

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.

This is distinct from reconfig notifiers that are
post-apply or post-remove and which issue very granular
notifications without providing access to the context
of a whole overlay.

The following 4 notificatons are issued:
  OF_OVERLAY_PRE_APPLY
  OF_OVERLAY_POST_APPLY
  OF_OVERLAY_PRE_REMOVE
  OF_OVERLAY_POST_REMOVE

In the case of pre-apply notification, if the notifier
returns error, the overlay will be rejected.

This patch exports two functions for registering/unregistering
notifications:
  of_overlay_notifier_register(struct notifier_block *nb)
  of_overlay_notifier_unregister(struct notifier_block *nb)

The notification data includes pointers to the overlay
target and the overlay:

struct of_overlay_notify_data {
       struct device_node *overlay;
       struct device_node *target;
};

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
 drivers/of/overlay.c |   47 ++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/of.h   |   25 +++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 8225081..a26f0ed 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -56,6 +56,41 @@ struct of_overlay {
 static int of_overlay_apply_one(struct of_overlay *ov,
 		struct device_node *target, const struct device_node *overlay);
 
+static BLOCKING_NOTIFIER_HEAD(of_overlay_chain);
+
+int of_overlay_notifier_register(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&of_overlay_chain, nb);
+}
+EXPORT_SYMBOL_GPL(of_overlay_notifier_register);
+
+int of_overlay_notifier_unregister(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&of_overlay_chain, nb);
+}
+EXPORT_SYMBOL_GPL(of_overlay_notifier_unregister);
+
+static int of_overlay_notify(struct of_overlay *ov,
+			     enum of_overlay_notify_action action)
+{
+	struct of_overlay_notify_data nd;
+	int i, ret;
+
+	for (i = 0; i < ov->count; i++) {
+		struct of_overlay_info *ovinfo = &ov->ovinfo_tab[i];
+
+		nd.target = ovinfo->target;
+		nd.overlay = ovinfo->overlay;
+
+		ret = blocking_notifier_call_chain(&of_overlay_chain,
+						   action, &nd);
+		if (ret)
+			return notifier_to_errno(ret);
+	}
+
+	return 0;
+}
+
 static int of_overlay_apply_single_property(struct of_overlay *ov,
 		struct device_node *target, struct property *prop)
 {
@@ -370,6 +405,13 @@ int of_overlay_create(struct device_node *tree)
 		goto err_free_idr;
 	}
 
+	err = of_overlay_notify(ov, OF_OVERLAY_PRE_APPLY);
+	if (err < 0) {
+		pr_err("%s: Pre-apply notifier failed (err=%d)\n",
+		       __func__, err);
+		goto err_free_idr;
+	}
+
 	/* apply the overlay */
 	err = of_overlay_apply(ov);
 	if (err) {
@@ -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);
 
 	return id;
@@ -509,9 +553,10 @@ int of_overlay_destroy(int id)
 		goto out;
 	}
 
-
+	of_overlay_notify(ov, OF_OVERLAY_PRE_REMOVE);
 	list_del(&ov->node);
 	__of_changeset_revert(&ov->cset);
+	of_overlay_notify(ov, OF_OVERLAY_POST_REMOVE);
 	of_free_overlay_info(ov);
 	idr_remove(&ov_idr, id);
 	of_changeset_destroy(&ov->cset);
diff --git a/include/linux/of.h b/include/linux/of.h
index dc6e396..e70a686 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1083,6 +1083,21 @@ int of_overlay_create(struct device_node *tree);
 int of_overlay_destroy(int id);
 int of_overlay_destroy_all(void);
 
+enum of_overlay_notify_action {
+	OF_OVERLAY_PRE_APPLY,
+	OF_OVERLAY_POST_APPLY,
+	OF_OVERLAY_PRE_REMOVE,
+	OF_OVERLAY_POST_REMOVE,
+};
+
+struct of_overlay_notify_data {
+	struct device_node *overlay;
+	struct device_node *target;
+};
+
+int of_overlay_notifier_register(struct notifier_block *nb);
+int of_overlay_notifier_unregister(struct notifier_block *nb);
+
 #else
 
 static inline int of_overlay_create(struct device_node *tree)
@@ -1100,6 +1115,16 @@ static inline int of_overlay_destroy_all(void)
 	return -ENOTSUPP;
 }
 
+int of_overlay_notifier_register(struct notifier_block *nb)
+{
+	return 0;
+}
+
+int of_overlay_notifier_unregister(struct notifier_block *nb)
+{
+	return 0;
+}
+
 #endif
 
 #endif /* _LINUX_OF_H */
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-03-03 15:13 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-26 21:44 [PATCH] of/overlay: add of overlay notifications Alan Tull
2016-03-02  2:27 ` Rob Herring
2016-03-02 18:49   ` atull
2016-03-03 14:24     ` Rob Herring
2016-03-03 15:11       ` atull
  -- strict thread matches above, loose matches on Subject: below --
2016-02-25 23:36 Alan Tull
2016-02-26  0:03 ` kbuild test robot
2016-02-26  1:14 ` atull

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).