[PATCH 0/3 net-next] dsa: bugfixing and typos after Device Tree bindings

[PATCH 0/3 net-next] dsa: bugfixing and typos after Device Tree bindings

[Florian Fainelli]

Hello David,

Please find here 3 minor bugfixes related to my recent device tree bindings
additions to DSA. Thanks!

Florian Fainelli (3):
  dsa: fix device tree binding documentation typo on #address-cells
  dsa: factor freeing of dsa_platform_data
  dsa: fix freeing of sparse port allocation

 Documentation/devicetree/bindings/net/dsa/dsa.txt |    2 +-
 net/dsa/dsa.c                                     |   40 ++++++++++-----------
 2 files changed, 21 insertions(+), 21 deletions(-)

-- 
1.7.10.4

[PATCH 1/3] dsa: fix device tree binding documentation typo on #address-cells

[Florian Fainelli]

The device tree binding documentation for dsa explicitely states that a
DSA node should have its #address-cells property set to 2, yet the
example still used 1, fix that typo.

Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
 Documentation/devicetree/bindings/net/dsa/dsa.txt |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.txt b/Documentation/devicetree/bindings/net/dsa/dsa.txt
index db92f55..49f4f7a 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa.txt
+++ b/Documentation/devicetree/bindings/net/dsa/dsa.txt
@@ -43,7 +43,7 @@ Example:
 
 	dsa@0 {
 		compatible = "marvell,dsa";
-		#address-cells = <1>;
+		#address-cells = <2>;
 		#size-cells = <0>;
 
 		interrupts = <10>;
-- 
1.7.10.4

[PATCH 2/3] dsa: factor freeing of dsa_platform_data

[Florian Fainelli]

This patch factors the freeing of the struct dsa_platform_data
manipulated by the driver identically in two places to a single
function.

Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
 net/dsa/dsa.c |   38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 908bc11..aa2ff58 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -343,6 +343,21 @@ out:
 	return ret;
 }
 
+static void dsa_of_free_platform_data(struct dsa_platform_data *pd)
+{
+	int i;
+	int port_index;
+
+	for (i = 0; i < pd->nr_chips; i++) {
+		port_index = 0;
+		while (pd->chip[i].port_names &&
+			pd->chip[i].port_names[++port_index])
+			kfree(pd->chip[i].port_names[port_index]);
+		kfree(pd->chip[i].rtable);
+	}
+	kfree(pd->chip);
+}
+
 static int dsa_of_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -354,7 +369,7 @@ static int dsa_of_probe(struct platform_device *pdev)
 	const char *port_name;
 	int chip_index, port_index;
 	const unsigned int *sw_addr, *port_reg;
-	int ret, i;
+	int ret;
 
 	mdio = of_parse_phandle(np, "dsa,mii-bus", 0);
 	if (!mdio)
@@ -439,14 +454,7 @@ static int dsa_of_probe(struct platform_device *pdev)
 	return 0;
 
 out_free_chip:
-	for (i = 0; i < pd->nr_chips; i++) {
-		port_index = 0;
-		while (pd->chip[i].port_names &&
-			pd->chip[i].port_names[++port_index])
-			kfree(pd->chip[i].port_names[port_index]);
-		kfree(pd->chip[i].rtable);
-	}
-	kfree(pd->chip);
+	dsa_of_free_platform_data(pd);
 out_free:
 	kfree(pd);
 	pdev->dev.platform_data = NULL;
@@ -456,21 +464,11 @@ out_free:
 static void dsa_of_remove(struct platform_device *pdev)
 {
 	struct dsa_platform_data *pd = pdev->dev.platform_data;
-	int i;
-	int port_index;
 
 	if (!pdev->dev.of_node)
 		return;
 
-	for (i = 0; i < pd->nr_chips; i++) {
-		port_index = 0;
-		while (pd->chip[i].port_names &&
-			pd->chip[i].port_names[++port_index])
-			kfree(pd->chip[i].port_names[port_index]);
-		kfree(pd->chip[i].rtable);
-	}
-
-	kfree(pd->chip);
+	dsa_of_free_platform_data(pd);
 	kfree(pd);
 }
 #else
