| Message ID | 20220504093000.132579-9-clement.leger@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | add support for Renesas RZ/N1 ethernet subsystem devices | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 9 of 9 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 202 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Wed, May 04, 2022 at 11:29:56AM +0200, Clément Léger wrote: > +static int a5psw_port_fdb_del(struct dsa_switch *ds, int port, > + const unsigned char *addr, u16 vid, > + struct dsa_db db) > +{ > + struct a5psw *a5psw = ds->priv; > + union lk_data lk_data = {0}; > + bool clear = false; > + int ret = 0; > + u16 entry; > + u32 reg; > + > + ether_addr_copy(lk_data.entry.mac, addr); > + > + spin_lock(&a5psw->lk_lock); > + > + ret = a5psw_lk_execute_lookup(a5psw, &lk_data, &entry); > + if (ret) { > + dev_err(a5psw->dev, "Failed to lookup mac address\n"); > + goto lk_unlock; > + } > + > + lk_data.hi = a5psw_reg_readl(a5psw, A5PSW_LK_DATA_HI); > + if (!lk_data.entry.valid) { > + dev_err(a5psw->dev, "Tried to remove non-existing entry\n"); > + ret = -ENOENT; > + goto lk_unlock; These error messages can happen under quite normal use with your hardware. For example: ip link add br0 type bridge && ip link set br0 master br0 bridge fdb add dev swp0 00:01:02:03:04:05 vid 1 master static bridge fdb add dev swp0 00:01:02:03:04:05 vid 2 master static bridge fdb del dev swp0 00:01:02:03:04:05 vid 2 master static bridge fdb del dev swp0 00:01:02:03:04:05 vid 1 master static Because the driver ignores the VID, then as soon as the VID 2 FDB entry is removed, the VID 1 FDB entry doesn't exist anymore, either. The above is a bit artificial. More practically, the bridge installs local FDB entries (MAC address of bridge device, MAC addresses of ports) once in the VLAN-unaware database (VID 0), and once per VLAN installed on br0. When the MAC address of br0 is different from that of the ports, and it is changed by the user, you'll get these errors too, since those changes translate into a deletion of the old local FDB entries (installed as FDB entries towards the CPU port). Do you want the users to see them and think something is wrong? I mean, something _is_ wrong (the hardware), but you have to work with that somehow. > + } > + > + lk_data.entry.port_mask &= ~BIT(port); > + /* If there is no more port in the mask, clear the entry */ > + if (lk_data.entry.port_mask == 0) > + clear = true; > + > + a5psw_reg_writel(a5psw, A5PSW_LK_DATA_HI, lk_data.hi); > + > + reg = entry; > + if (clear) > + reg |= A5PSW_LK_ADDR_CTRL_CLEAR; > + else > + reg |= A5PSW_LK_ADDR_CTRL_WRITE; > + > + ret = a5psw_lk_execute_ctrl(a5psw, ®); > + if (ret) > + goto lk_unlock; > + > + /* Decrement LEARNCOUNT */ > + if (clear) { > + reg = A5PSW_LK_LEARNCOUNT_MODE_DEC; > + a5psw_reg_writel(a5psw, A5PSW_LK_LEARNCOUNT, reg); > + } > + > +lk_unlock: > + spin_unlock(&a5psw->lk_lock); > + > + return ret; > +}
Le Wed, 4 May 2022 19:24:57 +0300, Vladimir Oltean <olteanv@gmail.com> a écrit : > On Wed, May 04, 2022 at 11:29:56AM +0200, Clément Léger wrote: > > +static int a5psw_port_fdb_del(struct dsa_switch *ds, int port, > > + const unsigned char *addr, u16 vid, > > + struct dsa_db db) > > +{ > > + struct a5psw *a5psw = ds->priv; > > + union lk_data lk_data = {0}; > > + bool clear = false; > > + int ret = 0; > > + u16 entry; > > + u32 reg; > > + > > + ether_addr_copy(lk_data.entry.mac, addr); > > + > > + spin_lock(&a5psw->lk_lock); > > + > > + ret = a5psw_lk_execute_lookup(a5psw, &lk_data, &entry); > > + if (ret) { > > + dev_err(a5psw->dev, "Failed to lookup mac address\n"); > > + goto lk_unlock; > > + } > > + > > + lk_data.hi = a5psw_reg_readl(a5psw, A5PSW_LK_DATA_HI); > > + if (!lk_data.entry.valid) { > > + dev_err(a5psw->dev, "Tried to remove non-existing entry\n"); > > + ret = -ENOENT; > > + goto lk_unlock; > > These error messages can happen under quite normal use with your hardware. > For example: > > ip link add br0 type bridge && ip link set br0 master br0 > bridge fdb add dev swp0 00:01:02:03:04:05 vid 1 master static > bridge fdb add dev swp0 00:01:02:03:04:05 vid 2 master static > bridge fdb del dev swp0 00:01:02:03:04:05 vid 2 master static > bridge fdb del dev swp0 00:01:02:03:04:05 vid 1 master static > > Because the driver ignores the VID, then as soon as the VID 2 FDB entry > is removed, the VID 1 FDB entry doesn't exist anymore, either. Argh did not thought about that but indeed, that will for sure trigger the error. > > The above is a bit artificial. More practically, the bridge installs > local FDB entries (MAC address of bridge device, MAC addresses of ports) > once in the VLAN-unaware database (VID 0), and once per VLAN installed > on br0. Ok, seems clear. > > When the MAC address of br0 is different from that of the ports, and it > is changed by the user, you'll get these errors too, since those changes > translate into a deletion of the old local FDB entries (installed as FDB > entries towards the CPU port). Do you want the users to see them and > think something is wrong? I mean, something _is_ wrong (the hardware), > but you have to work with that somehow. Indeed, I'll simply remove these error message. Should I still return an error value however ? Seems like I should not to avoid triggering any error that might confuse the user.
On Thu, May 05, 2022 at 03:44:31PM +0200, Clément Léger wrote: > Indeed, I'll simply remove these error message. Should I still return > an error value however ? Seems like I should not to avoid triggering > any error that might confuse the user. The error code will not be propagated to the bridge driver anyway, but will trigger prints in dsa_slave_switchdev_event_work() however. You have to choose between 2 alternatives (a) keep the FDB entry around until it gets removed from all VLANs (b) delete the FDB entry as soon as it gets removed from the first VLAN Both options are going to raise eyebrows. (a) will result in "huh, why did my packet get delivered to this port when the address wasn't in the FDB?", while (b) will result in "huh, why was my packet flooded when the address was in the FDB?" And since (b) is of lower complexity than (a), I'd just silently exit, maybe add a comment explaining why, and hope for the best.
diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c index 7268bfbea26d..55cf392b6791 100644 --- a/drivers/net/dsa/rzn1_a5psw.c +++ b/drivers/net/dsa/rzn1_a5psw.c @@ -377,6 +377,170 @@ static void a5psw_port_fast_age(struct dsa_switch *ds, int port) a5psw_port_fdb_flush(a5psw, port); } +static int a5psw_lk_execute_lookup(struct a5psw *a5psw, union lk_data *lk_data, + u16 *entry) +{ + u32 ctrl; + int ret; + + a5psw_reg_writel(a5psw, A5PSW_LK_DATA_LO, lk_data->lo); + a5psw_reg_writel(a5psw, A5PSW_LK_DATA_HI, lk_data->hi); + + ctrl = A5PSW_LK_ADDR_CTRL_LOOKUP; + ret = a5psw_lk_execute_ctrl(a5psw, &ctrl); + if (ret) + return ret; + + *entry = ctrl & A5PSW_LK_ADDR_CTRL_ADDRESS; + + return 0; +} + +static int a5psw_port_fdb_add(struct dsa_switch *ds, int port, + const unsigned char *addr, u16 vid, + struct dsa_db db) +{ + struct a5psw *a5psw = ds->priv; + union lk_data lk_data = {0}; + bool inc_learncount = false; + int ret = 0; + u16 entry; + u32 reg; + + ether_addr_copy(lk_data.entry.mac, addr); + lk_data.entry.port_mask = BIT(port); + + spin_lock(&a5psw->lk_lock); + + /* Set the value to be written in the lookup table */ + ret = a5psw_lk_execute_lookup(a5psw, &lk_data, &entry); + if (ret) + goto lk_unlock; + + lk_data.hi = a5psw_reg_readl(a5psw, A5PSW_LK_DATA_HI); + if (!lk_data.entry.valid) { + inc_learncount = true; + /* port_mask set to 0x1f when entry is not valid, clear it */ + lk_data.entry.port_mask = 0; + lk_data.entry.prio = 0; + } + + lk_data.entry.port_mask |= BIT(port); + lk_data.entry.is_static = 1; + lk_data.entry.valid = 1; + + a5psw_reg_writel(a5psw, A5PSW_LK_DATA_HI, lk_data.hi); + + reg = A5PSW_LK_ADDR_CTRL_WRITE | entry; + ret = a5psw_lk_execute_ctrl(a5psw, ®); + if (ret) + goto lk_unlock; + + if (inc_learncount) { + reg = A5PSW_LK_LEARNCOUNT_MODE_INC; + a5psw_reg_writel(a5psw, A5PSW_LK_LEARNCOUNT, reg); + } + +lk_unlock: + spin_unlock(&a5psw->lk_lock); + + return ret; +} + +static int a5psw_port_fdb_del(struct dsa_switch *ds, int port, + const unsigned char *addr, u16 vid, + struct dsa_db db) +{ + struct a5psw *a5psw = ds->priv; + union lk_data lk_data = {0}; + bool clear = false; + int ret = 0; + u16 entry; + u32 reg; + + ether_addr_copy(lk_data.entry.mac, addr); + + spin_lock(&a5psw->lk_lock); + + ret = a5psw_lk_execute_lookup(a5psw, &lk_data, &entry); + if (ret) { + dev_err(a5psw->dev, "Failed to lookup mac address\n"); + goto lk_unlock; + } + + lk_data.hi = a5psw_reg_readl(a5psw, A5PSW_LK_DATA_HI); + if (!lk_data.entry.valid) { + dev_err(a5psw->dev, "Tried to remove non-existing entry\n"); + ret = -ENOENT; + goto lk_unlock; + } + + lk_data.entry.port_mask &= ~BIT(port); + /* If there is no more port in the mask, clear the entry */ + if (lk_data.entry.port_mask == 0) + clear = true; + + a5psw_reg_writel(a5psw, A5PSW_LK_DATA_HI, lk_data.hi); + + reg = entry; + if (clear) + reg |= A5PSW_LK_ADDR_CTRL_CLEAR; + else + reg |= A5PSW_LK_ADDR_CTRL_WRITE; + + ret = a5psw_lk_execute_ctrl(a5psw, ®); + if (ret) + goto lk_unlock; + + /* Decrement LEARNCOUNT */ + if (clear) { + reg = A5PSW_LK_LEARNCOUNT_MODE_DEC; + a5psw_reg_writel(a5psw, A5PSW_LK_LEARNCOUNT, reg); + } + +lk_unlock: + spin_unlock(&a5psw->lk_lock); + + return ret; +} + +static int a5psw_port_fdb_dump(struct dsa_switch *ds, int port, + dsa_fdb_dump_cb_t *cb, void *data) +{ + struct a5psw *a5psw = ds->priv; + union lk_data lk_data; + int i = 0, ret; + u32 reg; + + for (i = 0; i < A5PSW_TABLE_ENTRIES; i++) { + reg = A5PSW_LK_ADDR_CTRL_READ | A5PSW_LK_ADDR_CTRL_WAIT | i; + spin_lock(&a5psw->lk_lock); + + ret = a5psw_lk_execute_ctrl(a5psw, ®); + if (ret) { + spin_unlock(&a5psw->lk_lock); + return ret; + } + + lk_data.hi = a5psw_reg_readl(a5psw, A5PSW_LK_DATA_HI); + /* If entry is not valid or does not contain the port, skip */ + if (!lk_data.entry.valid || + !(lk_data.entry.port_mask & BIT(port))) { + spin_unlock(&a5psw->lk_lock); + continue; + } + + lk_data.lo = a5psw_reg_readl(a5psw, A5PSW_LK_DATA_LO); + spin_unlock(&a5psw->lk_lock); + + ret = cb(lk_data.entry.mac, 0, lk_data.entry.is_static, data); + if (ret) + return ret; + } + + return 0; +} + static u64 a5psw_read_stat(struct a5psw *a5psw, u32 offset, int port) { u32 reg_lo, reg_hi; @@ -593,6 +757,9 @@ static const struct dsa_switch_ops a5psw_switch_ops = { .port_bridge_leave = a5psw_port_bridge_leave, .port_stp_state_set = a5psw_port_stp_state_set, .port_fast_age = a5psw_port_fast_age, + .port_fdb_add = a5psw_port_fdb_add, + .port_fdb_del = a5psw_port_fdb_del, + .port_fdb_dump = a5psw_port_fdb_dump, }; static int a5psw_mdio_wait_busy(struct a5psw *a5psw) diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h index 149fa82b19e1..818ed449e323 100644 --- a/drivers/net/dsa/rzn1_a5psw.h +++ b/drivers/net/dsa/rzn1_a5psw.h @@ -212,6 +212,23 @@ #define A5PSW_CTRL_TIMEOUT 1000 #define A5PSW_TABLE_ENTRIES 8192 +struct fdb_entry { + u8 mac[ETH_ALEN]; + u16 valid:1; + u16 is_static:1; + u16 prio:3; + u16 port_mask:5; + u16 reserved:6; +} __packed; + +union lk_data { + struct { + u32 lo; + u32 hi; + }; + struct fdb_entry entry; +}; + /** * struct a5psw - switch struct * @base: Base address of the switch
This commits add forwarding database support to the driver. It implements fdb_add(), fdb_del() and fdb_dump(). Signed-off-by: Clément Léger <clement.leger@bootlin.com> --- drivers/net/dsa/rzn1_a5psw.c | 167 +++++++++++++++++++++++++++++++++++ drivers/net/dsa/rzn1_a5psw.h | 17 ++++ 2 files changed, 184 insertions(+)