Messages in this thread | | | From | Paweł Dembicki <> | Date | Fri, 22 Sep 2023 16:26:00 +0200 | Subject | Re: [PATCH net-next v3 5/8] net: dsa: vsc73xx: Add vlan filtering |
| |
Hi Vladimir,
wt., 12 wrz 2023 o 18:17 Vladimir Oltean <olteanv@gmail.com> napisał(a): > > On Tue, Sep 12, 2023 at 02:21:59PM +0200, Pawel Dembicki wrote: > > This patch implements VLAN filtering for the vsc73xx driver. > > > > After starting VLAN filtering, the switch is reconfigured from QinQ to > > a simple VLAN aware mode. This is required because VSC73XX chips do not > > support inner VLAN tag filtering. > > > > Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com> > > --- > > v3: > > - reworked all vlan commits > > - added storage variables for pvid and untagged vlans > > - move length extender settings to port setup > > - remove vlan table cleaning in wrong places > > - note: dev_warn was keept because function 'vsc73xx_vlan_set_untagged' > > and 'vsc73xx_vlan_set_pvid' are used later in tag implementation > > v2: > > - no changes done > > > > drivers/net/dsa/vitesse-vsc73xx-core.c | 425 ++++++++++++++++++++++++- > > drivers/net/dsa/vitesse-vsc73xx.h | 2 + > > 2 files changed, 425 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c > > index 541fbc195df1..d9a6eac1fcce 100644 > > --- a/drivers/net/dsa/vitesse-vsc73xx-core.c > > +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c > > @@ -21,14 +21,18 @@ > > #include <linux/of_mdio.h> > > #include <linux/bitops.h> > > #include <linux/if_bridge.h> > > +#include <linux/if_vlan.h> > > #include <linux/etherdevice.h> > > #include <linux/gpio/consumer.h> > > #include <linux/gpio/driver.h> > > +#include <linux/dsa/8021q.h> > > #include <linux/random.h> > > #include <net/dsa.h> > > > > #include "vitesse-vsc73xx.h" > > > > +#define VSC73XX_IS_CONFIGURED 0x1 > > + > > #define VSC73XX_BLOCK_MAC 0x1 /* Subblocks 0-4, 6 (CPU port) */ > > #define VSC73XX_BLOCK_ANALYZER 0x2 /* Only subblock 0 */ > > #define VSC73XX_BLOCK_MII 0x3 /* Subblocks 0 and 1 */ > > @@ -61,6 +65,8 @@ > > #define VSC73XX_CAT_DROP 0x6e > > #define VSC73XX_CAT_PR_MISC_L2 0x6f > > #define VSC73XX_CAT_PR_USR_PRIO 0x75 > > +#define VSC73XX_CAT_VLAN_MISC 0x79 > > +#define VSC73XX_CAT_PORT_VLAN 0x7a > > #define VSC73XX_Q_MISC_CONF 0xdf > > > > /* MAC_CFG register bits */ > > @@ -121,6 +127,17 @@ > > #define VSC73XX_ADVPORTM_IO_LOOPBACK BIT(1) > > #define VSC73XX_ADVPORTM_HOST_LOOPBACK BIT(0) > > > > +/* TXUPDCFG transmit modify setup bits */ > > +#define VSC73XX_TXUPDCFG_DSCP_REWR_MODE GENMASK(20, 19) > > +#define VSC73XX_TXUPDCFG_DSCP_REWR_ENA BIT(18) > > +#define VSC73XX_TXUPDCFG_TX_INT_TO_USRPRIO_ENA BIT(17) > > +#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID GENMASK(15, 4) > > +#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA BIT(3) > > +#define VSC73XX_TXUPDCFG_TX_UPDATE_CRC_CPU_ENA BIT(1) > > +#define VSC73XX_TXUPDCFG_TX_INSERT_TAG BIT(0) > > + > > +#define VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT 4 > > + > > /* CAT_DROP categorizer frame dropping register bits */ > > #define VSC73XX_CAT_DROP_DROP_MC_SMAC_ENA BIT(6) > > #define VSC73XX_CAT_DROP_FWD_CTRL_ENA BIT(4) > > @@ -134,6 +151,15 @@ > > #define VSC73XX_Q_MISC_CONF_EARLY_TX_512 (1 << 1) > > #define VSC73XX_Q_MISC_CONF_MAC_PAUSE_MODE BIT(0) > > > > +/* CAT_VLAN_MISC categorizer VLAN miscellaneous bits*/ > > +#define VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA BIT(8) > > +#define VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA BIT(7) > > + > > +/* CAT_PORT_VLAN categorizer port VLAN*/ > > +#define VSC73XX_CAT_PORT_VLAN_VLAN_CFI BIT(15) > > +#define VSC73XX_CAT_PORT_VLAN_VLAN_USR_PRIO GENMASK(14, 12) > > +#define VSC73XX_CAT_PORT_VLAN_VLAN_VID GENMASK(11, 0) > > + > > /* Frame analyzer block 2 registers */ > > #define VSC73XX_STORMLIMIT 0x02 > > #define VSC73XX_ADVLEARN 0x03 > > @@ -188,7 +214,8 @@ > > #define VSC73XX_VLANACCESS_VLAN_MIRROR BIT(29) > > #define VSC73XX_VLANACCESS_VLAN_SRC_CHECK BIT(28) > > #define VSC73XX_VLANACCESS_VLAN_PORT_MASK GENMASK(9, 2) > > -#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK GENMASK(2, 0) > > +#define VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT 2 > > +#define VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK GENMASK(1, 0) > > #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_IDLE 0 > > #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_READ_ENTRY 1 > > #define VSC73XX_VLANACCESS_VLAN_TBL_CMD_WRITE_ENTRY 2 > > @@ -343,6 +370,11 @@ static const struct vsc73xx_counter vsc73xx_tx_counters[] = { > > { 29, "TxQoSClass3" }, /* non-standard counter */ > > }; > > > > +enum vsc73xx_port_vlan_conf { > > + VSC73XX_VLAN_FILTER, > > + VSC73XX_VLAN_IGNORE, > > +}; > > + > > int vsc73xx_is_addr_valid(u8 block, u8 subblock) > > { > > switch (block) { > > @@ -563,7 +595,7 @@ static enum dsa_tag_protocol vsc73xx_get_tag_protocol(struct dsa_switch *ds, > > static int vsc73xx_setup(struct dsa_switch *ds) > > { > > struct vsc73xx *vsc = ds->priv; > > - int i; > > + int i, ret; > > ../drivers/net/dsa/vitesse-vsc73xx-core.c:598:9: warning: unused variable 'ret' [-Wunused-variable] > int i, ret; > ^ > > > > > dev_info(vsc->dev, "set up the switch\n"); > > > > @@ -623,6 +655,9 @@ static int vsc73xx_setup(struct dsa_switch *ds) > > vsc73xx_write(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_GMIIDELAY, > > VSC73XX_GMIIDELAY_GMII0_GTXDELAY_2_0_NS | > > VSC73XX_GMIIDELAY_GMII0_RXDELAY_2_0_NS); > > + /* Ingess VLAN reception mask (table 145) */ > > + vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANMASK, > > + 0x5f); > > /* IP multicast flood mask (table 144) */ > > vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_IFLODMSK, > > 0xff); > > @@ -1031,8 +1066,387 @@ static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port) > > return 9600 - ETH_HLEN - ETH_FCS_LEN; > > } > > > > +static int vsc73xx_wait_for_vlan_table_cmd(struct vsc73xx *vsc) > > +{ > > + int ret, err; > > + u32 val; > > + > > + ret = read_poll_timeout(vsc73xx_read, err, > > + err < 0 || ((val & VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK) == > > + VSC73XX_VLANACCESS_VLAN_TBL_CMD_IDLE), > > + 1000, 10000, false, vsc, VSC73XX_BLOCK_ANALYZER, > > + 0, VSC73XX_VLANACCESS, &val); > > + if (ret) > > + return ret; > > + return err; > > +} > > + > > +static int > > +vsc73xx_read_vlan_table_entry(struct vsc73xx *vsc, u16 vid, u8 *portmap) > > +{ > > + u32 val; > > + int ret; > > + > > + vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid); > > + ret = vsc73xx_wait_for_vlan_table_cmd(vsc); > > + if (ret) > > + return ret; > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS, > > + VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK, > > + VSC73XX_VLANACCESS_VLAN_TBL_CMD_READ_ENTRY); > > + ret = vsc73xx_wait_for_vlan_table_cmd(vsc); > > + if (ret) > > + return ret; > > + vsc73xx_read(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS, &val); > > + *portmap = (val & VSC73XX_VLANACCESS_VLAN_PORT_MASK) >> > > + VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT; > > + return 0; > > +} > > + > > +static int > > +vsc73xx_write_vlan_table_entry(struct vsc73xx *vsc, u16 vid, u8 portmap) > > +{ > > + int ret; > > + > > + vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANTIDX, vid); > > + ret = vsc73xx_wait_for_vlan_table_cmd(vsc); > > + if (ret) > > + return ret; > > + > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_VLANACCESS, > > + VSC73XX_VLANACCESS_VLAN_TBL_CMD_MASK | > > + VSC73XX_VLANACCESS_VLAN_SRC_CHECK | > > + VSC73XX_VLANACCESS_VLAN_PORT_MASK, > > + VSC73XX_VLANACCESS_VLAN_TBL_CMD_WRITE_ENTRY | > > + VSC73XX_VLANACCESS_VLAN_SRC_CHECK | > > + (portmap << > > + VSC73XX_VLANACCESS_VLAN_PORT_MASK_SHIFT)); > > + > > + return vsc73xx_wait_for_vlan_table_cmd(vsc); > > +} > > + > > +static int > > +vsc73xx_update_vlan_table(struct vsc73xx *vsc, int port, u16 vid, bool set) > > +{ > > + u8 portmap; > > + int ret; > > + > > + ret = vsc73xx_read_vlan_table_entry(vsc, vid, &portmap); > > + if (ret) > > + return ret; > > + > > + if (set) > > + portmap |= BIT(port) | BIT(CPU_PORT); > > + else > > + portmap &= ~BIT(port); > > + > > + if (portmap == BIT(CPU_PORT)) > > + portmap = 0; > > Why did you need to do this? To my knowledge, the DSA framework code > will decide when to remove VLANs from the CPU port. >
I will fix it.
> > + > > + return vsc73xx_write_vlan_table_entry(vsc, vid, portmap); > > +} > > + > > +static void > > +vsc73xx_set_vlan_conf(struct vsc73xx *vsc, int port, > > + enum vsc73xx_port_vlan_conf port_vlan_conf) > > +{ > > + if (port_vlan_conf == VSC73XX_VLAN_IGNORE) { > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, > > + VSC73XX_CAT_VLAN_MISC, > > + VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA, > > + VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA); > > IMO this, and all the other places that write the same registers from 2 > branches, would look less clutered like this: > > u32 val; > > val = (port_vlan_conf == VSC73XX_VLAN_IGNORE) ? > VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA : 0; > > vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_VLAN_MISC, > VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA, val); > > val = ... > > Otherwise, the larger the branches become, the harder it gets to see > what's common and what's different. > > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, > > + VSC73XX_CAT_VLAN_MISC, > > + VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA, > > + VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA); > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, > > + VSC73XX_TXUPDCFG, > > + VSC73XX_TXUPDCFG_TX_INSERT_TAG, 0); > > + } else { > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, > > + VSC73XX_CAT_VLAN_MISC, > > + VSC73XX_CAT_VLAN_MISC_VLAN_TCI_IGNORE_ENA, > > + 0); > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, > > + VSC73XX_CAT_VLAN_MISC, > > + VSC73XX_CAT_VLAN_MISC_VLAN_KEEP_TAG_ENA, 0); > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, > > + VSC73XX_TXUPDCFG, > > + VSC73XX_TXUPDCFG_TX_INSERT_TAG, > > + VSC73XX_TXUPDCFG_TX_INSERT_TAG); > > + } > > +} > > + > > +static int > > +vsc73xx_vlan_change_untagged(struct vsc73xx *vsc, int port, u16 vid, bool set) > > +{ > > + u32 val = set ? VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA : 0; > > + > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, > > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, val); > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, > > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, > > + (vid << VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT) & > > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID); > > + return 0; > > +} > > + > > +static int > > +vsc73xx_vlan_change_pvid(struct vsc73xx *vsc, int port, u16 vid, bool set) > > Would it simplify callers to add a "bool operate_on_storage" argument here, > and absorb that logic? The callers could look like this, notice how there is > less code duplication between the "if" and "else" branches. >
I will rework it.
> static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid, > bool tag_8021q_vlan) > { > struct dsa_port *dp = dsa_to_port(ds, port); > struct vsc73xx *vsc = ds->priv; > bool operate_on_storage; > u16 existing_pvid; > bool had_pvid; > > operate_on_storage = vsc73xx_tag_8021q_active(dp) ^ tag_8021q_vlan; > > had_pvid = vsc73xx_port_get_pvid(vsc, port, &existing_pvid, > &operate_on_storage); > if (had_pvid && existing_pvid != vid) { > dev_warn(vsc->dev, > "Port %d can have only one pvid! Now is: %d.\n", > port, existing_pvid); > return -EOPNOTSUPP; > } > > return vsc73xx_vlan_change_pvid(vsc, port, vid, true, operate_on_storage); > } > > > +{ > > + u32 val = set ? 0 : VSC73XX_CAT_DROP_UNTAGGED_ENA; > > + > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP, > > + VSC73XX_CAT_DROP_UNTAGGED_ENA, val); > > + > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, > > + VSC73XX_CAT_PORT_VLAN_VLAN_VID, > > + vid & VSC73XX_CAT_PORT_VLAN_VLAN_VID); > > + return 0; > > +} > > + > > +static int vsc73xx_vlan_port_is_pvid(struct vsc73xx *vsc, int port, u16 *vid) > > I believe it would look better if this would return bool, and if it had > a shorter name (vsc73xx_port_get_pvid). > > > +{ > > + u32 val; > > + > > + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP, &val); > > + if (val & VSC73XX_CAT_DROP_UNTAGGED_ENA) > > + return 0; > > + > > + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, &val); > > + *vid = val & VSC73XX_CAT_PORT_VLAN_VLAN_VID; > > + > > + return VSC73XX_IS_CONFIGURED; > > +} > > + > > +static int vsc73xx_vlan_port_is_untagged(struct vsc73xx *vsc, int port, u16 *vid) > > +{ > > + u32 val; > > + > > + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, &val); > > + if (!(val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA)) > > + return 0; > > + > > + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, > > + &val); > > Don't need to read the same register twice. > > > + *vid = (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) >> > > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT; > > + > > + return VSC73XX_IS_CONFIGURED; > > +} > > + > > +static int vsc73xx_vlan_set_untagged(struct dsa_switch *ds, int port, u16 vid, > > + bool port_vlan) > > +{ > > + struct vsc73xx *vsc = ds->priv; > > + u16 vlan_no = 0; > > + > > + if (dsa_port_is_vlan_filtering(dsa_to_port(ds, port)) ^ port_vlan) { > > I think it would be valuable for readability to create a helper function named: > > static bool vsc73xx_tag_8021q_active(struct dsa_port *dp) > { > return !dsa_port_is_vlan_filtering(dp); > } > > and then, to rename "port_vlan" to "!tag_8021q_vlan". It would make it > clearer as to what the checks are about. > > if (vsc73xx_tag_8021q_active(dp) ^ tag_8021q_vlan) { > // update cached storage > } else { > // update hardware > } > > > + if (vsc->untagged_storage[port] < VLAN_N_VID && > > + !vid_is_dsa_8021q(vsc->untagged_storage[port]) && > > + !vid_is_dsa_8021q(vid) && > > The problem here which led to these vid_is_dsa_8021q() checks is that > dsa_switch_tag_8021q_vlan_add() sets all flags on user ports to > BRIDGE_VLAN_INFO_UNTAGGED | BRIDGE_VLAN_INFO_PVID, and you can't offload > those, correct?
In my case, the major problem with tag8021q vlans is "dsa_tag_8021q_bridge_join" function: "dsa_port_tag_8021q_vlan_add" is called before "dsa_port_tag_8021q_vlan_del". I must disable pvid/untagged checking, because it will always fail. I let kernel do the job, it keeps only one untagged/pvid per port after "dsa_tag_8021q_bridge_join".
> But when the port is VSC73XX_VLAN_IGNORE mode (and > tag_8021q is active), VSC73XX_TXUPDCFG_TX_INSERT_TAG is 0, and thus, > *all* VLANs are egress-untagged VLANs, correct? > > If that is the case, why do you call vsc73xx_vlan_set_untagged() in the > first place, for tag_8021q VLANs, if you don't rely on the port's native > VLAN for egress untagging? > > > + vsc->untagged_storage[port] != vid) { > > + dev_warn(vsc->dev, > > + "Port %d can have only one untagged vid! Now is: %d.\n", > > + port, vsc->untagged_storage[port]); > > + return -EOPNOTSUPP; > > + } > > + vsc->untagged_storage[port] = vid; > > + } else { > > + if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED && > > + !vid_is_dsa_8021q(vlan_no) && !vid_is_dsa_8021q(vid) && vlan_no != vid) { > > + dev_warn(vsc->dev, > > + "Port %d can have only one untagged vid! Now is: %d.\n", > > + port, vlan_no); > > + return -EOPNOTSUPP; > > + } > > + return vsc73xx_vlan_change_untagged(vsc, port, vid, 1); > > + } > > + > > + return 0; > > +} > > + > > +static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid, > > + bool port_vlan) > > +{ > > + struct dsa_port *dsa_port = dsa_to_port(ds, port); > > + struct vsc73xx *vsc = ds->priv; > > + u16 vlan_no; > > + > > + if (!dsa_port) > > + return -EINVAL; > > Why is this check here? >
Unnecessary defensive programming.
> > + > > + if (dsa_port_is_vlan_filtering(dsa_port) ^ port_vlan) { > > + if (vsc->pvid_storage[port] < VLAN_N_VID && > > + !vid_is_dsa_8021q(vsc->pvid_storage[port]) && > > + !vid_is_dsa_8021q(vid) && vsc->pvid_storage[port] != vid) { > > What are the vid_is_dsa_8021q() checks for?
The same reason as the "untagged" case.
> > > + dev_warn(vsc->dev, > > + "Port %d can have only one pvid! Now is: %d.\n", > > + port, vsc->pvid_storage[port]); > > + return -EOPNOTSUPP; > > + } > > + vsc->pvid_storage[port] = vid; > > + } else { > > + if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED && > > + !vid_is_dsa_8021q(vlan_no) && !vid_is_dsa_8021q(vid) && > > + vlan_no != vid) { > > + dev_warn(vsc->dev, > > + "Port %d can have only one pvid! Now is: %d.\n", > > + port, vlan_no); > > + return -EOPNOTSUPP; > > + } > > + return vsc73xx_vlan_change_pvid(vsc, port, vid, 1); > > + } > > + > > + return 0; > > +} > > + > > +static int > > +vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port, > > + bool vlan_filtering, struct netlink_ext_ack *extack) > > A comment would be good which states that the flipping between the > hardware and the storage values relies on the fact that vsc73xx_port_vlan_filtering() > only gets called on actual changes to vlan_filtering, and thus, to > vsc73xx_tag_8021q_active(). So, we know for sure that what is in storage > needs to go to hardware, and what is in hardware needs to go to storage. > > It's an interesting implementation for sure. >
Thank you.
> > +{ > > + struct vsc73xx *vsc = ds->priv; > > + bool store_untagged = false; > > + bool store_pvid = false; > > nit: replace the 2 spaces before the "=" with a single one. > > > + u16 vlan_no; > > + > > + if (vlan_filtering) > > + vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_FILTER); > > + else > > + vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_IGNORE); > > + > > + if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED) > > + store_pvid = true; > > + > > + if (vsc->pvid_storage[port] < VLAN_N_VID) > > + vsc73xx_vlan_change_pvid(vsc, port, vsc->pvid_storage[port], true); > > + else > > + vsc73xx_vlan_change_pvid(vsc, port, 0, false); > > + > > + vsc->pvid_storage[port] = store_pvid ? vlan_no : VLAN_N_VID; > > + > > + if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED) > > + store_untagged = true; > > + > > + if (vsc->untagged_storage[port] < VLAN_N_VID) > > + vsc73xx_vlan_change_untagged(vsc, port, vsc->untagged_storage[port], true); > > + else > > + vsc73xx_vlan_change_untagged(vsc, port, 0, false); > > + > > + vsc->untagged_storage[port] = store_untagged ? vlan_no : VLAN_N_VID; > > + > > + return 0; > > +} > > + > > +static int vsc73xx_port_vlan_add(struct dsa_switch *ds, int port, > > + const struct switchdev_obj_port_vlan *vlan, > > + struct netlink_ext_ack *extack) > > +{ > > + bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED; > > + bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID; > > + struct vsc73xx *vsc = ds->priv; > > + u16 vlan_no; > > + int ret; > > + > > + /* Be sure to deny alterations to the configuration done by tag_8021q. > > + */ > > + if (vid_is_dsa_8021q(vlan->vid)) { > > + NL_SET_ERR_MSG_MOD(extack, > > + "Range 3072-4095 reserved for dsa_8021q operation"); > > + return -EBUSY; > > + } > > + > > + if (port != CPU_PORT) { > > + if (untagged) { > > + ret = vsc73xx_vlan_set_untagged(ds, port, vlan->vid, true); > > + if (ret) > > + return ret; > > + } else { > > + if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) > > + == VSC73XX_IS_CONFIGURED && > > + vlan_no == vlan->vid) > > + vsc73xx_vlan_change_untagged(vsc, port, 0, false); > > + else if (vsc->untagged_storage[port] == vlan->vid) > > + vsc->untagged_storage[port] = VLAN_N_VID; > > + } > > + if (pvid) { > > + ret = vsc73xx_vlan_set_pvid(ds, port, vlan->vid, true); > > + if (ret) > > + return ret; > > + } else { > > + if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) > > + == VSC73XX_IS_CONFIGURED && > > + vlan_no == vlan->vid) > > + vsc73xx_vlan_change_pvid(vsc, port, 0, false); > > + else if (vsc->pvid_storage[port] == vlan->vid) > > + vsc->pvid_storage[port] = VLAN_N_VID; > > + } > > + } > > + > > + return vsc73xx_update_vlan_table(vsc, port, vlan->vid, 1); > > last argument is bool, so pass true or false (here and everywhere else) > > > +} > > + > > +static int vsc73xx_port_vlan_del(struct dsa_switch *ds, int port, > > + const struct switchdev_obj_port_vlan *vlan) > > +{ > > + struct vsc73xx *vsc = ds->priv; > > + u16 vlan_no; > > + int ret; > > + > > + ret = vsc73xx_update_vlan_table(vsc, port, vlan->vid, 0); > > + if (ret) > > + return ret; > > + > > + if (vsc73xx_vlan_port_is_untagged(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED && > > + vlan_no == vlan->vid) > > + vsc73xx_vlan_change_untagged(vsc, port, 0, false); > > + else if (vsc->untagged_storage[port] == vlan->vid) > > + vsc->untagged_storage[port] = VLAN_N_VID; > > + > > + if (vsc73xx_vlan_port_is_pvid(vsc, port, &vlan_no) == VSC73XX_IS_CONFIGURED && > > + vlan_no == vlan->vid) > > + vsc73xx_vlan_change_pvid(vsc, port, 0, false); > > + else if (vsc->pvid_storage[port] == vlan->vid) > > + vsc->pvid_storage[port] = VLAN_N_VID; > > + > > + return 0; > > +} > > + > > static int vsc73xx_port_setup(struct dsa_switch *ds, int port) > > { > > + struct vsc73xx *vsc = ds->priv; > > + int ret, i; > > + > > + /* Those bits are responsible for MTU only. Kernel take care about MTU, > > + * let's enable +8 bytes frame length unconditionally. > > + */ > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG, > > + VSC73XX_MAC_CFG_VLAN_AWR | VSC73XX_MAC_CFG_VLAN_DBLAWR, > > + VSC73XX_MAC_CFG_VLAN_AWR | VSC73XX_MAC_CFG_VLAN_DBLAWR); > > + > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP, > > + VSC73XX_CAT_DROP_TAGGED_ENA, 0); > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_DROP, > > + VSC73XX_CAT_DROP_UNTAGGED_ENA, > > + VSC73XX_CAT_DROP_UNTAGGED_ENA); > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, > > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA, 0); > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, > > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, 0); > > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, > > + VSC73XX_CAT_PORT_VLAN_VLAN_VID, 0); > > + > > + if (port == CPU_PORT) > > + vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_FILTER); > > + else > > + vsc73xx_set_vlan_conf(vsc, port, VSC73XX_VLAN_IGNORE); > > + > > + for (i = 0; i <= VLAN_N_VID; i++) { > > VLAN_N_VID is 4096, so i <= VLAN_N_VID must be a mistake? >
Yeep.
> > + ret = vsc73xx_update_vlan_table(vsc, port, i, 0); > > + if (ret) > > + return ret; > > + } > > Also, could you write an expedited version of this, which is not called > from ds->ops->port_setup() but from ds->ops->setup()? It is wasteful to > traverse the VLAN table for each port, when we know from the get go that > we need to clear all ports. >
I'll rework it in setup.
> > + > > /* Configure forward map to CPU <-> port only */ > > if (port == CPU_PORT) > > vsc->forward_map[CPU_PORT] = VSC73XX_SRCMASKS_PORTS_MASK & > > @@ -1041,6 +1455,10 @@ static int vsc73xx_port_setup(struct dsa_switch *ds, int port) > > vsc->forward_map[port] = VSC73XX_SRCMASKS_PORTS_MASK & > > BIT(CPU_PORT); > > > > + /* Set storage values out of range*/ > > Nit: Space after "*/" (there are a few other places left which have > this style issue) > > > + vsc->pvid_storage[port] = VLAN_N_VID; > > + vsc->untagged_storage[port] = VLAN_N_VID; > > + > > return 0; > > } > > > > @@ -1098,6 +1516,9 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = { > > .port_change_mtu = vsc73xx_change_mtu, > > .port_max_mtu = vsc73xx_get_max_mtu, > > .port_stp_state_set = vsc73xx_port_stp_state_set, > > + .port_vlan_filtering = vsc73xx_port_vlan_filtering, > > + .port_vlan_add = vsc73xx_port_vlan_add, > > + .port_vlan_del = vsc73xx_port_vlan_del, > > }; > > > > static int vsc73xx_gpio_get(struct gpio_chip *chip, unsigned int offset) > > diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h > > index 224e284a5573..b7614dd7d0eb 100644 > > --- a/drivers/net/dsa/vitesse-vsc73xx.h > > +++ b/drivers/net/dsa/vitesse-vsc73xx.h > > @@ -30,6 +30,8 @@ struct vsc73xx { > > const struct vsc73xx_ops *ops; > > void *priv; > > u8 forward_map[VSC73XX_MAX_NUM_PORTS]; > > + u16 pvid_storage[VSC73XX_MAX_NUM_PORTS]; > > + u16 untagged_storage[VSC73XX_MAX_NUM_PORTS]; > > }; > > > > struct vsc73xx_ops { > > -- > > 2.34.1 > > >
| |