On Sun, 25 Aug 2019 at 21:46, Vladimir Oltean <olte...@gmail.com> wrote:
>
> The bridge core assumes that enabling/disabling vlan_filtering will
> translate into the simple toggling of a flag for switchdev drivers.
>
> That is clearly not the case for sja1105, which alters the VLAN table
> and the pvids in order to obtain port separation in standalone mode.
>
> There are 2 parts to the issue.
>
> First, tag_8021q changes the pvid to a unique per-port rx_vid for frame
> identification. But we need to disable tag_8021q when vlan_filtering
> kicks in, and at that point, the VLAN configured as pvid will have to be
> removed from the filtering table of the ports. With an invalid pvid, the
> ports will drop all traffic. Since the bridge will not call any vlan
> operation through switchdev after enabling vlan_filtering, we need to
> ensure we're in a functional state ourselves. Hence read the pvid that
> the bridge is aware of, and program that into our ports.
>
> Secondly, tag_8021q uses the 1024-3071 range privately in
> vlan_filtering=0 mode. Had the user installed one of these VLANs during
> a previous vlan_filtering=1 session, then upon the next tag_8021q
> cleanup for vlan_filtering to kick in again, VLANs in that range will
> get deleted unconditionally, hence breaking user expectation. So when
> deleting the VLANs, check if the bridge had knowledge about them, and if
> it did, re-apply the settings. Wrap this logic inside a
> dsa_8021q_vid_apply helper function to reduce code duplication.
>
> Signed-off-by: Vladimir Oltean <olte...@gmail.com>
> ---
> net/dsa/tag_8021q.c | 91 +++++++++++++++++++++++++++++++++++----------
> 1 file changed, 71 insertions(+), 20 deletions(-)
>
> diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
> index 67a1bc635a7b..81f943e365b9 100644
> --- a/net/dsa/tag_8021q.c
> +++ b/net/dsa/tag_8021q.c
> @@ -93,6 +93,68 @@ int dsa_8021q_rx_source_port(u16 vid)
> }
> EXPORT_SYMBOL_GPL(dsa_8021q_rx_source_port);
>
> +static int dsa_8021q_restore_pvid(struct dsa_switch *ds, int port)
> +{
> + struct bridge_vlan_info vinfo;
> + struct net_device *slave;
> + u16 pvid;
> + int err;
> +
> + if (!dsa_is_user_port(ds, port))
> + return 0;
> +
> + slave = ds->ports[port].slave;
> +
> + err = br_vlan_get_pvid(slave, &pvid);
> + if (err < 0) {
> + dev_err(ds->dev, "Couldn't determine bridge PVID\n");
> + return err;
I now realize that I shouldn't error out here, since it is not an
error to not have a pvid on the port. Will make this change in v3,
along with the other change requests that will arise upon review.
> + }
> +
> + err = br_vlan_get_info(slave, pvid, &vinfo);
> + if (err < 0) {
> + dev_err(ds->dev, "Couldn't determine PVID attributes\n");
> + return err;
> + }
> +
> + return dsa_port_vid_add(&ds->ports[port], pvid, vinfo.flags);
> +}
> +
> +/* If @enabled is true, installs @vid with @flags into the switch port's HW
> + * filter.
> + * If @enabled is false, deletes @vid (ignores @flags) from the port. Had the
> + * user explicitly configured this @vid through the bridge core, then the
> @vid
> + * is installed again, but this time with the flags from the bridge layer.
> + */
> +static int dsa_8021q_vid_apply(struct dsa_switch *ds, int port, u16 vid,
> + u16 flags, bool enabled)
> +{
> + struct dsa_port *dp = &ds->ports[port];
> + struct bridge_vlan_info vinfo;
> + int err;
> +
> + if (enabled)
> + return dsa_port_vid_add(dp, vid, flags);
> +
> + err = dsa_port_vid_del(dp, vid);
> + if (err < 0)
> + return err;
> +
> + /* Nothing to restore from the bridge for a non-user port */
> + if (!dsa_is_user_port(ds, port))
> + return 0;
> +
> + err = br_vlan_get_info(dp->slave, vid, &vinfo);
> + /* Couldn't determine bridge attributes for this vid,
> + * it means the bridge had not configured it.
> + */
> + if (err < 0)
> + return 0;
> +
> + /* Restore the VID from the bridge */
> + return dsa_port_vid_add(dp, vid, vinfo.flags);
> +}
> +
> /* RX VLAN tagging (left) and TX VLAN tagging (right) setup shown for a
> single
> * front-panel switch port (here swp0).
> *
> @@ -148,8 +210,6 @@ EXPORT_SYMBOL_GPL(dsa_8021q_rx_source_port);
> int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool
> enabled)
> {
> int upstream = dsa_upstream_port(ds, port);
> - struct dsa_port *dp = &ds->ports[port];
> - struct dsa_port *upstream_dp = &ds->ports[upstream];
> u16 rx_vid = dsa_8021q_rx_vid(ds, port);
> u16 tx_vid = dsa_8021q_tx_vid(ds, port);
> int i, err;
> @@ -166,7 +226,6 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds,
> int port, bool enabled)
> * restrictions, so there are no concerns about leaking traffic.
> */
> for (i = 0; i < ds->num_ports; i++) {
> - struct dsa_port *other_dp = &ds->ports[i];
> u16 flags;
>
> if (i == upstream)
> @@ -179,10 +238,7 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds,
> int port, bool enabled)
> /* The RX VID is a regular VLAN on all others */
> flags = BRIDGE_VLAN_INFO_UNTAGGED;
>
> - if (enabled)
> - err = dsa_port_vid_add(other_dp, rx_vid, flags);
> - else
> - err = dsa_port_vid_del(other_dp, rx_vid);
> + err = dsa_8021q_vid_apply(ds, i, rx_vid, flags, enabled);
> if (err) {
> dev_err(ds->dev, "Failed to apply RX VID %d to port
> %d: %d\n",
> rx_vid, port, err);
> @@ -193,10 +249,7 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds,
> int port, bool enabled)
> /* CPU port needs to see this port's RX VID
> * as tagged egress.
> */
> - if (enabled)
> - err = dsa_port_vid_add(upstream_dp, rx_vid, 0);
> - else
> - err = dsa_port_vid_del(upstream_dp, rx_vid);
> + err = dsa_8021q_vid_apply(ds, upstream, rx_vid, 0, enabled);
> if (err) {
> dev_err(ds->dev, "Failed to apply RX VID %d to port %d: %d\n",
> rx_vid, port, err);
> @@ -204,26 +257,24 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds,
> int port, bool enabled)
> }
>
> /* Finally apply the TX VID on this port and on the CPU port */
> - if (enabled)
> - err = dsa_port_vid_add(dp, tx_vid, BRIDGE_VLAN_INFO_UNTAGGED);
> - else
> - err = dsa_port_vid_del(dp, tx_vid);
> + err = dsa_8021q_vid_apply(ds, port, tx_vid, BRIDGE_VLAN_INFO_UNTAGGED,
> + enabled);
> if (err) {
> dev_err(ds->dev, "Failed to apply TX VID %d on port %d: %d\n",
> tx_vid, port, err);
> return err;
> }
> - if (enabled)
> - err = dsa_port_vid_add(upstream_dp, tx_vid, 0);
> - else
> - err = dsa_port_vid_del(upstream_dp, tx_vid);
> + err = dsa_8021q_vid_apply(ds, upstream, tx_vid, 0, enabled);
> if (err) {
> dev_err(ds->dev, "Failed to apply TX VID %d on port %d: %d\n",
> tx_vid, upstream, err);
> return err;
> }
>
> - return 0;
> + if (!enabled)
> + err = dsa_8021q_restore_pvid(ds, port);
> +
> + return err;
> }
> EXPORT_SYMBOL_GPL(dsa_port_setup_8021q_tagging);
>
> --
> 2.17.1
>
Thanks!
-Vladimir
