[PATCH] net: dsa: fix filling rtable from OF description

[PATCH] net: dsa: fix filling rtable from OF description

[Pavel Nakonechny]

According to description in 'include/net/dsa.h', in cascade switches
configurations where there are more than one interconnected devices,
'rtable' array in 'dsa_chip_data' structure is used to indicate which
port on this switch should be used to send packets to that are destined
for corresponding switch.

However, dsa_of_setup_routing_table() fills 'rtable' with port numbers
of the _target_ switch, but not current one.

This commit removes redundant devicetree parsing and adds needed port
number as a function argument. So dsa_of_setup_routing_table() now just
looks for target switch number by parsing parent of 'link' device node.

This was tested on custom board with two Marvell 88E6095 switches with
following corresponding rtables: { -1, 10 } and { 8, -1 }.

Signed-off-by: Pavel Nakonechny <pavel.nakonechny@skitlab.ru>
---
 net/dsa/dsa.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 947f550..5593a0d 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -375,12 +375,10 @@ static struct net_device *dev_to_net_device(struct device *dev)
 #ifdef CONFIG_OF
 static int dsa_of_setup_routing_table(struct dsa_platform_data *pd,
 					struct dsa_chip_data *cd,
-					int chip_index,
+					int chip_index, int port_index,
 					struct device_node *link)
 {
-	int ret;
 	const __be32 *reg;
-	int link_port_addr;
 	int link_sw_addr;
 	struct device_node *parent_sw;
 	int len;
@@ -409,20 +407,9 @@ static int dsa_of_setup_routing_table(struct dsa_platform_data *pd,
 		memset(cd->rtable, -1, pd->nr_chips * sizeof(s8));
 	}
 
-	reg = of_get_property(link, "reg", NULL);
-	if (!reg) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	link_port_addr = be32_to_cpup(reg);
-
-	cd->rtable[link_sw_addr] = link_port_addr;
+	cd->rtable[link_sw_addr] = port_index;
 
 	return 0;
-out:
-	kfree(cd->rtable);
-	return ret;
 }
 
 static void dsa_of_free_platform_data(struct dsa_platform_data *pd)
@@ -528,7 +515,7 @@ static int dsa_of_probe(struct platform_device *pdev)
 			if (!strcmp(port_name, "dsa") && link &&
 					pd->nr_chips > 1) {
 				ret = dsa_of_setup_routing_table(pd, cd,
-						chip_index, link);
+						chip_index, port_index, link);
 				if (ret)
 					goto out_free_chip;
 			}
-- 

Re: [PATCH] net: dsa: fix filling rtable from OF description

[Andrew Lunn]

Hi Pavel

There is the code after applying your patch:

static int dsa_of_setup_routing_table(struct dsa_platform_data *pd,
                                        struct dsa_chip_data *cd,
                                        int chip_index, int port_index,
                                        struct device_node *link)
{
        const __be32 *reg;
        int link_sw_addr;
        struct device_node *parent_sw;
        int len;

        parent_sw = of_get_parent(link);
        if (!parent_sw)
                return -EINVAL;

link is the phandle to the port in other switch. parent_sw is then the
switch property.


        reg = of_get_property(parent_sw, "reg", &len);
        if (!reg || (len != sizeof(*reg) * 2))
                return -EINVAL;

So now you get the reg property. This is documented as:

- reg                   : Describes the switch address on the MII bus


        link_sw_addr = be32_to_cpup(reg + 1);

        if (link_sw_addr >= pd->nr_chips)
                return -EINVAL;

This is now not making much sense.

Looking up the MII bus address seems wrong. You want the chip number,
not its address.

    Andrew

        /* First time routing table allocation */
        if (!cd->rtable) {
                cd->rtable = kmalloc_array(pd->nr_chips, sizeof(s8),
                                           GFP_KERNEL);
                if (!cd->rtable)
                        return -ENOMEM;

                /* default to no valid uplink/downlink */
                memset(cd->rtable, -1, pd->nr_chips * sizeof(s8));
        }

        cd->rtable[link_sw_addr] = port_index;

        return 0;
}

Re: [PATCH] net: dsa: fix filling rtable from OF description

[Pavel Nakonechny]

Hi Andrew

