| Message ID | 20211217155353.460594-1-horatiu.vultur@microchip.com |
|---|---|
| Headers | show |
| Series | net: lan966x: Add switchdev and vlan support | expand |
The 12/17/2021 17:44, Vladimir Oltean wrote: > > On Fri, Dec 17, 2021 at 04:53:50PM +0100, Horatiu Vultur wrote: > > This patch adds basic support to offload in the HW the forwarding of the > > frames. The driver registers to the switchdev callbacks and implements > > the callbacks for attributes SWITCHDEV_ATTR_ID_PORT_STP_STATE and > > SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME. > > It is not allowed to add a lan966x port to a bridge that contains a > > different interface than lan966x. > > > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> > > --- > > .../net/ethernet/microchip/lan966x/Kconfig | 1 + > > .../net/ethernet/microchip/lan966x/Makefile | 2 +- > > .../ethernet/microchip/lan966x/lan966x_main.c | 32 +- > > .../ethernet/microchip/lan966x/lan966x_main.h | 9 + > > .../microchip/lan966x/lan966x_switchdev.c | 309 ++++++++++++++++++ > > 5 files changed, 351 insertions(+), 2 deletions(-) > > create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c > > > > diff --git a/drivers/net/ethernet/microchip/lan966x/Kconfig b/drivers/net/ethernet/microchip/lan966x/Kconfig > > index 2860a8c9923d..ac273f84b69e 100644 > > --- a/drivers/net/ethernet/microchip/lan966x/Kconfig > > +++ b/drivers/net/ethernet/microchip/lan966x/Kconfig > > @@ -2,6 +2,7 @@ config LAN966X_SWITCH > > tristate "Lan966x switch driver" > > depends on HAS_IOMEM > > depends on OF > > + depends on NET_SWITCHDEV > > select PHYLINK > > select PACKING > > help > > diff --git a/drivers/net/ethernet/microchip/lan966x/Makefile b/drivers/net/ethernet/microchip/lan966x/Makefile > > index 2989ba528236..974229c51f55 100644 > > --- a/drivers/net/ethernet/microchip/lan966x/Makefile > > +++ b/drivers/net/ethernet/microchip/lan966x/Makefile > > @@ -6,4 +6,4 @@ > > obj-$(CONFIG_LAN966X_SWITCH) += lan966x-switch.o > > > > lan966x-switch-objs := lan966x_main.o lan966x_phylink.o lan966x_port.o \ > > - lan966x_mac.o lan966x_ethtool.o > > + lan966x_mac.o lan966x_ethtool.o lan966x_switchdev.o > > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c > > index dc40ac2eb246..5af60234d81d 100644 > > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c > > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c > > @@ -355,6 +355,11 @@ static const struct net_device_ops lan966x_port_netdev_ops = { > > .ndo_get_port_parent_id = lan966x_port_get_parent_id, > > }; > > > > +bool lan966x_netdevice_check(const struct net_device *dev) > > +{ > > + return dev->netdev_ops == &lan966x_port_netdev_ops; > > +} > > + > > static int lan966x_port_xtr_status(struct lan966x *lan966x, u8 grp) > > { > > return lan_rd(lan966x, QS_XTR_RD(grp)); > > @@ -491,6 +496,9 @@ static irqreturn_t lan966x_xtr_irq_handler(int irq, void *args) > > > > skb->protocol = eth_type_trans(skb, dev); > > > > + if (lan966x->bridge_mask & BIT(src_port)) > > + skb->offload_fwd_mark = 1; > > + > > netif_rx_ni(skb); > > dev->stats.rx_bytes += len; > > dev->stats.rx_packets++; > > @@ -915,6 +923,7 @@ static int lan966x_remove(struct platform_device *pdev) > > { > > struct lan966x *lan966x = platform_get_drvdata(pdev); > > > > + lan966x_unregister_notifier_blocks(); > > You forgot to delete this from lan966x_remove. Good catch, I will remove this. > > > lan966x_cleanup_ports(lan966x); > > > > cancel_delayed_work_sync(&lan966x->stats_work); > > @@ -934,7 +943,28 @@ static struct platform_driver lan966x_driver = { > > .of_match_table = lan966x_match, > > }, > > }; > > -module_platform_driver(lan966x_driver); > > + > > +static int __init lan966x_switch_driver_init(void) > > +{ > > + int ret; > > + > > + ret = platform_driver_register(&lan966x_driver); > > + if (ret) > > + return ret; > > + > > + lan966x_register_notifier_blocks(); > > I think you might miss some events if you register the notifier blocks > after you've registered your net devices, so could you move this to be > first (and reverse the order for the driver exit, too)? Yes, I will do that. > > > + > > + return 0; > > +} > > + > > +static void __exit lan966x_switch_driver_exit(void) > > +{ > > + lan966x_unregister_notifier_blocks(); > > + platform_driver_unregister(&lan966x_driver); > > +} > > + > > +module_init(lan966x_switch_driver_init); > > +module_exit(lan966x_switch_driver_exit); > > > > MODULE_DESCRIPTION("Microchip LAN966X switch driver"); > > MODULE_AUTHOR("Horatiu Vultur <horatiu.vultur@microchip.com>"); > > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h > > index fcd5d09a070c..4723a904c13e 100644 > > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h > > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h > > @@ -75,6 +75,10 @@ struct lan966x { > > > > u8 base_mac[ETH_ALEN]; > > > > + struct net_device *bridge; > > + u16 bridge_mask; > > + u16 bridge_fwd_mask; > > + > > struct list_head mac_entries; > > spinlock_t mac_lock; /* lock for mac_entries list */ > > > > @@ -122,6 +126,11 @@ extern const struct phylink_mac_ops lan966x_phylink_mac_ops; > > extern const struct phylink_pcs_ops lan966x_phylink_pcs_ops; > > extern const struct ethtool_ops lan966x_ethtool_ops; > > > > +bool lan966x_netdevice_check(const struct net_device *dev); > > + > > +void lan966x_register_notifier_blocks(void); > > +void lan966x_unregister_notifier_blocks(void); > > + > > void lan966x_stats_get(struct net_device *dev, > > struct rtnl_link_stats64 *stats); > > int lan966x_stats_init(struct lan966x *lan966x); > > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c > > new file mode 100644 > > index 000000000000..9db17b677357 > > --- /dev/null > > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c > > @@ -0,0 +1,309 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > + > > +#include <linux/if_bridge.h> > > +#include <net/switchdev.h> > > + > > +#include "lan966x_main.h" > > + > > +static struct notifier_block lan966x_netdevice_nb __read_mostly; > > +static struct notifier_block lan966x_switchdev_nb __read_mostly; > > +static struct notifier_block lan966x_switchdev_blocking_nb __read_mostly; > > + > > +static void lan966x_update_fwd_mask(struct lan966x *lan966x) > > +{ > > + int i; > > + > > + for (i = 0; i < lan966x->num_phys_ports; i++) { > > + struct lan966x_port *port = lan966x->ports[i]; > > + unsigned long mask = 0; > > + > > + if (port && lan966x->bridge_fwd_mask & BIT(i)) > > + mask = lan966x->bridge_fwd_mask & ~BIT(i); > > + > > + mask |= BIT(CPU_PORT); > > + > > + lan_wr(ANA_PGID_PGID_SET(mask), > > + lan966x, ANA_PGID(PGID_SRC + i)); > > + } > > +} > > + > > +static void lan966x_port_stp_state_set(struct lan966x_port *port, u8 state) > > +{ > > + struct lan966x *lan966x = port->lan966x; > > + bool learn_ena = false; > > + > > + if (state == BR_STATE_FORWARDING || state == BR_STATE_LEARNING) > > + learn_ena = true; > > + > > + if (state == BR_STATE_FORWARDING) > > + lan966x->bridge_fwd_mask |= BIT(port->chip_port); > > + else > > + lan966x->bridge_fwd_mask &= ~BIT(port->chip_port); > > + > > + lan_rmw(ANA_PORT_CFG_LEARN_ENA_SET(learn_ena), > > + ANA_PORT_CFG_LEARN_ENA, > > + lan966x, ANA_PORT_CFG(port->chip_port)); > > + > > + lan966x_update_fwd_mask(lan966x); > > +} > > + > > +static void lan966x_port_ageing_set(struct lan966x_port *port, > > + unsigned long ageing_clock_t) > > +{ > > + unsigned long ageing_jiffies = clock_t_to_jiffies(ageing_clock_t); > > + u32 ageing_time = jiffies_to_msecs(ageing_jiffies) / 1000; > > + > > + lan966x_mac_set_ageing(port->lan966x, ageing_time); > > +} > > + > > +static int lan966x_port_attr_set(struct net_device *dev, const void *ctx, > > + const struct switchdev_attr *attr, > > + struct netlink_ext_ack *extack) > > +{ > > + struct lan966x_port *port = netdev_priv(dev); > > + int err = 0; > > + > > + if (ctx && ctx != port) > > + return 0; > > + > > + switch (attr->id) { > > + case SWITCHDEV_ATTR_ID_PORT_STP_STATE: > > + lan966x_port_stp_state_set(port, attr->u.stp_state); > > + break; > > + case SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME: > > + lan966x_port_ageing_set(port, attr->u.ageing_time); > > + break; > > + default: > > + err = -EOPNOTSUPP; > > + break; > > + } > > + > > + return err; > > +} > > + > > +static int lan966x_port_bridge_join(struct lan966x_port *port, > > + struct net_device *bridge, > > + struct netlink_ext_ack *extack) > > +{ > > + struct lan966x *lan966x = port->lan966x; > > + struct net_device *dev = port->dev; > > + int err; > > + > > + if (!lan966x->bridge_mask) { > > + lan966x->bridge = bridge; > > + } else { > > + if (lan966x->bridge != bridge) { > > + NL_SET_ERR_MSG_MOD(extack, "Not allow to add port to different bridge"); > > + return -ENODEV; > > + } > > + } > > + > > + err = switchdev_bridge_port_offload(dev, dev, port, > > + &lan966x_switchdev_nb, > > + &lan966x_switchdev_blocking_nb, > > + false, extack); > > + if (err) > > + return err; > > + > > + lan966x->bridge_mask |= BIT(port->chip_port); > > + > > + return 0; > > +} > > + > > +static void lan966x_port_bridge_leave(struct lan966x_port *port, > > + struct net_device *bridge) > > +{ > > + struct lan966x *lan966x = port->lan966x; > > + > > + lan966x->bridge_mask &= ~BIT(port->chip_port); > > + > > + if (!lan966x->bridge_mask) > > + lan966x->bridge = NULL; > > + > > + lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, PORT_PVID); > > +} > > + > > +static int lan966x_port_changeupper(struct net_device *dev, > > + struct netdev_notifier_changeupper_info *info) > > +{ > > + struct lan966x_port *port = netdev_priv(dev); > > + struct netlink_ext_ack *extack; > > + int err = 0; > > + > > + extack = netdev_notifier_info_to_extack(&info->info); > > + > > + if (netif_is_bridge_master(info->upper_dev)) { > > + if (info->linking) > > + err = lan966x_port_bridge_join(port, info->upper_dev, > > + extack); > > + else > > + lan966x_port_bridge_leave(port, info->upper_dev); > > + } > > + > > + return err; > > +} > > + > > +static int lan966x_port_prechangeupper(struct net_device *dev, > > + struct netdev_notifier_changeupper_info *info) > > +{ > > + struct lan966x_port *port = netdev_priv(dev); > > + > > + if (netif_is_bridge_master(info->upper_dev) && !info->linking) > > + switchdev_bridge_port_unoffload(port->dev, port, > > + &lan966x_switchdev_nb, > > + &lan966x_switchdev_blocking_nb); > > + > > + return NOTIFY_DONE; > > +} > > + > > +static int lan966x_foreign_bridging_check(struct net_device *bridge, > > + struct netlink_ext_ack *extack) > > +{ > > + struct lan966x *lan966x = NULL; > > + bool has_foreign = false; > > + struct net_device *dev; > > + struct list_head *iter; > > + > > + if (!netif_is_bridge_master(bridge)) > > + return 0; > > + > > + netdev_for_each_lower_dev(bridge, dev, iter) { > > + if (lan966x_netdevice_check(dev)) { > > + struct lan966x_port *port = netdev_priv(dev); > > + > > + if (lan966x) { > > + /* Bridge already has at least one port of a > > + * lan966x switch inside it, check that it's > > + * the same instance of the driver. > > + */ > > + if (port->lan966x != lan966x) { > > + NL_SET_ERR_MSG_MOD(extack, > > + "Bridging between multiple lan966x switches disallowed"); > > + return -EINVAL; > > + } > > + } else { > > + /* This is the first lan966x port inside this > > + * bridge > > + */ > > + lan966x = port->lan966x; > > + } > > + } else { > > + has_foreign = true; > > + } > > + > > + if (lan966x && has_foreign) { > > + NL_SET_ERR_MSG_MOD(extack, > > + "Bridging lan966x ports with foreign interfaces disallowed"); > > + return -EINVAL; > > + } > > + } > > + > > + return 0; > > +} > > + > > +static int lan966x_bridge_check(struct net_device *dev, > > + struct netdev_notifier_changeupper_info *info) > > +{ > > + return lan966x_foreign_bridging_check(info->upper_dev, > > + info->info.extack); > > +} > > + > > +static int lan966x_netdevice_port_event(struct net_device *dev, > > + struct notifier_block *nb, > > + unsigned long event, void *ptr) > > +{ > > + int err = 0; > > + > > + if (!lan966x_netdevice_check(dev)) { > > + if (event == NETDEV_CHANGEUPPER) > > + return lan966x_bridge_check(dev, ptr); > > + return 0; > > + } > > + > > + switch (event) { > > + case NETDEV_PRECHANGEUPPER: > > + err = lan966x_port_prechangeupper(dev, ptr); > > + break; > > + case NETDEV_CHANGEUPPER: > > + err = lan966x_bridge_check(dev, ptr); > > + if (err) > > + return err; > > + > > + err = lan966x_port_changeupper(dev, ptr); > > + break; > > + } > > + > > + return err; > > +} > > + > > +static int lan966x_netdevice_event(struct notifier_block *nb, > > + unsigned long event, void *ptr) > > +{ > > + struct net_device *dev = netdev_notifier_info_to_dev(ptr); > > + int ret; > > + > > + ret = lan966x_netdevice_port_event(dev, nb, event, ptr); > > + > > + return notifier_from_errno(ret); > > +} > > + > > +static int lan966x_switchdev_event(struct notifier_block *nb, > > + unsigned long event, void *ptr) > > +{ > > + struct net_device *dev = switchdev_notifier_info_to_dev(ptr); > > + int err; > > + > > + switch (event) { > > + case SWITCHDEV_PORT_ATTR_SET: > > + err = switchdev_handle_port_attr_set(dev, ptr, > > + lan966x_netdevice_check, > > + lan966x_port_attr_set); > > + return notifier_from_errno(err); > > + } > > + > > + return NOTIFY_DONE; > > +} > > + > > +static int lan966x_switchdev_blocking_event(struct notifier_block *nb, > > + unsigned long event, > > + void *ptr) > > +{ > > + struct net_device *dev = switchdev_notifier_info_to_dev(ptr); > > + int err; > > + > > + switch (event) { > > + case SWITCHDEV_PORT_ATTR_SET: > > + err = switchdev_handle_port_attr_set(dev, ptr, > > + lan966x_netdevice_check, > > + lan966x_port_attr_set); > > + return notifier_from_errno(err); > > + } > > + > > + return NOTIFY_DONE; > > +} > > + > > +static struct notifier_block lan966x_netdevice_nb __read_mostly = { > > + .notifier_call = lan966x_netdevice_event, > > +}; > > + > > +static struct notifier_block lan966x_switchdev_nb __read_mostly = { > > + .notifier_call = lan966x_switchdev_event, > > +}; > > + > > +static struct notifier_block lan966x_switchdev_blocking_nb __read_mostly = { > > + .notifier_call = lan966x_switchdev_blocking_event, > > +}; > > + > > +void lan966x_register_notifier_blocks(void) > > +{ > > + register_netdevice_notifier(&lan966x_netdevice_nb); > > + register_switchdev_notifier(&lan966x_switchdev_nb); > > + register_switchdev_blocking_notifier(&lan966x_switchdev_blocking_nb); > > +} > > + > > +void lan966x_unregister_notifier_blocks(void) > > +{ > > + unregister_switchdev_blocking_notifier(&lan966x_switchdev_blocking_nb); > > + unregister_switchdev_notifier(&lan966x_switchdev_nb); > > + unregister_netdevice_notifier(&lan966x_netdevice_nb); > > +} > > -- > > 2.33.0 > >
The 12/17/2021 18:14, Vladimir Oltean wrote: > > On Fri, Dec 17, 2021 at 04:53:51PM +0100, Horatiu Vultur wrote: > > @@ -120,7 +124,12 @@ static void lan966x_port_bridge_leave(struct lan966x_port *port, > > if (!lan966x->bridge_mask) > > lan966x->bridge = NULL; > > > > - lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, PORT_PVID); > > + /* Set the port back to host mode */ > > + lan966x_vlan_port_set_vlan_aware(port, false); > > + lan966x_vlan_port_set_vid(port, HOST_PVID, false, false); > > + lan966x_vlan_port_apply(port); > > + > > + lan966x_mac_cpu_learn(lan966x, port->dev->dev_addr, HOST_PVID); > > Do you still need to re-learn the port MAC address? It is not needed. I will remove this in the next version. > > > }
This patch series extends lan966x with switchdev and vlan support. The first patches just adds new registers and extend the MAC table to handle the interrupts when a new address is learn/forget. v6->v7: - fix build issues when compiling as a module v5->v6: - fix issues with the singletones, they were not really singletons - simplify the case where lan966x ports are added to bridges with foreign ports - drop the cases NETDEV_PRE_UP and NETDEV_DOWN - fix the change of MAC address - drop the callbacks .ndo_set_features, .ndo_vlan_rx_add_vid, .ndo_vlan_rx_kill_vid - remove duplicate code when port was added in a vlan, the MAC entries will be added by the fdb v4->v5: - make the notifier_block from lan966x to be singletones - use switchdev_handle_port_obj_add and switchdev_handle_fdb_event_to_device when getting callbacks in the lan966x - merge the two vlan patches in a single one v3->v4: - split the last patch in multiple patches - replace spin_lock_irqsave/restore with spin_lock/spin_unlock - remove lan966x_port_change_rx_flags because it was copying all the frames to the CPU instead of removing all RX filters. - implement SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS - remove calls to __dev_mc_unsync/sync as they are not needed - replace 0/1 with false/true - make sure that the lan966x ports are not added to bridges that have other interfaces except lan966x - and allow the lan966x ports to be part of only the same bridge. v2->v3: - separate the PVID used when the port is in host mode or vlan unaware - fix issue when the port was leaving the bridge v1->v2: - when allocating entries for the mac table use kzalloc instead of devm_kzalloc - also use GFP_KERNEL instead of GFP_ATOMIC, because is never called in atomic context - when deleting an mac table entry, the order of operations was wrong - if ana irq is enabled make sure it gets disabled when the driver is removed Horatiu Vultur (9): net: lan966x: Add registers that are used for switch and vlan functionality dt-bindings: net: lan966x: Extend with the analyzer interrupt net: lan966x: add support for interrupts from analyzer net: lan966x: More MAC table functionality net: lan966x: Remove .ndo_change_rx_flags net: lan966x: Add support to offload the forwarding. net: lan966x: Add vlan support. net: lan966x: Extend switchdev bridge flags net: lan966x: Extend switchdev with fdb support .../net/microchip,lan966x-switch.yaml | 2 + .../net/ethernet/microchip/lan966x/Kconfig | 1 + .../net/ethernet/microchip/lan966x/Makefile | 3 +- .../ethernet/microchip/lan966x/lan966x_fdb.c | 244 +++++++++ .../ethernet/microchip/lan966x/lan966x_mac.c | 342 +++++++++++++ .../ethernet/microchip/lan966x/lan966x_main.c | 103 +++- .../ethernet/microchip/lan966x/lan966x_main.h | 64 ++- .../ethernet/microchip/lan966x/lan966x_regs.h | 129 +++++ .../microchip/lan966x/lan966x_switchdev.c | 468 ++++++++++++++++++ .../ethernet/microchip/lan966x/lan966x_vlan.c | 312 ++++++++++++ 10 files changed, 1639 insertions(+), 29 deletions(-) create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_fdb.c create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_vlan.c