This patchset fixes various issues.
It mainly concerns VLANs support by fixing FID table management to
allow adding more than one VLAN.
It also fixes tag/untag behavior on ingress/egress packets.
Gilles DOFFE (6):
net: dsa: ksz: fix FID management
net: dsa: ksz: move tag/untag action
net: dsa: ksz: insert tag on ks8795 ingress packets
net: dsa: ksz: do not change tagging on del
net: dsa: ksz: fix wrong pvid
net: dsa: ksz: fix wrong read cast to u64
drivers/net/dsa/microchip/ksz8795.c | 71 +++++++++++++++++++++----
drivers/net/dsa/microchip/ksz8795_reg.h | 1 +
drivers/net/dsa/microchip/ksz_common.h | 3 +-
3 files changed, 63 insertions(+), 12 deletions(-)
--
2.25.1
If 802.1q VLAN tag is removed from egress traffic, ingress
traffic should by logic be tagged.
Signed-off-by: Gilles DOFFE <[email protected]>
---
drivers/net/dsa/microchip/ksz8795.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 4b060503b2e8..193f03ef9160 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -874,6 +874,7 @@ static void ksz8795_port_vlan_add(struct dsa_switch *ds, int port,
}
ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_REMOVE_TAG, untagged);
+ ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_INSERT_TAG, !untagged);
}
static int ksz8795_port_vlan_del(struct dsa_switch *ds, int port,
--
2.25.1
The FID (Filter ID) is a 7 bits field used to link the VLAN table
to the static and dynamic mac address tables.
Until now the KSZ8795 driver could only add one VLAN as the FID was
always set to 1.
This commit allows setting a FID for each new active VLAN.
The FID list is stored in a static table dynamically allocated from
ks8795_fid structure.
Each newly activated VLAN is associated to the next available FID.
Only the VLAN 0 is not added to the list as it is a special VLAN.
As it has a special meaning, see IEEE 802.1q.
When a VLAN is no more used, the associated FID table entry is reset
to 0.
Signed-off-by: Gilles DOFFE <[email protected]>
---
drivers/net/dsa/microchip/ksz8795.c | 59 +++++++++++++++++++++++--
drivers/net/dsa/microchip/ksz8795_reg.h | 1 +
drivers/net/dsa/microchip/ksz_common.h | 1 +
3 files changed, 57 insertions(+), 4 deletions(-)
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index c973db101b72..6962ba4ee125 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -648,7 +648,7 @@ static enum dsa_tag_protocol ksz8795_get_tag_protocol(struct dsa_switch *ds,
int port,
enum dsa_tag_protocol mp)
{
- return DSA_TAG_PROTO_KSZ8795;
+ return DSA_TAG_PROTO_NONE;
}
static void ksz8795_get_strings(struct dsa_switch *ds, int port,
@@ -796,6 +796,41 @@ static int ksz8795_port_vlan_filtering(struct dsa_switch *ds, int port,
return 0;
}
+static void ksz8795_del_fid(u16 *ksz_fid_table, u16 vid)
+{
+ u8 i = 0;
+
+ if (!ksz_fid_table)
+ return;
+
+ for (i = 0; i < VLAN_TABLE_FID_SIZE; i++) {
+ if (ksz_fid_table[i] == vid) {
+ ksz_fid_table[i] = 0;
+ break;
+ }
+ }
+}
+
+static int ksz8795_get_next_fid(u16 *ksz_fid_table, u16 vid, u8 *fid)
+{
+ u8 i = 0;
+ int ret = -EOVERFLOW;
+
+ if (!ksz_fid_table)
+ return ret;
+
+ for (i = 0; i < VLAN_TABLE_FID_SIZE; i++) {
+ if (!ksz_fid_table[i]) {
+ ksz_fid_table[i] = vid;
+ *fid = i;
+ ret = 0;
+ break;
+ }
+ }
+
+ return ret;
+}
+
static void ksz8795_port_vlan_add(struct dsa_switch *ds, int port,
const struct switchdev_obj_port_vlan *vlan)
{
@@ -803,17 +838,24 @@ static void ksz8795_port_vlan_add(struct dsa_switch *ds, int port,
struct ksz_device *dev = ds->priv;
u16 data, vid, new_pvid = 0;
u8 fid, member, valid;
+ int ret;
ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_REMOVE_TAG, untagged);
for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
+ if (vid == 0)
+ continue;
+
ksz8795_r_vlan_table(dev, vid, &data);
ksz8795_from_vlan(data, &fid, &member, &valid);
/* First time to setup the VLAN entry. */
if (!valid) {
- /* Need to find a way to map VID to FID. */
- fid = 1;
+ ret = ksz8795_get_next_fid(dev->ksz_fid_table, vid, &fid);
+ if (ret) {
+ dev_err(ds->dev, "Switch FID table is full, cannot add");
+ return;
+ }
valid = 1;
}
member |= BIT(port);
@@ -855,7 +897,7 @@ static int ksz8795_port_vlan_del(struct dsa_switch *ds, int port,
/* Invalidate the entry if no more member. */
if (!member) {
- fid = 0;
+ ksz8795_del_fid(dev->ksz_fid_table, vid);
valid = 0;
}
@@ -1087,6 +1129,9 @@ static int ksz8795_setup(struct dsa_switch *ds)
for (i = 0; i < (dev->num_vlans / 4); i++)
ksz8795_r_vlan_entries(dev, i);
+ /* Assign first FID to VLAN 1 and always return FID=0 for this vlan */
+ dev->ksz_fid_table[0] = 1;
+
/* Setup STP address for STP operation. */
memset(&alu, 0, sizeof(alu));
ether_addr_copy(alu.mac, eth_stp_addr);
@@ -1261,6 +1306,12 @@ static int ksz8795_switch_init(struct ksz_device *dev)
/* set the real number of ports */
dev->ds->num_ports = dev->port_cnt;
+ dev->ksz_fid_table = devm_kzalloc(dev->dev,
+ VLAN_TABLE_FID_SIZE * sizeof(u16),
+ GFP_KERNEL);
+ if (!dev->ksz_fid_table)
+ return -ENOMEM;
+
return 0;
}
diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h b/drivers/net/dsa/microchip/ksz8795_reg.h
index 40372047d40d..fe4c4f7fdd47 100644
--- a/drivers/net/dsa/microchip/ksz8795_reg.h
+++ b/drivers/net/dsa/microchip/ksz8795_reg.h
@@ -915,6 +915,7 @@
*/
#define VLAN_TABLE_FID 0x007F
+#define VLAN_TABLE_FID_SIZE 128
#define VLAN_TABLE_MEMBERSHIP 0x0F80
#define VLAN_TABLE_VALID 0x1000
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 720f22275c84..44e97c55c2da 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -77,6 +77,7 @@ struct ksz_device {
bool synclko_125;
struct vlan_table *vlan_cache;
+ u16 *ksz_fid_table;
struct ksz_port *ports;
struct delayed_work mib_read;
--
2.25.1
----- Le 13 Jan 21, à 13:45, Gilles Doffe [email protected] a écrit :
> The FID (Filter ID) is a 7 bits field used to link the VLAN table
> to the static and dynamic mac address tables.
> Until now the KSZ8795 driver could only add one VLAN as the FID was
> always set to 1.
> This commit allows setting a FID for each new active VLAN.
> The FID list is stored in a static table dynamically allocated from
> ks8795_fid structure.
> Each newly activated VLAN is associated to the next available FID.
> Only the VLAN 0 is not added to the list as it is a special VLAN.
> As it has a special meaning, see IEEE 802.1q.
> When a VLAN is no more used, the associated FID table entry is reset
> to 0.
>
> Signed-off-by: Gilles DOFFE <[email protected]>
> ---
> drivers/net/dsa/microchip/ksz8795.c | 59 +++++++++++++++++++++++--
> drivers/net/dsa/microchip/ksz8795_reg.h | 1 +
> drivers/net/dsa/microchip/ksz_common.h | 1 +
> 3 files changed, 57 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/dsa/microchip/ksz8795.c
> b/drivers/net/dsa/microchip/ksz8795.c
> index c973db101b72..6962ba4ee125 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -648,7 +648,7 @@ static enum dsa_tag_protocol ksz8795_get_tag_protocol(struct
> dsa_switch *ds,
> int port,
> enum dsa_tag_protocol mp)
> {
> - return DSA_TAG_PROTO_KSZ8795;
> + return DSA_TAG_PROTO_NONE;
This is an oversight, will be removed in V2.
> }
>
> static void ksz8795_get_strings(struct dsa_switch *ds, int port,
> @@ -796,6 +796,41 @@ static int ksz8795_port_vlan_filtering(struct dsa_switch
> *ds, int port,
> return 0;
> }
>
> +static void ksz8795_del_fid(u16 *ksz_fid_table, u16 vid)
> +{
> + u8 i = 0;
> +
> + if (!ksz_fid_table)
> + return;
> +
> + for (i = 0; i < VLAN_TABLE_FID_SIZE; i++) {
> + if (ksz_fid_table[i] == vid) {
> + ksz_fid_table[i] = 0;
> + break;
> + }
> + }
> +}
> +
> +static int ksz8795_get_next_fid(u16 *ksz_fid_table, u16 vid, u8 *fid)
> +{
> + u8 i = 0;
> + int ret = -EOVERFLOW;
> +
> + if (!ksz_fid_table)
> + return ret;
> +
> + for (i = 0; i < VLAN_TABLE_FID_SIZE; i++) {
> + if (!ksz_fid_table[i]) {
> + ksz_fid_table[i] = vid;
> + *fid = i;
> + ret = 0;
> + break;
> + }
> + }
> +
> + return ret;
> +}
> +
> static void ksz8795_port_vlan_add(struct dsa_switch *ds, int port,
> const struct switchdev_obj_port_vlan *vlan)
> {
> @@ -803,17 +838,24 @@ static void ksz8795_port_vlan_add(struct dsa_switch *ds,
> int port,
> struct ksz_device *dev = ds->priv;
> u16 data, vid, new_pvid = 0;
> u8 fid, member, valid;
> + int ret;
>
> ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_REMOVE_TAG, untagged);
>
> for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
> + if (vid == 0)
> + continue;
> +
> ksz8795_r_vlan_table(dev, vid, &data);
> ksz8795_from_vlan(data, &fid, &member, &valid);
>
> /* First time to setup the VLAN entry. */
> if (!valid) {
> - /* Need to find a way to map VID to FID. */
> - fid = 1;
> + ret = ksz8795_get_next_fid(dev->ksz_fid_table, vid, &fid);
> + if (ret) {
> + dev_err(ds->dev, "Switch FID table is full, cannot add");
> + return;
> + }
> valid = 1;
> }
> member |= BIT(port);
> @@ -855,7 +897,7 @@ static int ksz8795_port_vlan_del(struct dsa_switch *ds, int
> port,
>
> /* Invalidate the entry if no more member. */
> if (!member) {
> - fid = 0;
> + ksz8795_del_fid(dev->ksz_fid_table, vid);
> valid = 0;
> }
>
> @@ -1087,6 +1129,9 @@ static int ksz8795_setup(struct dsa_switch *ds)
> for (i = 0; i < (dev->num_vlans / 4); i++)
> ksz8795_r_vlan_entries(dev, i);
>
> + /* Assign first FID to VLAN 1 and always return FID=0 for this vlan */
> + dev->ksz_fid_table[0] = 1;
> +
> /* Setup STP address for STP operation. */
> memset(&alu, 0, sizeof(alu));
> ether_addr_copy(alu.mac, eth_stp_addr);
> @@ -1261,6 +1306,12 @@ static int ksz8795_switch_init(struct ksz_device *dev)
> /* set the real number of ports */
> dev->ds->num_ports = dev->port_cnt;
>
> + dev->ksz_fid_table = devm_kzalloc(dev->dev,
> + VLAN_TABLE_FID_SIZE * sizeof(u16),
> + GFP_KERNEL);
> + if (!dev->ksz_fid_table)
> + return -ENOMEM;
> +
> return 0;
> }
>
> diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h
> b/drivers/net/dsa/microchip/ksz8795_reg.h
> index 40372047d40d..fe4c4f7fdd47 100644
> --- a/drivers/net/dsa/microchip/ksz8795_reg.h
> +++ b/drivers/net/dsa/microchip/ksz8795_reg.h
> @@ -915,6 +915,7 @@
> */
>
> #define VLAN_TABLE_FID 0x007F
> +#define VLAN_TABLE_FID_SIZE 128
> #define VLAN_TABLE_MEMBERSHIP 0x0F80
> #define VLAN_TABLE_VALID 0x1000
>
> diff --git a/drivers/net/dsa/microchip/ksz_common.h
> b/drivers/net/dsa/microchip/ksz_common.h
> index 720f22275c84..44e97c55c2da 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -77,6 +77,7 @@ struct ksz_device {
> bool synclko_125;
>
> struct vlan_table *vlan_cache;
> + u16 *ksz_fid_table;
>
> struct ksz_port *ports;
> struct delayed_work mib_read;
> --
> 2.25.1
On 1/13/21 4:45 AM, Gilles DOFFE wrote:
> If 802.1q VLAN tag is removed from egress traffic, ingress
> traffic should by logic be tagged.
Which logic do you refer to? Software or hardware? What an user
configures with the "bridge vlan add ..." commands is the egress
tagging, but this also affects what egresses the CPU port, and therefore
what your Ethernet MAC used as a DSA master "sees", so I am not sure why
this is doing?
>
> Signed-off-by: Gilles DOFFE <[email protected]>
> ---
> drivers/net/dsa/microchip/ksz8795.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> index 4b060503b2e8..193f03ef9160 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -874,6 +874,7 @@ static void ksz8795_port_vlan_add(struct dsa_switch *ds, int port,
> }
>
> ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_REMOVE_TAG, untagged);
> + ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_INSERT_TAG, !untagged);
> }
>
> static int ksz8795_port_vlan_del(struct dsa_switch *ds, int port,
>
--
Florian
On Wed, Jan 13, 2021 at 01:45:19PM +0100, Gilles DOFFE wrote:
> If 802.1q VLAN tag is removed from egress traffic, ingress
> traffic should by logic be tagged.
>
> Signed-off-by: Gilles DOFFE <[email protected]>
> ---
> drivers/net/dsa/microchip/ksz8795.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> index 4b060503b2e8..193f03ef9160 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -874,6 +874,7 @@ static void ksz8795_port_vlan_add(struct dsa_switch *ds, int port,
> }
>
> ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_REMOVE_TAG, untagged);
> + ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_INSERT_TAG, !untagged);
> }
>
> static int ksz8795_port_vlan_del(struct dsa_switch *ds, int port,
> --
> 2.25.1
>
KSZ8795 manual says:
TABLE 4-4: PORT REGISTERS
Bit 2: Tag insertion
1 = When packets are output on the port, the switch
will add 802.1q tags to packets without 802.1q tags
when received. The switch will not add tags to
packets already tagged. The tag inserted is the
ingress port’s “Port VID.”
0 = Disable tag insertion.
Bit 1: Tag Removal
1 = When packets are output on the port, the switch
will remove 802.1q tags from packets with 802.1q
tags when received. The switch will not modify
packets received without tags.
0 = Disable tag removal.
What I understand from this is that the "Tag Removal" bit controls
whether the port will send all VLANs as egress-untagged or not.
Whereas the "Tag insertion" bit controls whether the pvid of the ingress
port will be sent as egress-tagged (if the insertion bit is 1), or as-is
(probably egress-untagged) (if the insertion bit is 0) on the egress
port.
I deduce that the "Tag Removal" bit overrules the "Tag insertion" bit of
a different port, if both are set. Example:
lan0: lan1
pvid=20
Tag insertion=1 Tag removal=0
An untagged packet forwarded from lan0 to lan1 should be transmitted as
egress-tagged, because lan0 is configured to insert its pvid into the
frames.
But:
lan0: lan1
pvid=20
Tag insertion=1 Tag removal=1
An untagged packet forwarded from lan0 to lan1 should be transmitted as
untagged, because even though lan0 inserted its pvid into the frame,
lan1 removed it.
Based on my interpretation of the manual, I believe you have a lot more
work to do than simply operating "by logic". You can test, but I don't
believe that the PORT_INSERT_TAG flag affects the port on which the
switchdev VLAN object is supposed to be offloading. On the contrary: it
affects every other port in the same bridge _except_ for that one.
On Wed, Jan 13, 2021 at 01:45:16PM +0100, Gilles DOFFE wrote:
>
> This patchset fixes various issues.
> It mainly concerns VLANs support by fixing FID table management to
> allow adding more than one VLAN.
> It also fixes tag/untag behavior on ingress/egress packets.
As far as I understand the series, it "fixes" something that was not
broken (but which nonetheless could take some improvement), and does not
fix something that was broken, because it was too broken.
Good thing you brought up the bugs now, because FYI a tsunami is coming
soon and it will cause major conflicts once Jakub merges net back into
net-next. Had you waited a little bit longer, and the bug fixes sent to
"net" would have not been backported too far down the line due to the
API rework.
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=b7a9e0da2d1c954b7c38217a29e002528b90d174
You should try to find a reasonable balance between bugs due to an
oversight, and "bugs" due to code which was put there as a joke more
than anything else. Then you should send the fixes for the former to net
and for the latter to net-next.
Good luck.
On Wed, Jan 13, 2021 at 01:45:17PM +0100, Gilles DOFFE wrote:
> The FID (Filter ID) is a 7 bits field used to link the VLAN table
> to the static and dynamic mac address tables.
> Until now the KSZ8795 driver could only add one VLAN as the FID was
> always set to 1.
What do you mean the ksz8769 driver could only add one VLAN? That is
obviously a false statement.
All VLANs use the same FID of 1 means that the switch is currently
configured for shared address learning. Whereas each VLAN having a
separate FID would mean that it is configured for individual address
learning.
> This commit allows setting a FID for each new active VLAN.
> The FID list is stored in a static table dynamically allocated from
> ks8795_fid structure.
> Each newly activated VLAN is associated to the next available FID.
> Only the VLAN 0 is not added to the list as it is a special VLAN.
> As it has a special meaning, see IEEE 802.1q.
> When a VLAN is no more used, the associated FID table entry is reset
> to 0.
Why is this patch targeting the "net" tree? What is the problem that it
resolves?