DTC unit-address checks

From: Stephen Warren <[email protected]>

ePAPR 1.1 section 2.2.1.1 "Node Name Requirements" specifies that any node that has a reg property must include a unit address in its name with value matching the first entry in its reg property. Conversely, if a node does not have a reg property, the node name must not include a unit address. Also allow ranges property as it is deemed valid, but ePAPR is not clear about it.

Implement a check for this. The code doesn't validate the format of the unit address; ePAPR implies this may vary from (containing bus) binding to binding, so doing so would be much more complex.

Signed-off-by Stephen Warren <[email protected]>

[robh: also allow non-empty ranges]

Signed-off-by Rob Herring <[email protected]>

Node name unit-addresses should never begin with 0x or leading 0s regardless of whether they have a bus specific address (i.e. one with commas) or not. Add warnings to check for these cases.

Signed-off-by Rob Herring <[email protected]>

---
 checks.c                       | 10 ++++++++++
 tests/run_tests.sh             |  2 ++
 tests/unit-addr-leading-0s.dts | 10 ++++++++++
 tests/unit-addr-leading-0x.dts | 10 ++++++++++
 4 files changed, 32 insertions(+)
 create mode 100644 tests/unit-addr-leading-0s.dts
 create mode 100644 tests/unit-addr-leading-0x.dts

checks.c +0 -0

