net: dsa: mxl862xx: add support for bridge offloading
Daniel Golle
maintainerv8 cover net: dsa: mxl862xx: add support for bridge offloading
As a next step to complete the mxl862xx DSA driver, add support for offloading forwarding between bridged ports to the switch hardware.
This works pretty much without any big surprises, apart from two subtleties:
* per-port control over flooding behavior has to be implemented by (ab)using a 0-rate QoS meters as stopper in lack of any better option.
* STP state transition unconditionally enables learning on a port even if it was previously explicitely disabled (a firmware bug)
Note that as the driver is still lacking all VLAN features (which are going to be added next), at this point some of the bridge_vlan_aware.sh tests are failing after applying this series.
This is expected and cannot be avoided without implementing port_vlan_filtering + port_vlan_add/del. And adding both bridge and VLAN offloading at the same time would be too much for anyone to review, so VLAN support is going to be submitted in a follow-up series immediately after this series has been accepted.
All other relevant selftests (including bridge_vlan_unaware.sh) are still passing.
Inspired by the comments received from Paolo Abeni as reply to v5 the driver now no longer caches bridge port membership in the driver, but instead imports an existing helper from yt921x.c to dsa.h in order to allow the driver to easily iterate over bridge members. The mapping between DSA bridge num and firmware bridge ID is done using a simple fixed-size array in mxl862xx_priv.
Changes since v7:
* add missing fields to kernel-doc for struct mxl862xx_priv
* const'ify some function parameters
Changes since v6:
* drop wrong addition of 'priv' pointer to struct dsa_bridge -> use simple array to track firmware bridge to dsa bridge mapping
* move existing dsa_bridge_ports() helper from yt921x.c to dsa.h
* replace propesed dsa_bridge_for_each_member() by new dsa_switch_for_each_bridge_member() taking (struct net_device *) instead of (struct dsa_bridge *), aligned with the existing helpers
* fix bridge allocation resource leak in case of mxl862xx_sync_bridge_members() failing for the first port to join a bridge
* zero-initialize struct mxl862xx_cfg in mxl862xx_set_ageing_time
* set initial=1 to always reset cursor at start when interating over FDB in .fdb_dump
* hack zero-rate meter to truly always block
* use asynchronous worker for port_set_host_flood which runs from atomic context and hence cannot sleep
* reorder function to minimize diffstat of planned follow-up commits
Changes since v5:
* add new DSA helpers to interate over bridge members
* add driver-private pointer to struct dsa_bridge
* eliminate struct mxl862xx_bridge and driver-private bridge list, store firmware bridge FID in new dsa_bridge->priv instead
* rework sync_bridge_members() to use dsa_bridge_for_each_member() instead of for_each_set_bit() on driver-private portmap
* rework port_bridge_join/leave to use dsa_bridge.priv for first- member detection and dsa_bridge_ports() for empty-bridge check
* derive active FID from dp->bridge->priv in set_bridge_port()
* simplify allocate_bridge()/free_bridge() to take struct dsa_bridge pointer directly, drop kzalloc/kfree/list management
* simplify get_fid() to read db.bridge.priv directly
* remove mxl862xx_find_bridge()
* remove unnecessary default bridge config call in setup()
Changes since v4:
* move link-local check before dsa_strip_etype_header()
* introduce port_map helpers
* properly implement port_mdb_{add,del} operations
Changes since v3:
* add missing cpu_to_le32 in mxl862xx_bridge_config_fwd()
* use little-endian 32-bit type for (unused) age_timer API field
* better comment in port_set_host_flood() documenting architectural limitation
* fix typo in comment "matche" should be "matches"
* few whitespace fixes
Changes since v2:
* refactor .port_bridge_join and .port_bridge_leave as requested by Vladimir Oltean
* include linux/etherdevice.h which was missing and causing build to fail (it accidentally slipped into a follow-up patch)
* remove left-over manual reset of learning state for port leaving bridge
* remove unnecessary call to mxl862xx_port_fast_age() for port leaving bridge
* add kernel-doc comments in mxl862xx.h instead of sporadic inline comments covering only some of the struct members
* some other minor cosmetics (linebreaks, whitespace) here and there
Changes since v1:
* don't set dsa_default_offload_fwd_mark() on link-local frames
* fix kernel-doc comments in API header
* use bitfield helpers for compound tci field in fdb API
* add missing endian conversion for mxl862xx_stp_port_cfg.port_state as well as mxl862xx_mac_table_read.tci (spotted by AI review)
* drop manually resetting port learning state on bridge<->standalone transitions, DSA framework takes care of that
* don't abort updating bridge ports on error, return error at the end
* report error in mxl862xx_port_bridge_leave()
* create mxl862xx_get_fid() helper and use it in mxl862xx_port_fdb_add() and mxl862xx_port_fdb_del()
* propagete error of callback function in mxl862xx_port_fdb_dump()
* manually mxl862xx_port_fast_age() in mxl862xx_port_stp_state_set() to avoid FDB poisoning due to race condition
Daniel Golle (4):
net: dsa: move dsa_bridge_ports() helper to dsa.h
net: dsa: add bridge member iteration macro
dsa: tag_mxl862xx: set dsa_default_offload_fwd_mark()
net: dsa: mxl862xx: implement bridge offloading
drivers/net/dsa/mxl862xx/mxl862xx-api.h | 225 ++++++- drivers/net/dsa/mxl862xx/mxl862xx-cmd.h | 20 +- drivers/net/dsa/mxl862xx/mxl862xx.c | 752 ++++++++++++++++++++++-- drivers/net/dsa/mxl862xx/mxl862xx.h | 133 +++++ drivers/net/dsa/yt921x.c | 13 - include/net/dsa.h | 16 + net/dsa/tag_mxl862xx.c | 3 + 7 files changed, 1106 insertions(+), 56 deletions(-)
Signature · 1 line
2.53.0
Daniel Golle
maintainerPATCH v8 1/4 net: dsa: move dsa_bridge_ports() helper to dsa.h
The yt921x driver contains a helper to create a bitmap of ports which are members of a bridge.
Move the helper as static inline function into dsa.h, so other driver can make use of it as well.
---
v8: no changes
v7: new patch
drivers/net/dsa/yt921x.c | 13 ------------- include/net/dsa.h | 13 +++++++++++++ 2 files changed, 13 insertions(+), 13 deletions(-)
drivers/net/dsa/yt921x.c +0 -13
@@ -2154,19 +2154,6 @@ yt921x_bridge_join(struct yt921x_priv *priv, int port, u16 ports_mask)return 0 ; } -static u32 -dsa_bridge_ports (struct dsa_switch *ds,const struct net_device *bdev) -{ -struct dsa_port *dp; -u32 mask =0 ; - -dsa_switch_for_each_user_port (dp, ds) -if (dsa_port_offloads_bridge_dev (dp, bdev)) - mask |=BIT (dp->index); - -return mask; -} -static int yt921x_bridge_flags (struct yt921x_priv *priv,int port,struct switchdev_brport_flags flags)
include/net/dsa.h +13 -0
@@ -831,6 +831,19 @@ dsa_tree_offloads_bridge_dev(struct dsa_switch_tree *dst,return false; } +static inline u32 +dsa_bridge_ports (struct dsa_switch *ds,const struct net_device *bdev) +{ +struct dsa_port *dp; +u32 mask =0 ; + +dsa_switch_for_each_user_port (dp, ds) +if (dsa_port_offloads_bridge_dev (dp, bdev)) + mask |=BIT (dp->index); + +return mask; +} +static inline bool dsa_port_tree_same (const struct dsa_port *a,const struct dsa_port *b) {
Signature · 1 line
2.53.0
Daniel Golle
maintainerPATCH v8 2/4 net: dsa: add bridge member iteration macro
In reply to Daniel GolleDrivers that offload bridges need to iterate over the ports that are members of a given bridge, for example to rebuild per-port forwarding bitmaps when membership changes. Currently drivers typically open-code this by combining dsa_switch_for_each_user_port() with a dsa_port_offloads_bridge_dev() check, or cache bridge membership within the driver.
Add dsa_switch_for_each_bridge_member() macro to express this pattern directly, and use it for the existing dsa_bridge_ports() inline helper.
---
v8: no changes
v7: reuse existing helper
v6: new patch
include/net/dsa.h | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
include/net/dsa.h +6 -3
@@ -831,15 +831,18 @@ dsa_tree_offloads_bridge_dev(struct dsa_switch_tree *dst,return false; } +#define dsa_switch_for_each_bridge_member (_dp, _ds, _bdev) \ + dsa_switch_for_each_user_port(_dp, _ds) \ + if (dsa_port_offloads_bridge_dev(_dp, _bdev)) +static inline u32 dsa_bridge_ports (struct dsa_switch *ds,const struct net_device *bdev) {struct dsa_port *dp;u32 mask =0 ; -dsa_switch_for_each_user_port (dp, ds) -if (dsa_port_offloads_bridge_dev (dp, bdev)) - mask |=BIT (dp->index); +dsa_switch_for_each_bridge_member (dp, ds, bdev) + mask |=BIT (dp->index);return mask; }
Signature · 1 line
2.53.0
Daniel Golle
maintainerPATCH v8 3/4 dsa: tag_mxl862xx: set dsa_default_offload_fwd_mark()
In reply to Daniel GolleThe MxL862xx offloads bridge forwarding in hardware, so set dsa_default_offload_fwd_mark() to avoid duplicate forwarding of packets of (eg. flooded) frames arriving at the CPU port.
Link-local frames are directly trapped to the CPU port only, so don't set dsa_default_offload_fwd_mark() on those.
---
v8: no changes
v7: no changes
v6: no changes
v5: move link-local check before dsa_strip_etype_header()
v4: no changes
v3: no changes
v2: don't set dsa_default_offload_fwd_mark() on link-local frames
net/dsa/tag_mxl862xx.c | 3 +++ 1 file changed, 3 insertions(+)
net/dsa/tag_mxl862xx.c +3 -0
@@ -86,6 +86,9 @@ static struct sk_buff *mxl862_tag_rcv(struct sk_buff *skb,return NULL ; } +if (likely (!is_link_local_ether_addr (eth_hdr (skb)->h_dest))) +dsa_default_offload_fwd_mark (skb); +/* remove the MxL862xx special tag between the MAC addresses and the * current ethertype field. */
Signature · 1 line
2.53.0
Daniel Golle
maintainerPATCH v8 4/4 net: dsa: mxl862xx: implement bridge offloading
In reply to Daniel GolleImplement joining and leaving bridges as well as add, delete and dump operations on isolated FDBs, port MDB membership management, and setting a port's STP state.
The switch supports a maximum of 63 bridges, however, up to 12 may be used as "single-port bridges" to isolate standalone ports. Allowing up to 48 bridges to be offloaded seems more than enough on that hardware, hence that is set as max_num_bridges.
A total of 128 bridge ports are supported in the bridge portmap, and virtual bridge ports have to be used eg. for link-aggregation, hence potentially exceeding the number of hardware ports.
The firmware-assigned bridge identifier (FID) for each offloaded bridge is stored in an array used to map DSA bridge num to firmware bridge ID, avoiding the need for a driver-private bridge tracking structure. Bridge member portmaps are rebuilt on join/leave using dsa_switch_for_each_bridge_member().
As there are now more users of the BRIDGEPORT_CONFIG_SET API and the state of each port is cached locally, introduce a helper function mxl862xx_set_bridge_port(struct dsa_switch *ds, int port) which is then used to replace the direct calls to the API in mxl862xx_setup_cpu_bridge() and mxl862xx_add_single_port_bridge().
Note that there is no convenient way to control flooding on per-port level, so the driver is using a 0-rate QoS meter setup as a stopper in lack of any better option. In order to be perfect the firmware-enforced minimum bucket size is bypassed by directly writing 0s to the relevant registers -- without that at least one 64-byte packet could still pass before the meter would change from 'yellow' into 'red' state.
---
v8:
* add missing fields to kernel-doc for struct mxl862xx_priv
* const'ify some function parameters
v7:
* use simple array to track firmware bridge to dsa bridge mapping
* use prototype of existing dsa_bridge_ports() helper moved from yt921x.c to dsa.h which takes struct net_device * instead of struct dsa_bridge * parameter.
* use new dsa_switch_for_each_bridge_member helper
* fix bridge allocation resource leak in case of mxl862xx_sync_bridge_members() failing for the first port to join a bridge
* zero-initialize struct mxl862xx_cfg in mxl862xx_set_ageing_time
* set initial=1 to always reset cursor at start when interating over FDB in .fdb_dump
* hack zero-rate meter to truly always block
* use asynchronous worker for port_set_host_flood which runs from atomic context and hence cannot sleep
* reorder function to minimize diffstat of planned follow-up commits
v6:
* eliminate struct mxl862xx_bridge and driver-private bridge list, store firmware bridge FID in new dsa_bridge->priv instead
* rework sync_bridge_members() to use dsa_bridge_for_each_member() instead of for_each_set_bit() on driver-private portmap
* rework port_bridge_join/leave to use dsa_bridge.priv for first- member detection and dsa_bridge_ports() for empty-bridge check
* derive active FID from dp->bridge->priv in set_bridge_port()
* simplify allocate_bridge()/free_bridge() to take struct dsa_bridge pointer directly, drop kzalloc/kfree/list management
* simplify get_fid() to read db.bridge.priv directly
* remove mxl862xx_find_bridge()
* remove unnecessary default bridge config call in setup()
v5:
* introduce port_map helpers
* properly implement port_mdb_{add,del} operations
v4:
* add missing cpu_to_le32 in mxl862xx_bridge_config_fwd()
* use little-endian 32-bit type for (unused) age_timer API field
* better comment in port_set_host_flood() documenting architectural limitation
* fix typo in comment "matche" should be "matches"
* few whitespace fixes
v3:
* refactor .port_bridge_join and .port_bridge_leave as requested by Vladimir Oltean
* include linux/etherdevice.h which was missing and causing build to fail (it accidentally slipped into a follow-up patch)
* remove left-over manual reset of learning state for port leaving bridge
* remove unnecessary call to mxl862xx_port_fast_age() for port leaving bridge
* add kernel-doc comments in mxl862xx.h instead of sporadic inline comments covering only some of the struct members
* some other minor cosmetics (linebreaks, whitespace) here and there
v2:
* fix kernel-doc comments in API header
* use bitfield helpers for compound tci field in fdb API
* add missing endian conversion for mxl862xx_stp_port_cfg.port_state as well as mxl862xx_mac_table_read.tci (spotted by AI review)
* drop manually resetting port learning state on bridge<->standalone transitions, DSA framework takes care of that
* don't abort updating bridge ports on error, return error at the end
* report error in mxl862xx_port_bridge_leave()
* create mxl862xx_get_fid() helper and use it in mxl862xx_port_fdb_add() and mxl862xx_port_fdb_del()
* propagete error of callback function in mxl862xx_port_fdb_dump()
* manually mxl862xx_port_fast_age() in mxl862xx_port_stp_state_set() to avoid FDB poisoning due to race condition
drivers/net/dsa/mxl862xx/mxl862xx-api.h | 225 ++++++- drivers/net/dsa/mxl862xx/mxl862xx-cmd.h | 20 +- drivers/net/dsa/mxl862xx/mxl862xx.c | 752 ++++++++++++++++++++++-- drivers/net/dsa/mxl862xx/mxl862xx.h | 133 +++++ 4 files changed, 1087 insertions(+), 43 deletions(-)
drivers/net/dsa/mxl862xx/mxl862xx-api.h +224 -1
@@ -3,6 +3,7 @@#ifndef __MXL862XX_API_H#define __MXL862XX_API_H +#include <linux/bits.h> #include <linux/if_ether.h> /**
@@ -34,6 +35,168 @@ struct mxl862xx_register_mod {__le16 mask; } __packed; +/** +* enum mxl862xx_mac_table_filter - Source/Destination MAC address filtering +* +* @MXL862XX_MAC_FILTER_NONE: no filter +* @MXL862XX_MAC_FILTER_SRC: source address filter +* @MXL862XX_MAC_FILTER_DEST: destination address filter +* @MXL862XX_MAC_FILTER_BOTH: both source and destination filter +*/ +enum mxl862xx_mac_table_filter { +MXL862XX_MAC_FILTER_NONE =0 , +MXL862XX_MAC_FILTER_SRC =BIT (0 ), +MXL862XX_MAC_FILTER_DEST =BIT (1 ), +MXL862XX_MAC_FILTER_BOTH =BIT (0 ) |BIT (1 ), +}; + +#define MXL862XX_TCI_VLAN_ID GENMASK(11, 0) +#define MXL862XX_TCI_VLAN_CFI_DEI BIT(12) +#define MXL862XX_TCI_VLAN_PRI GENMASK(15, 13) + +/* Set in port_id to use port_map[] as a portmap bitmap instead of a single +* port ID. When clear, port_id selects one port; when set, the firmware +* ignores the lower bits of port_id and writes port_map[] directly into +* the PCE bridge port map. +*/ +#define MXL862XX_PORTMAP_FLAG BIT(31) + +/** +* struct mxl862xx_mac_table_add - MAC Table Entry to be added +* @fid: Filtering Identifier (FID) (not supported by all switches) +* @port_id: Ethernet Port number +* @port_map: Bridge Port Map +* @sub_if_id: Sub-Interface Identifier Destination +* @age_timer: Aging Time in seconds +* @vlan_id: STAG VLAN Id +* @static_entry: Static Entry (value will be aged out if not set to static) +* @traffic_class: Egress queue traffic class +* @mac: MAC Address to add to the table +* @filter_flag: See &enum mxl862xx_mac_table_filter +* @igmp_controlled: Packet is marked as IGMP controlled if destination MAC +* address matches MAC in this entry +* @associated_mac: Associated Mac address +* @tci: TCI for B-Step +* Bit [0:11] - VLAN ID +* Bit [12] - VLAN CFI/DEI +* Bit [13:15] - VLAN PRI +*/ +struct mxl862xx_mac_table_add { +__le16 fid; +__le32 port_id; +__le16 port_map[8 ]; +__le16 sub_if_id; +__le32 age_timer; +__le16 vlan_id; +u8 static_entry; +u8 traffic_class; +u8 mac[ETH_ALEN ]; +u8 filter_flag; +u8 igmp_controlled; +u8 associated_mac[ETH_ALEN ]; +__le16 tci; +} __packed; + +/** +* struct mxl862xx_mac_table_remove - MAC Table Entry to be removed +* @fid: Filtering Identifier (FID) +* @mac: MAC Address to be removed from the table. +* @filter_flag: See &enum mxl862xx_mac_table_filter +* @tci: TCI for B-Step +* Bit [0:11] - VLAN ID +* Bit [12] - VLAN CFI/DEI +* Bit [13:15] - VLAN PRI +*/ +struct mxl862xx_mac_table_remove { +__le16 fid; +u8 mac[ETH_ALEN ]; +u8 filter_flag; +__le16 tci; +} __packed; + +/** +* struct mxl862xx_mac_table_read - MAC Table Entry to be read +* @initial: Restart the get operation from the beginning of the table +* @last: Indicates that the read operation returned last entry +* @fid: Get the MAC table entry belonging to the given Filtering Identifier +* @port_id: The Bridge Port ID +* @port_map: Bridge Port Map +* @age_timer: Aging Time +* @vlan_id: STAG VLAN Id +* @static_entry: Indicates if this is a Static Entry +* @sub_if_id: Sub-Interface Identifier Destination +* @mac: MAC Address. Filled out by the switch API implementation. +* @filter_flag: See &enum mxl862xx_mac_table_filter +* @igmp_controlled: Packet is marked as IGMP controlled if destination MAC +* address matches the MAC in this entry +* @entry_changed: Indicate if the Entry has Changed +* @associated_mac: Associated MAC address +* @hit_status: MAC Table Hit Status Update +* @tci: TCI for B-Step +* Bit [0:11] - VLAN ID +* Bit [12] - VLAN CFI/DEI +* Bit [13:15] - VLAN PRI +* @first_bridge_port_id: The port this MAC address has first been learned. +* This is used for loop detection. +*/ +struct mxl862xx_mac_table_read { +u8 initial; +u8 last; +__le16 fid; +__le32 port_id; +__le16 port_map[8 ]; +__le32 age_timer; +__le16 vlan_id; +u8 static_entry; +__le16 sub_if_id; +u8 mac[ETH_ALEN ]; +u8 filter_flag; +u8 igmp_controlled; +u8 entry_changed; +u8 associated_mac[ETH_ALEN ]; +u8 hit_status; +__le16 tci; +__le16 first_bridge_port_id; +} __packed; + +/** +* struct mxl862xx_mac_table_query - MAC Table Entry key-based lookup +* @mac: MAC Address to search for (input) +* @fid: Filtering Identifier (input) +* @found: Set by firmware: 1 if entry was found, 0 if not +* @port_id: Bridge Port ID (output; MSB set if portmap mode) +* @port_map: Bridge Port Map (output; valid for static entries) +* @sub_if_id: Sub-Interface Identifier Destination +* @age_timer: Aging Time +* @vlan_id: STAG VLAN Id +* @static_entry: Indicates if this is a Static Entry +* @filter_flag: See &enum mxl862xx_mac_table_filter (input+output) +* @igmp_controlled: IGMP controlled flag +* @entry_changed: Entry changed flag +* @associated_mac: Associated MAC address +* @hit_status: MAC Table Hit Status Update +* @tci: TCI (VLAN ID + CFI/DEI + PRI) (input) +* @first_bridge_port_id: First learned bridge port +*/ +struct mxl862xx_mac_table_query { +u8 mac[ETH_ALEN ]; +__le16 fid; +u8 found; +__le32 port_id; +__le16 port_map[8 ]; +__le16 sub_if_id; +__le32 age_timer; +__le16 vlan_id; +u8 static_entry; +u8 filter_flag; +u8 igmp_controlled; +u8 entry_changed; +u8 associated_mac[ETH_ALEN ]; +u8 hit_status; +__le16 tci; +__le16 first_bridge_port_id; +} __packed; +/** * enum mxl862xx_mac_clear_type - MAC table clear type * @MXL862XX_MAC_CLEAR_PHY_PORT: clear dynamic entries based on port_id
@@ -138,6 +301,40 @@ enum mxl862xx_bridge_port_egress_meter { MXL862XX_BRIDGE_PORT_EGRESS_METER_MAX, }; +/** +* struct mxl862xx_qos_meter_cfg - Rate meter configuration +* @enable: Enable/disable meter +* @meter_id: Meter ID (assigned by firmware on alloc) +* @meter_name: Meter name string +* @meter_type: Meter algorithm type (srTCM = 0, trTCM = 1) +* @cbs: Committed Burst Size (in bytes) +* @res1: Reserved +* @ebs: Excess Burst Size (in bytes) +* @res2: Reserved +* @rate: Committed Information Rate (in kbit/s) +* @pi_rate: Peak Information Rate (in kbit/s) +* @colour_blind_mode: Colour-blind mode enable +* @pkt_mode: Packet mode enable +* @local_overhd: Local overhead accounting enable +* @local_overhd_val: Local overhead accounting value +*/ +struct mxl862xx_qos_meter_cfg { +u8 enable; +__le16 meter_id; +char meter_name[32 ]; +__le32 meter_type; +__le32 cbs; +__le32 res1; +__le32 ebs; +__le32 res2; +__le32 rate; +__le32 pi_rate; +u8 colour_blind_mode; +u8 pkt_mode; +u8 local_overhd; +__le16 local_overhd_val; +} __packed; +/** * enum mxl862xx_bridge_forward_mode - Bridge forwarding type of packet * @MXL862XX_BRIDGE_FORWARD_FLOOD: Packet is flooded to port members of
@@ -456,7 +653,7 @@ struct mxl862xx_pmapper {*/ struct mxl862xx_bridge_port_config {__le16 bridge_port_id; -__le32 mask;/* enum mxl862xx_bridge_port_config_mask */ +__le32 mask;/* enum mxl862xx_bridge_port_config_mask */ __le16 bridge_id;u8 ingress_extended_vlan_enable;__le16 ingress_extended_vlan_block_id;
@@ -658,6 +855,32 @@ struct mxl862xx_ctp_port_assignment {__le16 bridge_port_id; } __packed; +/** +* enum mxl862xx_stp_port_state - Spanning Tree Protocol port states +* @MXL862XX_STP_PORT_STATE_FORWARD: Forwarding state +* @MXL862XX_STP_PORT_STATE_DISABLE: Disabled/Discarding state +* @MXL862XX_STP_PORT_STATE_LEARNING: Learning state +* @MXL862XX_STP_PORT_STATE_BLOCKING: Blocking/Listening +*/ +enum mxl862xx_stp_port_state { +MXL862XX_STP_PORT_STATE_FORWARD =0 , +MXL862XX_STP_PORT_STATE_DISABLE , +MXL862XX_STP_PORT_STATE_LEARNING , +MXL862XX_STP_PORT_STATE_BLOCKING , +}; + +/** +* struct mxl862xx_stp_port_cfg - Configures the Spanning Tree Protocol state +* @port_id: Port number +* @fid: Filtering Identifier (FID) +* @port_state: See &enum mxl862xx_stp_port_state +*/ +struct mxl862xx_stp_port_cfg { +__le16 port_id; +__le16 fid; +__le32 port_state;/* enum mxl862xx_stp_port_state */ +} __packed; +/** * struct mxl862xx_sys_fw_image_version - Firmware version information * @iv_major: firmware major version
drivers/net/dsa/mxl862xx/mxl862xx-cmd.h +16 -4
@@ -15,12 +15,15 @@#define MXL862XX_BRDG_MAGIC 0x300#define MXL862XX_BRDGPORT_MAGIC 0x400#define MXL862XX_CTP_MAGIC 0x500 +#define MXL862XX_QOS_MAGIC 0x600#define MXL862XX_SWMAC_MAGIC 0xa00 +#define MXL862XX_STP_MAGIC 0xf00#define MXL862XX_SS_MAGIC 0x1600#define GPY_GPY2XX_MAGIC 0x1800#define SYS_MISC_MAGIC 0x1900#define MXL862XX_COMMON_CFGGET (MXL862XX_COMMON_MAGIC + 0x9) +#define MXL862XX_COMMON_CFGSET (MXL862XX_COMMON_MAGIC + 0xa)#define MXL862XX_COMMON_REGISTERMOD (MXL862XX_COMMON_MAGIC + 0x11)#define MXL862XX_BRIDGE_ALLOC (MXL862XX_BRDG_MAGIC + 0x1)
@@ -35,14 +38,23 @@#define MXL862XX_CTP_PORTASSIGNMENTSET (MXL862XX_CTP_MAGIC + 0x3) +#define MXL862XX_QOS_METERCFGSET (MXL862XX_QOS_MAGIC + 0x2) +#define MXL862XX_QOS_METERALLOC (MXL862XX_QOS_MAGIC + 0x2a) + +#define MXL862XX_MAC_TABLEENTRYADD (MXL862XX_SWMAC_MAGIC + 0x2) +#define MXL862XX_MAC_TABLEENTRYREAD (MXL862XX_SWMAC_MAGIC + 0x3) +#define MXL862XX_MAC_TABLEENTRYQUERY (MXL862XX_SWMAC_MAGIC + 0x4) +#define MXL862XX_MAC_TABLEENTRYREMOVE (MXL862XX_SWMAC_MAGIC + 0x5)#define MXL862XX_MAC_TABLECLEARCOND (MXL862XX_SWMAC_MAGIC + 0x8) -#define MXL862XX_SS_SPTAG_SET (MXL862XX_SS_MAGIC + 0x02) +#define MXL862XX_SS_SPTAG_SET (MXL862XX_SS_MAGIC + 0x2) + +#define MXL862XX_STP_PORTCFGSET (MXL862XX_STP_MAGIC + 0x2) -#define INT_GPHY_READ (GPY_GPY2XX_MAGIC + 0x01) -#define INT_GPHY_WRITE (GPY_GPY2XX_MAGIC + 0x02) +#define INT_GPHY_READ (GPY_GPY2XX_MAGIC + 0x1) +#define INT_GPHY_WRITE (GPY_GPY2XX_MAGIC + 0x2) -#define SYS_MISC_FW_VERSION (SYS_MISC_MAGIC + 0x02) +#define SYS_MISC_FW_VERSION (SYS_MISC_MAGIC + 0x2)#define MMD_API_MAXIMUM_ID 0x7fff
drivers/net/dsa/mxl862xx/mxl862xx.c +714 -38
865 lines not on this page · raw message
drivers/net/dsa/mxl862xx/mxl862xx.h +133 -0
148 lines not on this page · raw message
Signature · 1 line
2.53.0
Paolo Abeni
maintainerRe: [PATCH net-next v8 4/4] net: dsa: mxl862xx: implement bridge offloading
Quoted 7 lines · Daniel Golle
drivers/net/dsa/mxl862xx/mxl862xx.c +7
-0+static int mxl862xx_set_bridge_port (struct dsa_switch *ds,int port) +{ +struct mxl862xx_bridge_port_config br_port_cfg = {}; +struct dsa_port *dp =dsa_to_port (ds, port); +struct mxl862xx_priv *priv = ds->priv; +struct mxl862xx_port *p = &priv->ports[port]; + u16 bridge_id = dp->bridge ?
drivers/net/dsa/mxl862xx/mxl862xx.c static int mxl862xx_setup_mdio(struct dsa_switch *ds) · line 286
Quoted 10 lines
drivers/net/dsa/mxl862xx/mxl862xx.c +10
-0+ priv->bridges[dp->bridge->num] : p->fid; +bool enable; +int i, idx; + + br_port_cfg.bridge_port_id =cpu_to_le16 (port); + br_port_cfg.bridge_id =cpu_to_le16 (bridge_id); + br_port_cfg.mask =cpu_to_le32 (MXL862XX_BRIDGE_PORT_CONFIG_MASK_BRIDGE_ID | +MXL862XX_BRIDGE_PORT_CONFIG_MASK_BRIDGE_PORT_MAP | +MXL862XX_BRIDGE_PORT_CONFIG_MASK_MC_SRC_MAC_LEARNING | +MXL862XX_BRIDGE_PORT_CONFIG_MASK_EGRESS_SUB_METER );
AI reviews say:
Does this silently switch the hardware from Independent VLAN Learning (IVL) to Shared VLAN Learning (SVL)? The mask no longer includes MXL862XX_BRIDGE_PORT_CONFIG_MASK_VLAN_BASED_MAC_LEARNING, and the vlan_src_mac_vid_enable and vlan_dst_mac_vid_enable fields are not set. For a VLAN-aware bridge, this appears to break VLAN isolation boundaries, as a MAC address learned on one VLAN could affect forwarding decisions on other VLANs.
[ ... ]
Quoted 9 lines
drivers/net/dsa/mxl862xx/mxl862xx.c +9
-0+static int mxl862xx_add_single_port_bridge (struct dsa_switch *ds,int port) +{ +struct dsa_port *dp =dsa_to_port (ds, port); +struct mxl862xx_priv *priv = ds->priv; +int ret; + + ret =mxl862xx_allocate_bridge (priv, &priv->ports[port].fid); +if (ret) { +dev_err (ds->dev,"failed to allocate a bridge for port %d\n" , port);
drivers/net/dsa/mxl862xx/mxl862xx.c static int mxl862xx_setup_mdio(struct dsa_switch *ds) · line 395
Quoted 10 lines
drivers/net/dsa/mxl862xx/mxl862xx.c +10
-0+return ret; + } + + priv->ports[port].learning = false; +bitmap_zero (priv->ports[port].portmap,MXL862XX_MAX_BRIDGE_PORTS ); +__set_bit (dp->cpu_dp->index, priv->ports[port].portmap); + + ret =mxl862xx_set_bridge_port (ds, port); +if (ret) +return ret;
AI review says:
If mxl862xx_set_bridge_port() fails, does this leak the newly allocated hardware bridge? It looks like there is no call to mxl862xx_free_bridge() on this error path.
[...]
Quoted 18 lines
drivers/net/dsa/mxl862xx/mxl862xx.c +8
-7+static int mxl862xx_port_bridge_join (struct dsa_switch *ds,int port, +const struct dsa_bridge bridge, +bool *tx_fwd_offload, +struct netlink_ext_ack *extack) { -struct mxl862xx_bridge_port_config br_port_cfg = {}; -struct dsa_port *dp =dsa_to_port (ds, port); -struct mxl862xx_bridge_alloc br_alloc = {}; +struct mxl862xx_priv *priv = ds->priv; +u16 fw_id;int ret; - ret =MXL862XX_API_READ (ds->priv,MXL862XX_BRIDGE_ALLOC , br_alloc); -if (ret) { -dev_err (ds->dev,"failed to allocate a bridge for port %d\n" , port); -return ret; +if (!priv->bridges[bridge.num]) { + ret =mxl862xx_allocate_bridge (priv, &fw_id);
drivers/net/dsa/mxl862xx/mxl862xx.c static int mxl862xx_configure_sp_tag_proto(struct dsa_switch *ds, int port, · line 539
Quoted 10 lines
drivers/net/dsa/mxl862xx/mxl862xx.c +10
-0+if (ret) +return ret; + + priv->bridges[bridge.num] = fw_id; + +/* Free bridge here on error, DSA rollback won't. */ + ret =mxl862xx_sync_bridge_members (ds, &bridge); +if (ret) { +mxl862xx_free_bridge (ds, &bridge); +return ret;
AI review says:
If mxl862xx_sync_bridge_members() encounters an error partway through updating the ports in its loop, are the already-updated ports left pointing to a bridge ID that is then freed here?
It seems the software portmaps and hardware states of the preceding ports are not rolled back, which might leave them corrupted and permanently referencing a freed bridge.
Quoted 24 lines
drivers/net/dsa/mxl862xx/mxl862xx.c +9
-13+ } + +return 0 ; } - br_port_cfg.bridge_id = br_alloc.bridge_id; - br_port_cfg.bridge_port_id =cpu_to_le16 (port); - br_port_cfg.mask =cpu_to_le32 (MXL862XX_BRIDGE_PORT_CONFIG_MASK_BRIDGE_ID | -MXL862XX_BRIDGE_PORT_CONFIG_MASK_BRIDGE_PORT_MAP | -MXL862XX_BRIDGE_PORT_CONFIG_MASK_MC_SRC_MAC_LEARNING | -MXL862XX_BRIDGE_PORT_CONFIG_MASK_VLAN_BASED_MAC_LEARNING ); - br_port_cfg.src_mac_learning_disable = true; - br_port_cfg.vlan_src_mac_vid_enable = false; - br_port_cfg.vlan_dst_mac_vid_enable = false; -/* As this function is only called for user ports it is safe to rely on -* cpu_dp being valid -*/ - br_port_cfg.bridge_port_map[0 ] =cpu_to_le16 (BIT (dp->cpu_dp->index)); +return mxl862xx_sync_bridge_members (ds, &bridge); +} + +static void mxl862xx_port_bridge_leave (struct dsa_switch *ds,int port, +const struct dsa_bridge bridge) +{
drivers/net/dsa/mxl862xx/mxl862xx.c static int mxl862xx_configure_sp_tag_proto(struct dsa_switch *ds, int port, · line 560
Quoted 10 lines
drivers/net/dsa/mxl862xx/mxl862xx.c +10
-0+struct dsa_port *dp =dsa_to_port (ds, port); +struct mxl862xx_priv *priv = ds->priv; +struct mxl862xx_port *p = &priv->ports[port]; +int err; + + err =mxl862xx_sync_bridge_members (ds, &bridge); +if (err) +dev_err (ds->dev, +"failed to sync bridge members after port %d left: %pe\n" , + port,ERR_PTR (err));
AI review says:
Is the leaving port still present in dp->bridge when this function is called?
The DSA core typically clears dp->bridge after the leave callback returns. If so, mxl862xx_sync_bridge_members() would still iterate over the leaving port and incorrectly add it to the hardware portmaps of all remaining bridge members.
drivers/net/dsa/mxl862xx/mxl862xx.c static int mxl862xx_configure_sp_tag_proto(struct dsa_switch *ds, int port, · line 568
Quoted 9 lines
drivers/net/dsa/mxl862xx/mxl862xx.c +7
-1-return MXL862XX_API_WRITE (ds->priv,MXL862XX_BRIDGEPORT_CONFIGSET , br_port_cfg); +/* Revert leaving port, omitted by the sync above, to its +* single-port bridge +*/ +bitmap_zero (p->portmap,MXL862XX_MAX_BRIDGE_PORTS ); +__set_bit (dp->cpu_dp->index, p->portmap); + p->flood_block =0 ; + err =mxl862xx_set_bridge_port (ds, port);
AI review says:
Could there be two issues during this revert?
First, if dp->bridge is still set as noted above, mxl862xx_set_bridge_port() will evaluate dp->bridge to true and use the shared bridge's hardware FID instead of the port's standalone FID, keeping the port assigned to the shared bridge.
Second, if learning was enabled while the port was in the bridge, p->learning
remains true here. Should p->learning be set to false when the port reverts to a standalone port to prevent inappropriate MAC learning?