Subject: Re: [PATCH net-next 4/9] net: dsa: Initialize ds->enabled_port_mask and ds->phys_mii_mask

From: Florian Fainelli <[email-masked]>
Date: Sun, 5 Jun 2016 15:38:59 -0700

Le 04/06/2016 13:29, Andrew Lunn a écrit :
>> @@ -517,6 +541,15 @@ static int dsa_parse_ports_dn(struct device_node *ports, struct dsa_switch *ds)
>>  			return -EINVAL;
>>  
>>  		ds->ports[reg].dn = port;
>> +
>> +		if (dsa_port_is_cpu(port))
>> +			ds->dst->cpu_port = reg;
>> +		else
>> +			/* Initialize enabled_port_mask now for drv->setup()
>> +			 * to have access to a correct value, just like what
>> +			 * net/dsa/dsa.c::dsa_switch_setup_one does.
>> +			 */
>> +			ds->enabled_port_mask |= 1 << reg;
> 
> Hi Florian
> 
> You need to be careful here. There can be multiple CPU ports, in
> different switches. We want dst->cpu_port to be deterministic,
> independent of the order switches are registered. Which is why i set
> it as part of dsa_cpu_parse(), which only happens when all the
> switches have registered, and we are parsing their device tree nodes
> in order. So we guarantee dst->cpu_port is the first CPU node.

Ah OK, I completely missed that part and just wanted to avoid walking
the ports children nodes more than twice.

We might be able to get away with just initializing
ds->enabled_port_mask here actually.

> 
> You now set dst->cpu_port via dsa_parse_ports_dn(), so it is now non
> deterministic, it depends on the probe order of the switches.
> 
> In the long run, i want to deprecate and then remove dst->cpu_port,
> but i'm not that far yet.
> 
> Please rethink this part of the patch, keeping in mind you have
> multiple switches, with multiple CPU and DSA ports, all connected in
> some crazy fashion.
-- 
Florian