From 8b6836d824702cacf68190982181f8ca3aff9c3e Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Thu, 7 Oct 2021 19:47:10 +0300 Subject: net: dsa: mv88e6xxx: keep the pvid at 0 when VLAN-unaware The VLAN support in mv88e6xxx has a loaded history. Commit 2ea7a679ca2a ("net: dsa: Don't add vlans when vlan filtering is disabled") noticed some issues with VLAN and decided the best way to deal with them was to make the DSA core ignore VLANs added by the bridge while VLAN awareness is turned off. Those issues were never explained, just presented as "at least one corner case". That approach had problems of its own, presented by commit 54a0ed0df496 ("net: dsa: provide an option for drivers to always receive bridge VLANs") for the DSA core, followed by commit 1fb74191988f ("net: dsa: mv88e6xxx: fix vlan setup") which applied ds->configure_vlan_while_not_filtering = true for mv88e6xxx in particular. We still don't know what corner case Andrew saw when he wrote commit 2ea7a679ca2a ("net: dsa: Don't add vlans when vlan filtering is disabled"), but Tobias now reports that when we use TX forwarding offload, pinging an external station from the bridge device is broken if the front-facing DSA user port has flooding turned off. The full description is in the link below, but for short, when a mv88e6xxx port is under a VLAN-unaware bridge, it inherits that bridge's pvid. So packets ingressing a user port will be classified to e.g. VID 1 (assuming that value for the bridge_default_pvid), whereas when tag_dsa.c xmits towards a user port, it always sends packets using a VID of 0 if that port is standalone or under a VLAN-unaware bridge - or at least it did so prior to commit d82f8ab0d874 ("net: dsa: tag_dsa: offload the bridge forwarding process"). In any case, when there is a conversation between the CPU and a station connected to a user port, the station's MAC address is learned in VID 1 but the CPU tries to transmit through VID 0. The packets reach the intended station, but via flooding and not by virtue of matching the existing ATU entry. DSA has established (and enforced in other drivers: sja1105, felix, mt7530) that a VLAN-unaware port should use a private pvid, and not inherit the one from the bridge. The bridge's pvid should only be inherited when that bridge is VLAN-aware, so all state transitions need to be handled. On the other hand, all bridge VLANs should sit in the VTU starting with the moment when the bridge offloads them via switchdev, they are just not used. This solves the problem that Tobias sees because packets ingressing on VLAN-unaware user ports now get classified to VID 0, which is also the VID used by tag_dsa.c on xmit. Fixes: d82f8ab0d874 ("net: dsa: tag_dsa: offload the bridge forwarding process") Link: https://patchwork.kernel.org/project/netdevbpf/patch/20211003222312.284175-2-vladimir.oltean@nxp.com/#24491503 Reported-by: Tobias Waldekranz Signed-off-by: Vladimir Oltean Signed-off-by: Jakub Kicinski --- drivers/net/dsa/mv88e6xxx/chip.c | 53 +++++++++++++++++++++++++++++++++++----- drivers/net/dsa/mv88e6xxx/chip.h | 6 +++++ drivers/net/dsa/mv88e6xxx/port.c | 21 ++++++++++++++++ drivers/net/dsa/mv88e6xxx/port.h | 2 ++ 4 files changed, 76 insertions(+), 6 deletions(-) (limited to 'drivers/net/dsa') diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 03744d1c43fc..d672112afffd 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -1677,6 +1677,26 @@ static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port, return 0; } +static int mv88e6xxx_port_commit_pvid(struct mv88e6xxx_chip *chip, int port) +{ + struct dsa_port *dp = dsa_to_port(chip->ds, port); + struct mv88e6xxx_port *p = &chip->ports[port]; + bool drop_untagged = false; + u16 pvid = 0; + int err; + + if (dp->bridge_dev && br_vlan_enabled(dp->bridge_dev)) { + pvid = p->bridge_pvid.vid; + drop_untagged = !p->bridge_pvid.valid; + } + + err = mv88e6xxx_port_set_pvid(chip, port, pvid); + if (err) + return err; + + return mv88e6xxx_port_drop_untagged(chip, port, drop_untagged); +} + static int mv88e6xxx_port_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering, struct netlink_ext_ack *extack) @@ -1690,7 +1710,16 @@ static int mv88e6xxx_port_vlan_filtering(struct dsa_switch *ds, int port, return -EOPNOTSUPP; mv88e6xxx_reg_lock(chip); + err = mv88e6xxx_port_set_8021q_mode(chip, port, mode); + if (err) + goto unlock; + + err = mv88e6xxx_port_commit_pvid(chip, port); + if (err) + goto unlock; + +unlock: mv88e6xxx_reg_unlock(chip); return err; @@ -2123,6 +2152,7 @@ static int mv88e6xxx_port_vlan_add(struct dsa_switch *ds, int port, struct mv88e6xxx_chip *chip = ds->priv; bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED; bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID; + struct mv88e6xxx_port *p = &chip->ports[port]; bool warn; u8 member; int err; @@ -2156,13 +2186,21 @@ static int mv88e6xxx_port_vlan_add(struct dsa_switch *ds, int port, } if (pvid) { - err = mv88e6xxx_port_set_pvid(chip, port, vlan->vid); - if (err) { - dev_err(ds->dev, "p%d: failed to set PVID %d\n", - port, vlan->vid); + p->bridge_pvid.vid = vlan->vid; + p->bridge_pvid.valid = true; + + err = mv88e6xxx_port_commit_pvid(chip, port); + if (err) + goto out; + } else if (vlan->vid && p->bridge_pvid.vid == vlan->vid) { + /* The old pvid was reinstalled as a non-pvid VLAN */ + p->bridge_pvid.valid = false; + + err = mv88e6xxx_port_commit_pvid(chip, port); + if (err) goto out; - } } + out: mv88e6xxx_reg_unlock(chip); @@ -2212,6 +2250,7 @@ static int mv88e6xxx_port_vlan_del(struct dsa_switch *ds, int port, const struct switchdev_obj_port_vlan *vlan) { struct mv88e6xxx_chip *chip = ds->priv; + struct mv88e6xxx_port *p = &chip->ports[port]; int err = 0; u16 pvid; @@ -2229,7 +2268,9 @@ static int mv88e6xxx_port_vlan_del(struct dsa_switch *ds, int port, goto unlock; if (vlan->vid == pvid) { - err = mv88e6xxx_port_set_pvid(chip, port, 0); + p->bridge_pvid.valid = false; + + err = mv88e6xxx_port_commit_pvid(chip, port); if (err) goto unlock; } diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h index 59f316cc8583..33d067e8396d 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.h +++ b/drivers/net/dsa/mv88e6xxx/chip.h @@ -246,9 +246,15 @@ struct mv88e6xxx_policy { u16 vid; }; +struct mv88e6xxx_vlan { + u16 vid; + bool valid; +}; + struct mv88e6xxx_port { struct mv88e6xxx_chip *chip; int port; + struct mv88e6xxx_vlan bridge_pvid; u64 serdes_stats[2]; u64 atu_member_violation; u64 atu_miss_violation; diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c index 451028c57af8..d9817b20ea64 100644 --- a/drivers/net/dsa/mv88e6xxx/port.c +++ b/drivers/net/dsa/mv88e6xxx/port.c @@ -1257,6 +1257,27 @@ int mv88e6xxx_port_set_8021q_mode(struct mv88e6xxx_chip *chip, int port, return 0; } +int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port, + bool drop_untagged) +{ + u16 old, new; + int err; + + err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_CTL2, &old); + if (err) + return err; + + if (drop_untagged) + new = old | MV88E6XXX_PORT_CTL2_DISCARD_UNTAGGED; + else + new = old & ~MV88E6XXX_PORT_CTL2_DISCARD_UNTAGGED; + + if (new == old) + return 0; + + return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL2, new); +} + int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port) { u16 reg; diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h index b10e5aebacf6..03382b66f800 100644 --- a/drivers/net/dsa/mv88e6xxx/port.h +++ b/drivers/net/dsa/mv88e6xxx/port.h @@ -423,6 +423,8 @@ int mv88e6393x_port_set_cmode(struct mv88e6xxx_chip *chip, int port, phy_interface_t mode); int mv88e6185_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode); int mv88e6352_port_get_cmode(struct mv88e6xxx_chip *chip, int port, u8 *cmode); +int mv88e6xxx_port_drop_untagged(struct mv88e6xxx_chip *chip, int port, + bool drop_untagged); int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port); int mv88e6095_port_set_upstream_port(struct mv88e6xxx_chip *chip, int port, int upstream_port); -- cgit v1.2.3 From 5bded8259ee3815a91791462dfb3312480779c3d Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Thu, 7 Oct 2021 19:47:11 +0300 Subject: net: dsa: mv88e6xxx: isolate the ATU databases of standalone and bridged ports Similar to commit 6087175b7991 ("net: dsa: mt7530: use independent VLAN learning on VLAN-unaware bridges"), software forwarding between an unoffloaded LAG port (a bonding interface with an unsupported policy) and a mv88e6xxx user port directly under a bridge is broken. We adopt the same strategy, which is to make the standalone ports not find any ATU entry learned on a bridge port. Theory: the mv88e6xxx ATU is looked up by FID and MAC address. There are as many FIDs as VIDs (4096). The FID is derived from the VID when possible (the VTU maps a VID to a FID), with a fallback to the port based default FID value when not (802.1Q Mode is disabled on the port, or the classified VID isn't present in the VTU). The mv88e6xxx driver makes the following use of FIDs and VIDs: - the port's DefaultVID (to which untagged & pvid-tagged packets get classified) is 0 and is absent from the VTU, so this kind of packets is processed in FID 0, the default FID assigned by mv88e6xxx_setup_port. - every time a bridge VLAN is created, mv88e6xxx_port_vlan_join() -> mv88e6xxx_atu_new() associates a FID with that VID which increases linearly starting from 1. Like this: bridge vlan add dev lan0 vid 100 # FID 1 bridge vlan add dev lan1 vid 100 # still FID 1 bridge vlan add dev lan2 vid 1024 # FID 2 The FID allocation made by the driver is sub-optimal for the following reasons: (a) A standalone port has a DefaultPVID of 0 and a default FID of 0 too. A VLAN-unaware bridged port has a DefaultPVID of 0 and a default FID of 0 too. The difference is that the bridged ports may learn ATU entries, while the standalone port has the requirement that it must not, and must not find them either. Standalone ports must not use the same FID as ports belonging to a bridge. All standalone ports can use the same FID, since the ATU will never have an entry in that FID. (b) Multiple VLAN-unaware bridges will all use a DefaultPVID of 0 and a default FID of 0 on all their ports. The FDBs will not be isolated between these bridges. Every VLAN-unaware bridge must use the same FID on all its ports, different from the FID of other bridge ports. (c) Each bridge VLAN uses a unique FID which is useful for Independent VLAN Learning, but the same VLAN ID on multiple VLAN-aware bridges will result in the same FID being used by mv88e6xxx_atu_new(). The correct behavior is for VLAN 1 in br0 to have a different FID compared to VLAN 1 in br1. This patch cannot fix all the above. Traditionally the DSA framework did not care about this, and the reality is that DSA core involvement is needed for the aforementioned issues to be solved. The only thing we can solve here is an issue which does not require API changes, and that is issue (a), aka use a different FID for standalone ports vs ports under VLAN-unaware bridges. The first step is deciding what VID and FID to use for standalone ports, and what VID and FID for bridged ports. The 0/0 pair for standalone ports is what they used up till now, let's keep using that. For bridged ports, there are 2 cases: - VLAN-aware ports will never end up using the port default FID, because packets will always be classified to a VID in the VTU or dropped otherwise. The FID is the one associated with the VID in the VTU. - On VLAN-unaware ports, we _could_ leave their DefaultVID (pvid) at zero (just as in the case of standalone ports), and just change the port's default FID from 0 to a different number (say 1). However, Tobias points out that there is one more requirement to cater to: cross-chip bridging. The Marvell DSA header does not carry the FID in it, only the VID. So once a packet crosses a DSA link, if it has a VID of zero it will get classified to the default FID of that cascade port. Relying on a port default FID for upstream cascade ports results in contradictions: a default FID of 0 breaks ATU isolation of bridged ports on the downstream switch, a default FID of 1 breaks standalone ports on the downstream switch. So not only must standalone ports have different FIDs compared to bridged ports, they must also have different DefaultVID values. IEEE 802.1Q defines two reserved VID values: 0 and 4095. So we simply choose 4095 as the DefaultVID of ports belonging to VLAN-unaware bridges, and VID 4095 maps to FID 1. For the xmit operation to look up the same ATU database, we need to put VID 4095 in DSA tags sent to ports belonging to VLAN-unaware bridges too. All shared ports are configured to map this VID to the bridging FID, because they are members of that VLAN in the VTU. Shared ports don't need to have 802.1QMode enabled in any way, they always parse the VID from the DSA header, they don't need to look at the 802.1Q header. We install VID 4095 to the VTU in mv88e6xxx_setup_port(), with the mention that mv88e6xxx_vtu_setup() which was located right below that call was flushing the VTU so those entries wouldn't be preserved. So we need to relocate the VTU flushing prior to the port initialization during ->setup(). Also note that this is why it is safe to assume that VID 4095 will get associated with FID 1: the user ports haven't been created, so there is no avenue for the user to create a bridge VLAN which could otherwise race with the creation of another FID which would otherwise use up the non-reserved FID value of 1. [ Currently mv88e6xxx_port_vlan_join() doesn't have the option of specifying a preferred FID, it always calls mv88e6xxx_atu_new(). ] mv88e6xxx_port_db_load_purge() is the function to access the ATU for FDB/MDB entries, and it used to determine the FID to use for VLAN-unaware FDB entries (VID=0) using mv88e6xxx_port_get_fid(). But the driver only called mv88e6xxx_port_set_fid() once, during probe, so no surprises, the port FID was always 0, the call to get_fid() was redundant. As much as I would have wanted to not touch that code, the logic is broken when we add a new FID which is not the port-based default. Now the port-based default FID only corresponds to standalone ports, and FDB/MDB entries belong to the bridging service. So while in the future, when the DSA API will support FDB isolation, we will have to figure out the FID based on the bridge number, for now there's a single bridging FID, so hardcode that. Lastly, the tagger needs to check, when it is transmitting a VLAN untagged skb, whether it is sending it towards a bridged or a standalone port. When we see it is bridged we assume the bridge is VLAN-unaware. Not because it cannot be VLAN-aware but: - if we are transmitting from a VLAN-aware bridge we are likely doing so using TX forwarding offload. That code path guarantees that skbs have a vlan hwaccel tag in them, so we would not enter the "else" branch of the "if (skb->protocol == htons(ETH_P_8021Q))" condition. - if we are transmitting on behalf of a VLAN-aware bridge but with no TX forwarding offload (no PVT support, out of space in the PVT, whatever), we would indeed be transmitting with VLAN 4095 instead of the bridge device's pvid. However we would be injecting a "From CPU" frame, and the switch won't learn from that - it only learns from "Forward" frames. So it is inconsequential for address learning. And VLAN 4095 is absolutely enough for the frame to exit the switch, since we never remove that VLAN from any port. Fixes: 57e661aae6a8 ("net: dsa: mv88e6xxx: Link aggregation support") Reported-by: Tobias Waldekranz Signed-off-by: Vladimir Oltean Signed-off-by: Jakub Kicinski --- MAINTAINERS | 1 + drivers/net/dsa/mv88e6xxx/chip.c | 67 ++++++++++++++++++++++++++++++++-------- drivers/net/dsa/mv88e6xxx/chip.h | 3 ++ include/linux/dsa/mv88e6xxx.h | 13 ++++++++ net/dsa/tag_dsa.c | 12 +++++-- 5 files changed, 80 insertions(+), 16 deletions(-) create mode 100644 include/linux/dsa/mv88e6xxx.h (limited to 'drivers/net/dsa') diff --git a/MAINTAINERS b/MAINTAINERS index a4a0c2baaf27..17f652b2f653 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11153,6 +11153,7 @@ S: Maintained F: Documentation/devicetree/bindings/net/dsa/marvell.txt F: Documentation/networking/devlink/mv88e6xxx.rst F: drivers/net/dsa/mv88e6xxx/ +F: include/linux/dsa/mv88e6xxx.h F: include/linux/platform_data/mv88e6xxx.h MARVELL ARMADA 3700 PHY DRIVERS diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index d672112afffd..d7b29792732b 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -12,6 +12,7 @@ #include #include +#include #include #include #include @@ -1681,13 +1682,17 @@ static int mv88e6xxx_port_commit_pvid(struct mv88e6xxx_chip *chip, int port) { struct dsa_port *dp = dsa_to_port(chip->ds, port); struct mv88e6xxx_port *p = &chip->ports[port]; + u16 pvid = MV88E6XXX_VID_STANDALONE; bool drop_untagged = false; - u16 pvid = 0; int err; - if (dp->bridge_dev && br_vlan_enabled(dp->bridge_dev)) { - pvid = p->bridge_pvid.vid; - drop_untagged = !p->bridge_pvid.valid; + if (dp->bridge_dev) { + if (br_vlan_enabled(dp->bridge_dev)) { + pvid = p->bridge_pvid.vid; + drop_untagged = !p->bridge_pvid.valid; + } else { + pvid = MV88E6XXX_VID_BRIDGED; + } } err = mv88e6xxx_port_set_pvid(chip, port, pvid); @@ -1754,11 +1759,15 @@ static int mv88e6xxx_port_db_load_purge(struct mv88e6xxx_chip *chip, int port, u16 fid; int err; - /* Null VLAN ID corresponds to the port private database */ + /* Ports have two private address databases: one for when the port is + * standalone and one for when the port is under a bridge and the + * 802.1Q mode is disabled. When the port is standalone, DSA wants its + * address database to remain 100% empty, so we never load an ATU entry + * into a standalone port's database. Therefore, translate the null + * VLAN ID into the port's database used for VLAN-unaware bridging. + */ if (vid == 0) { - err = mv88e6xxx_port_get_fid(chip, port, &fid); - if (err) - return err; + fid = MV88E6XXX_FID_BRIDGED; } else { err = mv88e6xxx_vtu_get(chip, vid, &vlan); if (err) @@ -2434,7 +2443,16 @@ static int mv88e6xxx_port_bridge_join(struct dsa_switch *ds, int port, int err; mv88e6xxx_reg_lock(chip); + err = mv88e6xxx_bridge_map(chip, br); + if (err) + goto unlock; + + err = mv88e6xxx_port_commit_pvid(chip, port); + if (err) + goto unlock; + +unlock: mv88e6xxx_reg_unlock(chip); return err; @@ -2444,11 +2462,20 @@ static void mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port, struct net_device *br) { struct mv88e6xxx_chip *chip = ds->priv; + int err; mv88e6xxx_reg_lock(chip); + if (mv88e6xxx_bridge_map(chip, br) || mv88e6xxx_port_vlan_map(chip, port)) dev_err(ds->dev, "failed to remap in-chip Port VLAN\n"); + + err = mv88e6xxx_port_commit_pvid(chip, port); + if (err) + dev_err(ds->dev, + "port %d failed to restore standalone pvid: %pe\n", + port, ERR_PTR(err)); + mv88e6xxx_reg_unlock(chip); } @@ -2894,6 +2921,20 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port) if (err) return err; + /* Associate MV88E6XXX_VID_BRIDGED with MV88E6XXX_FID_BRIDGED in the + * ATU by virtue of the fact that mv88e6xxx_atu_new() will pick it as + * the first free FID after MV88E6XXX_FID_STANDALONE. This will be used + * as the private PVID on ports under a VLAN-unaware bridge. + * Shared (DSA and CPU) ports must also be members of it, to translate + * the VID from the DSA tag into MV88E6XXX_FID_BRIDGED, instead of + * relying on their port default FID. + */ + err = mv88e6xxx_port_vlan_join(chip, port, MV88E6XXX_VID_BRIDGED, + MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNTAGGED, + false); + if (err) + return err; + if (chip->info->ops->port_set_jumbo_size) { err = chip->info->ops->port_set_jumbo_size(chip, port, 10218); if (err) @@ -2966,7 +3007,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port) * database, and allow bidirectional communication between the * CPU and DSA port(s), and the other ports. */ - err = mv88e6xxx_port_set_fid(chip, port, 0); + err = mv88e6xxx_port_set_fid(chip, port, MV88E6XXX_FID_STANDALONE); if (err) return err; @@ -3156,6 +3197,10 @@ static int mv88e6xxx_setup(struct dsa_switch *ds) } } + err = mv88e6xxx_vtu_setup(chip); + if (err) + goto unlock; + /* Setup Switch Port Registers */ for (i = 0; i < mv88e6xxx_num_ports(chip); i++) { if (dsa_is_unused_port(ds, i)) @@ -3185,10 +3230,6 @@ static int mv88e6xxx_setup(struct dsa_switch *ds) if (err) goto unlock; - err = mv88e6xxx_vtu_setup(chip); - if (err) - goto unlock; - err = mv88e6xxx_pvt_setup(chip); if (err) goto unlock; diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h index 33d067e8396d..8271b8aa7b71 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.h +++ b/drivers/net/dsa/mv88e6xxx/chip.h @@ -21,6 +21,9 @@ #define EDSA_HLEN 8 #define MV88E6XXX_N_FID 4096 +#define MV88E6XXX_FID_STANDALONE 0 +#define MV88E6XXX_FID_BRIDGED 1 + /* PVT limits for 4-bit port and 5-bit switch */ #define MV88E6XXX_MAX_PVT_SWITCHES 32 #define MV88E6XXX_MAX_PVT_PORTS 16 diff --git a/include/linux/dsa/mv88e6xxx.h b/include/linux/dsa/mv88e6xxx.h new file mode 100644 index 000000000000..8c3d45eca46b --- /dev/null +++ b/include/linux/dsa/mv88e6xxx.h @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0 + * Copyright 2021 NXP + */ + +#ifndef _NET_DSA_TAG_MV88E6XXX_H +#define _NET_DSA_TAG_MV88E6XXX_H + +#include + +#define MV88E6XXX_VID_STANDALONE 0 +#define MV88E6XXX_VID_BRIDGED (VLAN_N_VID - 1) + +#endif diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c index 68d5ddc3ef35..b3da4b2ea11c 100644 --- a/net/dsa/tag_dsa.c +++ b/net/dsa/tag_dsa.c @@ -45,6 +45,7 @@ * 6 6 2 2 4 2 N */ +#include #include #include #include @@ -164,16 +165,21 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev, dsa_header[2] &= ~0x10; } } else { + struct net_device *br = dp->bridge_dev; + u16 vid; + + vid = br ? MV88E6XXX_VID_BRIDGED : MV88E6XXX_VID_STANDALONE; + skb_push(skb, DSA_HLEN + extra); dsa_alloc_etype_header(skb, DSA_HLEN + extra); - /* Construct untagged DSA tag. */ + /* Construct DSA header from untagged frame. */ dsa_header = dsa_etype_header_pos_tx(skb) + extra; dsa_header[0] = (cmd << 6) | tag_dev; dsa_header[1] = tag_port << 3; - dsa_header[2] = 0; - dsa_header[3] = 0; + dsa_header[2] = vid >> 8; + dsa_header[3] = vid & 0xff; } return skb; -- cgit v1.2.3 From 4a3e0aeddf091f00974b02627c157843ce382a24 Mon Sep 17 00:00:00 2001 From: Maarten Zanders Date: Mon, 11 Oct 2021 16:27:20 +0200 Subject: net: dsa: mv88e6xxx: don't use PHY_DETECT on internal PHY's mv88e6xxx_port_ppu_updates() interpretes data in the PORT_STS register incorrectly for internal ports (ie no PPU). In these cases, the PHY_DETECT bit indicates link status. This results in forcing the MAC state whenever the PHY link goes down which is not intended. As a side effect, LED's configured to show link status stay lit even though the physical link is down. Add a check in mac_link_down and mac_link_up to see if it concerns an external port and only then, look at PPU status. Fixes: 5d5b231da7ac (net: dsa: mv88e6xxx: use PHY_DETECT in mac_link_up/mac_link_down) Reported-by: Maarten Zanders Reviewed-by: Maxime Chevallier Signed-off-by: Maarten Zanders Signed-off-by: David S. Miller --- drivers/net/dsa/mv88e6xxx/chip.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) (limited to 'drivers/net/dsa') diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index d7b29792732b..8dadcae93c9b 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -750,7 +750,11 @@ static void mv88e6xxx_mac_link_down(struct dsa_switch *ds, int port, ops = chip->info->ops; mv88e6xxx_reg_lock(chip); - if ((!mv88e6xxx_port_ppu_updates(chip, port) || + /* Internal PHYs propagate their configuration directly to the MAC. + * External PHYs depend on whether the PPU is enabled for this port. + */ + if (((!mv88e6xxx_phy_is_internal(ds, port) && + !mv88e6xxx_port_ppu_updates(chip, port)) || mode == MLO_AN_FIXED) && ops->port_sync_link) err = ops->port_sync_link(chip, port, mode, false); mv88e6xxx_reg_unlock(chip); @@ -773,7 +777,12 @@ static void mv88e6xxx_mac_link_up(struct dsa_switch *ds, int port, ops = chip->info->ops; mv88e6xxx_reg_lock(chip); - if (!mv88e6xxx_port_ppu_updates(chip, port) || mode == MLO_AN_FIXED) { + /* Internal PHYs propagate their configuration directly to the MAC. + * External PHYs depend on whether the PPU is enabled for this port. + */ + if ((!mv88e6xxx_phy_is_internal(ds, port) && + !mv88e6xxx_port_ppu_updates(chip, port)) || + mode == MLO_AN_FIXED) { /* FIXME: for an automedia port, should we force the link * down here - what if the link comes up due to "other" media * while we're bringing the port up, how is the exclusivity -- cgit v1.2.3 From ef1100ef20f29aec4e62abeccdb5bdbebba1e378 Mon Sep 17 00:00:00 2001 From: Arun Ramadoss Date: Mon, 11 Oct 2021 21:18:08 +0530 Subject: net: dsa: microchip: Added the condition for scheduling ksz_mib_read_work When the ksz module is installed and removed using rmmod, kernel crashes with null pointer dereferrence error. During rmmod, ksz_switch_remove function tries to cancel the mib_read_workqueue using cancel_delayed_work_sync routine and unregister switch from dsa. During dsa_unregister_switch it calls ksz_mac_link_down, which in turn reschedules the workqueue since mib_interval is non-zero. Due to which queue executed after mib_interval and it tries to access dp->slave. But the slave is unregistered in the ksz_switch_remove function. Hence kernel crashes. To avoid this crash, before canceling the workqueue, resetted the mib_interval to 0. v1 -> v2: -Removed the if condition in ksz_mib_read_work Fixes: 469b390e1ba3 ("net: dsa: microchip: use delayed_work instead of timer + work") Signed-off-by: Arun Ramadoss Signed-off-by: David S. Miller --- drivers/net/dsa/microchip/ksz_common.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'drivers/net/dsa') diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index 1542bfb8b5e5..7c2968a639eb 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -449,8 +449,10 @@ EXPORT_SYMBOL(ksz_switch_register); void ksz_switch_remove(struct ksz_device *dev) { /* timer started */ - if (dev->mib_read_interval) + if (dev->mib_read_interval) { + dev->mib_read_interval = 0; cancel_delayed_work_sync(&dev->mib_read); + } dev->dev_ops->exit(dev); dsa_unregister_switch(dev->ds); -- cgit v1.2.3 From 28da0555c3b542d605e4ca26eea6a740cf2c9174 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Wed, 22 Sep 2021 17:37:25 +0300 Subject: net: dsa: move sja1110_process_meta_tstamp inside the tagging protocol driver The problem is that DSA tagging protocols really must not depend on the switch driver, because this creates a circular dependency at insmod time, and the switch driver will effectively not load when the tagging protocol driver is missing. The code was structured in the way it was for a reason, though. The DSA driver-facing API for PTP timestamping relies on the assumption that two-step TX timestamps are provided by the hardware in an out-of-band manner, typically by raising an interrupt and making that timestamp available inside some sort of FIFO which is to be accessed over SPI/MDIO/etc. So the API puts .port_txtstamp into dsa_switch_ops, because it is expected that the switch driver needs to save some state (like put the skb into a queue until its TX timestamp arrives). On SJA1110, TX timestamps are provided by the switch as Ethernet packets, so this makes them be received and processed by the tagging protocol driver. This in itself is great, because the timestamps are full 64-bit and do not require reconstruction, and since Ethernet is the fastest I/O method available to/from the switch, PTP timestamps arrive very quickly, no matter how bottlenecked the SPI connection is, because SPI interaction is not needed at all. DSA's code structure and strict isolation between the tagging protocol driver and the switch driver break the natural code organization. When the tagging protocol driver receives a packet which is classified as a metadata packet containing timestamps, it passes those timestamps one by one to the switch driver, which then proceeds to compare them based on the recorded timestamp ID that was generated in .port_txtstamp. The communication between the tagging protocol and the switch driver is done through a method exported by the switch driver, sja1110_process_meta_tstamp. To satisfy build requirements, we force a dependency to build the tagging protocol driver as a module when the switch driver is a module. However, as explained in the first paragraph, that causes the circular dependency. To solve this, move the skb queue from struct sja1105_private :: struct sja1105_ptp_data to struct sja1105_private :: struct sja1105_tagger_data. The latter is a data structure for which hacks have already been put into place to be able to create persistent storage per switch that is accessible from the tagging protocol driver (see sja1105_setup_ports). With the skb queue directly accessible from the tagging protocol driver, we can now move sja1110_process_meta_tstamp into the tagging driver itself, and avoid exporting a symbol. Fixes: 566b18c8b752 ("net: dsa: sja1105: implement TX timestamping for SJA1110") Link: https://lore.kernel.org/netdev/20210908220834.d7gmtnwrorhharna@skbuf/ Signed-off-by: Vladimir Oltean Signed-off-by: Jakub Kicinski --- drivers/net/dsa/sja1105/sja1105_ptp.c | 45 +++++------------------------------ drivers/net/dsa/sja1105/sja1105_ptp.h | 19 --------------- include/linux/dsa/sja1105.h | 29 +++++++++++----------- net/dsa/tag_sja1105.c | 43 +++++++++++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 73 deletions(-) (limited to 'drivers/net/dsa') diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c index 691f6dd7e669..54396992a919 100644 --- a/drivers/net/dsa/sja1105/sja1105_ptp.c +++ b/drivers/net/dsa/sja1105/sja1105_ptp.c @@ -64,6 +64,7 @@ enum sja1105_ptp_clk_mode { static int sja1105_change_rxtstamping(struct sja1105_private *priv, bool on) { + struct sja1105_tagger_data *tagger_data = &priv->tagger_data; struct sja1105_ptp_data *ptp_data = &priv->ptp_data; struct sja1105_general_params_entry *general_params; struct sja1105_table *table; @@ -79,7 +80,7 @@ static int sja1105_change_rxtstamping(struct sja1105_private *priv, priv->tagger_data.stampable_skb = NULL; } ptp_cancel_worker_sync(ptp_data->clock); - skb_queue_purge(&ptp_data->skb_txtstamp_queue); + skb_queue_purge(&tagger_data->skb_txtstamp_queue); skb_queue_purge(&ptp_data->skb_rxtstamp_queue); return sja1105_static_config_reload(priv, SJA1105_RX_HWTSTAMPING); @@ -452,40 +453,6 @@ bool sja1105_port_rxtstamp(struct dsa_switch *ds, int port, return priv->info->rxtstamp(ds, port, skb); } -void sja1110_process_meta_tstamp(struct dsa_switch *ds, int port, u8 ts_id, - enum sja1110_meta_tstamp dir, u64 tstamp) -{ - struct sja1105_private *priv = ds->priv; - struct sja1105_ptp_data *ptp_data = &priv->ptp_data; - struct sk_buff *skb, *skb_tmp, *skb_match = NULL; - struct skb_shared_hwtstamps shwt = {0}; - - /* We don't care about RX timestamps on the CPU port */ - if (dir == SJA1110_META_TSTAMP_RX) - return; - - spin_lock(&ptp_data->skb_txtstamp_queue.lock); - - skb_queue_walk_safe(&ptp_data->skb_txtstamp_queue, skb, skb_tmp) { - if (SJA1105_SKB_CB(skb)->ts_id != ts_id) - continue; - - __skb_unlink(skb, &ptp_data->skb_txtstamp_queue); - skb_match = skb; - - break; - } - - spin_unlock(&ptp_data->skb_txtstamp_queue.lock); - - if (WARN_ON(!skb_match)) - return; - - shwt.hwtstamp = ns_to_ktime(sja1105_ticks_to_ns(tstamp)); - skb_complete_tx_timestamp(skb_match, &shwt); -} -EXPORT_SYMBOL_GPL(sja1110_process_meta_tstamp); - /* In addition to cloning the skb which is done by the common * sja1105_port_txtstamp, we need to generate a timestamp ID and save the * packet to the TX timestamping queue. @@ -494,7 +461,6 @@ void sja1110_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb) { struct sk_buff *clone = SJA1105_SKB_CB(skb)->clone; struct sja1105_private *priv = ds->priv; - struct sja1105_ptp_data *ptp_data = &priv->ptp_data; struct sja1105_port *sp = &priv->ports[port]; u8 ts_id; @@ -510,7 +476,7 @@ void sja1110_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb) spin_unlock(&sp->data->meta_lock); - skb_queue_tail(&ptp_data->skb_txtstamp_queue, clone); + skb_queue_tail(&sp->data->skb_txtstamp_queue, clone); } /* Called from dsa_skb_tx_timestamp. This callback is just to clone @@ -953,7 +919,7 @@ int sja1105_ptp_clock_register(struct dsa_switch *ds) /* Only used on SJA1105 */ skb_queue_head_init(&ptp_data->skb_rxtstamp_queue); /* Only used on SJA1110 */ - skb_queue_head_init(&ptp_data->skb_txtstamp_queue); + skb_queue_head_init(&tagger_data->skb_txtstamp_queue); spin_lock_init(&tagger_data->meta_lock); ptp_data->clock = ptp_clock_register(&ptp_data->caps, ds->dev); @@ -971,6 +937,7 @@ int sja1105_ptp_clock_register(struct dsa_switch *ds) void sja1105_ptp_clock_unregister(struct dsa_switch *ds) { struct sja1105_private *priv = ds->priv; + struct sja1105_tagger_data *tagger_data = &priv->tagger_data; struct sja1105_ptp_data *ptp_data = &priv->ptp_data; if (IS_ERR_OR_NULL(ptp_data->clock)) @@ -978,7 +945,7 @@ void sja1105_ptp_clock_unregister(struct dsa_switch *ds) del_timer_sync(&ptp_data->extts_timer); ptp_cancel_worker_sync(ptp_data->clock); - skb_queue_purge(&ptp_data->skb_txtstamp_queue); + skb_queue_purge(&tagger_data->skb_txtstamp_queue); skb_queue_purge(&ptp_data->skb_rxtstamp_queue); ptp_clock_unregister(ptp_data->clock); ptp_data->clock = NULL; diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.h b/drivers/net/dsa/sja1105/sja1105_ptp.h index 3c874bb4c17b..3ae6b9fdd492 100644 --- a/drivers/net/dsa/sja1105/sja1105_ptp.h +++ b/drivers/net/dsa/sja1105/sja1105_ptp.h @@ -8,21 +8,6 @@ #if IS_ENABLED(CONFIG_NET_DSA_SJA1105_PTP) -/* Timestamps are in units of 8 ns clock ticks (equivalent to - * a fixed 125 MHz clock). - */ -#define SJA1105_TICK_NS 8 - -static inline s64 ns_to_sja1105_ticks(s64 ns) -{ - return ns / SJA1105_TICK_NS; -} - -static inline s64 sja1105_ticks_to_ns(s64 ticks) -{ - return ticks * SJA1105_TICK_NS; -} - /* Calculate the first base_time in the future that satisfies this * relationship: * @@ -77,10 +62,6 @@ struct sja1105_ptp_data { struct timer_list extts_timer; /* Used only on SJA1105 to reconstruct partial timestamps */ struct sk_buff_head skb_rxtstamp_queue; - /* Used on SJA1110 where meta frames are generated only for - * 2-step TX timestamps - */ - struct sk_buff_head skb_txtstamp_queue; struct ptp_clock_info caps; struct ptp_clock *clock; struct sja1105_ptp_cmd cmd; diff --git a/include/linux/dsa/sja1105.h b/include/linux/dsa/sja1105.h index 171106202fe5..0485ab2fcc46 100644 --- a/include/linux/dsa/sja1105.h +++ b/include/linux/dsa/sja1105.h @@ -48,6 +48,10 @@ struct sja1105_tagger_data { spinlock_t meta_lock; unsigned long state; u8 ts_id; + /* Used on SJA1110 where meta frames are generated only for + * 2-step TX timestamps + */ + struct sk_buff_head skb_txtstamp_queue; }; struct sja1105_skb_cb { @@ -69,25 +73,20 @@ struct sja1105_port { bool hwts_tx_en; }; -enum sja1110_meta_tstamp { - SJA1110_META_TSTAMP_TX = 0, - SJA1110_META_TSTAMP_RX = 1, -}; - -#if IS_ENABLED(CONFIG_NET_DSA_SJA1105_PTP) - -void sja1110_process_meta_tstamp(struct dsa_switch *ds, int port, u8 ts_id, - enum sja1110_meta_tstamp dir, u64 tstamp); - -#else +/* Timestamps are in units of 8 ns clock ticks (equivalent to + * a fixed 125 MHz clock). + */ +#define SJA1105_TICK_NS 8 -static inline void sja1110_process_meta_tstamp(struct dsa_switch *ds, int port, - u8 ts_id, enum sja1110_meta_tstamp dir, - u64 tstamp) +static inline s64 ns_to_sja1105_ticks(s64 ns) { + return ns / SJA1105_TICK_NS; } -#endif /* IS_ENABLED(CONFIG_NET_DSA_SJA1105_PTP) */ +static inline s64 sja1105_ticks_to_ns(s64 ticks) +{ + return ticks * SJA1105_TICK_NS; +} #if IS_ENABLED(CONFIG_NET_DSA_SJA1105) diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c index c054f48541c8..2edede9ddac9 100644 --- a/net/dsa/tag_sja1105.c +++ b/net/dsa/tag_sja1105.c @@ -4,6 +4,7 @@ #include #include #include +#include #include #include "dsa_priv.h" @@ -53,6 +54,11 @@ #define SJA1110_TX_TRAILER_LEN 4 #define SJA1110_MAX_PADDING_LEN 15 +enum sja1110_meta_tstamp { + SJA1110_META_TSTAMP_TX = 0, + SJA1110_META_TSTAMP_RX = 1, +}; + /* Similar to is_link_local_ether_addr(hdr->h_dest) but also covers PTP */ static inline bool sja1105_is_link_local(const struct sk_buff *skb) { @@ -520,6 +526,43 @@ static struct sk_buff *sja1105_rcv(struct sk_buff *skb, is_meta); } +static void sja1110_process_meta_tstamp(struct dsa_switch *ds, int port, + u8 ts_id, enum sja1110_meta_tstamp dir, + u64 tstamp) +{ + struct sk_buff *skb, *skb_tmp, *skb_match = NULL; + struct dsa_port *dp = dsa_to_port(ds, port); + struct skb_shared_hwtstamps shwt = {0}; + struct sja1105_port *sp = dp->priv; + + if (!dsa_port_is_sja1105(dp)) + return; + + /* We don't care about RX timestamps on the CPU port */ + if (dir == SJA1110_META_TSTAMP_RX) + return; + + spin_lock(&sp->data->skb_txtstamp_queue.lock); + + skb_queue_walk_safe(&sp->data->skb_txtstamp_queue, skb, skb_tmp) { + if (SJA1105_SKB_CB(skb)->ts_id != ts_id) + continue; + + __skb_unlink(skb, &sp->data->skb_txtstamp_queue); + skb_match = skb; + + break; + } + + spin_unlock(&sp->data->skb_txtstamp_queue.lock); + + if (WARN_ON(!skb_match)) + return; + + shwt.hwtstamp = ns_to_ktime(sja1105_ticks_to_ns(tstamp)); + skb_complete_tx_timestamp(skb_match, &shwt); +} + static struct sk_buff *sja1110_rcv_meta(struct sk_buff *skb, u16 rx_header) { u8 *buf = dsa_etype_header_pos_rx(skb) + SJA1110_HEADER_LEN; -- cgit v1.2.3 From 4ac0567e40b334b54988e3c28a2425ff9c8bdd35 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Wed, 22 Sep 2021 17:37:26 +0300 Subject: net: dsa: sja1105: break dependency between dsa_port_is_sja1105 and switch driver It's nice to be able to test a tagging protocol with dsa_loop, but not at the cost of losing the ability of building the tagging protocol and switch driver as modules, because as things stand, there is a circular dependency between the two. Tagging protocol drivers cannot depend on switch drivers, that is a hard fact. The reasoning behind the blamed patch was that accessing dp->priv should first make sure that the structure behind that pointer is what we really think it is. Currently the "sja1105" and "sja1110" tagging protocols only operate with the sja1105 switch driver, just like any other tagging protocol and switch combination. The only way to mix and match them is by modifying the code, and this applies to dsa_loop as well (by default that uses DSA_TAG_PROTO_NONE). So while in principle there is an issue, in practice there isn't one. Until we extend dsa_loop to allow user space configuration, treat the problem as a non-issue and just say that DSA ports found by tag_sja1105 are always sja1105 ports, which is in fact true. But keep the dsa_port_is_sja1105 function so that it's easy to patch it during testing, and rely on dead code elimination. Fixes: 994d2cbb08ca ("net: dsa: tag_sja1105: be dsa_loop-safe") Link: https://lore.kernel.org/netdev/20210908220834.d7gmtnwrorhharna@skbuf/ Signed-off-by: Vladimir Oltean Signed-off-by: Jakub Kicinski --- drivers/net/dsa/sja1105/sja1105_main.c | 3 +-- include/linux/dsa/sja1105.h | 15 +-------------- net/dsa/Kconfig | 1 - 3 files changed, 2 insertions(+), 17 deletions(-) (limited to 'drivers/net/dsa') diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c index 7c0db80eff00..924c3f129992 100644 --- a/drivers/net/dsa/sja1105/sja1105_main.c +++ b/drivers/net/dsa/sja1105/sja1105_main.c @@ -3117,7 +3117,7 @@ static void sja1105_teardown(struct dsa_switch *ds) sja1105_static_config_free(&priv->static_config); } -const struct dsa_switch_ops sja1105_switch_ops = { +static const struct dsa_switch_ops sja1105_switch_ops = { .get_tag_protocol = sja1105_get_tag_protocol, .setup = sja1105_setup, .teardown = sja1105_teardown, @@ -3166,7 +3166,6 @@ const struct dsa_switch_ops sja1105_switch_ops = { .port_bridge_tx_fwd_offload = dsa_tag_8021q_bridge_tx_fwd_offload, .port_bridge_tx_fwd_unoffload = dsa_tag_8021q_bridge_tx_fwd_unoffload, }; -EXPORT_SYMBOL_GPL(sja1105_switch_ops); static const struct of_device_id sja1105_dt_ids[]; diff --git a/include/linux/dsa/sja1105.h b/include/linux/dsa/sja1105.h index 0485ab2fcc46..9e07079528a5 100644 --- a/include/linux/dsa/sja1105.h +++ b/include/linux/dsa/sja1105.h @@ -88,22 +88,9 @@ static inline s64 sja1105_ticks_to_ns(s64 ticks) return ticks * SJA1105_TICK_NS; } -#if IS_ENABLED(CONFIG_NET_DSA_SJA1105) - -extern const struct dsa_switch_ops sja1105_switch_ops; - -static inline bool dsa_port_is_sja1105(struct dsa_port *dp) -{ - return dp->ds->ops == &sja1105_switch_ops; -} - -#else - static inline bool dsa_port_is_sja1105(struct dsa_port *dp) { - return false; + return true; } -#endif - #endif /* _NET_DSA_SJA1105_H */ diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig index 548285539752..bca1b5d66df2 100644 --- a/net/dsa/Kconfig +++ b/net/dsa/Kconfig @@ -138,7 +138,6 @@ config NET_DSA_TAG_LAN9303 config NET_DSA_TAG_SJA1105 tristate "Tag driver for NXP SJA1105 switches" - depends on NET_DSA_SJA1105 || !NET_DSA_SJA1105 select PACKING help Say Y or M if you want to enable support for tagging frames with the -- cgit v1.2.3 From 52849bcf0029ccc553be304e4f804938a39112e2 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Tue, 12 Oct 2021 14:40:36 +0300 Subject: net: mscc: ocelot: avoid overflowing the PTP timestamp FIFO PTP packets with 2-step TX timestamp requests are matched to packets based on the egress port number and a 6-bit timestamp identifier. All PTP timestamps are held in a common FIFO that is 128 entry deep. This patch ensures that back-to-back timestamping requests cannot exceed the hardware FIFO capacity. If that happens, simply send the packets without requesting a TX timestamp to be taken (in the case of felix, since the DSA API has a void return code in ds->ops->port_txtstamp) or drop them (in the case of ocelot). I've moved the ts_id_lock from a per-port basis to a per-switch basis, because we need separate accounting for both numbers of PTP frames in flight. And since we need locking to inc/dec the per-switch counter, that also offers protection for the per-port counter and hence there is no reason to have a per-port counter anymore. Fixes: 4e3b0468e6d7 ("net: mscc: PTP Hardware Clock (PHC) support") Signed-off-by: Vladimir Oltean Signed-off-by: Jakub Kicinski --- drivers/net/dsa/ocelot/felix.c | 6 +++++- drivers/net/ethernet/mscc/ocelot.c | 37 ++++++++++++++++++++++++++++++------- include/soc/mscc/ocelot.h | 5 ++++- include/soc/mscc/ocelot_ptp.h | 1 + 4 files changed, 40 insertions(+), 9 deletions(-) (limited to 'drivers/net/dsa') diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c index a3a9636430d6..50ef20724958 100644 --- a/drivers/net/dsa/ocelot/felix.c +++ b/drivers/net/dsa/ocelot/felix.c @@ -1291,8 +1291,12 @@ static void felix_txtstamp(struct dsa_switch *ds, int port, if (!ocelot->ptp) return; - if (ocelot_port_txtstamp_request(ocelot, port, skb, &clone)) + if (ocelot_port_txtstamp_request(ocelot, port, skb, &clone)) { + dev_err_ratelimited(ds->dev, + "port %d delivering skb without TX timestamp\n", + port); return; + } if (clone) OCELOT_SKB_CB(skb)->clone = clone; diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c index c43c8f53faaf..9c62f1d13adc 100644 --- a/drivers/net/ethernet/mscc/ocelot.c +++ b/drivers/net/ethernet/mscc/ocelot.c @@ -569,22 +569,36 @@ void ocelot_phylink_mac_link_up(struct ocelot *ocelot, int port, } EXPORT_SYMBOL_GPL(ocelot_phylink_mac_link_up); -static void ocelot_port_add_txtstamp_skb(struct ocelot *ocelot, int port, - struct sk_buff *clone) +static int ocelot_port_add_txtstamp_skb(struct ocelot *ocelot, int port, + struct sk_buff *clone) { struct ocelot_port *ocelot_port = ocelot->ports[port]; + unsigned long flags; + + spin_lock_irqsave(&ocelot->ts_id_lock, flags); - spin_lock(&ocelot_port->ts_id_lock); + if (ocelot_port->ptp_skbs_in_flight == OCELOT_MAX_PTP_ID || + ocelot->ptp_skbs_in_flight == OCELOT_PTP_FIFO_SIZE) { + spin_unlock_irqrestore(&ocelot->ts_id_lock, flags); + return -EBUSY; + } skb_shinfo(clone)->tx_flags |= SKBTX_IN_PROGRESS; /* Store timestamp ID in OCELOT_SKB_CB(clone)->ts_id */ OCELOT_SKB_CB(clone)->ts_id = ocelot_port->ts_id; + ocelot_port->ts_id++; if (ocelot_port->ts_id == OCELOT_MAX_PTP_ID) ocelot_port->ts_id = 0; + + ocelot_port->ptp_skbs_in_flight++; + ocelot->ptp_skbs_in_flight++; + skb_queue_tail(&ocelot_port->tx_skbs, clone); - spin_unlock(&ocelot_port->ts_id_lock); + spin_unlock_irqrestore(&ocelot->ts_id_lock, flags); + + return 0; } u32 ocelot_ptp_rew_op(struct sk_buff *skb) @@ -633,6 +647,7 @@ int ocelot_port_txtstamp_request(struct ocelot *ocelot, int port, { struct ocelot_port *ocelot_port = ocelot->ports[port]; u8 ptp_cmd = ocelot_port->ptp_cmd; + int err; /* Store ptp_cmd in OCELOT_SKB_CB(skb)->ptp_cmd */ if (ptp_cmd == IFH_REW_OP_ORIGIN_PTP) { @@ -650,7 +665,10 @@ int ocelot_port_txtstamp_request(struct ocelot *ocelot, int port, if (!(*clone)) return -ENOMEM; - ocelot_port_add_txtstamp_skb(ocelot, port, *clone); + err = ocelot_port_add_txtstamp_skb(ocelot, port, *clone); + if (err) + return err; + OCELOT_SKB_CB(skb)->ptp_cmd = ptp_cmd; } @@ -709,9 +727,14 @@ void ocelot_get_txtstamp(struct ocelot *ocelot) id = SYS_PTP_STATUS_PTP_MESS_ID_X(val); txport = SYS_PTP_STATUS_PTP_MESS_TXPORT_X(val); - /* Retrieve its associated skb */ port = ocelot->ports[txport]; + spin_lock(&ocelot->ts_id_lock); + port->ptp_skbs_in_flight--; + ocelot->ptp_skbs_in_flight--; + spin_unlock(&ocelot->ts_id_lock); + + /* Retrieve its associated skb */ spin_lock_irqsave(&port->tx_skbs.lock, flags); skb_queue_walk_safe(&port->tx_skbs, skb, skb_tmp) { @@ -1950,7 +1973,6 @@ void ocelot_init_port(struct ocelot *ocelot, int port) struct ocelot_port *ocelot_port = ocelot->ports[port]; skb_queue_head_init(&ocelot_port->tx_skbs); - spin_lock_init(&ocelot_port->ts_id_lock); /* Basic L2 initialization */ @@ -2083,6 +2105,7 @@ int ocelot_init(struct ocelot *ocelot) mutex_init(&ocelot->stats_lock); mutex_init(&ocelot->ptp_lock); spin_lock_init(&ocelot->ptp_clock_lock); + spin_lock_init(&ocelot->ts_id_lock); snprintf(queue_name, sizeof(queue_name), "%s-stats", dev_name(ocelot->dev)); ocelot->stats_queue = create_singlethread_workqueue(queue_name); diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h index 06706a9fd5b1..b0ece85d9a76 100644 --- a/include/soc/mscc/ocelot.h +++ b/include/soc/mscc/ocelot.h @@ -603,10 +603,10 @@ struct ocelot_port { /* The VLAN ID that will be transmitted as untagged, on egress */ struct ocelot_vlan native_vlan; + unsigned int ptp_skbs_in_flight; u8 ptp_cmd; struct sk_buff_head tx_skbs; u8 ts_id; - spinlock_t ts_id_lock; phy_interface_t phy_mode; @@ -680,6 +680,9 @@ struct ocelot { struct ptp_clock *ptp_clock; struct ptp_clock_info ptp_info; struct hwtstamp_config hwtstamp_config; + unsigned int ptp_skbs_in_flight; + /* Protects the 2-step TX timestamp ID logic */ + spinlock_t ts_id_lock; /* Protects the PTP interface state */ struct mutex ptp_lock; /* Protects the PTP clock */ diff --git a/include/soc/mscc/ocelot_ptp.h b/include/soc/mscc/ocelot_ptp.h index 6e54442b49ad..f085884b1fa2 100644 --- a/include/soc/mscc/ocelot_ptp.h +++ b/include/soc/mscc/ocelot_ptp.h @@ -14,6 +14,7 @@ #include #define OCELOT_MAX_PTP_ID 63 +#define OCELOT_PTP_FIFO_SIZE 128 #define PTP_PIN_CFG_RSZ 0x20 #define PTP_PIN_TOD_SEC_MSB_RSZ PTP_PIN_CFG_RSZ -- cgit v1.2.3 From 49f885b2d97093451410e7279aa29d81e094e108 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Tue, 12 Oct 2021 14:40:41 +0300 Subject: net: dsa: tag_ocelot_8021q: break circular dependency with ocelot switch lib Michael reported that when using the "ocelot-8021q" tagging protocol, the switch driver module must be manually loaded before the tagging protocol can be loaded/is available. This appears to be the same problem described here: https://lore.kernel.org/netdev/20210908220834.d7gmtnwrorhharna@skbuf/ where due to the fact that DSA tagging protocols make use of symbols exported by the switch drivers, circular dependencies appear and this breaks module autoloading. The ocelot_8021q driver needs the ocelot_can_inject() and ocelot_port_inject_frame() functions from the switch library. Previously the wrong approach was taken to solve that dependency: shims were provided for the case where the ocelot switch library was compiled out, but that turns out to be insufficient, because the dependency when the switch lib _is_ compiled is problematic too. We cannot declare ocelot_can_inject() and ocelot_port_inject_frame() as static inline functions, because these access I/O functions like __ocelot_write_ix() which is called by ocelot_write_rix(). Making those static inline basically means exposing the whole guts of the ocelot switch library, not ideal... We already have one tagging protocol driver which calls into the switch driver during xmit but not using any exported symbol: sja1105_defer_xmit. We can do the same thing here: create a kthread worker and one work item per skb, and let the switch driver itself do the register accesses to send the skb, and then consume it. Fixes: 0a6f17c6ae21 ("net: dsa: tag_ocelot_8021q: add support for PTP timestamping") Reported-by: Michael Walle Signed-off-by: Vladimir Oltean Signed-off-by: Jakub Kicinski --- drivers/net/dsa/ocelot/felix.c | 96 ++++++++++++++++++++++++++++++++++++++++-- drivers/net/dsa/ocelot/felix.h | 1 + include/linux/dsa/ocelot.h | 12 ++++++ include/soc/mscc/ocelot.h | 27 ------------ net/dsa/Kconfig | 2 - net/dsa/tag_ocelot_8021q.c | 38 +++++++++++------ 6 files changed, 130 insertions(+), 46 deletions(-) (limited to 'drivers/net/dsa') diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c index 50ef20724958..f8603e068e7c 100644 --- a/drivers/net/dsa/ocelot/felix.c +++ b/drivers/net/dsa/ocelot/felix.c @@ -1074,6 +1074,73 @@ static int felix_init_structs(struct felix *felix, int num_phys_ports) return 0; } +#define work_to_xmit_work(w) \ + container_of((w), struct felix_deferred_xmit_work, work) + +static void felix_port_deferred_xmit(struct kthread_work *work) +{ + struct felix_deferred_xmit_work *xmit_work = work_to_xmit_work(work); + struct dsa_switch *ds = xmit_work->dp->ds; + struct sk_buff *skb = xmit_work->skb; + u32 rew_op = ocelot_ptp_rew_op(skb); + struct ocelot *ocelot = ds->priv; + int port = xmit_work->dp->index; + int retries = 10; + + do { + if (ocelot_can_inject(ocelot, 0)) + break; + + cpu_relax(); + } while (--retries); + + if (!retries) { + dev_err(ocelot->dev, "port %d failed to inject skb\n", + port); + kfree_skb(skb); + return; + } + + ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb); + + consume_skb(skb); + kfree(xmit_work); +} + +static int felix_port_setup_tagger_data(struct dsa_switch *ds, int port) +{ + struct dsa_port *dp = dsa_to_port(ds, port); + struct ocelot *ocelot = ds->priv; + struct felix *felix = ocelot_to_felix(ocelot); + struct felix_port *felix_port; + + if (!dsa_port_is_user(dp)) + return 0; + + felix_port = kzalloc(sizeof(*felix_port), GFP_KERNEL); + if (!felix_port) + return -ENOMEM; + + felix_port->xmit_worker = felix->xmit_worker; + felix_port->xmit_work_fn = felix_port_deferred_xmit; + + dp->priv = felix_port; + + return 0; +} + +static void felix_port_teardown_tagger_data(struct dsa_switch *ds, int port) +{ + struct dsa_port *dp = dsa_to_port(ds, port); + struct felix_port *felix_port = dp->priv; + + if (!felix_port) + return; + + dp->priv = NULL; + kfree(felix_port); +} + /* Hardware initialization done here so that we can allocate structures with * devm without fear of dsa_register_switch returning -EPROBE_DEFER and causing * us to allocate structures twice (leak memory) and map PCI memory twice @@ -1102,6 +1169,12 @@ static int felix_setup(struct dsa_switch *ds) } } + felix->xmit_worker = kthread_create_worker(0, "felix_xmit"); + if (IS_ERR(felix->xmit_worker)) { + err = PTR_ERR(felix->xmit_worker); + goto out_deinit_timestamp; + } + for (port = 0; port < ds->num_ports; port++) { if (dsa_is_unused_port(ds, port)) continue; @@ -1112,6 +1185,14 @@ static int felix_setup(struct dsa_switch *ds) * bits of vlan tag. */ felix_port_qos_map_init(ocelot, port); + + err = felix_port_setup_tagger_data(ds, port); + if (err) { + dev_err(ds->dev, + "port %d failed to set up tagger data: %pe\n", + port, ERR_PTR(err)); + goto out_deinit_ports; + } } err = ocelot_devlink_sb_register(ocelot); @@ -1138,9 +1219,13 @@ out_deinit_ports: if (dsa_is_unused_port(ds, port)) continue; + felix_port_teardown_tagger_data(ds, port); ocelot_deinit_port(ocelot, port); } + kthread_destroy_worker(felix->xmit_worker); + +out_deinit_timestamp: ocelot_deinit_timestamp(ocelot); ocelot_deinit(ocelot); @@ -1164,17 +1249,20 @@ static void felix_teardown(struct dsa_switch *ds) felix_del_tag_protocol(ds, port, felix->tag_proto); } - ocelot_devlink_sb_unregister(ocelot); - ocelot_deinit_timestamp(ocelot); - ocelot_deinit(ocelot); - for (port = 0; port < ocelot->num_phys_ports; port++) { if (dsa_is_unused_port(ds, port)) continue; + felix_port_teardown_tagger_data(ds, port); ocelot_deinit_port(ocelot, port); } + kthread_destroy_worker(felix->xmit_worker); + + ocelot_devlink_sb_unregister(ocelot); + ocelot_deinit_timestamp(ocelot); + ocelot_deinit(ocelot); + if (felix->info->mdio_bus_free) felix->info->mdio_bus_free(ocelot); } diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h index 54024b6f9498..be3e42e135c0 100644 --- a/drivers/net/dsa/ocelot/felix.h +++ b/drivers/net/dsa/ocelot/felix.h @@ -62,6 +62,7 @@ struct felix { resource_size_t switch_base; resource_size_t imdio_base; enum dsa_tag_protocol tag_proto; + struct kthread_worker *xmit_worker; }; struct net_device *felix_port_to_netdev(struct ocelot *ocelot, int port); diff --git a/include/linux/dsa/ocelot.h b/include/linux/dsa/ocelot.h index 50641a7529ad..8ae999f587c4 100644 --- a/include/linux/dsa/ocelot.h +++ b/include/linux/dsa/ocelot.h @@ -5,6 +5,7 @@ #ifndef _NET_DSA_TAG_OCELOT_H #define _NET_DSA_TAG_OCELOT_H +#include #include #include @@ -160,6 +161,17 @@ struct ocelot_skb_cb { * +------+------+------+------+------+------+------+------+ */ +struct felix_deferred_xmit_work { + struct dsa_port *dp; + struct sk_buff *skb; + struct kthread_work work; +}; + +struct felix_port { + void (*xmit_work_fn)(struct kthread_work *work); + struct kthread_worker *xmit_worker; +}; + static inline void ocelot_xfh_get_rew_val(void *extraction, u64 *rew_val) { packing(extraction, rew_val, 116, 85, OCELOT_TAG_LEN, UNPACK, 0); diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h index 66b2e65c1179..d7055b41982d 100644 --- a/include/soc/mscc/ocelot.h +++ b/include/soc/mscc/ocelot.h @@ -737,8 +737,6 @@ u32 __ocelot_target_read_ix(struct ocelot *ocelot, enum ocelot_target target, void __ocelot_target_write_ix(struct ocelot *ocelot, enum ocelot_target target, u32 val, u32 reg, u32 offset); -#if IS_ENABLED(CONFIG_MSCC_OCELOT_SWITCH_LIB) - /* Packet I/O */ bool ocelot_can_inject(struct ocelot *ocelot, int grp); void ocelot_port_inject_frame(struct ocelot *ocelot, int port, int grp, @@ -746,31 +744,6 @@ void ocelot_port_inject_frame(struct ocelot *ocelot, int port, int grp, int ocelot_xtr_poll_frame(struct ocelot *ocelot, int grp, struct sk_buff **skb); void ocelot_drain_cpu_queue(struct ocelot *ocelot, int grp); -#else - -static inline bool ocelot_can_inject(struct ocelot *ocelot, int grp) -{ - return false; -} - -static inline void ocelot_port_inject_frame(struct ocelot *ocelot, int port, - int grp, u32 rew_op, - struct sk_buff *skb) -{ -} - -static inline int ocelot_xtr_poll_frame(struct ocelot *ocelot, int grp, - struct sk_buff **skb) -{ - return -EIO; -} - -static inline void ocelot_drain_cpu_queue(struct ocelot *ocelot, int grp) -{ -} - -#endif - /* Hardware initialization */ int ocelot_regfields_init(struct ocelot *ocelot, const struct reg_field *const regfields); diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig index d166377d7085..d8ee15f1c7a9 100644 --- a/net/dsa/Kconfig +++ b/net/dsa/Kconfig @@ -112,8 +112,6 @@ config NET_DSA_TAG_OCELOT config NET_DSA_TAG_OCELOT_8021Q tristate "Tag driver for Ocelot family of switches, using VLAN" - depends on MSCC_OCELOT_SWITCH_LIB || \ - (MSCC_OCELOT_SWITCH_LIB=n && COMPILE_TEST) help Say Y or M if you want to enable support for tagging frames with a custom VLAN-based header. Frames that require timestamping, such as diff --git a/net/dsa/tag_ocelot_8021q.c b/net/dsa/tag_ocelot_8021q.c index 1e4e66ea6796..d05c352f96e5 100644 --- a/net/dsa/tag_ocelot_8021q.c +++ b/net/dsa/tag_ocelot_8021q.c @@ -10,10 +10,31 @@ */ #include #include -#include -#include #include "dsa_priv.h" +static struct sk_buff *ocelot_defer_xmit(struct dsa_port *dp, + struct sk_buff *skb) +{ + struct felix_deferred_xmit_work *xmit_work; + struct felix_port *felix_port = dp->priv; + + xmit_work = kzalloc(sizeof(*xmit_work), GFP_ATOMIC); + if (!xmit_work) + return NULL; + + /* Calls felix_port_deferred_xmit in felix.c */ + kthread_init_work(&xmit_work->work, felix_port->xmit_work_fn); + /* Increase refcount so the kfree_skb in dsa_slave_xmit + * won't really free the packet. + */ + xmit_work->dp = dp; + xmit_work->skb = skb_get(skb); + + kthread_queue_work(felix_port->xmit_worker, &xmit_work->work); + + return NULL; +} + static struct sk_buff *ocelot_xmit(struct sk_buff *skb, struct net_device *netdev) { @@ -21,18 +42,9 @@ static struct sk_buff *ocelot_xmit(struct sk_buff *skb, u16 tx_vid = dsa_8021q_tx_vid(dp->ds, dp->index); u16 queue_mapping = skb_get_queue_mapping(skb); u8 pcp = netdev_txq_to_tc(netdev, queue_mapping); - struct ocelot *ocelot = dp->ds->priv; - int port = dp->index; - u32 rew_op = 0; - rew_op = ocelot_ptp_rew_op(skb); - if (rew_op) { - if (!ocelot_can_inject(ocelot, 0)) - return NULL; - - ocelot_port_inject_frame(ocelot, port, 0, rew_op, skb); - return NULL; - } + if (ocelot_ptp_rew_op(skb)) + return ocelot_defer_xmit(dp, skb); return dsa_8021q_xmit(skb, netdev, ETH_P_8021Q, ((pcp << VLAN_PRIO_SHIFT) | tx_vid)); -- cgit v1.2.3 From 1328a883258b4507909090ed0a9ad63771f9f780 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Tue, 12 Oct 2021 14:40:42 +0300 Subject: net: dsa: felix: purge skb from TX timestamping queue if it cannot be sent At present, when a PTP packet which requires TX timestamping gets dropped under congestion by the switch, things go downhill very fast. The driver keeps a clone of that skb in a queue of packets awaiting TX timestamp interrupts, but interrupts will never be raised for the dropped packets. Moreover, matching timestamped packets to timestamps is done by a 2-bit timestamp ID, and this can wrap around and we can match on the wrong skb. Since with the default NPI-based tagging protocol, we get no notification about packet drops, the best we can do is eventually recover from the drop of a PTP frame: its skb will be dead memory until another skb which was assigned the same timestamp ID happens to find it. However, with the ocelot-8021q tagger which injects packets using the manual register interface, it appears that we can check for more information, such as: - whether the input queue has reached the high watermark or not - whether the injection group's FIFO can accept additional data or not so we know that a PTP frame is likely to get dropped before actually sending it, and drop it ourselves (because DSA uses NETIF_F_LLTX, so it can't return NETDEV_TX_BUSY to ask the qdisc to requeue the packet). But when we do that, we can also remove the skb from the timestamping queue, because there surely won't be any timestamp that matches it. Fixes: 0a6f17c6ae21 ("net: dsa: tag_ocelot_8021q: add support for PTP timestamping") Signed-off-by: Vladimir Oltean Signed-off-by: Jakub Kicinski --- drivers/net/dsa/ocelot/felix.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) (limited to 'drivers/net/dsa') diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c index f8603e068e7c..9af8f900aa56 100644 --- a/drivers/net/dsa/ocelot/felix.c +++ b/drivers/net/dsa/ocelot/felix.c @@ -1074,6 +1074,33 @@ static int felix_init_structs(struct felix *felix, int num_phys_ports) return 0; } +static void ocelot_port_purge_txtstamp_skb(struct ocelot *ocelot, int port, + struct sk_buff *skb) +{ + struct ocelot_port *ocelot_port = ocelot->ports[port]; + struct sk_buff *clone = OCELOT_SKB_CB(skb)->clone; + struct sk_buff *skb_match = NULL, *skb_tmp; + unsigned long flags; + + if (!clone) + return; + + spin_lock_irqsave(&ocelot_port->tx_skbs.lock, flags); + + skb_queue_walk_safe(&ocelot_port->tx_skbs, skb, skb_tmp) { + if (skb != clone) + continue; + __skb_unlink(skb, &ocelot_port->tx_skbs); + skb_match = skb; + break; + } + + spin_unlock_irqrestore(&ocelot_port->tx_skbs.lock, flags); + + WARN_ONCE(!skb_match, + "Could not find skb clone in TX timestamping list\n"); +} + #define work_to_xmit_work(w) \ container_of((w), struct felix_deferred_xmit_work, work) @@ -1097,6 +1124,7 @@ static void felix_port_deferred_xmit(struct kthread_work *work) if (!retries) { dev_err(ocelot->dev, "port %d failed to inject skb\n", port); + ocelot_port_purge_txtstamp_skb(ocelot, port, skb); kfree_skb(skb); return; } -- cgit v1.2.3 From 8d5f7954b7c8de54902a8beda141064a7e2e6ee0 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Tue, 12 Oct 2021 14:40:44 +0300 Subject: net: dsa: felix: break at first CPU port during init and teardown The NXP LS1028A switch has two Ethernet ports towards the CPU, but only one of them is capable of acting as an NPI port at a time (inject and extract packets using DSA tags). However, using the alternative ocelot-8021q tagging protocol, it should be possible to use both CPU ports symmetrically, but for that we need to mark both ports in the device tree as DSA masters. In the process of doing that, it can be seen that traffic to/from the network stack gets broken, and this is because the Felix driver iterates through all DSA CPU ports and configures them as NPI ports. But since there can only be a single NPI port, we effectively end up in a situation where DSA thinks the default CPU port is the first one, but the hardware port configured to be an NPI is the last one. I would like to treat this as a bug, because if the updated device trees are going to start circulating, it would be really good for existing kernels to support them, too. Fixes: adb3dccf090b ("net: dsa: felix: convert to the new .change_tag_protocol DSA API") Signed-off-by: Vladimir Oltean Reviewed-by: Florian Fainelli Signed-off-by: Jakub Kicinski --- drivers/net/dsa/ocelot/felix.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) (limited to 'drivers/net/dsa') diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c index 9af8f900aa56..341236dcbdb4 100644 --- a/drivers/net/dsa/ocelot/felix.c +++ b/drivers/net/dsa/ocelot/felix.c @@ -266,12 +266,12 @@ static void felix_8021q_cpu_port_deinit(struct ocelot *ocelot, int port) */ static int felix_setup_mmio_filtering(struct felix *felix) { - unsigned long user_ports = 0, cpu_ports = 0; + unsigned long user_ports = dsa_user_ports(felix->ds); struct ocelot_vcap_filter *redirect_rule; struct ocelot_vcap_filter *tagging_rule; struct ocelot *ocelot = &felix->ocelot; struct dsa_switch *ds = felix->ds; - int port, ret; + int cpu = -1, port, ret; tagging_rule = kzalloc(sizeof(struct ocelot_vcap_filter), GFP_KERNEL); if (!tagging_rule) @@ -284,12 +284,15 @@ static int felix_setup_mmio_filtering(struct felix *felix) } for (port = 0; port < ocelot->num_phys_ports; port++) { - if (dsa_is_user_port(ds, port)) - user_ports |= BIT(port); - if (dsa_is_cpu_port(ds, port)) - cpu_ports |= BIT(port); + if (dsa_is_cpu_port(ds, port)) { + cpu = port; + break; + } } + if (cpu < 0) + return -EINVAL; + tagging_rule->key_type = OCELOT_VCAP_KEY_ETYPE; *(__be16 *)tagging_rule->key.etype.etype.value = htons(ETH_P_1588); *(__be16 *)tagging_rule->key.etype.etype.mask = htons(0xffff); @@ -325,7 +328,7 @@ static int felix_setup_mmio_filtering(struct felix *felix) * the CPU port module */ redirect_rule->action.mask_mode = OCELOT_MASK_MODE_REDIRECT; - redirect_rule->action.port_mask = cpu_ports; + redirect_rule->action.port_mask = BIT(cpu); } else { /* Trap PTP packets only to the CPU port module (which is * redirected to the NPI port) @@ -1235,6 +1238,7 @@ static int felix_setup(struct dsa_switch *ds) * there's no real point in checking for errors. */ felix_set_tag_protocol(ds, port, felix->tag_proto); + break; } ds->mtu_enforcement_ingress = true; @@ -1275,6 +1279,7 @@ static void felix_teardown(struct dsa_switch *ds) continue; felix_del_tag_protocol(ds, port, felix->tag_proto); + break; } for (port = 0; port < ocelot->num_phys_ports; port++) { -- cgit v1.2.3