Re: [PATCH RFC 1/2] net: dsa: integrate with SWITCHDEV for HW bridging

[Guenter Roeck <linux@roeck-us.net>]

On 02/22/2015 07:14 PM, Andrew Lunn wrote:
> On Sun, Feb 22, 2015 at 06:20:44PM -0800, Guenter Roeck wrote:
>> On 02/17/2015 11:26 AM, Florian Fainelli wrote:
>>> In order to support bridging offloads in DSA switch drivers, select
>>> NET_SWITCHDEV to get access to the port_stp_update and parent_get_id
>>> NDOs that we are required to implement.
>>>
>>> To facilitate the integratation at the DSA driver level, we implement 3
>>> types of operations:
>>>
>>
>> Hi Florian,
>>
>>>
>>> +/* Return a bitmask of all ports being currently bridged. Note that on
>>> + * leave, the mask will still return the bitmask of ports currently bridged,
>>> + * prior to port removal, and this is exactly what we want.
>>> + */
>>> +static u32 dsa_slave_br_port_mask(struct dsa_switch *ds)
>>> +{
>>> +	unsigned int port;
>>> +	u32 mask = 0;
>>> +
>>> +	for (port = 0; port < DSA_MAX_PORTS; port++) {
>>> +		if (!((1 << port) & ds->phys_port_mask))
>>> +			continue;
>>> +
>>> +		if (ds->ports[port]->priv_flags & IFF_BRIDGE_PORT)
>>> +			mask |= 1 << port;
>>
>> Problem is that the function can be called through dsa_slave_netdevice_event
>> before the slave devices are fully initialized.
>>
>> After adding
>>
>> +               if (!ds->ports[port]) {
>> +                       netdev_err(bridge,
>> +                                  "No ports data for port %d, mask=0x%x\n",
>> +                                  port, ds->phys_port_mask);
>> +                       continue;
>> +               }
>>
>> and with some more debug messages added to dsa_switch_setup(), I see the following.
>>
>> [   14.187290] e1000e 0000:00:19.0 em1: [0]: Creating slave device for port 1(port1)
>> [   14.272605] e1000e 0000:00:19.0 em1: [0]: Creating slave device for port 2(port2)
>> [   14.353118] e1000e 0000:00:19.0 em1: [0]: Creating slave device for port 3(port3)
>> [   14.472002] br0: No ports data for port 3, mask=0x1e
>> [   14.472053] br0: No ports data for port 4, mask=0x1e
>> [   14.472753] e1000e 0000:00:19.0 em1: [0]: Creating slave device for port 4(host2esb)
>>
>> This happens if I add the bridge configuration to /etc/network/interfaces instead
>> of creating the bridge manually. Apparently dsa_switch_setup() is not yet complete
>> when dsa_slave_netdevice_event is executed to handle a state change on one of its
>> newly created slave interfaces.
>>
>> The relevant information from /etc/network/interfaces is:
>>
>> auto br0
>>
>> iface port1 inet manual
>> iface port2 inet manual
>>
>> iface br0 inet dhcp
>> 	bridge_ports port1 port2
>
> Hi Guenter
>
> Does this actually matter? The ports which don't exists yet are not
> being added to the bridge. The mask will come out correct. What
> happens when port4 is made a member of the bridge? I expect it
> works. It is the creation of the interface which triggers hotplug to
> read interfaces and add the interface to the port.
>

	if (!ds->ports[port])
		continue;

might be an option. However, I am not sure that what you say is correct,
at least not strictly speaking. dsa_slave_create() returns the created
slave device, which is added to ds->ports[port] in dsa_switch_setup().
Since there is no protection in dsa_switch_setup(), there is no guarantee
that the callback doesn't happen prior to the initialization of
ds->ports[port]. So the above would leave a race condition, where the
port being added to the bridge _is_ one for which ds->ports[port] is
not yet initialized.

Protecting the entire slave creation loop in dsa_switch_setup()
and using register_netdevice() in dsa_slave_create() solves the problem
as far as I can see, I just don't know if it is an acceptable solution.

Guenter