diff mbox series

[v2,4/4] checks: Add bus checks for PCI buses

Message ID 20170210164717.1234-5-robh@kernel.org
State New
Headers show
Series dtc unit-address and character set checks | expand

Commit Message

Rob Herring Feb. 10, 2017, 4:47 p.m. UTC 
Add PCI bridge and device node checks. We identify PCI bridges with
'device_type = "pci"' as only PCI bridges should set that property. For
bridges, check that ranges is present and #address-cells and

For devices, the primary check is the reg property and the unit address.
Device unit addresses are in the form DD or DD,F where DD is the
device 0-0x1f and F is the function 0-7.

Signed-off-by: Rob Herring <robh@kernel.org>

---
v2:
- Remove bus_type functions. Combine test for bus_type and bridge check
  into single check.
- Add a check that PCI bridge node name is pci or pcie.

 checks.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 dtc.h    |  7 ++++++
 2 files changed, 90 insertions(+)

-- 
2.10.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

David Gibson Feb. 13, 2017, 5:03 a.m. UTC | #1 
On Fri, Feb 10, 2017 at 10:47:17AM -0600, Rob Herring wrote:
> Add PCI bridge and device node checks. We identify PCI bridges with

> 'device_type = "pci"' as only PCI bridges should set that property. For

> bridges, check that ranges is present and #address-cells and

> 

> For devices, the primary check is the reg property and the unit address.

> Device unit addresses are in the form DD or DD,F where DD is the

> device 0-0x1f and F is the function 0-7.

> 

> Signed-off-by: Rob Herring <robh@kernel.org>

> ---

> v2:

> - Remove bus_type functions. Combine test for bus_type and bridge check

>   into single check.

> - Add a check that PCI bridge node name is pci or pcie.

> 

>  checks.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

>  dtc.h    |  7 ++++++

>  2 files changed, 90 insertions(+)

> 

> diff --git a/checks.c b/checks.c

> index 16d17d20caec..9ebb148f947a 100644

> --- a/checks.c

> +++ b/checks.c

> @@ -702,6 +702,86 @@ static void check_ranges_format(struct check *c, struct dt_info *dti,

>  }

>  WARNING(ranges_format, check_ranges_format, NULL, &addr_size_cells);

>  

> +static const struct bus_type pci_bus = {

> +	.type = PCI_BUS_TYPE,


Since you can use the struct pointer itself as a handle on the bus
type, I don't think there's any value to having the enum-style type
value.  What _would_ be useful is a human readable bus type name.

> +};

> +

> +static void check_pci_bridge(struct check *c, struct dt_info *dti, struct node *node)

> +{

> +	struct property *prop;

> +

> +	prop = get_property(node, "device_type");

> +	if (!prop || strcmp(prop->val.val, "pci"))

> +		return;

> +

> +	node->bus = &pci_bus;

> +

> +	if (strncmp(node->name, "pci", node->basenamelen) &&

> +	    strncmp(node->name, "pcie", node->basenamelen))

> +		FAIL(c, "Node %s node name is not \"pci\" or \"pcie\"",

> +			     node->fullpath);


Please use the strneq() macro - I frequently get confused about
whether strcmp()/strncmp() comparisons need an ! or not for equality
testing.  streq() / strneq() help me remember.

> +

> +	prop = get_property(node, "ranges");

> +	if (!prop)

> +		FAIL(c, "Node %s missing ranges for PCI bridge (or not a bridge)",

> +			     node->fullpath);

> +

> +	if (node_addr_cells(node) != 3)

> +		FAIL(c, "Node %s incorrect #address-cells for PCI bridge",

> +			     node->fullpath);

> +	if (node_size_cells(node) != 2)

> +		FAIL(c, "Node %s incorrect #size-cells for PCI bridge",

> +			     node->fullpath);

> +}

> +WARNING(pci_bridge, check_pci_bridge, NULL,

> +	&device_type_is_string, &addr_size_cells);

> +

> +static void check_pci_device(struct check *c, struct dt_info *dti, struct node *node)

