Hi Grant,
Thanks for the review.
I will be sending you the improved patch shortly.
On Wed, Jul 9, 2014 at 11:10 AM, Grant Likely <grant.likely@xxxxxxxxxx> wrote:
> On Mon, 7 Jul 2014 08:08:42 -0700, Gaurav Minocha <gaurav.minocha.os@xxxxxxxxx> wrote:
>> This patch attaches selftest's device tree data (required by /drivers/of/selftest.c)
>> dynamically into live device tree. First, it links selftest device tree data into the
>> kernel image and then iterates over all the nodes and attaches them into the live tree.
>> Once the testcases are complete, it removes the data attached.
>
> Hi Gaurav,
>
> Looks really good. Comments below, but first you asked some questions in
> your private email, but I'll reply to them here because it is better to
> have technical discussions on-list.
>
>> Q1: I have been trying to free the memory that I used as below:
>> /* store a copy for removal */
>> of_fdt_unflatten_tree((unsigned long *)selftest_data, &remove_data_node);
>> - Should I create a separate function that frees each property
>> member of struct device_node and the node itself? Or, is there a
>> function in kernel that can handle such a case(i.e. deleting
>> struct and its members)?
>
> The funny thing about the unflattening function is that it puts all the
> nodes into a single contiguous block of memory. You can free the entire
> thing by freeing the pointer to the first node. However, you need to
> first be absolutely sure that all node references are dropped. There is
> no guarantee that another part of the kernel isn't walking the tree at
> the same time. The OF_DYNAMIC code prevents nodes from getting freed
> using reference counting, but the refcounting doesn't work for nodes
> created with of_fdt_unflatten_tree().
>
> For now, don't worry about freeing the memory. This is a test utility
> after all. It can be fixed latter. It also overlaps with some of the
> work Panto is doing on overlays and a plan to switch the DT structures
> to use RCU. When that is done it will be possible to free the testcase
> data.
>
>> Q2: Also, there is another approach that is rather than storing a copy
>> of whole tree, add the full_path name of each node in a global
>> string array (const char *a[N]) while attaching the nodes, the
>> drawback of this approach is: size N will have to be changed
>> whenever the testcases are increased in future..
>
> Instead of the full path, why not an array of device_node pointers? That
> is a lot more efficient.
>
> You don't need to store every node pointer, only the root of every
> added sub-tree. The removal path can walk the children for each subtree
> root and remove them. That way the array can be pretty small. You could
> statically allocate it with just a few entries.
>
> I would do the same thing for properties added to existing nodes
> (/aliases)
>
>> This patch will remove the manual process of addition and removal of selftest device
>> tree data into the machine's dts file. Also, it can be build as a loadable kernel
>> module by setting the config symbol OF_SELFTEST=m.
>
> Have you been able to test module loading?
Yes, it should work.
>
>> Tested successfully with current selftest's testcases.
>>
>> Signed-off-by: Gaurav Minocha <gaurav.minocha.os@xxxxxxxxx>
>> ---
>> arch/arm/boot/dts/versatile-pb.dts | 2 -
>> drivers/of/Kconfig | 3 +-
>> drivers/of/Makefile | 3 +-
>> drivers/of/base.c | 5 ++
>> drivers/of/platform.c | 1 +
>> drivers/of/selftest.c | 135 +++++++++++++++++++++++++++++++
>> drivers/of/testcase-data/testcases.dts | 5 ++
>> drivers/of/testcase-data/testcases.dtsi | 4 -
>> 8 files changed, 150 insertions(+), 8 deletions(-)
>> create mode 100644 drivers/of/testcase-data/testcases.dts
>> delete mode 100644 drivers/of/testcase-data/testcases.dtsi
>>
>> diff --git a/arch/arm/boot/dts/versatile-pb.dts b/arch/arm/boot/dts/versatile-pb.dts
>> index 65f6577..8d39677 100644
>> --- a/arch/arm/boot/dts/versatile-pb.dts
>> +++ b/arch/arm/boot/dts/versatile-pb.dts
>> @@ -46,5 +46,3 @@
>> };
>> };
>> };
>> -
>> -#include <testcases.dtsi>
>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
>> index 2dcb054..4e4f6f3 100644
>> --- a/drivers/of/Kconfig
>> +++ b/drivers/of/Kconfig
>> @@ -8,8 +8,9 @@ menu "Device Tree and Open Firmware support"
>> depends on OF
>>
>> config OF_SELFTEST
>> - bool "Device Tree Runtime self tests"
>> + tristate "Device Tree Runtime self tests"
>> depends on OF_IRQ
>> + select OF_DYNAMIC
>
> I just thought of something we should add to your task list. This patch
> requires OF_DYNAMIC, but not all platforms want OF_DYNAMIC turned on.
> OF_DYNAMIC turns on reference counting which has a bit of a cost to it.
> I would like to investigate if we can make OF_SELFTEST work without the
> OF_DYNAMIC code.
>
> There would need to be some rework of the code code to handle attaching
> nodes without OF_DYNAMIC, but it should be doable.
>
> (don't get derailed from what you're doing now, this will be a follow-on
> task)
>
>> help
>> This option builds in test cases for the device tree infrastructure
>> that are executed once at boot time, and the results dumped to the
>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>> index 099b1fb..b9e753b 100644
>> --- a/drivers/of/Makefile
>> +++ b/drivers/of/Makefile
>> @@ -5,7 +5,8 @@ obj-$(CONFIG_OF_PROMTREE) += pdt.o
>> obj-$(CONFIG_OF_ADDRESS) += address.o
>> obj-$(CONFIG_OF_IRQ) += irq.o
>> obj-$(CONFIG_OF_NET) += of_net.o
>> -obj-$(CONFIG_OF_SELFTEST) += selftest.o
>> +obj-$(CONFIG_OF_SELFTEST) += of_selftest.o
>> +of_selftest-objs := selftest.o testcase-data/testcases.dtb.o
>> obj-$(CONFIG_OF_MDIO) += of_mdio.o
>> obj-$(CONFIG_OF_PCI) += of_pci.o
>> obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index b986480..aaf7219 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -1814,6 +1814,7 @@ int of_add_property(struct device_node *np, struct property *prop)
>>
>> return rc;
>> }
>> +EXPORT_SYMBOL_GPL(of_add_property);
>>
>> /**
>> * of_remove_property - Remove a property from a node.
>> @@ -1860,6 +1861,7 @@ int of_remove_property(struct device_node *np, struct property *prop)
>>
>> return 0;
>> }
>> +EXPORT_SYMBOL_GPL(of_remove_property);
>>
>> /*
>> * of_update_property - Update a property in a node, if the property does
>> @@ -1915,6 +1917,7 @@ int of_update_property(struct device_node *np, struct property *newprop)
>>
>> return 0;
>> }
>> +EXPORT_SYMBOL_GPL(of_update_property);
>>
>> #if defined(CONFIG_OF_DYNAMIC)
>> /*
>> @@ -1970,6 +1973,7 @@ int of_attach_node(struct device_node *np)
>> of_node_add(np);
>> return 0;
>> }
>> +EXPORT_SYMBOL_GPL(of_attach_node);
>>
>> /**
>> * of_detach_node - "Unplug" a node from the device tree.
>> @@ -2029,6 +2033,7 @@ int of_detach_node(struct device_node *np)
>> of_node_remove(np);
>> return rc;
>> }
>> +EXPORT_SYMBOL_GPL(of_detach_node);
>> #endif /* defined(CONFIG_OF_DYNAMIC) */
>>
>> static void of_alias_add(struct alias_prop *ap, struct device_node *np,
>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>> index 500436f..b7a82d6 100644
>> --- a/drivers/of/platform.c
>> +++ b/drivers/of/platform.c
>> @@ -30,6 +30,7 @@ const struct of_device_id of_default_bus_match_table[] = {
>> #endif /* CONFIG_ARM_AMBA */
>> {} /* Empty terminated list */
>> };
>> +EXPORT_SYMBOL(of_default_bus_match_table);
>>
>> static int of_dev_node_match(struct device *dev, void *data)
>> {
>> diff --git a/drivers/of/selftest.c b/drivers/of/selftest.c
>> index 077314e..f40a1e6 100644
>> --- a/drivers/of/selftest.c
>> +++ b/drivers/of/selftest.c
>> @@ -9,6 +9,7 @@
>> #include <linux/errno.h>
>> #include <linux/module.h>
>> #include <linux/of.h>
>> +#include <linux/of_fdt.h>
>> #include <linux/of_irq.h>
>> #include <linux/of_platform.h>
>> #include <linux/list.h>
>> @@ -21,6 +22,10 @@ static struct selftest_results {
>> int failed;
>> } selftest_results;
>>
>> +static void *selftest_data;
>> +static struct device_node *selftest_data_node;
>
> Move both of these pointers into selftest_data_init(). There are no
> users outside of that function, so they shouldn't be in the file scope.
>
>> +static struct device_node *remove_data_node;
>> +
>> #define selftest(result, fmt, ...) { \
>> if (!(result)) { \
>> selftest_results.failed++; \
>> @@ -517,9 +522,134 @@ static void __init of_selftest_platform_populate(void)
>> }
>> }
>>
>> +/**
>> + * update_node_properties - adds the properties
>> + * of np into dup node (present in live tree) and
>> + * updates parent of children of np to dup.
>> + *
>> + * @np: node already present in live tree
>> + * @dup: node present in live tree to be updated
>> + */
>> +static void update_node_properties(struct device_node *np,
>> + struct device_node *dup)
>> +{
>> + struct property *prop;
>> + struct device_node *child;
>> +
>> + for_each_property_of_node(np, prop)
>> + of_add_property(dup, prop);
>> +
>> + for_each_child_of_node(np, child)
>> + child->parent = dup;
>> +}
>> +
>> +/**
>> + * attach_node_and_children - attaches nodes
>> + * and its children to live tree
>> + *
>> + * @np: Node to attach to live tree
>> + */
>> +static int attach_node_and_children(struct device_node *np)
>> +{
>> + struct device_node *next, *root = np, *dup;
>> +
>> + if (!np) {
>> + pr_warn("%s: No tree to attach; not running tests\n",
>> + __func__);
>> + return -ENODATA;
>> + }
>> +
>> +
>> + /* skip root node */
>> + np = np->child;
>> +
>> + while (np) {
>> + next = np->allnext;
>> + dup = of_find_node_by_path(np->full_name);
>> + if (dup)
>> + update_node_properties(np, dup);
>> + else {
>> + np->child = NULL;
>> + if (np->parent == root)
>> + np->parent = of_allnodes;
>> + of_attach_node(np);
>> + }
>> + np = next;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * selftest_data_init - Reads, copies data from
>> + * linked tree and attaches it to the live tree
>> + */
>> +static int __init selftest_data_init(void)
>> +{
>> + int size;
>> + extern uint8_t __dtb_testcases_begin[];
>> + extern uint8_t __dtb_testcases_end[];
>
> move size down two lines and do:
> const int size = __dtb_testcases_end - __dtb_testcases_begin;
>
>> +
>> + selftest_data = (void *)__dtb_testcases_begin;
>
> Unnecessary cast. Any pointer can be assigned to a void* variable. A
> general rule of thumb with kernel code is that if you need to do a cast
> then it probably means you've done something wrong with the data types.
> When casts are required, you should be able to describe exactly why it
> is required if someone asks you. :-)
>
>> +
>> + if (!selftest_data) {
>> + pr_warn("%s: No testcase data to attach; not running tests\n",
>> + __func__);
>> + return -ENODATA;
>> + }
>
> Unnecessary test. __dtb_testcases_begin will never be NULL. Check for
> if (!size) instead.
>
>> +
>> + size = __dtb_testcases_end - __dtb_testcases_begin;
>> +
>> + /* creating copy */
>> + selftest_data = kmemdup(selftest_data, size, GFP_KERNEL);
>
> Instead of assigning and then reassigning selftest_data, you can pass
> the __dtb_testcases_begin address directly into kmemdup:
>
> selftest_data = kmemdup(__dtb_testcases_begin, size, GFP_KERNEL);
>
>> +
>> + if (!selftest_data) {
>> + pr_warn("%s: Failed to allocate memory for selftest_data; "
>> + "not running tests\n", __func__);
>> + return -ENOMEM;
>> + }
>> + of_fdt_unflatten_tree((unsigned long *)selftest_data,
>> + &selftest_data_node);
>
> Another unnecessary cast.
>
>> + /* store a copy for removal */
>> + of_fdt_unflatten_tree((unsigned long *)selftest_data,
>> + &remove_data_node);
>> +
>> +
>> + /* attach the sub-tree to live tree */
>> + return attach_node_and_children(selftest_data_node);
>> +}
>> +
>> +static void selftest_data_remove(struct device_node *np)
>> +{
>> + struct property *prop;
>> +
>> + if (!np) {
>> + pr_info("No testcase data in device tree; unable to remove data\n");
>> + return;
>> + }
>> + if (np->allnext)
>> + selftest_data_remove(np->allnext);
>> +
>> + np = of_find_node_by_path(np->full_name);
>> +
>> + if (strcmp(np->full_name, "/aliases") == 0) {
>> + for_each_property_of_node(np, prop) {
>> + if (strcmp(prop->name, "testcase-alias") == 0)
>> + of_remove_property(np, prop);
>> + }
>> + } else
>> + of_detach_node(np);
>> +}
>> +
>> static int __init of_selftest(void)
>> {
>> struct device_node *np;
>> + int res;
>> +
>> + /* adding data for selftest */
>> + res = selftest_data_init();
>> + if (res)
>> + return res;
>>
>> np = of_find_node_by_path("/testcase-data/phandle-tests/consumer-a");
>> if (!np) {
>> @@ -539,6 +669,11 @@ static int __init of_selftest(void)
>> of_selftest_platform_populate();
>> pr_info("end of selftest - %i passed, %i failed\n",
>> selftest_results.passed, selftest_results.failed);
>> +
>> + /* removing data for selftest */
>> + selftest_data_remove(remove_data_node);
>> +
>> return 0;
>> }
>> late_initcall(of_selftest);
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/of/testcase-data/testcases.dts b/drivers/of/testcase-data/testcases.dts
>> new file mode 100644
>> index 0000000..8e7568e
>> --- /dev/null
>> +++ b/drivers/of/testcase-data/testcases.dts
>> @@ -0,0 +1,5 @@
>> +/dts-v1/;
>> +#include "tests-phandle.dtsi"
>> +#include "tests-interrupts.dtsi"
>> +#include "tests-match.dtsi"
>> +#include "tests-platform.dtsi"
>> diff --git a/drivers/of/testcase-data/testcases.dtsi b/drivers/of/testcase-data/testcases.dtsi
>> deleted file mode 100644
>> index 6d8d980a..0000000
>> --- a/drivers/of/testcase-data/testcases.dtsi
>> +++ /dev/null
>> @@ -1,4 +0,0 @@
>> -#include "tests-phandle.dtsi"
>> -#include "tests-interrupts.dtsi"
>> -#include "tests-match.dtsi"
>> -#include "tests-platform.dtsi"
>> --
>> 1.7.9.5
>>
>
Please let me know if I'm missing something.
Regards
Gaurav Minocha
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
