| Message ID | 20220414122250.158113-9-clement.leger@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | add support for Renesas RZ/N1 ethernet subsystem devices | expand |
On Thu, Apr 14, 2022 at 02:22:46PM +0200, Clément Léger wrote: > 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 | 163 +++++++++++++++++++++++++++++++++++ > drivers/net/dsa/rzn1_a5psw.h | 16 ++++ > 2 files changed, 179 insertions(+) > > diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c > index 7ab7d9054427..8c763c2a1a1f 100644 > --- a/drivers/net/dsa/rzn1_a5psw.c > +++ b/drivers/net/dsa/rzn1_a5psw.c > @@ -369,6 +369,166 @@ 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) This isn't something that is documented because I haven't had time to update that, but new drivers should comply to the requirements for FDB isolation (not ignore the passed "db" here) and eventually set ds->fdb_isolation = true. Doing so would allow your switch to behave correctly when - there is more than one bridge spanning its ports, - some ports are standalone and some ports are bridged - standalone ports are looped back via an external cable with bridged ports - unrecognized upper interfaces (bond, team) are used, and those are bridged directly with some other switch ports The most basic thing you need to do to satisfy the requirements is to figure out what mechanism for FDB partitioning does your hardware have. If the answer is "none", then we'll have to use VLANs for that: all standalone ports to share a VLAN, each VLAN-unaware bridge to share a VLAN across all member ports, each VLAN of a VLAN-aware bridge to reserve its own VLAN. Up to a total of 32 VLANs, since I notice that's what the limit for your hardware is. But I see this patch set doesn't include VLAN functionality (and also ignores the "vid" from FDB entries), so I can't really say more right now. But if you could provide more information about the hardware capabilities we can discuss implementation options. > +{ > + 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) > + 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); > + > + cb(lk_data.entry.mac, 0, lk_data.entry.is_static, data); ret = cb(...) if (ret) return ret; This actually matters because the netlink skb used for the FDB dump may run out of space and you'll have missing FDB entries. > + } > + > + return 0; > +} > + > static void a5psw_get_strings(struct dsa_switch *ds, int port, u32 stringset, > uint8_t *data) > { > @@ -500,6 +660,9 @@ 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, > > }; > > diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h > index b34ea549e936..37aa89383e70 100644 > --- a/drivers/net/dsa/rzn1_a5psw.h > +++ b/drivers/net/dsa/rzn1_a5psw.h > @@ -167,6 +167,22 @@ > #define A5PSW_CTRL_TIMEOUT 1000 > #define A5PSW_TABLE_ENTRIES 8192 > > +struct fdb_entry { Shouldn't this contain something along the lines of a VID, FID, something? > + u8 mac[ETH_ALEN]; > + u8 valid:1; > + u8 is_static:1; > + u8 prio:3; > + u8 port_mask:5; > +} __packed; > + > +union lk_data { > + struct { > + u32 lo; > + u32 hi; > + }; > + struct fdb_entry entry; > +}; > + > /** > * struct a5psw - switch struct > * @base: Base address of the switch > -- > 2.34.1 >
Le Thu, 14 Apr 2022 20:51:40 +0300, Vladimir Oltean <olteanv@gmail.com> a écrit : > > + > > +static int a5psw_port_fdb_add(struct dsa_switch *ds, int port, > > + const unsigned char *addr, u16 vid, > > + struct dsa_db db) > > This isn't something that is documented because I haven't had time to > update that, but new drivers should comply to the requirements for FDB > isolation (not ignore the passed "db" here) and eventually set > ds->fdb_isolation = true. Doing so would allow your switch to behave > correctly when > - there is more than one bridge spanning its ports, > - some ports are standalone and some ports are bridged > - standalone ports are looped back via an external cable with bridged > ports > - unrecognized upper interfaces (bond, team) are used, and those are > bridged directly with some other switch ports > > The most basic thing you need to do to satisfy the requirements is to > figure out what mechanism for FDB partitioning does your hardware have. > If the answer is "none", then we'll have to use VLANs for that: all > standalone ports to share a VLAN, each VLAN-unaware bridge to share a > VLAN across all member ports, each VLAN of a VLAN-aware bridge to > reserve its own VLAN. Up to a total of 32 VLANs, since I notice that's > what the limit for your hardware is. Ok, I see the idea. In the mean time, could we make a first step with a single bridge and without VLAN support ? This is expected to come later anyway. > > But I see this patch set doesn't include VLAN functionality (and also > ignores the "vid" from FDB entries), so I can't really say more right now. > But if you could provide more information about the hardware > capabilities we can discuss implementation options. That's indeed the problem. The FDB table does not seems to have partitionning at all (except for ports) and entries (such as seen below) do not contain any VLAN information. > > diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h > > index b34ea549e936..37aa89383e70 100644 > > --- a/drivers/net/dsa/rzn1_a5psw.h > > +++ b/drivers/net/dsa/rzn1_a5psw.h > > @@ -167,6 +167,22 @@ > > #define A5PSW_CTRL_TIMEOUT 1000 > > #define A5PSW_TABLE_ENTRIES 8192 > > > > +struct fdb_entry { > > Shouldn't this contain something along the lines of a VID, FID, something? This is extracted directly from the datasheet [1]. The switch FDB table does not seems to store the VID with the entries (See page 300). [1] https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-rzn1l-group-users-manual-r-engine-and-ethernet-peripherals > > > + u8 mac[ETH_ALEN]; > > + u8 valid:1; > > + u8 is_static:1; > > + u8 prio:3; > > + u8 port_mask:5; > > +} __packed; > > + > > +union lk_data { > > + struct { > > + u32 lo; > > + u32 hi; > > + }; > > + struct fdb_entry entry; > > +}; > > + > > /** > > * struct a5psw - switch struct > > * @base: Base address of the switch > > -- > > 2.34.1 > > >
On Wed, Apr 20, 2022 at 10:16:48AM +0200, Clément Léger wrote: > Le Thu, 14 Apr 2022 20:51:40 +0300, > Vladimir Oltean <olteanv@gmail.com> a écrit : > > > > + > > > +static int a5psw_port_fdb_add(struct dsa_switch *ds, int port, > > > + const unsigned char *addr, u16 vid, > > > + struct dsa_db db) > > > > This isn't something that is documented because I haven't had time to > > update that, but new drivers should comply to the requirements for FDB > > isolation (not ignore the passed "db" here) and eventually set > > ds->fdb_isolation = true. Doing so would allow your switch to behave > > correctly when > > - there is more than one bridge spanning its ports, > > - some ports are standalone and some ports are bridged > > - standalone ports are looped back via an external cable with bridged > > ports > > - unrecognized upper interfaces (bond, team) are used, and those are > > bridged directly with some other switch ports > > > > The most basic thing you need to do to satisfy the requirements is to > > figure out what mechanism for FDB partitioning does your hardware have. > > If the answer is "none", then we'll have to use VLANs for that: all > > standalone ports to share a VLAN, each VLAN-unaware bridge to share a > > VLAN across all member ports, each VLAN of a VLAN-aware bridge to > > reserve its own VLAN. Up to a total of 32 VLANs, since I notice that's > > what the limit for your hardware is. > > Ok, I see the idea. In the mean time, could we make a first step with a > single bridge and without VLAN support ? This is expected to come later > anyway. > > > > > But I see this patch set doesn't include VLAN functionality (and also > > ignores the "vid" from FDB entries), so I can't really say more right now. > > But if you could provide more information about the hardware > > capabilities we can discuss implementation options. > > That's indeed the problem. The FDB table does not seems to have > partitionning at all (except for ports) and entries (such as seen below) > do not contain any VLAN information. > > > > diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h > > > index b34ea549e936..37aa89383e70 100644 > > > --- a/drivers/net/dsa/rzn1_a5psw.h > > > +++ b/drivers/net/dsa/rzn1_a5psw.h > > > @@ -167,6 +167,22 @@ > > > #define A5PSW_CTRL_TIMEOUT 1000 > > > #define A5PSW_TABLE_ENTRIES 8192 > > > > > > +struct fdb_entry { > > > > Shouldn't this contain something along the lines of a VID, FID, something? > > This is extracted directly from the datasheet [1]. The switch FDB table > does not seems to store the VID with the entries (See page 300). > > [1] > https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-rzn1l-group-users-manual-r-engine-and-ethernet-peripherals Thanks for the link. I see that the switch has a non-partitionable lookup table, not even by VLAN. A shame. This is also in contrast with the software bridge driver, where FDB and MDB entries can have independent destinations per VID. So there's nothing you can do beyond limiting to a single offloaded bridge and hoping for the best w.r.t. per-VLAN forwarding destinations. Note that if you limit to a single bridge does not mean that you can declare ds->fdb_isolation = true. Declaring that would opt you into unicast and multicast filtering towards the CPU, i.o.w. a method for software to only receive the addresses it has expressed an interest in, rather than all packets received on standalone ports. The way that is implemented in DSA is by adding FDB and MDB entries on the management port, and it would break a lot of things without a partitioning scheme for the lookup table.
Le Wed, 20 Apr 2022 22:52:14 +0300, Vladimir Oltean <olteanv@gmail.com> a écrit : > > > > > > Shouldn't this contain something along the lines of a VID, FID, something? > > > > This is extracted directly from the datasheet [1]. The switch FDB table > > does not seems to store the VID with the entries (See page 300). > > > > [1] > > https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-rzn1l-group-users-manual-r-engine-and-ethernet-peripherals > > Thanks for the link. I see that the switch has a non-partitionable > lookup table, not even by VLAN. A shame. > > This is also in contrast with the software bridge driver, where FDB and > MDB entries can have independent destinations per VID. > > So there's nothing you can do beyond limiting to a single offloaded > bridge and hoping for the best w.r.t. per-VLAN forwarding destinations. > > Note that if you limit to a single bridge does not mean that you can > declare ds->fdb_isolation = true. Declaring that would opt you into > unicast and multicast filtering towards the CPU, i.o.w. a method for > software to only receive the addresses it has expressed an interest in, > rather than all packets received on standalone ports. The way that is > implemented in DSA is by adding FDB and MDB entries on the management > port, and it would break a lot of things without a partitioning scheme > for the lookup table. Thanks Vladimir, it confirms what I thought.
diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c index 7ab7d9054427..8c763c2a1a1f 100644 --- a/drivers/net/dsa/rzn1_a5psw.c +++ b/drivers/net/dsa/rzn1_a5psw.c @@ -369,6 +369,166 @@ 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) + 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); + + cb(lk_data.entry.mac, 0, lk_data.entry.is_static, data); + } + + return 0; +} + static void a5psw_get_strings(struct dsa_switch *ds, int port, u32 stringset, uint8_t *data) { @@ -500,6 +660,9 @@ 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, }; diff --git a/drivers/net/dsa/rzn1_a5psw.h b/drivers/net/dsa/rzn1_a5psw.h index b34ea549e936..37aa89383e70 100644 --- a/drivers/net/dsa/rzn1_a5psw.h +++ b/drivers/net/dsa/rzn1_a5psw.h @@ -167,6 +167,22 @@ #define A5PSW_CTRL_TIMEOUT 1000 #define A5PSW_TABLE_ENTRIES 8192 +struct fdb_entry { + u8 mac[ETH_ALEN]; + u8 valid:1; + u8 is_static:1; + u8 prio:3; + u8 port_mask:5; +} __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 | 163 +++++++++++++++++++++++++++++++++++ drivers/net/dsa/rzn1_a5psw.h | 16 ++++ 2 files changed, 179 insertions(+)