* [RFC 0/2] of: Add whitelist @ 2017-11-27 20:58 Alan Tull 2017-11-27 20:58 ` [RFC 1/2] of: overlay: add whitelist Alan Tull ` (2 more replies) 0 siblings, 3 replies; 20+ messages in thread From: Alan Tull @ 2017-11-27 20:58 UTC (permalink / raw) To: Rob Herring, Frank Rowand, Pantelis Antoniou Cc: Moritz Fischer, Alan Tull, devicetree, linux-kernel, linux-fpga Here's a proposal for a whitelist to lock down the dynamic device tree. For an overlay to be accepted, all of its targets are required to be on a target node whitelist. Currently the only way I have to get on the whitelist is calling a function to add a node. That works for fpga regions, but I think other uses will need a way of having adding specific nodes from the base device tree, such as by adding a property like 'allow-overlay;' or 'allow-overlay = "okay";' If that is acceptable, I could use some advice on where that particular code should go. Alan Alan Tull (2): of: overlay: add whitelist fpga: of region: add of-fpga-region to whitelist drivers/fpga/of-fpga-region.c | 9 ++++++ drivers/of/overlay.c | 73 +++++++++++++++++++++++++++++++++++++++++++ include/linux/of.h | 12 +++++++ 3 files changed, 94 insertions(+) -- 2.7.4 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC 1/2] of: overlay: add whitelist 2017-11-27 20:58 [RFC 0/2] of: Add whitelist Alan Tull @ 2017-11-27 20:58 ` Alan Tull 2017-11-28 15:15 ` Rob Herring 2017-11-27 20:58 ` [RFC 2/2] fpga: of region: add of-fpga-region to whitelist Alan Tull 2017-11-29 9:20 ` [RFC 0/2] of: Add whitelist Frank Rowand 2 siblings, 1 reply; 20+ messages in thread From: Alan Tull @ 2017-11-27 20:58 UTC (permalink / raw) To: Rob Herring, Frank Rowand, Pantelis Antoniou Cc: Moritz Fischer, Alan Tull, devicetree, linux-kernel, linux-fpga Add simple whitelist. When an overlay is submitted, if any target in the overlay is not in the whitelist, the overlay is rejected. Drivers that support dynamic configuration can register their device node with: int of_add_whitelist_node(struct device_node *np) and remove themselves with: void of_remove_whitelist_node(struct device_node *np) Signed-off-by: Alan Tull <atull@kernel.org> --- drivers/of/overlay.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/of.h | 12 +++++++++ 2 files changed, 85 insertions(+) diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c index c150abb..5f952a1 100644 --- a/drivers/of/overlay.c +++ b/drivers/of/overlay.c @@ -21,6 +21,7 @@ #include <linux/slab.h> #include <linux/err.h> #include <linux/idr.h> +#include <linux/spinlock.h> #include "of_private.h" @@ -646,6 +647,74 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs) kfree(ovcs); } +/* lock for adding/removing device nodes to the whitelist */ +static spinlock_t whitelist_lock; + +static struct list_head whitelist_list = LIST_HEAD_INIT(whitelist_list); + +struct dt_overlay_whitelist { + struct device_node *np; + struct list_head node; +}; + +int of_add_whitelist_node(struct device_node *np) +{ + unsigned long flags; + struct dt_overlay_whitelist *wln; + + wln = kzalloc(sizeof(*wln), GFP_KERNEL); + if (!wln) + return -ENOMEM; + + wln->np = np; + + spin_lock_irqsave(&whitelist_lock, flags); + list_add(&wln->node, &whitelist_list); + spin_unlock_irqrestore(&whitelist_lock, flags); + + return 0; +} +EXPORT_SYMBOL_GPL(of_add_whitelist_node); + +void of_remove_whitelist_node(struct device_node *np) +{ + struct dt_overlay_whitelist *wln; + unsigned long flags; + + list_for_each_entry(wln, &whitelist_list, node) { + if (np == wln->np) { + spin_lock_irqsave(&whitelist_lock, flags); + list_del(&wln->node); + spin_unlock_irqrestore(&whitelist_lock, flags); + kfree(wln); + return; + } + } +} +EXPORT_SYMBOL_GPL(of_remove_whitelist_node); + +static int of_check_whitelist(struct overlay_changeset *ovcs) +{ + struct dt_overlay_whitelist *wln; + struct device_node *target; + int i; + + for (i = 0; i < ovcs->count; i++) { + target = ovcs->fragments[i].target; + if (!of_node_cmp(target->name, "__symbols__")) + continue; + + list_for_each_entry(wln, &whitelist_list, node) + if (target == wln->np) + break; + + if (target != wln->np) + return -ENODEV; + } + + return 0; +} + /** * of_overlay_apply() - Create and apply an overlay changeset * @tree: Expanded overlay device tree @@ -717,6 +786,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id) if (ret) goto err_free_overlay_changeset; + ret = of_check_whitelist(ovcs); + if (ret) + goto err_free_overlay_changeset; + ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY); if (ret) { pr_err("overlay changeset pre-apply notify error %d\n", ret); diff --git a/include/linux/of.h b/include/linux/of.h index d3dea1d..5bf652a1 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -1364,6 +1364,9 @@ int of_overlay_remove_all(void); int of_overlay_notifier_register(struct notifier_block *nb); int of_overlay_notifier_unregister(struct notifier_block *nb); +int of_add_whitelist_node(struct device_node *np); +void of_remove_whitelist_node(struct device_node *np); + #else static inline int of_overlay_apply(struct device_node *tree, int *ovcs_id) @@ -1391,6 +1394,15 @@ static inline int of_overlay_notifier_unregister(struct notifier_block *nb) return 0; } +static inline int of_add_whitelist_node(struct device_node *np) +{ + return 0; +} + +static inline void of_remove_whitelist_node(struct device_node *np) +{ +} + #endif #endif /* _LINUX_OF_H */ -- 2.7.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [RFC 1/2] of: overlay: add whitelist 2017-11-27 20:58 ` [RFC 1/2] of: overlay: add whitelist Alan Tull @ 2017-11-28 15:15 ` Rob Herring 2017-11-28 19:26 ` Alan Tull 0 siblings, 1 reply; 20+ messages in thread From: Rob Herring @ 2017-11-28 15:15 UTC (permalink / raw) To: Alan Tull Cc: Frank Rowand, Pantelis Antoniou, Moritz Fischer, devicetree, linux-kernel, linux-fpga On Mon, Nov 27, 2017 at 02:58:03PM -0600, Alan Tull wrote: > Add simple whitelist. When an overlay is submitted, if any target in > the overlay is not in the whitelist, the overlay is rejected. Drivers > that support dynamic configuration can register their device node with: > > int of_add_whitelist_node(struct device_node *np) > > and remove themselves with: > > void of_remove_whitelist_node(struct device_node *np) I think these should be named for what they do, not how it is implemented. > > Signed-off-by: Alan Tull <atull@kernel.org> > --- > drivers/of/overlay.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/of.h | 12 +++++++++ > 2 files changed, 85 insertions(+) > > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c > index c150abb..5f952a1 100644 > --- a/drivers/of/overlay.c > +++ b/drivers/of/overlay.c > @@ -21,6 +21,7 @@ > #include <linux/slab.h> > #include <linux/err.h> > #include <linux/idr.h> > +#include <linux/spinlock.h> > > #include "of_private.h" > > @@ -646,6 +647,74 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs) > kfree(ovcs); > } > > +/* lock for adding/removing device nodes to the whitelist */ > +static spinlock_t whitelist_lock; > + > +static struct list_head whitelist_list = LIST_HEAD_INIT(whitelist_list); > + > +struct dt_overlay_whitelist { > + struct device_node *np; > + struct list_head node; > +}; Can't we just add a flags bit in device_node.flags? That would be much simpler. > + > +int of_add_whitelist_node(struct device_node *np) > +{ > + unsigned long flags; > + struct dt_overlay_whitelist *wln; > + > + wln = kzalloc(sizeof(*wln), GFP_KERNEL); > + if (!wln) > + return -ENOMEM; > + > + wln->np = np; > + > + spin_lock_irqsave(&whitelist_lock, flags); > + list_add(&wln->node, &whitelist_list); > + spin_unlock_irqrestore(&whitelist_lock, flags); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(of_add_whitelist_node); > + > +void of_remove_whitelist_node(struct device_node *np) > +{ > + struct dt_overlay_whitelist *wln; > + unsigned long flags; > + > + list_for_each_entry(wln, &whitelist_list, node) { > + if (np == wln->np) { > + spin_lock_irqsave(&whitelist_lock, flags); > + list_del(&wln->node); > + spin_unlock_irqrestore(&whitelist_lock, flags); > + kfree(wln); > + return; > + } > + } > +} > +EXPORT_SYMBOL_GPL(of_remove_whitelist_node); > + > +static int of_check_whitelist(struct overlay_changeset *ovcs) > +{ > + struct dt_overlay_whitelist *wln; > + struct device_node *target; > + int i; > + > + for (i = 0; i < ovcs->count; i++) { > + target = ovcs->fragments[i].target; > + if (!of_node_cmp(target->name, "__symbols__")) > + continue; > + > + list_for_each_entry(wln, &whitelist_list, node) > + if (target == wln->np) > + break; > + > + if (target != wln->np) > + return -ENODEV; > + } > + > + return 0; > +} > + > /** > * of_overlay_apply() - Create and apply an overlay changeset > * @tree: Expanded overlay device tree > @@ -717,6 +786,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id) > if (ret) > goto err_free_overlay_changeset; > > + ret = of_check_whitelist(ovcs); > + if (ret) > + goto err_free_overlay_changeset; This will break you until the next patch and breaks any other users. I think this is now just the unittest as tilcdc overlay is getting removed. You have to make this chunk the last patch in the series. Rob ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 1/2] of: overlay: add whitelist 2017-11-28 15:15 ` Rob Herring @ 2017-11-28 19:26 ` Alan Tull 2017-11-29 9:25 ` Frank Rowand 0 siblings, 1 reply; 20+ messages in thread From: Alan Tull @ 2017-11-28 19:26 UTC (permalink / raw) To: Rob Herring Cc: Frank Rowand, Pantelis Antoniou, Moritz Fischer, devicetree, linux-kernel, linux-fpga On Tue, Nov 28, 2017 at 9:15 AM, Rob Herring <robh@kernel.org> wrote: > On Mon, Nov 27, 2017 at 02:58:03PM -0600, Alan Tull wrote: >> Add simple whitelist. When an overlay is submitted, if any target in >> the overlay is not in the whitelist, the overlay is rejected. Drivers >> that support dynamic configuration can register their device node with: >> >> int of_add_whitelist_node(struct device_node *np) >> >> and remove themselves with: >> >> void of_remove_whitelist_node(struct device_node *np) > > I think these should be named for what they do, not how it is > implemented. Sure, such as of_node_overlay_enable and of_node_overlay_disable? > >> >> Signed-off-by: Alan Tull <atull@kernel.org> >> --- >> drivers/of/overlay.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/of.h | 12 +++++++++ >> 2 files changed, 85 insertions(+) >> >> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c >> index c150abb..5f952a1 100644 >> --- a/drivers/of/overlay.c >> +++ b/drivers/of/overlay.c >> @@ -21,6 +21,7 @@ >> #include <linux/slab.h> >> #include <linux/err.h> >> #include <linux/idr.h> >> +#include <linux/spinlock.h> >> >> #include "of_private.h" >> >> @@ -646,6 +647,74 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs) >> kfree(ovcs); >> } >> >> +/* lock for adding/removing device nodes to the whitelist */ >> +static spinlock_t whitelist_lock; >> + >> +static struct list_head whitelist_list = LIST_HEAD_INIT(whitelist_list); >> + >> +struct dt_overlay_whitelist { >> + struct device_node *np; >> + struct list_head node; >> +}; > > Can't we just add a flags bit in device_node.flags? That would be much > simpler. Yes, much simpler. Such as: #define OF_OVERLAY_ENABLED 5 /* allow DT overlay targeting this node */ > >> + >> +int of_add_whitelist_node(struct device_node *np) >> +{ >> + unsigned long flags; >> + struct dt_overlay_whitelist *wln; >> + >> + wln = kzalloc(sizeof(*wln), GFP_KERNEL); >> + if (!wln) >> + return -ENOMEM; >> + >> + wln->np = np; >> + >> + spin_lock_irqsave(&whitelist_lock, flags); >> + list_add(&wln->node, &whitelist_list); >> + spin_unlock_irqrestore(&whitelist_lock, flags); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(of_add_whitelist_node); >> + >> +void of_remove_whitelist_node(struct device_node *np) >> +{ >> + struct dt_overlay_whitelist *wln; >> + unsigned long flags; >> + >> + list_for_each_entry(wln, &whitelist_list, node) { >> + if (np == wln->np) { >> + spin_lock_irqsave(&whitelist_lock, flags); >> + list_del(&wln->node); >> + spin_unlock_irqrestore(&whitelist_lock, flags); >> + kfree(wln); >> + return; >> + } >> + } >> +} >> +EXPORT_SYMBOL_GPL(of_remove_whitelist_node); >> + >> +static int of_check_whitelist(struct overlay_changeset *ovcs) >> +{ >> + struct dt_overlay_whitelist *wln; >> + struct device_node *target; >> + int i; >> + >> + for (i = 0; i < ovcs->count; i++) { >> + target = ovcs->fragments[i].target; >> + if (!of_node_cmp(target->name, "__symbols__")) >> + continue; >> + >> + list_for_each_entry(wln, &whitelist_list, node) >> + if (target == wln->np) >> + break; >> + >> + if (target != wln->np) >> + return -ENODEV; >> + } >> + >> + return 0; >> +} >> + >> /** >> * of_overlay_apply() - Create and apply an overlay changeset >> * @tree: Expanded overlay device tree >> @@ -717,6 +786,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id) >> if (ret) >> goto err_free_overlay_changeset; >> >> + ret = of_check_whitelist(ovcs); >> + if (ret) >> + goto err_free_overlay_changeset; > > This will break you until the next patch and breaks any other users. I > think this is now just the unittest as tilcdc overlay is getting > removed. > > You have to make this chunk the last patch in the series. I'd rather squash the two patches. In either case, the contents of second patch are dependent on stuff in char-misc-testing today, so it won't be able to apply yet on linux-next or anywhere else. Thanks Alan > > Rob ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 1/2] of: overlay: add whitelist 2017-11-28 19:26 ` Alan Tull @ 2017-11-29 9:25 ` Frank Rowand 0 siblings, 0 replies; 20+ messages in thread From: Frank Rowand @ 2017-11-29 9:25 UTC (permalink / raw) To: Alan Tull, Rob Herring Cc: Pantelis Antoniou, Moritz Fischer, devicetree, linux-kernel, linux-fpga On 11/28/17 14:26, Alan Tull wrote: > On Tue, Nov 28, 2017 at 9:15 AM, Rob Herring <robh@kernel.org> wrote: >> On Mon, Nov 27, 2017 at 02:58:03PM -0600, Alan Tull wrote: >>> Add simple whitelist. When an overlay is submitted, if any target in >>> the overlay is not in the whitelist, the overlay is rejected. Drivers >>> that support dynamic configuration can register their device node with: >>> >>> int of_add_whitelist_node(struct device_node *np) >>> >>> and remove themselves with: >>> >>> void of_remove_whitelist_node(struct device_node *np) >> >> I think these should be named for what they do, not how it is >> implemented. > > Sure, such as of_node_overlay_enable and of_node_overlay_disable? of_allow_overlay_on_node(), of_disallow_overlay_on_node()? > >> >>> >>> Signed-off-by: Alan Tull <atull@kernel.org> >>> --- >>> drivers/of/overlay.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/of.h | 12 +++++++++ >>> 2 files changed, 85 insertions(+) >>> >>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c >>> index c150abb..5f952a1 100644 >>> --- a/drivers/of/overlay.c >>> +++ b/drivers/of/overlay.c >>> @@ -21,6 +21,7 @@ >>> #include <linux/slab.h> >>> #include <linux/err.h> >>> #include <linux/idr.h> >>> +#include <linux/spinlock.h> >>> >>> #include "of_private.h" >>> >>> @@ -646,6 +647,74 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs) >>> kfree(ovcs); >>> } >>> >>> +/* lock for adding/removing device nodes to the whitelist */ >>> +static spinlock_t whitelist_lock; >>> + >>> +static struct list_head whitelist_list = LIST_HEAD_INIT(whitelist_list); >>> + >>> +struct dt_overlay_whitelist { >>> + struct device_node *np; >>> + struct list_head node; >>> +}; >> >> Can't we just add a flags bit in device_node.flags? That would be much >> simpler. > > Yes, much simpler. Such as: > > #define OF_OVERLAY_ENABLED 5 /* allow DT overlay targeting this node */ > >> >>> + >>> +int of_add_whitelist_node(struct device_node *np) >>> +{ >>> + unsigned long flags; >>> + struct dt_overlay_whitelist *wln; >>> + >>> + wln = kzalloc(sizeof(*wln), GFP_KERNEL); >>> + if (!wln) >>> + return -ENOMEM; >>> + >>> + wln->np = np; >>> + >>> + spin_lock_irqsave(&whitelist_lock, flags); >>> + list_add(&wln->node, &whitelist_list); >>> + spin_unlock_irqrestore(&whitelist_lock, flags); >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL_GPL(of_add_whitelist_node); >>> + >>> +void of_remove_whitelist_node(struct device_node *np) >>> +{ >>> + struct dt_overlay_whitelist *wln; >>> + unsigned long flags; >>> + >>> + list_for_each_entry(wln, &whitelist_list, node) { >>> + if (np == wln->np) { >>> + spin_lock_irqsave(&whitelist_lock, flags); >>> + list_del(&wln->node); >>> + spin_unlock_irqrestore(&whitelist_lock, flags); >>> + kfree(wln); >>> + return; >>> + } >>> + } >>> +} >>> +EXPORT_SYMBOL_GPL(of_remove_whitelist_node); >>> + >>> +static int of_check_whitelist(struct overlay_changeset *ovcs) >>> +{ >>> + struct dt_overlay_whitelist *wln; >>> + struct device_node *target; >>> + int i; >>> + >>> + for (i = 0; i < ovcs->count; i++) { >>> + target = ovcs->fragments[i].target; >>> + if (!of_node_cmp(target->name, "__symbols__")) >>> + continue; >>> + >>> + list_for_each_entry(wln, &whitelist_list, node) >>> + if (target == wln->np) >>> + break; >>> + >>> + if (target != wln->np) >>> + return -ENODEV; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> /** >>> * of_overlay_apply() - Create and apply an overlay changeset >>> * @tree: Expanded overlay device tree >>> @@ -717,6 +786,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id) >>> if (ret) >>> goto err_free_overlay_changeset; >>> >>> + ret = of_check_whitelist(ovcs); >>> + if (ret) >>> + goto err_free_overlay_changeset; >> >> This will break you until the next patch and breaks any other users. I >> think this is now just the unittest as tilcdc overlay is getting >> removed. >> >> You have to make this chunk the last patch in the series. > > I'd rather squash the two patches. In either case, the contents of > second patch are dependent on stuff in char-misc-testing today, so it > won't be able to apply yet on linux-next or anywhere else. > > Thanks > Alan > >> >> Rob > ^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC 2/2] fpga: of region: add of-fpga-region to whitelist 2017-11-27 20:58 [RFC 0/2] of: Add whitelist Alan Tull 2017-11-27 20:58 ` [RFC 1/2] of: overlay: add whitelist Alan Tull @ 2017-11-27 20:58 ` Alan Tull 2017-11-29 9:20 ` [RFC 0/2] of: Add whitelist Frank Rowand 2 siblings, 0 replies; 20+ messages in thread From: Alan Tull @ 2017-11-27 20:58 UTC (permalink / raw) To: Rob Herring, Frank Rowand, Pantelis Antoniou Cc: Moritz Fischer, Alan Tull, devicetree, linux-kernel, linux-fpga During each FPGA region's probe, add to whitelist. Remove during driver remove. Signed-off-by: Alan Tull <atull@kernel.org> --- drivers/fpga/of-fpga-region.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c index 7dfaa95..abb57a9 100644 --- a/drivers/fpga/of-fpga-region.c +++ b/drivers/fpga/of-fpga-region.c @@ -435,6 +435,10 @@ static int of_fpga_region_probe(struct platform_device *pdev) /* Specify how to get bridges for this type of region. */ region->get_bridges = of_fpga_region_get_bridges; + ret = of_add_whitelist_node(np); + if (ret) + goto eprobe_wl_err; + ret = fpga_region_register(region); if (ret) goto eprobe_mgr_put; @@ -447,6 +451,8 @@ static int of_fpga_region_probe(struct platform_device *pdev) return 0; eprobe_mgr_put: + of_remove_whitelist_node(np); +eprobe_wl_err: fpga_mgr_put(mgr); return ret; } @@ -454,7 +460,10 @@ static int of_fpga_region_probe(struct platform_device *pdev) static int of_fpga_region_remove(struct platform_device *pdev) { struct fpga_region *region = platform_get_drvdata(pdev); + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + of_remove_whitelist_node(np); fpga_region_unregister(region); fpga_mgr_put(region->mgr); -- 2.7.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [RFC 0/2] of: Add whitelist 2017-11-27 20:58 [RFC 0/2] of: Add whitelist Alan Tull 2017-11-27 20:58 ` [RFC 1/2] of: overlay: add whitelist Alan Tull 2017-11-27 20:58 ` [RFC 2/2] fpga: of region: add of-fpga-region to whitelist Alan Tull @ 2017-11-29 9:20 ` Frank Rowand 2017-11-29 13:31 ` Rob Herring ` (2 more replies) 2 siblings, 3 replies; 20+ messages in thread From: Frank Rowand @ 2017-11-29 9:20 UTC (permalink / raw) To: Alan Tull, Rob Herring, Pantelis Antoniou Cc: Moritz Fischer, devicetree, linux-kernel, linux-fpga On 11/27/17 15:58, Alan Tull wrote: > Here's a proposal for a whitelist to lock down the dynamic device tree. > > For an overlay to be accepted, all of its targets are required to be > on a target node whitelist. > > Currently the only way I have to get on the whitelist is calling a > function to add a node. That works for fpga regions, but I think > other uses will need a way of having adding specific nodes from the > base device tree, such as by adding a property like 'allow-overlay;' > or 'allow-overlay = "okay";' If that is acceptable, I could use some > advice on where that particular code should go. > > Alan > > Alan Tull (2): > of: overlay: add whitelist > fpga: of region: add of-fpga-region to whitelist > > drivers/fpga/of-fpga-region.c | 9 ++++++ > drivers/of/overlay.c | 73 +++++++++++++++++++++++++++++++++++++++++++ > include/linux/of.h | 12 +++++++ > 3 files changed, 94 insertions(+) > The plan was to use connectors to restrict where an overlay could be applied. I would prefer not to have multiple methods for accomplishing the same thing unless there is a compelling reason to do so. -Frnank ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/2] of: Add whitelist 2017-11-29 9:20 ` [RFC 0/2] of: Add whitelist Frank Rowand @ 2017-11-29 13:31 ` Rob Herring 2017-11-29 16:11 ` Alan Tull 2017-11-30 12:18 ` Frank Rowand 2017-11-29 22:47 ` Frank Rowand 2017-11-30 0:58 ` Frank Rowand 2 siblings, 2 replies; 20+ messages in thread From: Rob Herring @ 2017-11-29 13:31 UTC (permalink / raw) To: Frank Rowand Cc: Alan Tull, Pantelis Antoniou, Moritz Fischer, devicetree, linux-kernel, linux-fpga On Wed, Nov 29, 2017 at 3:20 AM, Frank Rowand <frowand.list@gmail.com> wrote: > On 11/27/17 15:58, Alan Tull wrote: >> Here's a proposal for a whitelist to lock down the dynamic device tree. >> >> For an overlay to be accepted, all of its targets are required to be >> on a target node whitelist. >> >> Currently the only way I have to get on the whitelist is calling a >> function to add a node. That works for fpga regions, but I think >> other uses will need a way of having adding specific nodes from the >> base device tree, such as by adding a property like 'allow-overlay;' >> or 'allow-overlay = "okay";' If that is acceptable, I could use some >> advice on where that particular code should go. >> >> Alan >> >> Alan Tull (2): >> of: overlay: add whitelist >> fpga: of region: add of-fpga-region to whitelist >> >> drivers/fpga/of-fpga-region.c | 9 ++++++ >> drivers/of/overlay.c | 73 +++++++++++++++++++++++++++++++++++++++++++ >> include/linux/of.h | 12 +++++++ >> 3 files changed, 94 insertions(+) >> > > The plan was to use connectors to restrict where an overlay could be applied. > I would prefer not to have multiple methods for accomplishing the same thing > unless there is a compelling reason to do so. Connector nodes need a mechanism to enable themselves, too. I don't think connector nodes are going to solve every usecase. Rob ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/2] of: Add whitelist 2017-11-29 13:31 ` Rob Herring @ 2017-11-29 16:11 ` Alan Tull 2017-11-30 12:46 ` Frank Rowand 2017-11-30 12:18 ` Frank Rowand 1 sibling, 1 reply; 20+ messages in thread From: Alan Tull @ 2017-11-29 16:11 UTC (permalink / raw) To: Rob Herring Cc: Frank Rowand, Pantelis Antoniou, Moritz Fischer, devicetree, linux-kernel, linux-fpga On Wed, Nov 29, 2017 at 7:31 AM, Rob Herring <robh+dt@kernel.org> wrote: > On Wed, Nov 29, 2017 at 3:20 AM, Frank Rowand <frowand.list@gmail.com> wrote: >> On 11/27/17 15:58, Alan Tull wrote: >>> Here's a proposal for a whitelist to lock down the dynamic device tree. >>> >>> For an overlay to be accepted, all of its targets are required to be >>> on a target node whitelist. >>> >>> Currently the only way I have to get on the whitelist is calling a >>> function to add a node. That works for fpga regions, but I think >>> other uses will need a way of having adding specific nodes from the >>> base device tree, such as by adding a property like 'allow-overlay;' >>> or 'allow-overlay = "okay";' If that is acceptable, I could use some >>> advice on where that particular code should go. >>> >>> Alan >>> >>> Alan Tull (2): >>> of: overlay: add whitelist >>> fpga: of region: add of-fpga-region to whitelist >>> >>> drivers/fpga/of-fpga-region.c | 9 ++++++ >>> drivers/of/overlay.c | 73 +++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/of.h | 12 +++++++ >>> 3 files changed, 94 insertions(+) >>> >> >> The plan was to use connectors to restrict where an overlay could be applied. >> I would prefer not to have multiple methods for accomplishing the same thing >> unless there is a compelling reason to do so. > > Connector nodes need a mechanism to enable themselves, too. I don't > think connector nodes are going to solve every usecase. > > Rob The two methods I'm suggesting are intended to handle different cases. There will exist some drivers that by their nature will want every instance to be enabled for overlays, such as fpga regions. The other case is where drivers could support overlays but that's not the widespread use for them. So no need to enable every instance of that driver for overlays. In that case the DT property provides some granularity, only enabling overlays for specific instances of that driver, leaving the rest of the DT locked down. If we only want one method, I would choose having the DT property only and not exporting the functions. Users would have to add the property for every FPGA region but that's not really painful. This would have the benefit of still keeping the DT locked down unless someone specifically wanted to enable some regions for overlays for their particular use. Alan ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/2] of: Add whitelist 2017-11-29 16:11 ` Alan Tull @ 2017-11-30 12:46 ` Frank Rowand 2017-12-05 16:33 ` Alan Tull 0 siblings, 1 reply; 20+ messages in thread From: Frank Rowand @ 2017-11-30 12:46 UTC (permalink / raw) To: Alan Tull, Rob Herring Cc: Pantelis Antoniou, Moritz Fischer, devicetree, linux-kernel, linux-fpga On 11/29/17 11:11, Alan Tull wrote: > On Wed, Nov 29, 2017 at 7:31 AM, Rob Herring <robh+dt@kernel.org> wrote: >> On Wed, Nov 29, 2017 at 3:20 AM, Frank Rowand <frowand.list@gmail.com> wrote: >>> On 11/27/17 15:58, Alan Tull wrote: >>>> Here's a proposal for a whitelist to lock down the dynamic device tree. >>>> >>>> For an overlay to be accepted, all of its targets are required to be >>>> on a target node whitelist. >>>> >>>> Currently the only way I have to get on the whitelist is calling a >>>> function to add a node. That works for fpga regions, but I think >>>> other uses will need a way of having adding specific nodes from the >>>> base device tree, such as by adding a property like 'allow-overlay;' >>>> or 'allow-overlay = "okay";' If that is acceptable, I could use some >>>> advice on where that particular code should go. >>>> >>>> Alan >>>> >>>> Alan Tull (2): >>>> of: overlay: add whitelist >>>> fpga: of region: add of-fpga-region to whitelist >>>> >>>> drivers/fpga/of-fpga-region.c | 9 ++++++ >>>> drivers/of/overlay.c | 73 +++++++++++++++++++++++++++++++++++++++++++ >>>> include/linux/of.h | 12 +++++++ >>>> 3 files changed, 94 insertions(+) >>>> >>> >>> The plan was to use connectors to restrict where an overlay could be applied. >>> I would prefer not to have multiple methods for accomplishing the same thing >>> unless there is a compelling reason to do so. >> >> Connector nodes need a mechanism to enable themselves, too. I don't >> think connector nodes are going to solve every usecase. >> >> Rob > > The two methods I'm suggesting are intended to handle different cases. > There will exist some drivers that by their nature will want every > instance to be enabled for overlays, such as fpga regions. The other > case is where drivers could support overlays but that's not the > widespread use for them. So no need to enable every instance of that > driver for overlays. I understand what the paragraph, to this point, means. But I had to read it several times to understand it because the way the concept is phrased clashed with my mental model. The device node is not an instance of a driver, which is why I was getting confused. (Yes, I do understand that the paragraph is talking about multiple device nodes that are bound to the same driver, but my mental model is tied to the device node, not to the driver.) If each of the device nodes in question is a connector, then each of the nodes will bind to a connector driver, based on the value of the compatible property. (This is of course a theoretical assumption on my part since the connectors are not yet implemented.) If the connector node is an fpga, or an fpga region (I may be getting my terminology wrong here - please correct as needed) then an fpga overlay could be applied to the node. If I understand what you are saying, there will be some fpga connector nodes for which the usage at a given moment might be programmed to function in a manner that will not be described by an overlay, but at a different moment in time may be programmed in a way that needs to be described by an overlay. So there may be some times that it is valid to apply an overlay to the connector node and times that it is not valid to apply an overlay to the connector node. Is my understanding correct, or am I still confused? -Frank > In that case the DT property provides some > granularity, only enabling overlays for specific instances of that > driver, leaving the rest of the DT locked down.> > If we only want one method, I would choose having the DT property only > and not exporting the functions. Users would have to add the property > for every FPGA region but that's not really painful. This would have > the benefit of still keeping the DT locked down unless someone > specifically wanted to enable some regions for overlays for their > particular use. > > Alan > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/2] of: Add whitelist 2017-11-30 12:46 ` Frank Rowand @ 2017-12-05 16:33 ` Alan Tull 2017-12-06 11:56 ` Frank Rowand 0 siblings, 1 reply; 20+ messages in thread From: Alan Tull @ 2017-12-05 16:33 UTC (permalink / raw) To: Frank Rowand Cc: Rob Herring, Pantelis Antoniou, Moritz Fischer, devicetree, linux-kernel, linux-fpga On Thu, Nov 30, 2017 at 6:46 AM, Frank Rowand <frowand.list@gmail.com> wrote: > On 11/29/17 11:11, Alan Tull wrote: >> On Wed, Nov 29, 2017 at 7:31 AM, Rob Herring <robh+dt@kernel.org> wrote: >>> On Wed, Nov 29, 2017 at 3:20 AM, Frank Rowand <frowand.list@gmail.com> wrote: >>>> On 11/27/17 15:58, Alan Tull wrote: >>>>> Here's a proposal for a whitelist to lock down the dynamic device tree. >>>>> >>>>> For an overlay to be accepted, all of its targets are required to be >>>>> on a target node whitelist. >>>>> >>>>> Currently the only way I have to get on the whitelist is calling a >>>>> function to add a node. That works for fpga regions, but I think >>>>> other uses will need a way of having adding specific nodes from the >>>>> base device tree, such as by adding a property like 'allow-overlay;' >>>>> or 'allow-overlay = "okay";' If that is acceptable, I could use some >>>>> advice on where that particular code should go. >>>>> >>>>> Alan >>>>> >>>>> Alan Tull (2): >>>>> of: overlay: add whitelist >>>>> fpga: of region: add of-fpga-region to whitelist >>>>> >>>>> drivers/fpga/of-fpga-region.c | 9 ++++++ >>>>> drivers/of/overlay.c | 73 +++++++++++++++++++++++++++++++++++++++++++ >>>>> include/linux/of.h | 12 +++++++ >>>>> 3 files changed, 94 insertions(+) >>>>> >>>> >>>> The plan was to use connectors to restrict where an overlay could be applied. >>>> I would prefer not to have multiple methods for accomplishing the same thing >>>> unless there is a compelling reason to do so. >>> >>> Connector nodes need a mechanism to enable themselves, too. I don't >>> think connector nodes are going to solve every usecase. >>> >>> Rob >> >> The two methods I'm suggesting are intended to handle different cases. >> There will exist some drivers that by their nature will want every >> instance to be enabled for overlays, such as fpga regions. The other >> case is where drivers could support overlays but that's not the >> widespread use for them. So no need to enable every instance of that >> driver for overlays. > > I understand what the paragraph, to this point, means. But I had to > read it several times to understand it because the way the concept is > phrased clashed with my mental model. Hi Frank, I see where my explanation is confusing things. I was talking about two methods for marking a node as being a valid target for an overlay (use a function or add a DT property). I'll drop the idea of using a DT property to enable a node for overlays and only focus on my proposal of a function to enable nodes. > > The device node is not an instance of a driver, which is why I was > getting confused. (Yes, I do understand that the paragraph is talking > about multiple device nodes that are bound to the same driver, but > my mental model is tied to the device node, not to the driver.) > > If each of the device nodes in question is a connector, then each of > the nodes will bind to a connector driver, based on the value of the > compatible property. (This is of course a theoretical assumption on > my part since the connectors are not yet implemented.) > > If the connector node is an fpga, or an fpga region (I may be getting > my terminology wrong here - please correct as needed) then an fpga > overlay could be applied to the node. We're still pre-connector currently, but yes I want to mark FPGA regions as being valid targets. Then I can use Pantelis' configfs interface to apply overlays while leaving the rest of the DT locked down. That's the FPGA use of this patch in the pre-connector era of things. > > If I understand what you are saying, there will be some fpga connector > nodes for which the usage at a given moment might be programmed to > function in a manner that will not be described by an overlay, but > at a different moment in time may be programmed in a way that needs > to be described by an overlay. So there may be some times that it > is valid to apply an overlay to the connector node and times that > it is not valid to apply an overlay to the connector node. I think connectors would likely always be valid targets (but I could be wrong) and other nodes would not be valid targets. The DT needs a way to mark some nodes as valid targets, currently it doesn't have a way of doing that. Every connector driver's probe could use this code to mark itself as a valid target. > > Is my understanding correct, or am I still confused? Hope that helps, sorry for the muddled explanation earlier. Alan > > -Frank > >> In that case the DT property provides some >> granularity, only enabling overlays for specific instances of that >> driver, leaving the rest of the DT locked down.> >> If we only want one method, I would choose having the DT property only >> and not exporting the functions. Users would have to add the property >> for every FPGA region but that's not really painful. This would have >> the benefit of still keeping the DT locked down unless someone >> specifically wanted to enable some regions for overlays for their >> particular use. >> >> Alan >> > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/2] of: Add whitelist 2017-12-05 16:33 ` Alan Tull @ 2017-12-06 11:56 ` Frank Rowand 2017-12-07 19:22 ` Alan Tull 0 siblings, 1 reply; 20+ messages in thread From: Frank Rowand @ 2017-12-06 11:56 UTC (permalink / raw) To: Alan Tull Cc: Rob Herring, Pantelis Antoniou, Moritz Fischer, devicetree, linux-kernel, linux-fpga On 12/05/17 11:33, Alan Tull wrote: > On Thu, Nov 30, 2017 at 6:46 AM, Frank Rowand <frowand.list@gmail.com> wrote: >> On 11/29/17 11:11, Alan Tull wrote: >>> On Wed, Nov 29, 2017 at 7:31 AM, Rob Herring <robh+dt@kernel.org> wrote: >>>> On Wed, Nov 29, 2017 at 3:20 AM, Frank Rowand <frowand.list@gmail.com> wrote: >>>>> On 11/27/17 15:58, Alan Tull wrote: >>>>>> Here's a proposal for a whitelist to lock down the dynamic device tree. >>>>>> >>>>>> For an overlay to be accepted, all of its targets are required to be >>>>>> on a target node whitelist. >>>>>> >>>>>> Currently the only way I have to get on the whitelist is calling a >>>>>> function to add a node. That works for fpga regions, but I think >>>>>> other uses will need a way of having adding specific nodes from the >>>>>> base device tree, such as by adding a property like 'allow-overlay;' >>>>>> or 'allow-overlay = "okay";' If that is acceptable, I could use some >>>>>> advice on where that particular code should go. >>>>>> >>>>>> Alan >>>>>> >>>>>> Alan Tull (2): >>>>>> of: overlay: add whitelist >>>>>> fpga: of region: add of-fpga-region to whitelist >>>>>> >>>>>> drivers/fpga/of-fpga-region.c | 9 ++++++ >>>>>> drivers/of/overlay.c | 73 +++++++++++++++++++++++++++++++++++++++++++ >>>>>> include/linux/of.h | 12 +++++++ >>>>>> 3 files changed, 94 insertions(+) >>>>>> >>>>> >>>>> The plan was to use connectors to restrict where an overlay could be applied. >>>>> I would prefer not to have multiple methods for accomplishing the same thing >>>>> unless there is a compelling reason to do so. >>>> >>>> Connector nodes need a mechanism to enable themselves, too. I don't >>>> think connector nodes are going to solve every usecase. >>>> >>>> Rob >>> >>> The two methods I'm suggesting are intended to handle different cases. >>> There will exist some drivers that by their nature will want every >>> instance to be enabled for overlays, such as fpga regions. The other >>> case is where drivers could support overlays but that's not the >>> widespread use for them. So no need to enable every instance of that >>> driver for overlays. >> >> I understand what the paragraph, to this point, means. But I had to >> read it several times to understand it because the way the concept is >> phrased clashed with my mental model. > > Hi Frank, > > I see where my explanation is confusing things. I was talking about > two methods for marking a node as being a valid target for an overlay > (use a function or add a DT property). I'll drop the idea of using a > DT property to enable a node for overlays and only focus on my > proposal of a function to enable nodes. > >> >> The device node is not an instance of a driver, which is why I was >> getting confused. (Yes, I do understand that the paragraph is talking >> about multiple device nodes that are bound to the same driver, but >> my mental model is tied to the device node, not to the driver.) >> >> If each of the device nodes in question is a connector, then each of >> the nodes will bind to a connector driver, based on the value of the >> compatible property. (This is of course a theoretical assumption on >> my part since the connectors are not yet implemented.) >> >> If the connector node is an fpga, or an fpga region (I may be getting >> my terminology wrong here - please correct as needed) then an fpga >> overlay could be applied to the node. > > We're still pre-connector currently, but yes I want to mark FPGA > regions as being valid targets. Then I can use Pantelis' configfs > interface to apply overlays while leaving the rest of the DT locked > down. That's the FPGA use of this patch in the pre-connector era of > things. > >> >> If I understand what you are saying, there will be some fpga connector >> nodes for which the usage at a given moment might be programmed to >> function in a manner that will not be described by an overlay, but >> at a different moment in time may be programmed in a way that needs >> to be described by an overlay. So there may be some times that it >> is valid to apply an overlay to the connector node and times that >> it is not valid to apply an overlay to the connector node. > > I think connectors would likely always be valid targets (but I could > be wrong) and other nodes would not be valid targets. The DT needs a > way to mark some nodes as valid targets, currently it doesn't have a > way of doing that. Every connector driver's probe could use this code > to mark itself as a valid target. > >> >> Is my understanding correct, or am I still confused? > > Hope that helps, sorry for the muddled explanation earlier. No need to be sorry, I always value what you have to say, and usually become more educated from reading what you write. We still seem to be talking at cross purposes. It seems that the model that you are describing is driver centric. My model is node centric. Once we figure out what the connector implementation and architecture are, it might be the case that each connector node has a driver bound to it, and that driver is able to tell the devicetree core code that the node that it is bound to is a valid place to apply an overlay. But I currently think that the core infrastructure code is what should recognize that a connector node is a valid place to apply an overlay. It _might_ even be the case the the connector architecture does not result in a driver being bound to the connector node. I would really prefer to get the connector architecture (and maybe also the implementation) before deciding how to handle the question of how to determine what nodes overlays can be appplied to. > > Alan > >> >> -Frank >> >>> In that case the DT property provides some >>> granularity, only enabling overlays for specific instances of that >>> driver, leaving the rest of the DT locked down.> >>> If we only want one method, I would choose having the DT property only >>> and not exporting the functions. Users would have to add the property >>> for every FPGA region but that's not really painful. This would have >>> the benefit of still keeping the DT locked down unless someone >>> specifically wanted to enable some regions for overlays for their >>> particular use. >>> >>> Alan >>> >> > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/2] of: Add whitelist 2017-12-06 11:56 ` Frank Rowand @ 2017-12-07 19:22 ` Alan Tull 0 siblings, 0 replies; 20+ messages in thread From: Alan Tull @ 2017-12-07 19:22 UTC (permalink / raw) To: Frank Rowand Cc: Rob Herring, Pantelis Antoniou, Moritz Fischer, devicetree, linux-kernel, linux-fpga On Wed, Dec 6, 2017 at 5:56 AM, Frank Rowand <frowand.list@gmail.com> wrote: > On 12/05/17 11:33, Alan Tull wrote: >> On Thu, Nov 30, 2017 at 6:46 AM, Frank Rowand <frowand.list@gmail.com> wrote: >>> On 11/29/17 11:11, Alan Tull wrote: >>>> On Wed, Nov 29, 2017 at 7:31 AM, Rob Herring <robh+dt@kernel.org> wrote: >>>>> On Wed, Nov 29, 2017 at 3:20 AM, Frank Rowand <frowand.list@gmail.com> wrote: >>>>>> On 11/27/17 15:58, Alan Tull wrote: >>>>>>> Here's a proposal for a whitelist to lock down the dynamic device tree. >>>>>>> >>>>>>> For an overlay to be accepted, all of its targets are required to be >>>>>>> on a target node whitelist. >>>>>>> >>>>>>> Currently the only way I have to get on the whitelist is calling a >>>>>>> function to add a node. That works for fpga regions, but I think >>>>>>> other uses will need a way of having adding specific nodes from the >>>>>>> base device tree, such as by adding a property like 'allow-overlay;' >>>>>>> or 'allow-overlay = "okay";' If that is acceptable, I could use some >>>>>>> advice on where that particular code should go. >>>>>>> >>>>>>> Alan >>>>>>> >>>>>>> Alan Tull (2): >>>>>>> of: overlay: add whitelist >>>>>>> fpga: of region: add of-fpga-region to whitelist >>>>>>> >>>>>>> drivers/fpga/of-fpga-region.c | 9 ++++++ >>>>>>> drivers/of/overlay.c | 73 +++++++++++++++++++++++++++++++++++++++++++ >>>>>>> include/linux/of.h | 12 +++++++ >>>>>>> 3 files changed, 94 insertions(+) >>>>>>> >>>>>> >>>>>> The plan was to use connectors to restrict where an overlay could be applied. >>>>>> I would prefer not to have multiple methods for accomplishing the same thing >>>>>> unless there is a compelling reason to do so. >>>>> >>>>> Connector nodes need a mechanism to enable themselves, too. I don't >>>>> think connector nodes are going to solve every usecase. >>>>> >>>>> Rob >>>> >>>> The two methods I'm suggesting are intended to handle different cases. >>>> There will exist some drivers that by their nature will want every >>>> instance to be enabled for overlays, such as fpga regions. The other >>>> case is where drivers could support overlays but that's not the >>>> widespread use for them. So no need to enable every instance of that >>>> driver for overlays. >>> >>> I understand what the paragraph, to this point, means. But I had to >>> read it several times to understand it because the way the concept is >>> phrased clashed with my mental model. >> >> Hi Frank, >> >> I see where my explanation is confusing things. I was talking about >> two methods for marking a node as being a valid target for an overlay >> (use a function or add a DT property). I'll drop the idea of using a >> DT property to enable a node for overlays and only focus on my >> proposal of a function to enable nodes. >> >>> >>> The device node is not an instance of a driver, which is why I was >>> getting confused. (Yes, I do understand that the paragraph is talking >>> about multiple device nodes that are bound to the same driver, but >>> my mental model is tied to the device node, not to the driver.) >>> >>> If each of the device nodes in question is a connector, then each of >>> the nodes will bind to a connector driver, based on the value of the >>> compatible property. (This is of course a theoretical assumption on >>> my part since the connectors are not yet implemented.) >>> >>> If the connector node is an fpga, or an fpga region (I may be getting >>> my terminology wrong here - please correct as needed) then an fpga >>> overlay could be applied to the node. >> >> We're still pre-connector currently, but yes I want to mark FPGA >> regions as being valid targets. Then I can use Pantelis' configfs >> interface to apply overlays while leaving the rest of the DT locked >> down. That's the FPGA use of this patch in the pre-connector era of >> things. >> >>> >>> If I understand what you are saying, there will be some fpga connector >>> nodes for which the usage at a given moment might be programmed to >>> function in a manner that will not be described by an overlay, but >>> at a different moment in time may be programmed in a way that needs >>> to be described by an overlay. So there may be some times that it >>> is valid to apply an overlay to the connector node and times that >>> it is not valid to apply an overlay to the connector node. >> >> I think connectors would likely always be valid targets (but I could >> be wrong) and other nodes would not be valid targets. The DT needs a >> way to mark some nodes as valid targets, currently it doesn't have a >> way of doing that. Every connector driver's probe could use this code >> to mark itself as a valid target. >> >>> >>> Is my understanding correct, or am I still confused? >> >> Hope that helps, sorry for the muddled explanation earlier. > > No need to be sorry, I always value what you have to say, and usually > become more educated from reading what you write. > > We still seem to be talking at cross purposes. It seems that the model > that you are describing is driver centric. My model is node centric. > > Once we figure out what the connector implementation and architecture > are, it might be the case that each connector node has a driver bound > to it, and that driver is able to tell the devicetree core code that > the node that it is bound to is a valid place to apply an overlay. > > But I currently think that the core infrastructure code is what > should recognize that a connector node is a valid place to apply > an overlay. It _might_ even be the case the the connector > architecture does not result in a driver being bound to the > connector node. > > I would really prefer to get the connector architecture (and > maybe also the implementation) before deciding how to handle > the question of how to determine what nodes overlays can be > appplied to. Hi Frank, I understand that you would want to wait until it is clear how connectors will be implemented. So this method may work for connectors or may not depending on how that all pans out. Thanks for the discussion! Alan > > >> >> Alan >> >>> >>> -Frank >>> >>>> In that case the DT property provides some >>>> granularity, only enabling overlays for specific instances of that >>>> driver, leaving the rest of the DT locked down.> >>>> If we only want one method, I would choose having the DT property only >>>> and not exporting the functions. Users would have to add the property >>>> for every FPGA region but that's not really painful. This would have >>>> the benefit of still keeping the DT locked down unless someone >>>> specifically wanted to enable some regions for overlays for their >>>> particular use. >>>> >>>> Alan >>>> >>> >> > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/2] of: Add whitelist 2017-11-29 13:31 ` Rob Herring 2017-11-29 16:11 ` Alan Tull @ 2017-11-30 12:18 ` Frank Rowand 2017-12-05 16:55 ` Alan Tull 1 sibling, 1 reply; 20+ messages in thread From: Frank Rowand @ 2017-11-30 12:18 UTC (permalink / raw) To: Rob Herring Cc: Alan Tull, Pantelis Antoniou, Moritz Fischer, devicetree, linux-kernel, linux-fpga On 11/29/17 08:31, Rob Herring wrote: > On Wed, Nov 29, 2017 at 3:20 AM, Frank Rowand <frowand.list@gmail.com> wrote: >> On 11/27/17 15:58, Alan Tull wrote: >>> Here's a proposal for a whitelist to lock down the dynamic device tree. >>> >>> For an overlay to be accepted, all of its targets are required to be >>> on a target node whitelist. >>> >>> Currently the only way I have to get on the whitelist is calling a >>> function to add a node. That works for fpga regions, but I think >>> other uses will need a way of having adding specific nodes from the >>> base device tree, such as by adding a property like 'allow-overlay;' >>> or 'allow-overlay = "okay";' If that is acceptable, I could use some >>> advice on where that particular code should go. >>> >>> Alan >>> >>> Alan Tull (2): >>> of: overlay: add whitelist >>> fpga: of region: add of-fpga-region to whitelist >>> >>> drivers/fpga/of-fpga-region.c | 9 ++++++ >>> drivers/of/overlay.c | 73 +++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/of.h | 12 +++++++ >>> 3 files changed, 94 insertions(+) >>> >> >> The plan was to use connectors to restrict where an overlay could be applied. >> I would prefer not to have multiple methods for accomplishing the same thing >> unless there is a compelling reason to do so. > > Connector nodes need a mechanism to enable themselves, too. I don't > think connector nodes are going to solve every usecase. > > Rob > The overlay code related to connectors does not exist yet, so my comment is going to be theoretical. I would expect the overlay code to check that the target of the overlay fragment is a connector node, so there is no need to explicitly "enable" applying an overlay to a connector node. -Frank ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/2] of: Add whitelist 2017-11-30 12:18 ` Frank Rowand @ 2017-12-05 16:55 ` Alan Tull 2017-12-06 11:47 ` Frank Rowand 0 siblings, 1 reply; 20+ messages in thread From: Alan Tull @ 2017-12-05 16:55 UTC (permalink / raw) To: Frank Rowand Cc: Rob Herring, Pantelis Antoniou, Moritz Fischer, devicetree, linux-kernel, linux-fpga On Thu, Nov 30, 2017 at 6:18 AM, Frank Rowand <frowand.list@gmail.com> wrote: > On 11/29/17 08:31, Rob Herring wrote: >> On Wed, Nov 29, 2017 at 3:20 AM, Frank Rowand <frowand.list@gmail.com> wrote: >>> On 11/27/17 15:58, Alan Tull wrote: >>>> Here's a proposal for a whitelist to lock down the dynamic device tree. >>>> >>>> For an overlay to be accepted, all of its targets are required to be >>>> on a target node whitelist. >>>> >>>> Currently the only way I have to get on the whitelist is calling a >>>> function to add a node. That works for fpga regions, but I think >>>> other uses will need a way of having adding specific nodes from the >>>> base device tree, such as by adding a property like 'allow-overlay;' >>>> or 'allow-overlay = "okay";' If that is acceptable, I could use some >>>> advice on where that particular code should go. >>>> >>>> Alan >>>> >>>> Alan Tull (2): >>>> of: overlay: add whitelist >>>> fpga: of region: add of-fpga-region to whitelist >>>> >>>> drivers/fpga/of-fpga-region.c | 9 ++++++ >>>> drivers/of/overlay.c | 73 +++++++++++++++++++++++++++++++++++++++++++ >>>> include/linux/of.h | 12 +++++++ >>>> 3 files changed, 94 insertions(+) >>>> >>> >>> The plan was to use connectors to restrict where an overlay could be applied. >>> I would prefer not to have multiple methods for accomplishing the same thing >>> unless there is a compelling reason to do so. >> >> Connector nodes need a mechanism to enable themselves, too. I don't >> think connector nodes are going to solve every usecase. >> >> Rob >> > > The overlay code related to connectors does not exist yet, so my comment > is going to be theoretical. > > I would expect the overlay code to check that the target of the overlay > fragment is a connector node, so there is no need to explicitly "enable" > applying an overlay to a connector node. This will depend on how connectors are implemented. My proposal in v1 is that device nodes can have a flag bit. If its not set, then an overlay that contains fragments that target that node can't be applied. There's probably other ways a connector node could be marked as different from other nodes, but a flag bit seems simple. The advantage to this scheme is that it gives me something I can use while connectors don't exist yet and it will still will be useful later for the implementation of connectors (giving connector drivers a way of marking their device nodes as valid targets). > > -Frank ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/2] of: Add whitelist 2017-12-05 16:55 ` Alan Tull @ 2017-12-06 11:47 ` Frank Rowand 0 siblings, 0 replies; 20+ messages in thread From: Frank Rowand @ 2017-12-06 11:47 UTC (permalink / raw) To: Alan Tull Cc: Rob Herring, Pantelis Antoniou, Moritz Fischer, devicetree, linux-kernel, linux-fpga On 12/05/17 11:55, Alan Tull wrote: > On Thu, Nov 30, 2017 at 6:18 AM, Frank Rowand <frowand.list@gmail.com> wrote: >> On 11/29/17 08:31, Rob Herring wrote: >>> On Wed, Nov 29, 2017 at 3:20 AM, Frank Rowand <frowand.list@gmail.com> wrote: >>>> On 11/27/17 15:58, Alan Tull wrote: >>>>> Here's a proposal for a whitelist to lock down the dynamic device tree. >>>>> >>>>> For an overlay to be accepted, all of its targets are required to be >>>>> on a target node whitelist. >>>>> >>>>> Currently the only way I have to get on the whitelist is calling a >>>>> function to add a node. That works for fpga regions, but I think >>>>> other uses will need a way of having adding specific nodes from the >>>>> base device tree, such as by adding a property like 'allow-overlay;' >>>>> or 'allow-overlay = "okay";' If that is acceptable, I could use some >>>>> advice on where that particular code should go. >>>>> >>>>> Alan >>>>> >>>>> Alan Tull (2): >>>>> of: overlay: add whitelist >>>>> fpga: of region: add of-fpga-region to whitelist >>>>> >>>>> drivers/fpga/of-fpga-region.c | 9 ++++++ >>>>> drivers/of/overlay.c | 73 +++++++++++++++++++++++++++++++++++++++++++ >>>>> include/linux/of.h | 12 +++++++ >>>>> 3 files changed, 94 insertions(+) >>>>> >>>> >>>> The plan was to use connectors to restrict where an overlay could be applied. >>>> I would prefer not to have multiple methods for accomplishing the same thing >>>> unless there is a compelling reason to do so. >>> >>> Connector nodes need a mechanism to enable themselves, too. I don't >>> think connector nodes are going to solve every usecase. >>> >>> Rob >>> >> >> The overlay code related to connectors does not exist yet, so my comment >> is going to be theoretical. >> >> I would expect the overlay code to check that the target of the overlay >> fragment is a connector node, so there is no need to explicitly "enable" >> applying an overlay to a connector node. > > This will depend on how connectors are implemented. My proposal in v1 > is that device nodes can have a flag bit. If its not set, then an > overlay that contains fragments that target that node can't be > applied. There's probably other ways a connector node could be marked > as different from other nodes, but a flag bit seems simple. The > advantage to this scheme is that it gives me something I can use while > connectors don't exist yet and it will still will be useful later for > the implementation of connectors (giving connector drivers a way of > marking their device nodes as valid targets). I think it is premature to add this code to the kernel when we don't have an agreed upon architecture for what we are trying to achieve. > >> >> -Frank > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/2] of: Add whitelist 2017-11-29 9:20 ` [RFC 0/2] of: Add whitelist Frank Rowand 2017-11-29 13:31 ` Rob Herring @ 2017-11-29 22:47 ` Frank Rowand 2017-11-30 14:39 ` Rob Herring 2017-11-30 0:58 ` Frank Rowand 2 siblings, 1 reply; 20+ messages in thread From: Frank Rowand @ 2017-11-29 22:47 UTC (permalink / raw) To: Alan Tull, Rob Herring, Pantelis Antoniou Cc: Moritz Fischer, devicetree, linux-kernel, linux-fpga On 11/29/17 04:20, Frank Rowand wrote: > On 11/27/17 15:58, Alan Tull wrote: >> Here's a proposal for a whitelist to lock down the dynamic device tree. >> >> For an overlay to be accepted, all of its targets are required to be >> on a target node whitelist. >> >> Currently the only way I have to get on the whitelist is calling a >> function to add a node. That works for fpga regions, but I think >> other uses will need a way of having adding specific nodes from the >> base device tree, such as by adding a property like 'allow-overlay;' >> or 'allow-overlay = "okay";' If that is acceptable, I could use some >> advice on where that particular code should go. >> >> Alan >> >> Alan Tull (2): >> of: overlay: add whitelist >> fpga: of region: add of-fpga-region to whitelist >> >> drivers/fpga/of-fpga-region.c | 9 ++++++ >> drivers/of/overlay.c | 73 +++++++++++++++++++++++++++++++++++++++++++ >> include/linux/of.h | 12 +++++++ >> 3 files changed, 94 insertions(+) >> > > The plan was to use connectors to restrict where an overlay could be applied. > I would prefer not to have multiple methods for accomplishing the same thing > unless there is a compelling reason to do so. Going back one level in my thinking, I don't think that having a driver mark a node as a location where an overlay fragment can be applied is serving a useful purpose. Any driver, including any driver loaded as a module, could mark a node as ok. I don't see how this is providing any meaningful restriction on where an overlay fragment can be applied. -Frank ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/2] of: Add whitelist 2017-11-29 22:47 ` Frank Rowand @ 2017-11-30 14:39 ` Rob Herring 2017-12-06 11:44 ` Frank Rowand 0 siblings, 1 reply; 20+ messages in thread From: Rob Herring @ 2017-11-30 14:39 UTC (permalink / raw) To: Frank Rowand Cc: Alan Tull, Rob Herring, Pantelis Antoniou, Moritz Fischer, devicetree, Linux Kernel Mailing List, linux-fpga On Wed, Nov 29, 2017 at 4:47 PM, Frank Rowand <frowand.list@gmail.com> wrote: > On 11/29/17 04:20, Frank Rowand wrote: >> On 11/27/17 15:58, Alan Tull wrote: >>> Here's a proposal for a whitelist to lock down the dynamic device tree. >>> >>> For an overlay to be accepted, all of its targets are required to be >>> on a target node whitelist. >>> >>> Currently the only way I have to get on the whitelist is calling a >>> function to add a node. That works for fpga regions, but I think >>> other uses will need a way of having adding specific nodes from the >>> base device tree, such as by adding a property like 'allow-overlay;' >>> or 'allow-overlay = "okay";' If that is acceptable, I could use some >>> advice on where that particular code should go. >>> >>> Alan >>> >>> Alan Tull (2): >>> of: overlay: add whitelist >>> fpga: of region: add of-fpga-region to whitelist >>> >>> drivers/fpga/of-fpga-region.c | 9 ++++++ >>> drivers/of/overlay.c | 73 +++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/of.h | 12 +++++++ >>> 3 files changed, 94 insertions(+) >>> >> >> The plan was to use connectors to restrict where an overlay could be applied. >> I would prefer not to have multiple methods for accomplishing the same thing >> unless there is a compelling reason to do so. > > Going back one level in my thinking, I don't think that having a driver mark > a node as a location where an overlay fragment can be applied is serving a > useful purpose. Any driver, including any driver loaded as a module, > could mark a node as ok. I don't see how this is providing any meaningful > restriction on where an overlay fragment can be applied. It serves to separate the setting of which nodes overlays can be applied to and the mechanism to apply them (checking permissions). The former can't be centralized and the latter can be. For example, something in the kernel enables overlays on a node or nodes, then the overlay is applied with configfs interface and no board specific code involved. My concern is not whether any kernel component can enable applying of overlays, but userspace. If it is a kernel component, then it is explicit. And an OOT kernel module doesn't count because there's no ABI guarantee there. I agree that this patch series alone is not all that useful with only in kernel users. It is only really interesting when we have a userspace interface. However, an implementation with a flag bit is so little code, I'm fine taking it now and not having to update all in kernel users when adding a userspace interface. Rob ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/2] of: Add whitelist 2017-11-30 14:39 ` Rob Herring @ 2017-12-06 11:44 ` Frank Rowand 0 siblings, 0 replies; 20+ messages in thread From: Frank Rowand @ 2017-12-06 11:44 UTC (permalink / raw) To: Rob Herring Cc: Alan Tull, Rob Herring, Pantelis Antoniou, Moritz Fischer, devicetree, Linux Kernel Mailing List, linux-fpga On 11/30/17 09:39, Rob Herring wrote: > On Wed, Nov 29, 2017 at 4:47 PM, Frank Rowand <frowand.list@gmail.com> wrote: >> On 11/29/17 04:20, Frank Rowand wrote: >>> On 11/27/17 15:58, Alan Tull wrote: >>>> Here's a proposal for a whitelist to lock down the dynamic device tree. >>>> >>>> For an overlay to be accepted, all of its targets are required to be >>>> on a target node whitelist. >>>> >>>> Currently the only way I have to get on the whitelist is calling a >>>> function to add a node. That works for fpga regions, but I think >>>> other uses will need a way of having adding specific nodes from the >>>> base device tree, such as by adding a property like 'allow-overlay;' >>>> or 'allow-overlay = "okay";' If that is acceptable, I could use some >>>> advice on where that particular code should go. >>>> >>>> Alan >>>> >>>> Alan Tull (2): >>>> of: overlay: add whitelist >>>> fpga: of region: add of-fpga-region to whitelist >>>> >>>> drivers/fpga/of-fpga-region.c | 9 ++++++ >>>> drivers/of/overlay.c | 73 +++++++++++++++++++++++++++++++++++++++++++ >>>> include/linux/of.h | 12 +++++++ >>>> 3 files changed, 94 insertions(+) >>>> >>> >>> The plan was to use connectors to restrict where an overlay could be applied. >>> I would prefer not to have multiple methods for accomplishing the same thing >>> unless there is a compelling reason to do so. >> >> Going back one level in my thinking, I don't think that having a driver mark >> a node as a location where an overlay fragment can be applied is serving a >> useful purpose. Any driver, including any driver loaded as a module, >> could mark a node as ok. I don't see how this is providing any meaningful >> restriction on where an overlay fragment can be applied. > > It serves to separate the setting of which nodes overlays can be > applied to and the mechanism to apply them (checking permissions). The > former can't be centralized My expectation is that determining which nodes overlays can be applied to can and _should_ be centralized, at least to begin with. If we loosen the restrictions on valid overlay application nodes then we _might_ find that we have to provide additional non-centralized permission granting. I think that the core devicetree code is the place (for initial implementation) that determining which nodes an overlay can be applied to. My expectation is that it will be implicitly obvious to the core devicetree code which nodes are connector nodes. Given that there have been several different proposals for connector implementation, my expectation may be completely wrong. So I am sure I will revisit my expectations the actual implementation of connectors arrives. Since the architecture and implementation of connectors is still so uncertain, I think it is premature to accept the changes proposed in the patch set, and the next patch set that has been proposed in response to the conversation in this thread. > and the latter can be. For example, > something in the kernel enables overlays on a node or nodes, then the > overlay is applied with configfs interface and no board specific code > involved. I agree that the permission checking should not need to involve board specific code. > My concern is not whether any kernel component can enable applying of > overlays, but userspace. If it is a kernel component, then it is > explicit. And an OOT kernel module doesn't count because there's no > ABI guarantee there. > > I agree that this patch series alone is not all that useful with only > in kernel users. It is only really interesting when we have a > userspace interface. However, an implementation with a flag bit is so > little code, I'm fine taking it now and not having to update all in > kernel users when adding a userspace interface. I think the concept of an API called by a driver, instead of the devicetree core code determining which nodes an overlay can be applied to is premature, since there is no direct need for it, and given that it is little code it can easily be added when it is needed, and we better understand how it will be used. > > Rob > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC 0/2] of: Add whitelist 2017-11-29 9:20 ` [RFC 0/2] of: Add whitelist Frank Rowand 2017-11-29 13:31 ` Rob Herring 2017-11-29 22:47 ` Frank Rowand @ 2017-11-30 0:58 ` Frank Rowand 2 siblings, 0 replies; 20+ messages in thread From: Frank Rowand @ 2017-11-30 0:58 UTC (permalink / raw) To: Alan Tull, Rob Herring, Pantelis Antoniou Cc: Moritz Fischer, devicetree, linux-kernel, linux-fpga On 11/29/17 04:20, Frank Rowand wrote: > On 11/27/17 15:58, Alan Tull wrote: >> Here's a proposal for a whitelist to lock down the dynamic device tree. >> >> For an overlay to be accepted, all of its targets are required to be >> on a target node whitelist. >> >> Currently the only way I have to get on the whitelist is calling a >> function to add a node. That works for fpga regions, but I think >> other uses will need a way of having adding specific nodes from the >> base device tree, such as by adding a property like 'allow-overlay;' >> or 'allow-overlay = "okay";' If that is acceptable, I could use some >> advice on where that particular code should go. >> >> Alan >> >> Alan Tull (2): >> of: overlay: add whitelist >> fpga: of region: add of-fpga-region to whitelist >> >> drivers/fpga/of-fpga-region.c | 9 ++++++ >> drivers/of/overlay.c | 73 +++++++++++++++++++++++++++++++++++++++++++ >> include/linux/of.h | 12 +++++++ >> 3 files changed, 94 insertions(+) >> > > The plan was to use connectors to restrict where an overlay could be applied. > I would prefer not to have multiple methods for accomplishing the same thing > unless there is a compelling reason to do so. Going back one level in my thinking, I don't think that having a driver mark a node as a location where an overlay fragment can be applied is serving a useful purpose. Any driver, including any driver loaded as a module, could mark a node as ok. I don't see how this is providing any meaningful restriction on where an overlay fragment can be applied. -Frank ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2017-12-07 19:22 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-27 20:58 [RFC 0/2] of: Add whitelist Alan Tull 2017-11-27 20:58 ` [RFC 1/2] of: overlay: add whitelist Alan Tull 2017-11-28 15:15 ` Rob Herring 2017-11-28 19:26 ` Alan Tull 2017-11-29 9:25 ` Frank Rowand 2017-11-27 20:58 ` [RFC 2/2] fpga: of region: add of-fpga-region to whitelist Alan Tull 2017-11-29 9:20 ` [RFC 0/2] of: Add whitelist Frank Rowand 2017-11-29 13:31 ` Rob Herring 2017-11-29 16:11 ` Alan Tull 2017-11-30 12:46 ` Frank Rowand 2017-12-05 16:33 ` Alan Tull 2017-12-06 11:56 ` Frank Rowand 2017-12-07 19:22 ` Alan Tull 2017-11-30 12:18 ` Frank Rowand 2017-12-05 16:55 ` Alan Tull 2017-12-06 11:47 ` Frank Rowand 2017-11-29 22:47 ` Frank Rowand 2017-11-30 14:39 ` Rob Herring 2017-12-06 11:44 ` Frank Rowand 2017-11-30 0:58 ` Frank Rowand
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).