> +{

> +	struct property *prop;

> +	const char *unitname = get_unitname(node);

> +	char unit_addr[5];

> +	unsigned int dev, func, reg;

> +

> +	if (!node->parent || !node->parent->bus ||

> +	    (node->parent->bus->type != PCI_BUS_TYPE))


You can just use node->parent->bus != &pci_bus here.

> +		return;

> +

> +	prop = get_property(node, "reg");

> +	if (!prop)

> +		return;

> +

> +	reg = fdt32_to_cpu(*((cell_t *)prop->val.val));

> +

> +	dev = (reg & 0xf800) >> 11;

> +	func = (reg & 0x700) >> 8;

> +

> +	if (reg & 0xff000000)

> +		FAIL(c, "Node %s PCI reg address is not configuration space",

> +			     node->fullpath);

> +

> +	if (dev > 0x1f)

> +		FAIL(c, "Node %s PCI device number out of range",

> +			     node->fullpath);

> +	if (func > 7)

> +		FAIL(c, "Node %s PCI function number out of range",

> +		     node->fullpath);

> +

> +	if (func == 0) {

> +		snprintf(unit_addr, sizeof(unit_addr), "%x", dev);

> +		if (!strcmp(unitname, unit_addr))

> +			return;

> +	}

> +

> +	snprintf(unit_addr, sizeof(unit_addr), "%x,%x", dev, func);

> +	if (!strcmp(unitname, unit_addr))

> +		return;


So as mentioned in my comments to 3/4, the test above, I would put
back into unit_address_vs_reg, using a callback in the bus_type which
formats a reg into the correct unit address.

> +

> +	FAIL(c, "Node %s PCI unit address format error, expected \"%s\"",

> +	     node->fullpath, unit_addr);

> +}

> +WARNING(pci_device, check_pci_device, NULL, &reg_format);

> +

>  /*

>   * Style checks

>   */

