| Message ID | 20230912122201.3752918-6-paweldembicki@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: vsc73xx: Make vsc73xx usable | expand |
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. > + > + 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. 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? 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? > + > + 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? > + 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. > +{ > + 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? > + 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. > + > /* 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 >
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 > > >
On Fri, Sep 22, 2023 at 04:26:00PM +0200, Paweł Dembicki wrote: > > > + 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". I'm not sure that you described the problem in a way that I can understand, here. After dsa_tag_8021q_bridge_join(): -> dsa_port_tag_8021q_vlan_add(dp, bridge_vid) -> dsa_port_tag_8021q_vlan_del(dp, standalone_vid) it's *expected* that there should be only one untagged/pvid per port: the bridge_vid. For context, consider the fact that you can run the following commands: bridge vlan add dev eth0 vid 10 pvid bridge vlan add dev eth0 vid 11 pvid and after the second command, vid 10 stops being a pvid. So I think that the "Port %d can have only one pvid! Now is: %d.\n" behavior is not correct. You need to implement the pvid overwriting behavior, since there's always only 1 pvid. So that leaves the "untagged" flag as being problematic, correct? Could you comment... > > > 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? ... on this? Here I'm pointing out that "all VLANs have the egress-untagged flag" is a configuration that can actually be supported by vsc73xx. You just need to ensure that VSC73XX_TXUPDCFG_TX_INSERT_TAG is 0. And tag_8021q basically requests exactly that configuration on user ports (both the bridge_vid and the standalone_vid are egress-untagged). So your check is too restrictive, you are denying a configuration that would work. The problem only appears when you mix egress-tagged with egress-untagged VLANs on a port. Only then there can be at most 1 egress-untagged VID, because you need to enable VSC73XX_TXUPDCFG_TX_INSERT_TAG for the egress-tagged VIDs to work. > > 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. I'm not sure if that was a compliment :) At least in this form, it's certainly non-trivial to determine by looking at the code if it is correct or not, and it uses different patterns than the other VLAN implementations in DSA drivers. Generally, boring and obvious is preferable. But after I took the time to understand, it seems plausible that the approach might work. Let's see how the same idea looks, cleaned up a bit but not redesigned, in v4.
śr., 27 wrz 2023 o 01:58 Vladimir Oltean <olteanv@gmail.com> napisał(a): > > On Fri, Sep 22, 2023 at 04:26:00PM +0200, Paweł Dembicki wrote: > > > > + 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". > > I'm not sure that you described the problem in a way that I can understand, here. > > After dsa_tag_8021q_bridge_join(): > -> dsa_port_tag_8021q_vlan_add(dp, bridge_vid) > -> dsa_port_tag_8021q_vlan_del(dp, standalone_vid) > > it's *expected* that there should be only one untagged/pvid per port: the bridge_vid. > > For context, consider the fact that you can run the following commands: > > bridge vlan add dev eth0 vid 10 pvid > bridge vlan add dev eth0 vid 11 pvid > > and after the second command, vid 10 stops being a pvid. > > So I think that the "Port %d can have only one pvid! Now is: %d.\n" behavior > is not correct. You need to implement the pvid overwriting behavior, since > there's always only 1 pvid. > Yes, overwriting pvid is the only proper way. Kernel mechanism will take care about the number of pvids. I will fixit in v4. > So that leaves the "untagged" flag as being problematic, correct? Could > you comment... > > > > > > 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? > > ... on this? Here I'm pointing out that "all VLANs have the egress-untagged flag" > is a configuration that can actually be supported by vsc73xx. You just > need to ensure that VSC73XX_TXUPDCFG_TX_INSERT_TAG is 0. And tag_8021q > basically requests exactly that configuration on user ports (both the > bridge_vid and the standalone_vid are egress-untagged). So your check is > too restrictive, you are denying a configuration that would work. > The problem only appears when you mix egress-tagged with egress-untagged > VLANs on a port. Only then there can be at most 1 egress-untagged VID, > because you need to enable VSC73XX_TXUPDCFG_TX_INSERT_TAG for the > egress-tagged VIDs to work. Should I make a local copy of the quantity of egress untagged and tagged vlans per port to resolve this issue, shouldn't I? And then I check how many vlans are egress tagged or untagged for a properly restricted solution? I see another problem. Even if I return an error value, the untagged will be marked in 'bridge vlan' listing. I'm not sure how it should work in this case. > > > > 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. > > I'm not sure if that was a compliment :) Touché. :) >At least in this form, it's > certainly non-trivial to determine by looking at the code if it is > correct or not, and it uses different patterns than the other VLAN > implementations in DSA drivers. Generally, boring and obvious is > preferable. But after I took the time to understand, it seems plausible > that the approach might work. > > Let's see how the same idea looks, cleaned up a bit but not redesigned, > in v4. I try to at least clean pvid and untagged issues before v4.
On Tue, Oct 03, 2023 at 11:14:55PM +0200, Paweł Dembicki wrote: > Should I make a local copy of the quantity of egress untagged and > tagged vlans per port to resolve this issue, shouldn't I? > And then I check how many vlans are egress tagged or untagged for a > properly restricted solution? Yeah, I guess so. It is the same problem that ocelot_vlan_prepare() tries to solve, and that counts ocelot_port_num_untagged_vlans() and ocelot_port_num_tagged_vlans() indeed. > I see another problem. Even if I return an error value, the untagged > will be marked in 'bridge vlan' listing. I'm not sure how it should > work in this case. What error code are you returning, -EOPNOTSUPP I guess? That's deliberately ignored by callers of br_switchdev_port_vlan_add(), because it means "no one responded to the switchdev notifier, so the VLAN is not offloaded". If you want to deny the configuration you need to return something else, like -EBUSY.
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; 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; + + 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); + 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) +{ + 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) +{ + 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); + *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) { + if (vsc->untagged_storage[port] < VLAN_N_VID && + !vid_is_dsa_8021q(vsc->untagged_storage[port]) && + !vid_is_dsa_8021q(vid) && + 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; + + 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) { + 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) +{ + struct vsc73xx *vsc = ds->priv; + bool store_untagged = false; + bool store_pvid = false; + 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); +} + +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++) { + ret = vsc73xx_update_vlan_table(vsc, port, i, 0); + if (ret) + return ret; + } + /* 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*/ + 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 {
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(-)