On Fri, Jul 29, 2022 at 08:47:33PM +0530, Arun Ramadoss wrote:
quoted hunk
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 516fb9d35c87..8a5583b1f2f4 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -161,6 +161,7 @@ static const struct ksz_dev_ops ksz8_dev_ops = {
.vlan_filtering = ksz8_port_vlan_filtering,
.vlan_add = ksz8_port_vlan_add,
.vlan_del = ksz8_port_vlan_del,
+ .drop_untagged = ksz8_port_enable_pvid,
You'll have to explain this one. What impact does PVID insertion on KSZ8
have upon dropping/not dropping untagged packets? This patch is saying
that when untagged packets should be dropped, PVID insertion should be
enabled, and when untagged packets should be accepted, PVID insertion
should be disabled. How come?
quoted hunk
.mirror_add = ksz8_port_mirror_add,
.mirror_del = ksz8_port_mirror_del,
.get_caps = ksz8_get_caps,
@@ -187,6 +188,7 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
.vlan_filtering = ksz9477_port_vlan_filtering,
.vlan_add = ksz9477_port_vlan_add,
.vlan_del = ksz9477_port_vlan_del,
+ .drop_untagged = ksz9477_port_drop_untagged,
.mirror_add = ksz9477_port_mirror_add,
.mirror_del = ksz9477_port_mirror_del,
.get_caps = ksz9477_get_caps,@@ -220,6 +222,7 @@ static const struct ksz_dev_ops lan937x_dev_ops = {
.vlan_filtering = ksz9477_port_vlan_filtering,
.vlan_add = ksz9477_port_vlan_add,
.vlan_del = ksz9477_port_vlan_del,
+ .drop_untagged = ksz9477_port_drop_untagged,
.mirror_add = ksz9477_port_mirror_add,
.mirror_del = ksz9477_port_mirror_del,
.get_caps = lan937x_phylink_get_caps,@@ -1254,6 +1257,9 @@ static int ksz_enable_port(struct dsa_switch *ds, int port,
{
struct ksz_device *dev = ds->priv;
+ dev->dev_ops->vlan_add(dev, port, KSZ_DEFAULT_VLAN,
+ BRIDGE_VLAN_INFO_UNTAGGED);
+
How many times can this be executed before the VLAN add operation fails
due to the VLAN already being present on the port? I notice you're
ignoring the return code. Wouldn't it be better to do this at
port_setup() time instead?
(side note, the PVID for standalone mode can be added at port_setup
time. The PVID to use for bridges in VLAN-unaware mode can be allocated
at port_bridge_join time)
if (!dsa_is_user_port(ds, port))
return 0;
+static int ksz_commit_pvid(struct dsa_switch *ds, int port)
+{
+ struct dsa_port *dp = dsa_to_port(ds, port);
+ struct net_device *br = dsa_port_bridge_dev_get(dp);
+ struct ksz_device *dev = ds->priv;
+ u16 pvid = KSZ_DEFAULT_VLAN;
+ bool drop_untagged = false;
+ struct ksz_port *p;
+
+ p = &dev->ports[port];
+
+ if (br && br_vlan_enabled(br)) {
+ pvid = p->bridge_pvid.vid;
+ drop_untagged = !p->bridge_pvid.valid;
+ }
This is better in the sense that it resolves the need for the
configure_vlan_while_not_filtering hack. But standalone and VLAN-unaware
bridge ports still share the same PVID. Even more so, standalone ports
have address learning enabled, which will poison the address database of
VLAN-unaware bridge ports (and of other standalone ports):
https://patchwork.kernel.org/project/netdevbpf/patch/20220802002636.3963025-1-vladimir.oltean@nxp.com/
Are you going to do further work in this area?
+
+ ksz_set_pvid(dev, port, pvid);
+
+ if (dev->dev_ops->drop_untagged)
+ dev->dev_ops->drop_untagged(dev, port, drop_untagged);
+
+ return 0;
+}
On Tue, 2022-08-02 at 13:59 +0300, Vladimir Oltean wrote:
EXTERNAL EMAIL: Do not click links or open attachments unless you
know the content is safe
On Fri, Jul 29, 2022 at 08:47:33PM +0530, Arun Ramadoss wrote:
quoted
diff --git a/drivers/net/dsa/microchip/ksz_common.c
b/drivers/net/dsa/microchip/ksz_common.c
index 516fb9d35c87..8a5583b1f2f4 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -161,6 +161,7 @@ static const struct ksz_dev_ops ksz8_dev_ops =
{
.vlan_filtering = ksz8_port_vlan_filtering,
.vlan_add = ksz8_port_vlan_add,
.vlan_del = ksz8_port_vlan_del,
+ .drop_untagged = ksz8_port_enable_pvid,
You'll have to explain this one. What impact does PVID insertion on
KSZ8
have upon dropping/not dropping untagged packets? This patch is
saying
that when untagged packets should be dropped, PVID insertion should
be
enabled, and when untagged packets should be accepted, PVID insertion
should be disabled. How come?
Its my mistake. I referred KSZ87xx datasheet but I couldn't find the
register for the dropping the untagged packet. If that is the case,
shall I remove the dropping of untagged packet feature from the ksz8
switches?
quoted
.mirror_add = ksz8_port_mirror_add,
.mirror_del = ksz8_port_mirror_del,
.get_caps = ksz8_get_caps,@@ -187,6 +188,7 @@ static const struct ksz_dev_ops ksz9477_dev_ops
= {
.vlan_filtering = ksz9477_port_vlan_filtering,
.vlan_add = ksz9477_port_vlan_add,
.vlan_del = ksz9477_port_vlan_del,
+ .drop_untagged = ksz9477_port_drop_untagged,
.mirror_add = ksz9477_port_mirror_add,
.mirror_del = ksz9477_port_mirror_del,
.get_caps = ksz9477_get_caps,@@ -220,6 +222,7 @@ static const struct ksz_dev_ops lan937x_dev_ops
= {
.vlan_filtering = ksz9477_port_vlan_filtering,
.vlan_add = ksz9477_port_vlan_add,
.vlan_del = ksz9477_port_vlan_del,
+ .drop_untagged = ksz9477_port_drop_untagged,
.mirror_add = ksz9477_port_mirror_add,
.mirror_del = ksz9477_port_mirror_del,
.get_caps = lan937x_phylink_get_caps,@@ -1254,6 +1257,9 @@ static int ksz_enable_port(struct dsa_switch
*ds, int port,
{
struct ksz_device *dev = ds->priv;
+ dev->dev_ops->vlan_add(dev, port, KSZ_DEFAULT_VLAN,
+ BRIDGE_VLAN_INFO_UNTAGGED);
+
How many times can this be executed before the VLAN add operation
fails
due to the VLAN already being present on the port? I notice you're
ignoring the return code. Wouldn't it be better to do this at
port_setup() time instead?
(side note, the PVID for standalone mode can be added at port_setup
time. The PVID to use for bridges in VLAN-unaware mode can be
allocated
at port_bridge_join time)
Ok. I will add in port_bridge_join function for bridge vlan_aware
ports.
quoted
if (!dsa_is_user_port(ds, port))
return 0;
+static int ksz_commit_pvid(struct dsa_switch *ds, int port)
+{
+ struct dsa_port *dp = dsa_to_port(ds, port);
+ struct net_device *br = dsa_port_bridge_dev_get(dp);
+ struct ksz_device *dev = ds->priv;
+ u16 pvid = KSZ_DEFAULT_VLAN;
+ bool drop_untagged = false;
+ struct ksz_port *p;
+
+ p = &dev->ports[port];
+
+ if (br && br_vlan_enabled(br)) {
+ pvid = p->bridge_pvid.vid;
+ drop_untagged = !p->bridge_pvid.valid;
+ }
This is better in the sense that it resolves the need for the
configure_vlan_while_not_filtering hack. But standalone and VLAN-
unaware
bridge ports still share the same PVID. Even more so, standalone
ports
have address learning enabled, which will poison the address database
of
VLAN-unaware bridge ports (and of other standalone ports):
https://patchwork.kernel.org/project/netdevbpf/patch/20220802002636.3963025-1-vladimir.oltean@nxp.com/
Are you going to do further work in this area?
For now, I thought I can fix the issue for bridge vlan unaware port. I
have few other patch series to be submitted like gPTP, tc commands. If
standalone port fix also needed for your patch series I can work on it
otherwise I can take up later stage.
quoted
+
+ ksz_set_pvid(dev, port, pvid);
+
+ if (dev->dev_ops->drop_untagged)
+ dev->dev_ops->drop_untagged(dev, port,
drop_untagged);
+
+ return 0;
+}
On Tue, Aug 02, 2022 at 02:40:09PM +0000, Arun.Ramadoss@microchip.com wrote:
On Tue, 2022-08-02 at 13:59 +0300, Vladimir Oltean wrote:
quoted
EXTERNAL EMAIL: Do not click links or open attachments unless you
know the content is safe
On Fri, Jul 29, 2022 at 08:47:33PM +0530, Arun Ramadoss wrote:
quoted
diff --git a/drivers/net/dsa/microchip/ksz_common.c
b/drivers/net/dsa/microchip/ksz_common.c
index 516fb9d35c87..8a5583b1f2f4 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -161,6 +161,7 @@ static const struct ksz_dev_ops ksz8_dev_ops =
{
.vlan_filtering = ksz8_port_vlan_filtering,
.vlan_add = ksz8_port_vlan_add,
.vlan_del = ksz8_port_vlan_del,
+ .drop_untagged = ksz8_port_enable_pvid,
You'll have to explain this one. What impact does PVID insertion on KSZ8
have upon dropping/not dropping untagged packets? This patch is saying
that when untagged packets should be dropped, PVID insertion should be
enabled, and when untagged packets should be accepted, PVID insertion
should be disabled. How come?
Its my mistake. I referred KSZ87xx datasheet but I couldn't find the
register for the dropping the untagged packet. If that is the case,
shall I remove the dropping of untagged packet feature from the ksz8
switches?
You'll have to see how KSZ8 behaves when the ingress port is configured
with a PVID (through REG_PORT_CTRL_VID) which isn't present in the VLAN
table. If untagged packets are dropped, that's your "drop untagged"
setting. Some other switches will not do this, and accept untagged
packets even if the VLAN table doesn't contain an entry for the PVID
(or doesn't have this port as a member of that VLAN), but have a
separate knob for dropping untagged traffic.
quoted
This is better in the sense that it resolves the need for the
configure_vlan_while_not_filtering hack. But standalone and VLAN-unaware
bridge ports still share the same PVID. Even more so, standalone ports
have address learning enabled, which will poison the address database of
VLAN-unaware bridge ports (and of other standalone ports):
https://patchwork.kernel.org/project/netdevbpf/patch/20220802002636.3963025-1-vladimir.oltean@nxp.com/
quoted
Are you going to do further work in this area?
For now, I thought I can fix the issue for bridge vlan unaware port. I
have few other patch series to be submitted like gPTP, tc commands. If
standalone port fix also needed for your patch series I can work on it
otherwise I can take up later stage.
I think the most imperative thing for you to do is to make sure you are
not introducing regressions with the port default VID change - this can
be done by running the bridge related selftests (and making them pass).
Something which I forgot to mention is that normally, I'd expect a
change of VLAN-unaware PVID to also need a change in the way that
VLAN-unaware FDB entries are added (other drivers need to remap vid=0
from port_fdb_add() to the PVID that they use for that VLAN-unaware
bridge, in your case 4095, for those FDB entries to continue matching
properly).
However, I see that currently, ksz9477_fdb_add() sets the "USE FID" bit
only for VLAN-aware FDB entries (vid != 0), which leaves me with more
questions than answers.
It isn't very well explained what it means to not use FID: let's say
there are 2 entries in the static address table, one has "USE FID"=false,
and the other has "USE FID"=true and FID=127, and a packet is received
which is classified to FID 127. On which entry will this packet match?
The bridge driver gives you all FDB entries at once (VLAN-aware and
VLAN-unaware), so if the USE_FID=false entries that the ksz9477 driver
uses for VLAN-unaware mode will shadow the VLAN-aware FDB entries, this
is going to be a problem.
Also, the way in which the ksz9477 driver translates a 12-bit VID into a
7-bit FID also has me incredibly confused (FID is vlan->vid & VLAN_FID_M,
or otherwise said, a simple truncation). This means that your
VLAN-unaware PVID of 4095 uses a FID of 127, which is also the same FID
as VLANs 127, 255, 383 etc, right? So there is potentially still full
address database leakage between VLAN-unaware and VLAN-aware bridges.
I think this phrase from the documentation is under-appreciated in
understanding how the hardware works:
| Table 4-8 details the forwarding and discarding actions that are taken
| for the various VLAN scenarios. The first entry in the table is
| explained by the fact that VLAN Table lookup is enabled even when 802.1Q
| VLAN is not enabled.
The last part ("VLAN Table lookup is enabled even when 802.1Q VLAN is
not enabled") is what makes it so that the PVID of the port must be
present in the VLAN table or otherwise you get packet drops. In turn,
if the VLAN table is being looked up, it means that regardless of
whether the switch is VLAN-unaware or not, the VID will be transformed
into a 7-bit FID.
I want that the FID that is being used for standalone ports and
VLAN-unaware bridges (127) to be a fully conscious decision, with the
implications understood, and not just something done for me to shut up.
There is a risk here that you may think things are fine and work on
other features, but things are not fine at all. And in this area,
standalone ports/bridge VLANs/ FDB entries/FIDs are very inter-related
things. When you change one, you may find that the entire scheme needs
to be re-thought.