Re: [PATCH v5 2/7] net: dsa: Add DSA driver for Hirschmann Hellcreek switches

[Kurt Kanzenbach <kurt@linutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3845 bytes --]

On Sat Sep 05 2020, Vladimir Oltean wrote:
> On Fri, Sep 04, 2020 at 08:27:34AM +0200, Kurt Kanzenbach wrote:
>> Add a basic DSA driver for Hirschmann Hellcreek switches. Those switches are
>> implementing features needed for Time Sensitive Networking (TSN) such as support
>> for the Time Precision Protocol and various shapers like the Time Aware Shaper.
>> 
>> This driver includes basic support for networking:
>> 
>>  * VLAN handling
>>  * FDB handling
>>  * Port statistics
>>  * STP
>>  * Phylink
>> 
>> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
>> ---
>> +
>> +/* Default setup for DSA:
>> + *  VLAN 2: CPU and Port 1 egress untagged.
>> + *  VLAN 3: CPU and Port 2 egress untagged.
>> + */
>> +static int hellcreek_setup_vlan_membership(struct dsa_switch *ds, int port,
>> +					   bool enabled)
>
> If you use VLAN 2 and 3 for port separation, then how does the driver
> deal with the following:
>
> ip link add link swp1 name swp1.100 type vlan id 100
> ip link add link swp2 name swp2.100 type vlan id 100
>
> In this case, frames with VLAN 100 shouldn't leak from one port to the
> other, will they?

Well, that depends on whether hellcreek_vlan_add() is called for
creating that vlan interfaces. In general: As soon as both ports are
members of the same vlan that traffic is switched.

>
>> +{
>> +	int upstream = dsa_upstream_port(ds, port);
>> +	struct switchdev_obj_port_vlan vlan = {
>> +		.vid_begin = port,
>> +		.vid_end = port,
>> +	};
>> +	int err = 0;
>> +
>> +	/* The CPU port is implicitly configured by
>> +	 * configuring the front-panel ports
>> +	 */
>> +	if (!dsa_is_user_port(ds, port))
>> +		return 0;
>> +
>> +	/* Apply vid to port as egress untagged and port vlan id */
>> +	vlan.flags = BRIDGE_VLAN_INFO_UNTAGGED | BRIDGE_VLAN_INFO_PVID;
>> +	if (enabled)
>> +		hellcreek_vlan_add(ds, port, &vlan);
>> +	else
>> +		err = hellcreek_vlan_del(ds, port, &vlan);
>> +	if (err) {
>> +		dev_err(ds->dev, "Failed to apply VID %d to port %d: %d\n",
>> +			port, port, err);
>> +		return err;
>> +	}
>> +
>> +	/* Apply vid to cpu port as well */
>> +	vlan.flags = BRIDGE_VLAN_INFO_UNTAGGED;
>> +	if (enabled)
>> +		hellcreek_vlan_add(ds, upstream, &vlan);
>> +	else
>> +		err = hellcreek_vlan_del(ds, upstream, &vlan);
>> +	if (err) {
>> +		dev_err(ds->dev, "Failed to apply VID %d to port %d: %d\n",
>> +			port, port, err);
>> +		return err;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void hellcreek_setup_ingressflt(struct hellcreek *hellcreek, int port,
>> +				       bool enable)
>> +{
>> +	struct hellcreek_port *hellcreek_port = &hellcreek->ports[port];
>> +	u16 ptcfg;
>> +
>> +	mutex_lock(&hellcreek->reg_lock);
>> +
>> +	ptcfg = hellcreek_port->ptcfg;
>> +
>> +	if (enable)
>> +		ptcfg |= HR_PTCFG_INGRESSFLT;
>> +	else
>> +		ptcfg &= ~HR_PTCFG_INGRESSFLT;
>> +
>> +	hellcreek_select_port(hellcreek, port);
>> +	hellcreek_write(hellcreek, ptcfg, HR_PTCFG);
>> +	hellcreek_port->ptcfg = ptcfg;
>> +
>> +	mutex_unlock(&hellcreek->reg_lock);
>> +}
>> +
>> +static int hellcreek_vlan_filtering(struct dsa_switch *ds, int port,
>> +				    bool vlan_filtering)
>
> I remember asking in Message-ID: <20200716082935.snokd33kn52ixk5h@skbuf>
> whether it would be possible for you to set
> ds->configure_vlan_while_not_filtering = true during hellcreek_setup.
> Did anything unexpected happen while trying that?

No, that comment got lost.

So looking at the flag: Does it mean the driver can receive vlan
configurations when a bridge without vlan filtering is used? That might
be problematic as this driver uses vlans for the port separation by
default. This is undone when vlan filtering is set to 1 meaning vlan
configurations can be received without any problems.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]