Hi Vladimir,
thanks for the review.
On Sun Oct 04 2020, Vladimir Oltean wrote:
> On Sun, Oct 04, 2020 at 01:29:06PM +0200, Kurt Kanzenbach wrote:
>> +static int hellcreek_vlan_del(struct dsa_switch *ds, int port,
>> + const struct switchdev_obj_port_vlan *vlan)
>> +{
>> + struct hellcreek *hellcreek = ds->priv;
>> + u16 vid;
>> +
>> + dev_dbg(hellcreek->dev, "Remove VLANs (%d -- %d) on port %d\n",
>> + vlan->vid_begin, vlan->vid_end, port);
>> +
>> + for (vid = vlan->vid_begin; vid <= vlan->vid_end; ++vid) {
>> + mutex_lock(&hellcreek->ports[port].vlan_lock);
>> + if (hellcreek->ports[port].vlan_filtering)
>> + hellcreek_unapply_vlan(hellcreek, port, vid);
>
> I don't think this works.
>
> ip link add br0 type bridge vlan_filtering 1
> ip link set swp0 master br0
> bridge vlan add dev swp0 vid 100
> ip link set br0 type bridge vlan_filtering 0
> bridge vlan del dev swp0 vid 100
> ip link set br0 type bridge vlan_filtering 1
>
> The expectation would be that swp0 blocks vid 100 now, but with your
> scheme it doesn't (it is not unapplied, and not unqueued either, because
> it was never queued in the first place).
Yes, that's correct. So, I think we have to queue not only the addition
of VLANs, but rather the "action" itself such as add or del. And then
apply all pending actions whenever vlan_filtering is set.
>> +static int hellcreek_port_bridge_join(struct dsa_switch *ds, int port,
>> + struct net_device *br)
>> +{
>> + struct hellcreek *hellcreek = ds->priv;
>> + int i;
>> +
>> + dev_dbg(hellcreek->dev, "Port %d joins a bridge\n", port);
>> +
>> + /* Configure port's vid to all other ports as egress untagged */
>> + for (i = 0; i < ds->num_ports; ++i) {
>> + if (!dsa_is_user_port(ds, i))
>> + continue;
>> +
>> + if (i == port)
>> + continue;
>> +
>> + hellcreek_apply_vlan(hellcreek, i, port, false, true);
>> + }
>
> I think this is buggy when joining a VLAN filtering bridge. Your ports
> will pass frames with VID=2 with no problem, even without the user
> specifying 'bridge vlan add dev swp0 vid 2', and that's an issue. My
> understanding is that VLANs 1, 2, 3 stop having any sort of special
> meaning when the upper bridge has vlan_filtering=1.
Yes, that understanding is correct. So, what happens is when a port is
joining a VLAN filtering bridge is:
|root@tsn:~# ip link add name br0 type bridge
|root@tsn:~# ip link set dev br0 type bridge vlan_filtering 1
|root@tsn:~# ip link set dev lan0 master br0
|[ 209.375055] br0: port 1(lan0) entered blocking state
|[ 209.380073] br0: port 1(lan0) entered disabled state
|[ 209.385340] hellcreek ff240000.switch: Port 2 joins a bridge
|[ 209.391584] hellcreek ff240000.switch: Apply VLAN: port=3 vid=2 pvid=0 untagged=1
|[ 209.399439] device lan0 entered promiscuous mode
|[ 209.404043] device eth0 entered promiscuous mode
|[ 209.409204] hellcreek ff240000.switch: Enable VLAN filtering on port 2
|[ 209.415716] hellcreek ff240000.switch: Unapply VLAN: port=2 vid=2
|[ 209.421840] hellcreek ff240000.switch: Unapply VLAN: port=0 vid=2
|[ 209.428170] hellcreek ff240000.switch: Apply queued VLANs: port2
|[ 209.434158] hellcreek ff240000.switch: Apply VLAN: port=2 vid=0 pvid=0 untagged=0
|[ 209.441649] hellcreek ff240000.switch: Clear queued VLANs: port2
|[ 209.447920] hellcreek ff240000.switch: Apply queued VLANs: port0
|[ 209.453910] hellcreek ff240000.switch: Apply VLAN: port=0 vid=0 pvid=0 untagged=0
|[ 209.461402] hellcreek ff240000.switch: Clear queued VLANs: port0
|[ 209.467620] hellcreek ff240000.switch: VLAN prepare for port 2
|[ 209.473476] hellcreek ff240000.switch: VLAN prepare for port 0
|[ 209.479534] hellcreek ff240000.switch: Add VLANs (1 -- 1) on port 2, untagged, PVID
|[ 209.487164] hellcreek ff240000.switch: Apply VLAN: port=2 vid=1 pvid=1 untagged=1
|[ 209.494659] hellcreek ff240000.switch: Add VLANs (1 -- 1) on port 0, untagged, no PVID
|[ 209.502794] hellcreek ff240000.switch: Apply VLAN: port=0 vid=1 pvid=0 untagged=1
|root@tsn:~# bridge vlan show
|port vlan ids
|lan0 1 PVID Egress Untagged
|
|br0 1 PVID Egress Untagged
... which looks correct to me. The VLAN 2 is unapplied as expected. Or?
>
> And how do you deal with the case where swp1 and swp2 are bridged and
> have the VLAN 3 installed via 'bridge vlan', but swp3 isn't bridged?
> Will swp1/swp2 communicate with swp3? If yes, that's a problem.
There is no swp3. Currently there are only two ports and either they are
bridged or not.
>> +static int __hellcreek_fdb_del(struct hellcreek *hellcreek,
>> + const struct hellcreek_fdb_entry *entry)
>> +{
>> + dev_dbg(hellcreek->dev, "Delete FDB entry: MAC=%pM!\n", entry->mac);
>> +
>
> Do these dev_dbg statements bring much value at all, even to you?
Yes, they do. See the log snippet above.
>> +/* Default setup for DSA: VLAN <X>: CPU and Port <X> egress untagged. */
>> +static int hellcreek_setup_vlan_membership(struct dsa_switch *ds, int port,
>> + bool enabled)
>
> This function always returns zero, so it should be void.
Yes. I noticed that as well and wanted to fix it before sending. Sorry, my bad.
>> +static int hellcreek_vlan_filtering(struct dsa_switch *ds, int port,
>> + bool vlan_filtering)
>> +{
>> + struct hellcreek *hellcreek = ds->priv;
>> +
>> + dev_dbg(hellcreek->dev, "%s VLAN filtering on port %d\n",
>> + vlan_filtering ? "Enable" : "Disable", port);
>> +
>> + /* Configure port to drop packages with not known vids */
>> + hellcreek_setup_ingressflt(hellcreek, port, vlan_filtering);
>> +
>> + /* Drop DSA vlan membership config. The user can now do it. */
>> + hellcreek_setup_vlan_membership(ds, port, !vlan_filtering);
>> +
>> + /* Apply saved vlan configurations while not filtering for port <X>. */
>> + hellcreek_apply_vlan_filtering(hellcreek, port, vlan_filtering);
>> +
>> + /* Do the same for the cpu port. */
>> + hellcreek_apply_vlan_filtering(hellcreek, CPU_PORT, vlan_filtering);
>
> I think we should create a DSA_NOTIFIER_VLAN_FILTERING so you wouldn't
> have to do this, but not now.
OK.
>> +static int hellcreek_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct hellcreek *hellcreek;
>> + struct resource *res;
>> + int ret, i;
>> +
>> + hellcreek = devm_kzalloc(dev, sizeof(*hellcreek), GFP_KERNEL);
>> + if (!hellcreek)
>> + return -ENOMEM;
>> +
>> + hellcreek->vidmbrcfg = devm_kcalloc(dev, 4096,
>
> VLAN_N_VID
Thanks!
>> +static const struct hellcreek_platform_data de1soc_r1_pdata = {
>> + .num_ports = 4,
>> + .is_100_mbits = 1,
>> + .qbv_support = 1,
>> + .qbv_on_cpu_port = 1,
>
> Why does this matter?
Because Qbv on the CPU port is a feature and not all switch variants
have that. It will matter as soon as TAPRIO is implemented.
>> +#include <linux/bitops.h>
>> +#include <linux/kernel.h>
>> +#include <linux/device.h>
>> +#include <linux/ptp_clock_kernel.h>
>> +#include <linux/timecounter.h>
>> +#include <linux/mutex.h>
>
> Could you sort alphabetically?
Sure.
>
>> +#include <linux/platform_data/hirschmann-hellcreek.h>
>> +#include <net/dsa.h>
>> +
>> +/* Ports:
>> + * - 0: CPU
>> + * - 1: Tunnel
>> + * - 2: TSN front port 1
>> + * - 3: TSN front port 2
>> + * - ...
>> + */
>> +#define CPU_PORT 0
>> +#define TUNNEL_PORT 1
>
> What's a tunnel port exactly?
AFAIK it's a debugging port for mirroring or looping traffic. Anyhow,
that is not a regular port and cannot be treated as such.
Thanks,
Kurt
Attachment:
signature.asc
Description: PGP signature