index 4001b8c..300ab43 100644
@@ -310,6 +310,16 @@ static void check_unit_address_vs_reg(struct check *c, struct node *dt,
 		if (!unitname[0])
 			FAIL(c, "Node %s has a reg or ranges property, but no unit name",
 			    node->fullpath);

checks.c +0 -0

 	} else {
 		if (unitname[0])
 			FAIL(c, "Node %s has a unit name, but no reg property",

tests/run_tests.sh +2 -0

index 4b7a131..502caa6 100755
@@ -445,6 +445,8 @@ dtc_tests () {
     check_tests obsolete-chosen-interrupt-controller.dts obsolete_chosen_interrupt_controller
     check_tests reg-without-unit-addr.dts unit_address_vs_reg
     check_tests unit-addr-without-reg.dts unit_address_vs_reg
+    check_tests unit-addr-leading-0x.dts unit_address_vs_reg
+    check_tests unit-addr-leading-0s.dts unit_address_vs_reg
     run_sh_test dtc-checkfails.sh node_name_chars -- -I dtb -O dtb bad_node_char.dtb
     run_sh_test dtc-checkfails.sh node_name_format -- -I dtb -O dtb bad_node_format.dtb
     run_sh_test dtc-checkfails.sh prop_name_chars -- -I dtb -O dtb bad_prop_char.dtb

tests/unit-addr-leading-0s.dts new file +10 -0

new file mode 100644
index 0000000..7c8e2ce
@@ -0,0 +1,10 @@
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	node@001 {
+		reg = <1 0>;
+	};
+};

tests/unit-addr-leading-0x.dts new file +10 -0

new file mode 100644
index 0000000..7ed7254
@@ -0,0 +1,10 @@
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	node@0x1 {
+		reg = <1 0>;
+	};
+};

Quoted 3 lines · Rob Herring

Node name unit-addresses should never begin with 0x or leading 0s regardless of whether they have a bus specific address (i.e. one with commas) or not. Add warnings to check for these cases.

Hmm.. I'm pretty sure that's true in practice, but it's not true in theory. A bus could define it's unit address format just about however it wants, including with leading 0s.

I think a better approach would be to add a test specific to simple-bus devices (by looking at compatible on the parent) that fully checks the unit address.

From there we can start adding tests for other bus types.

Quoted 5 lines · David Gibson

Quoted 3 lines · Rob Herring

Node name unit-addresses should never begin with 0x or leading 0s regardless of whether they have a bus specific address (i.e. one with commas) or not. Add warnings to check for these cases.

Quoted 3 lines

Hmm.. I'm pretty sure that's true in practice, but it's not true in theory. A bus could define it's unit address format just about however it wants, including with leading 0s.

Only if it is not reviewed... This whole check is about what best practices are, not what is possible.

Quoted 5 lines

I think a better approach would be to add a test specific to simple-bus devices (by looking at compatible on the parent) that fully checks the unit address.

From there we can start adding tests for other bus types.

simple-bus is easy enough,

but then next up would be I2C and SPI. We can't generically tell if a node is on I2C or SPI bus.

If we do have
some bus with wacky addresses, it should definitely have a bus compatible and then we can simply exclude it from the check.

Another option would be skipping the test if there are any commas (or periods, etc.) in the unit address. That's pretty rare to begin with and a single number is pretty much not a bus specific unit-address.

Quoted 11 lines · Rob Herring

Quoted 8 lines · David Gibson

Quoted 3 lines · Rob Herring

Node name unit-addresses should never begin with 0x or leading 0s regardless of whether they have a bus specific address (i.e. one with commas) or not. Add warnings to check for these cases.

Hmm.. I'm pretty sure that's true in practice, but it's not true in theory. A bus could define it's unit address format just about however it wants, including with leading 0s.

Quoted 2 lines

Only if it is not reviewed... This whole check is about what best practices are, not what is possible.

Hmm. dtc checks are really about checking for best practice at the level of individual dts files, though, not bindings.

Quoted 7 lines

Quoted 5 lines

I think a better approach would be to add a test specific to simple-bus devices (by looking at compatible on the parent) that fully checks the unit address.

From there we can start adding tests for other bus types.

simple-bus is easy enough,

So, start with that, then tackle the next problem.

Quoted 2 lines

but then next up would be I2C and SPI. We can't generically tell if a node is on I2C or SPI bus.

Why not? Or perhaps.. how generically do you need? I think having a big list of i2c / spi controllers would be acceptable here, if not ideal.

Quoted 8 lines

If we do have
some bus with wacky addresses, it should definitely have a bus compatible and then we can simply exclude it from the check.

Another option would be skipping the test if there are any commas (or periods, etc.) in the unit address. That's pretty rare to begin with and a single number is pretty much not a bus specific unit-address.

Um.. no.. there are definitely bus types that don't typically use commas. ISA, for one.

Quoted 12 lines · David Gibson

Quoted 11 lines · Rob Herring

Quoted 8 lines · David Gibson

Quoted 3 lines · Rob Herring

Node name unit-addresses should never begin with 0x or leading 0s regardless of whether they have a bus specific address (i.e. one with commas) or not. Add warnings to check for these cases.

Hmm.. I'm pretty sure that's true in practice, but it's not true in theory. A bus could define it's unit address format just about however it wants, including with leading 0s.

Quoted 5 lines

Quoted 2 lines

Only if it is not reviewed... This whole check is about what best practices are, not what is possible.

Hmm. dtc checks are really about checking for best practice at the level of individual dts files, though, not bindings.

Checking simple-bus specifically would be checking a binding.

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

I think a better approach would be to add a test specific to simple-bus devices (by looking at compatible on the parent) that fully checks the unit address.

From there we can start adding tests for other bus types.

simple-bus is easy enough,

Quoted 8 lines

So, start with that, then tackle the next problem.

Quoted 2 lines

but then next up would be I2C and SPI. We can't generically tell if a node is on I2C or SPI bus.

Why not? Or perhaps.. how generically do you need? I think having a big list of i2c / spi controllers would be acceptable here, if not ideal.

So someone adds a new controller, puts crap in for unit addresses, and then no warnings until that compatible string is added to dtc. And I'm still left spending my time in reviews telling them to fix this trivial crap.

That's roughly 60 I2C controllers (families, so multiple compatible strings each) plus similar number of SPI controllers, OF-graph binding, and random other things where reg gets used.

Quoted 10 lines

Quoted 7 lines

If we do have
some bus with wacky addresses, it should definitely have a bus compatible and then we can simply exclude it from the check.

Another option would be skipping the test if there are any commas (or periods, etc.) in the unit address. That's pretty rare to begin with and a single number is pretty much not a bus specific unit-address.

Um.. no.. there are definitely bus types that don't typically use commas. ISA, for one.

All the cases of ISA in the kernel tree at least would pass this test. But we could either blacklist ISA or skip if any non-hex characters are present.

BTW, my next patch is stricter node and property name checks on the use of '#', '?', '.', '+', '*', and '_'. So if you don't think these kinds to checks belong in dtc, then tell me and suggest how we should check for this.

Quoted 14 lines · Rob Herring

Quoted 12 lines · David Gibson

Quoted 11 lines · Rob Herring

Quoted 8 lines · David Gibson

Quoted 3 lines · Rob Herring

Node name unit-addresses should never begin with 0x or leading 0s regardless of whether they have a bus specific address (i.e. one with commas) or not. Add warnings to check for these cases.

Hmm.. I'm pretty sure that's true in practice, but it's not true in theory. A bus could define it's unit address format just about however it wants, including with leading 0s.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Only if it is not reviewed... This whole check is about what best practices are, not what is possible.

Hmm. dtc checks are really about checking for best practice at the level of individual dts files, though, not bindings.

Checking simple-bus specifically would be checking a binding.

Sorry, I wasn't clear. dtc checking the dts against a binding is fine, but checking the sanity of the binding itself is beyond its scope.

Quoted 17 lines

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

I think a better approach would be to add a test specific to simple-bus devices (by looking at compatible on the parent) that fully checks the unit address.

From there we can start adding tests for other bus types.

simple-bus is easy enough,

Quoted 8 lines

So, start with that, then tackle the next problem.

Quoted 2 lines

but then next up would be I2C and SPI. We can't generically tell if a node is on I2C or SPI bus.

Why not? Or perhaps.. how generically do you need? I think having a big list of i2c / spi controllers would be acceptable here, if not ideal.

Quoted 8 lines

So someone adds a new controller, puts crap in for unit addresses, and then no warnings until that compatible string is added to dtc. And I'm still left spending my time in reviews telling them to fix this trivial crap.

That's roughly 60 I2C controllers (families, so multiple compatible strings each) plus similar number of SPI controllers, OF-graph binding, and random other things where reg gets used.

Ah, I see.

Ok, I guess we do need to have an option for a "fallback" scheme for unit addresses (i.e. hex) for bus types we don't specifically recognize. But I'd still like the logic to be:
if (known bus type)
check against format for this bus type
else
check against fallback format

Rather than putting the second test in with a hacked up set of exclusions.

To do this nicely, I think the best way will be to add a bus_type field to the node structure in dtc, and have it populated (with an option for "unknown") in an early check pass, that later unit address tests can references as a prereq.

Pointer to a struct with unit address formatting functions, with NULL for unknown is the obvious choice to me for bus_type.

Quoted 63 lines · David Gibson

Quoted 14 lines · Rob Herring

Quoted 12 lines · David Gibson

Quoted 11 lines · Rob Herring

Quoted 8 lines · David Gibson

Quoted 3 lines · Rob Herring

Node name unit-addresses should never begin with 0x or leading 0s regardless of whether they have a bus specific address (i.e. one with commas) or not. Add warnings to check for these cases.

Hmm.. I'm pretty sure that's true in practice, but it's not true in theory. A bus could define it's unit address format just about however it wants, including with leading 0s.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Only if it is not reviewed... This whole check is about what best practices are, not what is possible.

Hmm. dtc checks are really about checking for best practice at the level of individual dts files, though, not bindings.

Checking simple-bus specifically would be checking a binding.

Sorry, I wasn't clear. dtc checking the dts against a binding is fine, but checking the sanity of the binding itself is beyond its scope.

Quoted 17 lines

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

I think a better approach would be to add a test specific to simple-bus devices (by looking at compatible on the parent) that fully checks the unit address.

From there we can start adding tests for other bus types.

simple-bus is easy enough,

Quoted 8 lines

So, start with that, then tackle the next problem.

Quoted 2 lines

but then next up would be I2C and SPI. We can't generically tell if a node is on I2C or SPI bus.

Why not? Or perhaps.. how generically do you need? I think having a big list of i2c / spi controllers would be acceptable here, if not ideal.

Quoted 8 lines

So someone adds a new controller, puts crap in for unit addresses, and then no warnings until that compatible string is added to dtc. And I'm still left spending my time in reviews telling them to fix this trivial crap.

That's roughly 60 I2C controllers (families, so multiple compatible strings each) plus similar number of SPI controllers, OF-graph binding, and random other things where reg gets used.

Ah, I see.

Ok, I guess we do need to have an option for a "fallback" scheme for unit addresses (i.e. hex) for bus types we don't specifically recognize. But I'd still like the logic to be:
if (known bus type)
check against format for this bus type
else
check against fallback format

Quoted 2 lines

Rather than putting the second test in with a hacked up set of exclusions.

Okay, makes sense.

Do you think we still need simple-bus as an explicit type given the check is the same as the default? Might be useful to have if we want to add some checks that address translations work.

Quoted 7 lines

To do this nicely, I think the best way will be to add a bus_type field to the node structure in dtc, and have it populated (with an option for "unknown") in an early check pass, that later unit address tests can references as a prereq.

Pointer to a struct with unit address formatting functions, with NULL for unknown is the obvious choice to me for bus_type.

So, something like this for the first stage:

static bool pci_bus_check_is_type(struct node *node)

{
	struct property *prop;
	
	if (!node || !node->parent)
		return false;

	prop = get_property(node->parent, "device_type");
	if (!prop)
		return false;
		
	if (strcmp(prop->val, "pci") == 0)
		return true;
		
	return false;
}

static void pci_bus_check_unit_address()
{

}

struct bus_type_fns {
.check_is_type = pci_bus_check_is_type, .check_unit_address = pci_bus_check_unit_address,
} pci_bus_fns;

struct bus_type_fns * {
	&pci_bus_fns,
	NULL
} bus_types;

static void fixup_bus_type(struct check *c, struct node *root,
struct node *node)

{
	struct bus_type_fns **bus;
	
	for (bus = bus_types; *bus != NULL; bus++) {
		if (!(*bus)->check_is_type(node))
			continue;

		node->bus_type = *bus;
		break;
	}
}

ERROR(bus_type, NULL, NULL, fixup_bus_type, NULL, NULL);