Bridge-vlan bug? (mt7621/DSA)
Thibaut
hacks at slashdirt.org
Wed Aug 10 08:07:09 PDT 2022
[CC’ing DENG Qingfang <dqfext at gmail.com>]
Hi Felix,
> Le 9 août 2022 à 17:37, Felix Fietkau <nbd at nbd.name> a écrit :
>
>
> On 09.08.22 15:13, Thibaut wrote:
>>> Le 6 août 2022 à 11:58, Thibaut <hacks at slashdirt.org> a écrit :
>>>> Le 6 août 2022 à 00:50, Mark Mentovai <mark at mentovai.com> a écrit :
>>>> Thibaut wrote:
>>>>> I’m experiencing a strange bug on Yuncore AX820 (mt7621/mt7905/mt7975, DSA-enabled) when using a bridge-vlan setup. This bug affects at least OpenWRT 22.03.0-rc6.
>>>>> I’m not sure whether this bug is related to this particular SoC or only to DSA as I was unable to test with another DSA-enabled device (I don’t have any). However this bug does not affect e.g. QCA non-DSA devices.
>>>>> I’m running out of ideas on how to further debug this problem, so feel free to guide me if more information is needed. Please CC-me in replies.
>>>> This sounds very similar to the problem I experienced with the work-in-progress DSA patches for ipq40xx:
>>>> https://github.com/openwrt/openwrt/pull/4721#issuecomment-971162067
>>>> This kernel patch explains the situation fairly well:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d5f19486cee79d04c054427577ac96ed123706db
>>>> But the fix isn’t operative unless the switch driver opts in via assisted_learning_on_cpu_port. There were also comments from around that time that there may still be trouble with untagged traffic.
>>>> There’s a bit of discussion about this issue in the comments around there on the pull request. Hopefully you’ll find it helpful. It should at least get you oriented in the right direction, even if it’s not a fix for your untagged use case.
>>> Thanks a lot for these details. Based on your input and looking at our current 5.10 source and the current upstream, it seems this might have already been fixed upstream:
>>> https://github.com/torvalds/linux/commit/0b69c54c74bcb60e834013ccaf596caf05156a8e
>>> I’ll check if this can be backported without too much fuss.
>> Backport submitted:
>> http://patchwork.ozlabs.org/project/openwrt/patch/20220809125947.31775-1-hacks@slashdirt.org/
>> It fixes the issue and applies cleanly to 22.03: I have run tests in both master and 22.03.
>> I will now check if I can make it work in 21.02 which is also affected.
> Please take a look at
> https://git.openwrt.org/3e0daca6447c3d5b9eb6d24ecb8e52f256f385cc
>
> The changes were backported once already and reverted due to issues.
> See also: https://github.com/openwrt/openwrt/issues/9420
I tested current master with the testing kernel (5.15) and noticed that I couldn’t reproduce the bugs (i.e. everything seemed properly working).
So I set out to backport as much as I possibly could into 5.10 so as to make the mt7530 DSA driver there look almost identical to the 5.15 version, save of course for the broader kernel API changes.
The resulting changeset is available here: https://github.com/f00b4r0/openwrt/commit/426d8caa08ce3e0c7951866953012bc05d990880
Unfortunately the result was still exposing the throughput issue.
Below is the remainder diff of the resulting 5.10+426d8caa0 vs current master's 5.15.
The most salient differences that I can spot are:
- the leftover bits from https://github.com/torvalds/linux/commit/5a30833b9a16f8d1aa15de06636f9317ca51f9df, which probably cannot be backported since they relate to the new bridge flags API;
- the removal of the switchdev transactions (although if I read the code correctly they don’t do anything in mt7530?);
- the changes related to https://github.com/torvalds/linux/commit/b7a9e0da2d1c954b7c38217a29e002528b90d174, which are quite invasive
At this point I’m not really sure which of the above is the most likely culprit, or if we are seeing something completely unrelated to the driver itself.
I’m willing to keep looking so we can fix this for 22.03, although I would certainly appreciate some guidance from people with actual knowledge of this hardware and subsystem (that’s not me :), hence CC’ing DENG Qingfang.
HTH,
Thibaut
--- openwrt/build_dir/target-mipsel_24kc_musl/linux-ramips_mt7621/linux-5.10.135/drivers/net/dsa/mt7530.c 2022-08-10 15:52:40.041416102 +0200
+++ dsa-5.15/mt7530.c 2022-08-03 12:03:56.000000000 +0200
@@ -558,7 +558,7 @@ mt7531_pad_setup(struct dsa_switch *ds,
val |= 0x190000 << RG_COREPLL_SDM_PCW_S;
mt7530_write(priv, MT7531_PLLGP_CR0, val);
break;
- };
+ }
/* Set feedback divide ratio update signal to high */
val = mt7530_read(priv, MT7531_PLLGP_CR0);
@@ -1008,6 +1008,10 @@ mt753x_cpu_port_enable(struct dsa_switch
mt7530_write(priv, MT7530_PVC_P(port),
PORT_SPEC_TAG);
+ /* Disable flooding by default */
+ mt7530_rmw(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK | UNU_FFP_MASK,
+ BC_FFP(BIT(port)) | UNM_FFP(BIT(port)) | UNU_FFP(BIT(port)));
+
/* Set CPU port number */
if (priv->id == ID_MT7621)
mt7530_rmw(priv, MT7530_MFC, CPU_MASK, CPU_EN | CPU_PORT(port));
@@ -1143,15 +1147,39 @@ mt7530_stp_state_set(struct dsa_switch *
}
static int
-mt7530_port_egress_floods(struct dsa_switch *ds, int port,
- bool unicast, bool multicast)
+mt7530_port_pre_bridge_flags(struct dsa_switch *ds, int port,
+ struct switchdev_brport_flags flags,
+ struct netlink_ext_ack *extack)
+{
+ if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD |
+ BR_BCAST_FLOOD))
+ return -EINVAL;
+
+ return 0;
+}
+
+static int
+mt7530_port_bridge_flags(struct dsa_switch *ds, int port,
+ struct switchdev_brport_flags flags,
+ struct netlink_ext_ack *extack)
{
struct mt7530_priv *priv = ds->priv;
- mt7530_rmw(priv, MT7530_MFC,
- UNU_FFP(BIT(port)) | UNM_FFP(BIT(port)),
- (unicast ? UNU_FFP(BIT(port)) : 0) |
- (multicast ? UNM_FFP(BIT(port)) : 0));
+ if (flags.mask & BR_LEARNING)
+ mt7530_rmw(priv, MT7530_PSC_P(port), SA_DIS,
+ flags.val & BR_LEARNING ? 0 : SA_DIS);
+
+ if (flags.mask & BR_FLOOD)
+ mt7530_rmw(priv, MT7530_MFC, UNU_FFP(BIT(port)),
+ flags.val & BR_FLOOD ? UNU_FFP(BIT(port)) : 0);
+
+ if (flags.mask & BR_MCAST_FLOOD)
+ mt7530_rmw(priv, MT7530_MFC, UNM_FFP(BIT(port)),
+ flags.val & BR_MCAST_FLOOD ? UNM_FFP(BIT(port)) : 0);
+
+ if (flags.mask & BR_BCAST_FLOOD)
+ mt7530_rmw(priv, MT7530_MFC, BC_FFP(BIT(port)),
+ flags.val & BR_BCAST_FLOOD ? BC_FFP(BIT(port)) : 0);
return 0;
}
@@ -1379,13 +1407,6 @@ err:
}
static int
-mt7530_port_mdb_prepare(struct dsa_switch *ds, int port,
- const struct switchdev_obj_port_mdb *mdb)
-{
- return 0;
-}
-
-static void
mt7530_port_mdb_add(struct dsa_switch *ds, int port,
const struct switchdev_obj_port_mdb *mdb)
{
@@ -1393,6 +1414,7 @@ mt7530_port_mdb_add(struct dsa_switch *d
const u8 *addr = mdb->addr;
u16 vid = mdb->vid;
u8 port_mask = 0;
+ int ret;
mutex_lock(&priv->reg_mutex);
@@ -1403,9 +1425,11 @@ mt7530_port_mdb_add(struct dsa_switch *d
port_mask |= BIT(port);
mt7530_fdb_write(priv, vid, port_mask, addr, -1, STATIC_ENT);
- mt7530_fdb_cmd(priv, MT7530_FDB_WRITE, NULL);
+ ret = mt7530_fdb_cmd(priv, MT7530_FDB_WRITE, NULL);
mutex_unlock(&priv->reg_mutex);
+
+ return ret;
}
static int
@@ -1463,13 +1487,9 @@ mt7530_vlan_cmd(struct mt7530_priv *priv
}
static int
-mt7530_port_vlan_filtering(struct dsa_switch *ds, int port,
- bool vlan_filtering,
- struct switchdev_trans *trans)
+mt7530_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
+ struct netlink_ext_ack *extack)
{
- if (switchdev_trans_ph_prepare(trans))
- return 0;
-
if (vlan_filtering) {
/* The port is being kept as VLAN-unaware port when bridge is
* set up with vlan_filtering not being set, Otherwise, the
@@ -1485,15 +1505,6 @@ mt7530_port_vlan_filtering(struct dsa_sw
return 0;
}
-static int
-mt7530_port_vlan_prepare(struct dsa_switch *ds, int port,
- const struct switchdev_obj_port_vlan *vlan)
-{
- /* nothing needed */
-
- return 0;
-}
-
static void
mt7530_hw_vlan_add(struct mt7530_priv *priv,
struct mt7530_hw_vlan_entry *entry)
@@ -1582,26 +1593,38 @@ mt7530_hw_vlan_update(struct mt7530_priv
mt7530_vlan_cmd(priv, MT7530_VTCR_WR_VID, vid);
}
-static void
+static int
+mt7530_setup_vlan0(struct mt7530_priv *priv)
+{
+ u32 val;
+
+ /* Validate the entry with independent learning, keep the original
+ * ingress tag attribute.
+ */
+ val = IVL_MAC | EG_CON | PORT_MEM(MT7530_ALL_MEMBERS) | FID(FID_BRIDGED) |
+ VLAN_VALID;
+ mt7530_write(priv, MT7530_VAWD1, val);
+
+ return mt7530_vlan_cmd(priv, MT7530_VTCR_WR_VID, 0);
+}
+
+static int
mt7530_port_vlan_add(struct dsa_switch *ds, int port,
- const struct switchdev_obj_port_vlan *vlan)
+ const struct switchdev_obj_port_vlan *vlan,
+ struct netlink_ext_ack *extack)
{
bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
struct mt7530_hw_vlan_entry new_entry;
struct mt7530_priv *priv = ds->priv;
- u16 vid;
mutex_lock(&priv->reg_mutex);
- for (vid = vlan->vid_begin; vid <= vlan->vid_end; ++vid) {
- mt7530_hw_vlan_entry_init(&new_entry, port, untagged);
- mt7530_hw_vlan_update(priv, vid, &new_entry,
- mt7530_hw_vlan_add);
- }
+ mt7530_hw_vlan_entry_init(&new_entry, port, untagged);
+ mt7530_hw_vlan_update(priv, vlan->vid, &new_entry, mt7530_hw_vlan_add);
if (pvid) {
- priv->ports[port].pvid = vlan->vid_end;
+ priv->ports[port].pvid = vlan->vid;
/* Accept all frames if PVID is set */
mt7530_rmw(priv, MT7530_PVC_P(port), ACC_FRM_MASK,
@@ -1611,8 +1634,8 @@ mt7530_port_vlan_add(struct dsa_switch *
if (dsa_port_is_vlan_filtering(dsa_to_port(ds, port)))
mt7530_rmw(priv, MT7530_PPBV1_P(port),
G0_PORT_VID_MASK,
- G0_PORT_VID(vlan->vid_end));
- } else if (vlan->vid_end && priv->ports[port].pvid == vlan->vid_end) {
+ G0_PORT_VID(vlan->vid));
+ } else if (vlan->vid && priv->ports[port].pvid == vlan->vid) {
/* This VLAN is overwritten without PVID, so unset it */
priv->ports[port].pvid = G0_PORT_VID_DEF;
@@ -1626,21 +1649,8 @@ mt7530_port_vlan_add(struct dsa_switch *
}
mutex_unlock(&priv->reg_mutex);
-}
-
-static int
-mt7530_setup_vlan0(struct mt7530_priv *priv)
-{
- u32 val;
-
- /* Validate the entry with independent learning, keep the original
- * ingress tag attribute.
- */
- val = IVL_MAC | EG_CON | PORT_MEM(MT7530_ALL_MEMBERS) | FID(FID_BRIDGED) |
- VLAN_VALID;
- mt7530_write(priv, MT7530_VAWD1, val);
- return mt7530_vlan_cmd(priv, MT7530_VTCR_WR_VID, 0);
+ return 0;
}
static int
@@ -1649,29 +1659,26 @@ mt7530_port_vlan_del(struct dsa_switch *
{
struct mt7530_hw_vlan_entry target_entry;
struct mt7530_priv *priv = ds->priv;
- u16 vid;
mutex_lock(&priv->reg_mutex);
- for (vid = vlan->vid_begin; vid <= vlan->vid_end; ++vid) {
- mt7530_hw_vlan_entry_init(&target_entry, port, 0);
- mt7530_hw_vlan_update(priv, vid, &target_entry,
- mt7530_hw_vlan_del);
+ mt7530_hw_vlan_entry_init(&target_entry, port, 0);
+ mt7530_hw_vlan_update(priv, vlan->vid, &target_entry,
+ mt7530_hw_vlan_del);
- /* PVID is being restored to the default whenever the PVID port
- * is being removed from the VLAN.
- */
- if (priv->ports[port].pvid == vlan->vid_end) {
- priv->ports[port].pvid = G0_PORT_VID_DEF;
+ /* PVID is being restored to the default whenever the PVID port
+ * is being removed from the VLAN.
+ */
+ if (priv->ports[port].pvid == vlan->vid) {
+ priv->ports[port].pvid = G0_PORT_VID_DEF;
- /* Only accept tagged frames if the port is VLAN-aware */
- if (dsa_port_is_vlan_filtering(dsa_to_port(ds, port)))
- mt7530_rmw(priv, MT7530_PVC_P(port), ACC_FRM_MASK,
- MT7530_VLAN_ACC_TAGGED);
+ /* Only accept tagged frames if the port is VLAN-aware */
+ if (dsa_port_is_vlan_filtering(dsa_to_port(ds, port)))
+ mt7530_rmw(priv, MT7530_PVC_P(port), ACC_FRM_MASK,
+ MT7530_VLAN_ACC_TAGGED);
- mt7530_rmw(priv, MT7530_PPBV1_P(port), G0_PORT_VID_MASK,
- G0_PORT_VID_DEF);
- }
+ mt7530_rmw(priv, MT7530_PPBV1_P(port), G0_PORT_VID_MASK,
+ G0_PORT_VID_DEF);
}
@@ -1757,17 +1764,10 @@ static enum dsa_tag_protocol
mtk_get_tag_protocol(struct dsa_switch *ds, int port,
enum dsa_tag_protocol mp)
{
- struct mt7530_priv *priv = ds->priv;
-
- if (port != MT7530_CPU_PORT) {
- dev_warn(priv->dev,
- "port not matched with tagging CPU port\n");
- return DSA_TAG_PROTO_NONE;
- } else {
- return DSA_TAG_PROTO_MTK;
- }
+ return DSA_TAG_PROTO_MTK;
}
+#ifdef CONFIG_GPIOLIB
static inline u32
mt7530_gpio_to_bit(unsigned int offset)
{
@@ -1870,6 +1870,7 @@ mt7530_setup_gpio(struct mt7530_priv *pr
return devm_gpiochip_add_data(dev, gc, priv);
}
+#endif /* CONFIG_GPIOLIB */
static irqreturn_t
mt7530_irq_thread_fn(int irq, void *dev_id)
@@ -2092,7 +2093,6 @@ mt7530_setup(struct dsa_switch *ds)
* as two netdev instances.
*/
dn = dsa_to_port(ds, MT7530_CPU_PORT)->master->dev.of_node->parent;
- ds->configure_vlan_while_not_filtering = true;
ds->assisted_learning_on_cpu_port = true;
ds->mtu_enforcement_ingress = true;
@@ -2178,7 +2178,6 @@ mt7530_setup(struct dsa_switch *ds)
mt7530_rmw(priv, MT7530_PPBV1_P(i), G0_PORT_VID_MASK,
G0_PORT_VID_DEF);
}
-
/* Enable consistent egress tag */
mt7530_rmw(priv, MT7530_PVC_P(i), PVC_EG_TAG_MASK,
PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));
@@ -2232,11 +2231,13 @@ mt7530_setup(struct dsa_switch *ds)
}
}
+#ifdef CONFIG_GPIOLIB
if (of_property_read_bool(priv->dev->of_node, "gpio-controller")) {
ret = mt7530_setup_gpio(priv);
if (ret)
return ret;
}
+#endif /* CONFIG_GPIOLIB */
mt7530_setup_port5(ds, interface);
@@ -2365,7 +2366,6 @@ mt7531_setup(struct dsa_switch *ds)
if (ret)
return ret;
- ds->configure_vlan_while_not_filtering = true;
ds->assisted_learning_on_cpu_port = true;
ds->mtu_enforcement_ingress = true;
@@ -3112,17 +3112,16 @@ static const struct dsa_switch_ops mt753
.port_change_mtu = mt7530_port_change_mtu,
.port_max_mtu = mt7530_port_max_mtu,
.port_stp_state_set = mt7530_stp_state_set,
- .port_egress_floods = mt7530_port_egress_floods,
+ .port_pre_bridge_flags = mt7530_port_pre_bridge_flags,
+ .port_bridge_flags = mt7530_port_bridge_flags,
.port_bridge_join = mt7530_port_bridge_join,
.port_bridge_leave = mt7530_port_bridge_leave,
.port_fdb_add = mt7530_port_fdb_add,
.port_fdb_del = mt7530_port_fdb_del,
.port_fdb_dump = mt7530_port_fdb_dump,
- .port_mdb_prepare = mt7530_port_mdb_prepare,
.port_mdb_add = mt7530_port_mdb_add,
.port_mdb_del = mt7530_port_mdb_del,
.port_vlan_filtering = mt7530_port_vlan_filtering,
- .port_vlan_prepare = mt7530_port_vlan_prepare,
.port_vlan_add = mt7530_port_vlan_add,
.port_vlan_del = mt7530_port_vlan_del,
.port_mirror_add = mt753x_port_mirror_add,
@@ -3276,6 +3275,9 @@ mt7530_remove(struct mdio_device *mdiode
struct mt7530_priv *priv = dev_get_drvdata(&mdiodev->dev);
int ret = 0;
+ if (!priv)
+ return;
+
ret = regulator_disable(priv->core_pwr);
if (ret < 0)
dev_err(priv->dev,
@@ -3291,11 +3293,26 @@ mt7530_remove(struct mdio_device *mdiode
dsa_unregister_switch(priv->ds);
mutex_destroy(&priv->reg_mutex);
+
+ dev_set_drvdata(&mdiodev->dev, NULL);
+}
+
+static void mt7530_shutdown(struct mdio_device *mdiodev)
+{
+ struct mt7530_priv *priv = dev_get_drvdata(&mdiodev->dev);
+
+ if (!priv)
+ return;
+
+ dsa_switch_shutdown(priv->ds);
+
+ dev_set_drvdata(&mdiodev->dev, NULL);
}
static struct mdio_driver mt7530_mdio_driver = {
.probe = mt7530_probe,
.remove = mt7530_remove,
+ .shutdown = mt7530_shutdown,
.mdiodrv.driver = {
.name = "mt7530",
.of_match_table = mt7530_of_match,
More information about the openwrt-devel
mailing list