netdev - Re: [RFC 3/4] net-next: dsa: Add support for multiple cpu ports.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 4 Jan 2017 14:22:35 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     John Crispin <john@...ozen.org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
        netdev@...r.kernel.org
Subject: Re: [RFC 3/4] net-next: dsa: Add support for multiple cpu ports.

On Wed, Jan 04, 2017 at 08:38:03AM +0100, John Crispin wrote:
>  static int dsa_user_port_apply(struct device_node *port, u32 index,
> @@ -475,6 +475,28 @@ static int dsa_cpu_parse(struct device_node *port, u32 index,
>  
>  	dst->rcv = dst->tag_ops->rcv;
>  
> +	dev_hold(ethernet_dev);
> +	ds->cd->port_ethernet[index] = ethernet_dev;
> +
> +	return 0;
> +}
> +
> +static int dsa_user_parse(struct device_node *port, u32 index,
> +			  struct dsa_switch *ds)
> +{

Please put this function next to dsa_cpu_parse(). All the
apply/unapply functions are together, and all the _parse functions
should be together.

> +	struct device_node *cpu_port;
> +	const unsigned int *cpu_port_reg;
> +	int cpu_port_index;
> +
> +	cpu_port = of_parse_phandle(port, "cpu", 0);
> +	if (cpu_port) {
> +		cpu_port_reg = of_get_property(cpu_port, "reg", NULL);
> +		if (!cpu_port_reg)
> +			return -EINVAL;
> +		cpu_port_index = be32_to_cpup(cpu_port_reg);
> +		ds->cd->port_cpu[index] = cpu_port_index;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -482,18 +504,20 @@ static int dsa_ds_parse(struct dsa_switch_tree *dst, struct dsa_switch *ds)
>  {
>  	struct device_node *port;
>  	u32 index;
> -	int err;
> +	int err = 0;
>  
>  	for (index = 0; index < DSA_MAX_PORTS; index++) {
>  		port = ds->ports[index].dn;
>  		if (!port)
>  			continue;
>  
> -		if (dsa_port_is_cpu(port)) {
> +		if (dsa_port_is_cpu(port))
>  			err = dsa_cpu_parse(port, index, dst, ds);
> -			if (err)
> -				return err;
> -		}
> +		else if (!dsa_port_is_dsa(port))
> +			err = dsa_user_parse(port, index,  ds);
> +
> +		if (err)
> +			return err;

Having this if (err) here is correct, but it goes against the general
pattern we have in the code. Please indent it so it is under the
err =, and remove the initialisation of err.

Also, if one branch of an if/else has {}, the coding style says the
other branch should also use {}.

Just to make this code look nicer, i would be tempted to add a helper,
dsa_port_is_user().

>  	}
>  
>  	pr_info("DSA: switch %d %d parsed\n", dst->tree, ds->index);

Thanks
	Andrew