Re: [PATCH net-next v2 2/3] net: dsa: integrate with SWITCHDEV for HW bridging

[Florian Fainelli <f.fainelli@gmail.com>]

On 24/02/15 13:15, Guenter Roeck wrote:
> On Tue, Feb 24, 2015 at 01:04:48PM -0800, Florian Fainelli wrote:
>> On 24/02/15 12:58, Guenter Roeck wrote:
>>> On Tue, Feb 24, 2015 at 12:49:35PM -0800, Florian Fainelli wrote:
>>>> On 24/02/15 12:43, Guenter Roeck wrote:
>>>>> Hi Florian,
>>>>>
>>>>> On Tue, Feb 24, 2015 at 12:08:30PM -0800, 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:
>>>>>>
>>>>>> - port_join_bridge
>>>>>> - port_leave_bridge
>>>>>> - port_stp_update
>>>>>>
>>>>>> DSA will resolve which switch ports that are currently bridge port
>>>>>> members as some Switch hardware/drivers need to know about that to limit
>>>>>> the register programming to just the relevant registers (especially for
>>>>>> slow MDIO buses).
>>>>>>
>>>>>> We also take care of setting the correct STP state when slave network
>>>>>> devices are brought up/down while being bridge members.
>>>>>>
>>>>>> Finally, when a port is leaving the bridge, we make sure we set in
>>>>>> BR_STATE_FORWARDING state, otherwise the bridge layer would leave it
>>>>>> disabled as a result of having left the bridge.
>>>>>>
>>>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>>>>> ---
>>>>> [ ... ]
>>>>>
>>>>>>  
>>>>>> +/* Return a bitmask of all ports being currently bridged within a given bridge
>>>>>> + * device. 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,
>>>>>> +				  struct net_device *bridge)
>>>>>> +{
>>>>>> +	struct dsa_slave_priv *p;
>>>>>> +	unsigned int port;
>>>>>> +	u32 mask = 0;
>>>>>> +
>>>>>> +	for (port = 0; port < DSA_MAX_PORTS; port++) {
>>>>>> +		if (!((1 << port) & ds->phys_port_mask))
>>>>>> +			continue;
>>>>>> +
>>>>>> +		p = netdev_priv(ds->ports[port]);
>>>>>> +
>>>>>
>>>>> ds->ports[port] can still be NULL here since the function can be called
>>>>> before all ports are initialized (which is what I had actually seen).
>>>>
>>>> Can it now? I re-ordered the patches such that your change comes first
>>>> (and I forgot to update the commit message) and by the time we get
>>>> called from register_netdev(), ds->ports[port] has already been assigned.
>>>>
>>>> Am I missing something here?
>>>
>>> Yes, because phys_port_mask is set to include _all_ ports, not just
>>> the ones already registered.
>>>
>>> Assume ports 0..2 have been registered, phys_port_mask is 0x1f, and
>>> dsa_slave_br_port_mask is called for a state change on port 0.
>>> The loop will check ports 3 and 4 which have not yet been registered.
>>
>> Ok, I re-added the check.
>>
>>>
>>> Strictly speaking we might want to consider adding the same check
>>> into the suspend and resume functions, at least if suspend  /remove
>>> can ever happen before the system is fully initialized.
>>
>> Yes, that is possibly a problem, I don't think I could reproduce that
>> with my current setup because everything needs to be built into the kernel.
> 
> Does suspend/resume have anything to do with building code into the kernel ?

How do you suspend while you are initializing and have not scheduled any
user-space program, that's what I am referring to here, I am guaranteed
that all the slave network interfaces have been created before I even
schedule any user-space application.

> 
> There may be a more practical problem trying to reproduce this; you would
> have to somehow manage to suspend the system in the maybe one second
> or less where phys_port_mask is already initialized but not the slave ports.
> 
> I'll submit a patch to address that problem, just to be sure, following the
> logic of better safe than sorry.
> 
> Would it make sense to introduce a macro such as ds_port_initialized(ds, port)
> or ds_port_configured(ds, port) to check for this condition ? I see that it is
> used it in the bcm_sf2 driver's suspend/remove functions as well.

Sure, go ahead and we can update some of the existing code based on that.
-- 
Florian