В письме от 4 апреля 2015 18:38:14 пользователь Andrew Lunn написал:
> There is the code after applying your patch:
> 
> static int dsa_of_setup_routing_table(struct dsa_platform_data *pd,
>                                         struct dsa_chip_data *cd,
>                                         int chip_index, int port_index,
>                                         struct device_node *link)
> {
>         const __be32 *reg;
>         int link_sw_addr;
>         struct device_node *parent_sw;
>         int len;
> 
>         parent_sw = of_get_parent(link);
>         if (!parent_sw)
>                 return -EINVAL;
> 
> link is the phandle to the port in other switch. parent_sw is then the
> switch property.
yep

>         reg = of_get_property(parent_sw, "reg", &len);
>         if (!reg || (len != sizeof(*reg) * 2))
>                 return -EINVAL;
> 
> So now you get the reg property. This is documented as:
> 
> - reg                   : Describes the switch address on the MII bus
Yes, but read further in the entry example:
"reg = <16 0>;	/* MDIO address 16, switch 0 in tree */"


>         link_sw_addr = be32_to_cpup(reg + 1);
Now we extract switch number (" + 1"), but not switch MDIO address.

> 
>         if (link_sw_addr >= pd->nr_chips)
>                 return -EINVAL;
> 
> This is now not making much sense.
> 
> Looking up the MII bus address seems wrong. You want the chip number,
> not its address.
Yes, you are right that its wrong to take MII address, but this code is taking 
switch number, not its address. I can agree that this is silly, but it is not 
my code originally and patch fixes other thing.

Re: [PATCH] net: dsa: fix filling rtable from OF description

[Andrew Lunn]

On Sat, Apr 04, 2015 at 08:10:09PM +0300, Pavel Nakonechny wrote:
> Hi Andrew
> 
> ?? ???????????? ???? 4 ???????????? 2015 18:38:14 ???????????????????????? Andrew Lunn ??????????????:
> > There is the code after applying your patch:
> > 
> > static int dsa_of_setup_routing_table(struct dsa_platform_data *pd,
> >                                         struct dsa_chip_data *cd,
> >                                         int chip_index, int port_index,
> >                                         struct device_node *link)
> > {
> >         const __be32 *reg;
> >         int link_sw_addr;
> >         struct device_node *parent_sw;
> >         int len;
> > 
> >         parent_sw = of_get_parent(link);
> >         if (!parent_sw)
> >                 return -EINVAL;
> > 
> > link is the phandle to the port in other switch. parent_sw is then the
> > switch property.
> yep
> 
> >         reg = of_get_property(parent_sw, "reg", &len);
> >         if (!reg || (len != sizeof(*reg) * 2))
> >                 return -EINVAL;
> > 
> > So now you get the reg property. This is documented as:
> > 
> > - reg                   : Describes the switch address on the MII bus
> Yes, but read further in the entry example:
> "reg = <16 0>;	/* MDIO address 16, switch 0 in tree */"

Ah!

O.K. Please add a comment. If i read this wrong, it is quite likely
somebody else will also read this wrong.

I think it would also be good to add some text to dsa.txt explaining
this second field in reg.

     Andrew

Re: [PATCH] net: dsa: fix filling rtable from OF description

[Andrew Lunn]

On Sat, Apr 04, 2015 at 04:48:48PM +0300, Pavel Nakonechny wrote:
> According to description in 'include/net/dsa.h', in cascade switches
> configurations where there are more than one interconnected devices,
> 'rtable' array in 'dsa_chip_data' structure is used to indicate which
> port on this switch should be used to send packets to that are destined
> for corresponding switch.
> 
> However, dsa_of_setup_routing_table() fills 'rtable' with port numbers
> of the _target_ switch, but not current one.
> 
> This commit removes redundant devicetree parsing and adds needed port
> number as a function argument. So dsa_of_setup_routing_table() now just
> looks for target switch number by parsing parent of 'link' device node.
> 
> This was tested on custom board with two Marvell 88E6095 switches with
> following corresponding rtables: { -1, 10 } and { 8, -1 }.
> 
> Signed-off-by: Pavel Nakonechny <pavel.nakonechny@skitlab.ru>

Hi Pavel

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

As i said in my other email, it would be nice to add a comment about
the switch number being the second value in reg.

    Andrew

> ---
>  net/dsa/dsa.c | 19 +++----------------
>  1 file changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 947f550..5593a0d 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -375,12 +375,10 @@ static struct net_device *dev_to_net_device(struct device *dev)
>  #ifdef CONFIG_OF
>  static int dsa_of_setup_routing_table(struct dsa_platform_data *pd,
>  					struct dsa_chip_data *cd,
> -					int chip_index,
> +					int chip_index, int port_index,
>  					struct device_node *link)
>  {
> -	int ret;
>  	const __be32 *reg;
> -	int link_port_addr;
>  	int link_sw_addr;
>  	struct device_node *parent_sw;
>  	int len;
> @@ -409,20 +407,9 @@ static int dsa_of_setup_routing_table(struct dsa_platform_data *pd,
>  		memset(cd->rtable, -1, pd->nr_chips * sizeof(s8));
>  	}
>  
> -	reg = of_get_property(link, "reg", NULL);
> -	if (!reg) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -
> -	link_port_addr = be32_to_cpup(reg);
> -
> -	cd->rtable[link_sw_addr] = link_port_addr;
> +	cd->rtable[link_sw_addr] = port_index;
>  
>  	return 0;
> -out:
> -	kfree(cd->rtable);
> -	return ret;
>  }
>  
>  static void dsa_of_free_platform_data(struct dsa_platform_data *pd)
> @@ -528,7 +515,7 @@ static int dsa_of_probe(struct platform_device *pdev)
>  			if (!strcmp(port_name, "dsa") && link &&
>  					pd->nr_chips > 1) {
>  				ret = dsa_of_setup_routing_table(pd, cd,
> -						chip_index, link);
> +						chip_index, port_index, link);
>  				if (ret)
>  					goto out_free_chip;
>  			}
> -- 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html