-- 
1.7.10.4

[PATCH 3/3] dsa: fix freeing of sparse port allocation

[Florian Fainelli]

If we have defined a sparse port allocation which is non-contiguous and
contains gaps, the code freeing port_names will just stop when it
encouters a first NULL port_names, which is not right, we should iterate
over all possible number of ports (DSA_MAX_PORTS) until we are done.

Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
 net/dsa/dsa.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index aa2ff58..0eb5d5e 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -350,9 +350,11 @@ static void dsa_of_free_platform_data(struct dsa_platform_data *pd)
 
 	for (i = 0; i < pd->nr_chips; i++) {
 		port_index = 0;
-		while (pd->chip[i].port_names &&
-			pd->chip[i].port_names[++port_index])
-			kfree(pd->chip[i].port_names[port_index]);
+		while (port_index < DSA_MAX_PORTS) {
+			if (pd->chip[i].port_names[port_index])
+				kfree(pd->chip[i].port_names[port_index]);
+			port_index++;
+		}
 		kfree(pd->chip[i].rtable);
 	}
 	kfree(pd->chip);
-- 
1.7.10.4

RE: [PATCH 3/3] dsa: fix freeing of sparse port allocation

[David Laight]

> If we have defined a sparse port allocation which is non-contiguous and
> contains gaps, the code freeing port_names will just stop when it
> encouters a first NULL port_names, which is not right, we should iterate
> over all possible number of ports (DSA_MAX_PORTS) until we are done.
...
>  		port_index = 0;
> -		while (pd->chip[i].port_names &&
> -			pd->chip[i].port_names[++port_index])
> -			kfree(pd->chip[i].port_names[port_index]);
> +		while (port_index < DSA_MAX_PORTS) {
> +			if (pd->chip[i].port_names[port_index])
> +				kfree(pd->chip[i].port_names[port_index]);
> +			port_index++;
> +		}

A couple of 'odd' differences:

The old code checked pd->chip[i].port_names (as if it
might be a pointer) that is absent from the new version.
(If it is separately allocated it is leaked).

The new code tests and frees pd->chip[i].port_names[0]
whereas the old code ignored the 0th entry.

These may, or may not, be relevant!

	David

Re: [PATCH 3/3] dsa: fix freeing of sparse port allocation

[Florian Fainelli]

Le lundi 25 mars 2013 17:03:23, David Laight a écrit :
> > If we have defined a sparse port allocation which is non-contiguous and
> > contains gaps, the code freeing port_names will just stop when it
> > encouters a first NULL port_names, which is not right, we should iterate
> > over all possible number of ports (DSA_MAX_PORTS) until we are done.
> 
> ...
> 
> >  		port_index = 0;
> > 
> > -		while (pd->chip[i].port_names &&
> > -			pd->chip[i].port_names[++port_index])
> > -			kfree(pd->chip[i].port_names[port_index]);
> > +		while (port_index < DSA_MAX_PORTS) {
> > +			if (pd->chip[i].port_names[port_index])
> > +				kfree(pd->chip[i].port_names[port_index]);
> > +			port_index++;
> > +		}
> 
> A couple of 'odd' differences:
> 
> The old code checked pd->chip[i].port_names (as if it
> might be a pointer) that is absent from the new version.
> (If it is separately allocated it is leaked).
> 
> The new code tests and frees pd->chip[i].port_names[0]
> whereas the old code ignored the 0th entry.

The old code was wrong, it was off by one for the first array index, and would 
stop whenever it encountered the first NULL port_names[index] so we would not 
skip other these and free the possibly non-NULL next one.

I think the current code is now correct, but thanks for the review!
--
Florian

Re: [PATCH 0/3 net-next] dsa: bugfixing and typos after Device Tree bindings

[David Miller]

From: Florian Fainelli <florian@openwrt.org>
Date: Mon, 25 Mar 2013 16:03:37 +0100

> Please find here 3 minor bugfixes related to my recent device tree bindings
> additions to DSA. Thanks!

Series applied, thanks Florian.