> @@ -775,6 +855,9 @@ static struct check *check_table[] = {

>  	&unit_address_vs_reg,

>  	&unit_address_format,

>  

> +	&pci_bridge,

> +	&pci_device,

> +

>  	&avoid_default_addr_size,

>  	&obsolete_chosen_interrupt_controller,

>  

> diff --git a/dtc.h b/dtc.h

> index c6f125c68ba8..c538ef4afafb 100644

> --- a/dtc.h

> +++ b/dtc.h

> @@ -136,6 +136,12 @@ struct label {

>  	struct label *next;

>  };

>  

> +#define PCI_BUS_TYPE	1

> +

> +struct bus_type {

> +	int type;

> +};

> +

>  struct property {

>  	bool deleted;

>  	char *name;

> @@ -162,6 +168,7 @@ struct node {

>  	int addr_cells, size_cells;

>  

>  	struct label *labels;

> +	const struct bus_type *bus;

>  };

>  

>  #define for_each_label_withdel(l0, l) \


-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Rob Herring Feb. 13, 2017, 8:49 p.m. UTC | #2 
On Sun, Feb 12, 2017 at 11:03 PM, David Gibson
<david@gibson.dropbear.id.au> wrote:
> On Fri, Feb 10, 2017 at 10:47:17AM -0600, Rob Herring wrote:

>> Add PCI bridge and device node checks. We identify PCI bridges with

>> 'device_type = "pci"' as only PCI bridges should set that property. For

>> bridges, check that ranges is present and #address-cells and

>>

>> For devices, the primary check is the reg property and the unit address.

>> Device unit addresses are in the form DD or DD,F where DD is the

>> device 0-0x1f and F is the function 0-7.

>>

>> Signed-off-by: Rob Herring <robh@kernel.org>

>> ---

>> v2:

>> - Remove bus_type functions. Combine test for bus_type and bridge check

>>   into single check.

>> - Add a check that PCI bridge node name is pci or pcie.

>>

>>  checks.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

>>  dtc.h    |  7 ++++++

>>  2 files changed, 90 insertions(+)

>>

>> diff --git a/checks.c b/checks.c

>> index 16d17d20caec..9ebb148f947a 100644

>> --- a/checks.c

>> +++ b/checks.c

>> @@ -702,6 +702,86 @@ static void check_ranges_format(struct check *c, struct dt_info *dti,

>>  }

>>  WARNING(ranges_format, check_ranges_format, NULL, &addr_size_cells);

>>

>> +static const struct bus_type pci_bus = {

>> +     .type = PCI_BUS_TYPE,

>

> Since you can use the struct pointer itself as a handle on the bus

> type, I don't think there's any value to having the enum-style type

> value.  What _would_ be useful is a human readable bus type name.


Okay.

>> +};

>> +

>> +static void check_pci_bridge(struct check *c, struct dt_info *dti, struct node *node)

>> +{

>> +     struct property *prop;

>> +

>> +     prop = get_property(node, "device_type");

>> +     if (!prop || strcmp(prop->val.val, "pci"))

>> +             return;

>> +

>> +     node->bus = &pci_bus;

>> +

>> +     if (strncmp(node->name, "pci", node->basenamelen) &&

>> +         strncmp(node->name, "pcie", node->basenamelen))

>> +             FAIL(c, "Node %s node name is not \"pci\" or \"pcie\"",

>> +                          node->fullpath);

>

> Please use the strneq() macro - I frequently get confused about

> whether strcmp()/strncmp() comparisons need an ! or not for equality

> testing.  streq() / strneq() help me remember.

>

>> +

>> +     prop = get_property(node, "ranges");

>> +     if (!prop)

>> +             FAIL(c, "Node %s missing ranges for PCI bridge (or not a bridge)",

>> +                          node->fullpath);

>> +

>> +     if (node_addr_cells(node) != 3)

>> +             FAIL(c, "Node %s incorrect #address-cells for PCI bridge",

>> +                          node->fullpath);

>> +     if (node_size_cells(node) != 2)

>> +             FAIL(c, "Node %s incorrect #size-cells for PCI bridge",

>> +                          node->fullpath);

>> +}

>> +WARNING(pci_bridge, check_pci_bridge, NULL,

>> +     &device_type_is_string, &addr_size_cells);

>> +

>> +static void check_pci_device(struct check *c, struct dt_info *dti, struct node *node)

>> +{

>> +     struct property *prop;

>> +     const char *unitname = get_unitname(node);

>> +     char unit_addr[5];

>> +     unsigned int dev, func, reg;

>> +

>> +     if (!node->parent || !node->parent->bus ||

>> +         (node->parent->bus->type != PCI_BUS_TYPE))

>

> You can just use node->parent->bus != &pci_bus here.

>

>> +             return;

>> +

>> +     prop = get_property(node, "reg");

>> +     if (!prop)

>> +             return;

>> +

>> +     reg = fdt32_to_cpu(*((cell_t *)prop->val.val));

>> +

>> +     dev = (reg & 0xf800) >> 11;

>> +     func = (reg & 0x700) >> 8;

>> +

>> +     if (reg & 0xff000000)

>> +             FAIL(c, "Node %s PCI reg address is not configuration space",

>> +                          node->fullpath);

>> +

>> +     if (dev > 0x1f)

>> +             FAIL(c, "Node %s PCI device number out of range",

>> +                          node->fullpath);

>> +     if (func > 7)

>> +             FAIL(c, "Node %s PCI function number out of range",

>> +                  node->fullpath);


BTW, I just noticed these 2 checks I can drop. They can never be true
since I'm masking the values.

>> +

>> +     if (func == 0) {

>> +             snprintf(unit_addr, sizeof(unit_addr), "%x", dev);

>> +             if (!strcmp(unitname, unit_addr))

>> +                     return;

>> +     }

>> +

>> +     snprintf(unit_addr, sizeof(unit_addr), "%x,%x", dev, func);

>> +     if (!strcmp(unitname, unit_addr))

>> +             return;

>

> So as mentioned in my comments to 3/4, the test above, I would put

> back into unit_address_vs_reg, using a callback in the bus_type which

> formats a reg into the correct unit address.


Humm, that doesn't really work. The unit address can be in 2 different
forms when func# is 0. We can have either <dev> or <dev>,0.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Gibson Feb. 14, 2017, 2:12 a.m. UTC | #3 
On Mon, Feb 13, 2017 at 02:49:45PM -0600, Rob Herring wrote:
> On Sun, Feb 12, 2017 at 11:03 PM, David Gibson

> <david@gibson.dropbear.id.au> wrote:

> > On Fri, Feb 10, 2017 at 10:47:17AM -0600, Rob Herring wrote:

> >> Add PCI bridge and device node checks. We identify PCI bridges with

> >> 'device_type = "pci"' as only PCI bridges should set that property. For

> >> bridges, check that ranges is present and #address-cells and

> >>

> >> For devices, the primary check is the reg property and the unit address.

> >> Device unit addresses are in the form DD or DD,F where DD is the

> >> device 0-0x1f and F is the function 0-7.

> >>

> >> Signed-off-by: Rob Herring <robh@kernel.org>

> >> ---

> >> v2:

> >> - Remove bus_type functions. Combine test for bus_type and bridge check

> >>   into single check.

> >> - Add a check that PCI bridge node name is pci or pcie.

> >>

> >>  checks.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

> >>  dtc.h    |  7 ++++++

> >>  2 files changed, 90 insertions(+)

> >>

> >> diff --git a/checks.c b/checks.c

> >> index 16d17d20caec..9ebb148f947a 100644

> >> --- a/checks.c

> >> +++ b/checks.c

> >> @@ -702,6 +702,86 @@ static void check_ranges_format(struct check *c, struct dt_info *dti,

> >>  }

> >>  WARNING(ranges_format, check_ranges_format, NULL, &addr_size_cells);

> >>

> >> +static const struct bus_type pci_bus = {

> >> +     .type = PCI_BUS_TYPE,

> >

> > Since you can use the struct pointer itself as a handle on the bus

> > type, I don't think there's any value to having the enum-style type

> > value.  What _would_ be useful is a human readable bus type name.

> 

> Okay.

> 

> >> +};

> >> +

> >> +static void check_pci_bridge(struct check *c, struct dt_info *dti, struct node *node)

> >> +{

> >> +     struct property *prop;

> >> +

> >> +     prop = get_property(node, "device_type");

> >> +     if (!prop || strcmp(prop->val.val, "pci"))

> >> +             return;

> >> +

> >> +     node->bus = &pci_bus;

> >> +

> >> +     if (strncmp(node->name, "pci", node->basenamelen) &&

> >> +         strncmp(node->name, "pcie", node->basenamelen))

> >> +             FAIL(c, "Node %s node name is not \"pci\" or \"pcie\"",

> >> +                          node->fullpath);

> >

> > Please use the strneq() macro - I frequently get confused about

> > whether strcmp()/strncmp() comparisons need an ! or not for equality

> > testing.  streq() / strneq() help me remember.

> >

> >> +

> >> +     prop = get_property(node, "ranges");

> >> +     if (!prop)

> >> +             FAIL(c, "Node %s missing ranges for PCI bridge (or not a bridge)",

> >> +                          node->fullpath);

> >> +

> >> +     if (node_addr_cells(node) != 3)

> >> +             FAIL(c, "Node %s incorrect #address-cells for PCI bridge",

> >> +                          node->fullpath);

> >> +     if (node_size_cells(node) != 2)

> >> +             FAIL(c, "Node %s incorrect #size-cells for PCI bridge",

> >> +                          node->fullpath);

> >> +}

> >> +WARNING(pci_bridge, check_pci_bridge, NULL,

> >> +     &device_type_is_string, &addr_size_cells);

> >> +

> >> +static void check_pci_device(struct check *c, struct dt_info *dti, struct node *node)

> >> +{

> >> +     struct property *prop;

> >> +     const char *unitname = get_unitname(node);

> >> +     char unit_addr[5];

> >> +     unsigned int dev, func, reg;

> >> +

> >> +     if (!node->parent || !node->parent->bus ||

> >> +         (node->parent->bus->type != PCI_BUS_TYPE))

> >

> > You can just use node->parent->bus != &pci_bus here.

> >

> >> +             return;

> >> +

> >> +     prop = get_property(node, "reg");

> >> +     if (!prop)

> >> +             return;

> >> +

> >> +     reg = fdt32_to_cpu(*((cell_t *)prop->val.val));

> >> +

> >> +     dev = (reg & 0xf800) >> 11;

> >> +     func = (reg & 0x700) >> 8;

> >> +

> >> +     if (reg & 0xff000000)

> >> +             FAIL(c, "Node %s PCI reg address is not configuration space",

> >> +                          node->fullpath);

> >> +

> >> +     if (dev > 0x1f)

> >> +             FAIL(c, "Node %s PCI device number out of range",

> >> +                          node->fullpath);

> >> +     if (func > 7)

> >> +             FAIL(c, "Node %s PCI function number out of range",

> >> +                  node->fullpath);

> 

> BTW, I just noticed these 2 checks I can drop. They can never be true

> since I'm masking the values.


Ah, good point.  It looks like ther should be more to check though -
dev and func are 8 bits, and you don't allow anything in the top 8
bits, but there's no checking of the remaining 16 bits.  Or the other
2 address cells for that matter.

> >> +

> >> +     if (func == 0) {

> >> +             snprintf(unit_addr, sizeof(unit_addr), "%x", dev);

> >> +             if (!strcmp(unitname, unit_addr))

> >> +                     return;

> >> +     }

> >> +

> >> +     snprintf(unit_addr, sizeof(unit_addr), "%x,%x", dev, func);

> >> +     if (!strcmp(unitname, unit_addr))

> >> +             return;

> >

> > So as mentioned in my comments to 3/4, the test above, I would put

> > back into unit_address_vs_reg, using a callback in the bus_type which

> > formats a reg into the correct unit address.

> 

> Humm, that doesn't really work. The unit address can be in 2 different

> forms when func# is 0. We can have either <dev> or <dev>,0.


Ah, good point.  Alright leave it as is for now.  When we get more bus
types we can see if it makes sense to generalize something.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
diff mbox series

Patch

diff --git a/checks.c b/checks.c
index 16d17d20caec..9ebb148f947a 100644
--- a/checks.c
+++ b/checks.c
@@ -702,6 +702,86 @@  static void check_ranges_format(struct check *c, struct dt_info *dti,
 }
 WARNING(ranges_format, check_ranges_format, NULL, &addr_size_cells);
 
+static const struct bus_type pci_bus = {
+	.type = PCI_BUS_TYPE,
+};
+
+static void check_pci_bridge(struct check *c, struct dt_info *dti, struct node *node)
+{
+	struct property *prop;
+
+	prop = get_property(node, "device_type");
+	if (!prop || strcmp(prop->val.val, "pci"))
+		return;
+
+	node->bus = &pci_bus;
+
+	if (strncmp(node->name, "pci", node->basenamelen) &&
+	    strncmp(node->name, "pcie", node->basenamelen))
+		FAIL(c, "Node %s node name is not \"pci\" or \"pcie\"",
+			     node->fullpath);
+
+	prop = get_property(node, "ranges");
+	if (!prop)
+		FAIL(c, "Node %s missing ranges for PCI bridge (or not a bridge)",
+			     node->fullpath);
+
+	if (node_addr_cells(node) != 3)
+		FAIL(c, "Node %s incorrect #address-cells for PCI bridge",
+			     node->fullpath);
+	if (node_size_cells(node) != 2)
+		FAIL(c, "Node %s incorrect #size-cells for PCI bridge",
+			     node->fullpath);
+}
+WARNING(pci_bridge, check_pci_bridge, NULL,
+	&device_type_is_string, &addr_size_cells);
+
+static void check_pci_device(struct check *c, struct dt_info *dti, struct node *node)
+{
+	struct property *prop;
+	const char *unitname = get_unitname(node);
+	char unit_addr[5];
+	unsigned int dev, func, reg;
+
+	if (!node->parent || !node->parent->bus ||
+	    (node->parent->bus->type != PCI_BUS_TYPE))
+		return;
+
+	prop = get_property(node, "reg");
+	if (!prop)
+		return;
+
+	reg = fdt32_to_cpu(*((cell_t *)prop->val.val));
+
+	dev = (reg & 0xf800) >> 11;
+	func = (reg & 0x700) >> 8;
+
+	if (reg & 0xff000000)
+		FAIL(c, "Node %s PCI reg address is not configuration space",
+			     node->fullpath);
+
+	if (dev > 0x1f)
+		FAIL(c, "Node %s PCI device number out of range",
+			     node->fullpath);
+	if (func > 7)
+		FAIL(c, "Node %s PCI function number out of range",
+		     node->fullpath);
+
+	if (func == 0) {
+		snprintf(unit_addr, sizeof(unit_addr), "%x", dev);
+		if (!strcmp(unitname, unit_addr))
+			return;
+	}
+
+	snprintf(unit_addr, sizeof(unit_addr), "%x,%x", dev, func);
+	if (!strcmp(unitname, unit_addr))
+		return;
+
+	FAIL(c, "Node %s PCI unit address format error, expected \"%s\"",
+	     node->fullpath, unit_addr);
+}
+WARNING(pci_device, check_pci_device, NULL, &reg_format);
+
 /*
  * Style checks
  */
@@ -775,6 +855,9 @@  static struct check *check_table[] = {
 	&unit_address_vs_reg,
 	&unit_address_format,
 
+	&pci_bridge,
+	&pci_device,
+
 	&avoid_default_addr_size,
 	&obsolete_chosen_interrupt_controller,
 
diff --git a/dtc.h b/dtc.h
index c6f125c68ba8..c538ef4afafb 100644
--- a/dtc.h
+++ b/dtc.h
@@ -136,6 +136,12 @@  struct label {
 	struct label *next;
 };
 
+#define PCI_BUS_TYPE	1
+
+struct bus_type {
+	int type;
+};
+
 struct property {
 	bool deleted;
 	char *name;
@@ -162,6 +168,7 @@  struct node {
 	int addr_cells, size_cells;
 
 	struct label *labels;
+	const struct bus_type *bus;
 };
 
 #define for_each_label_withdel(l0, l) \