mbox series

[net-next,v7,0/9] net: lan966x: Add switchdev and vlan support

Message ID 20211217155353.460594-1-horatiu.vultur@microchip.com
Headers show
Series net: lan966x: Add switchdev and vlan support | expand

Message

Horatiu Vultur Dec. 17, 2021, 3:53 p.m. UTC
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

Comments

Vladimir Oltean Dec. 17, 2021, 5:40 p.m. UTC | #1
On Fri, Dec 17, 2021 at 04:53:52PM +0100, Horatiu Vultur wrote:
> Currently allow a port to be part or not of the multicast flooding mask.
> By implementing the switchdev calls SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS
> and SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  .../microchip/lan966x/lan966x_switchdev.c     | 34 +++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> index cef9e690fb82..af227b33cb3f 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> @@ -9,6 +9,34 @@ 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_port_bridge_flags(struct lan966x_port *port,
> +				      struct switchdev_brport_flags flags)
> +{
> +	u32 val = lan_rd(port->lan966x, ANA_PGID(PGID_MC));
> +
> +	val = ANA_PGID_PGID_GET(val);

Ideally you'd want to read PGID_MC only if you know that BR_MCAST_FLOOD
is the flag getting changed. Otherwise you'd have to refactor this when
you add support for more brport flags.

> +
> +	if (flags.mask & BR_MCAST_FLOOD) {
> +		if (flags.val & BR_MCAST_FLOOD)
> +			val |= BIT(port->chip_port);
> +		else
> +			val &= ~BIT(port->chip_port);
> +	}
> +
> +	lan_rmw(ANA_PGID_PGID_SET(val),
> +		ANA_PGID_PGID,
> +		port->lan966x, ANA_PGID(PGID_MC));
> +}
> +
> +static int lan966x_port_pre_bridge_flags(struct lan966x_port *port,
> +					 struct switchdev_brport_flags flags)
> +{
> +	if (flags.mask & ~BR_MCAST_FLOOD)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static void lan966x_update_fwd_mask(struct lan966x *lan966x)
>  {
>  	int i;
> @@ -67,6 +95,12 @@ static int lan966x_port_attr_set(struct net_device *dev, const void *ctx,
>  		return 0;
>  
>  	switch (attr->id) {
> +	case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS:
> +		lan966x_port_bridge_flags(port, attr->u.brport_flags);
> +		break;
> +	case SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS:
> +		err = lan966x_port_pre_bridge_flags(port, attr->u.brport_flags);
> +		break;
>  	case SWITCHDEV_ATTR_ID_PORT_STP_STATE:
>  		lan966x_port_stp_state_set(port, attr->u.stp_state);
>  		break;
> -- 
> 2.33.0
>
Vladimir Oltean Dec. 17, 2021, 5:44 p.m. UTC | #2
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.

>  	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)?

> +
> +	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
>
Vladimir Oltean Dec. 17, 2021, 6:12 p.m. UTC | #3
On Fri, Dec 17, 2021 at 04:53:53PM +0100, Horatiu Vultur wrote:
> Extend lan966x driver with fdb support by implementing the switchdev
> calls SWITCHDEV_FDB_ADD_TO_DEVICE and SWITCHDEV_FDB_DEL_TO_DEVICE.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---

Looks pretty good. Just one question, since I can't figure this out by
looking at the code. Is the CPU port in the unknown unicast flood mask
currently?

>  .../net/ethernet/microchip/lan966x/Makefile   |   2 +-
>  .../ethernet/microchip/lan966x/lan966x_fdb.c  | 244 ++++++++++++++++++
>  .../ethernet/microchip/lan966x/lan966x_main.c |   5 +
>  .../ethernet/microchip/lan966x/lan966x_main.h |  14 +
>  .../microchip/lan966x/lan966x_switchdev.c     |  21 ++
>  .../ethernet/microchip/lan966x/lan966x_vlan.c |  15 +-
>  6 files changed, 298 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_fdb.c
(...)
> +static void lan966x_fdb_add_entry(struct lan966x *lan966x,
> +				  struct switchdev_notifier_fdb_info *fdb_info)
> +{
> +	struct lan966x_fdb_entry *fdb_entry;
> +
> +	fdb_entry = lan966x_fdb_find_entry(lan966x, fdb_info);
> +	if (fdb_entry) {
> +		fdb_entry->references++;
> +		return;
> +	}
> +
> +	fdb_entry = kzalloc(sizeof(*fdb_entry), GFP_KERNEL);
> +	if (!fdb_entry)
> +		return;
> +
> +	memcpy(fdb_entry->mac, fdb_info->addr, ETH_ALEN);

ether_addr_copy

> +	fdb_entry->vid = fdb_info->vid;
> +	fdb_entry->references = 1;
> +	list_add_tail(&fdb_entry->list, &lan966x->fdb_entries);
> +}
Vladimir Oltean Dec. 17, 2021, 6:14 p.m. UTC | #4
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?

>  }
Horatiu Vultur Dec. 18, 2021, 12:55 p.m. UTC | #5
The 12/17/2021 17:40, Vladimir Oltean wrote:
> 
> On Fri, Dec 17, 2021 at 04:53:52PM +0100, Horatiu Vultur wrote:
> > Currently allow a port to be part or not of the multicast flooding mask.
> > By implementing the switchdev calls SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS
> > and SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  .../microchip/lan966x/lan966x_switchdev.c     | 34 +++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> > index cef9e690fb82..af227b33cb3f 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_switchdev.c
> > @@ -9,6 +9,34 @@ 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_port_bridge_flags(struct lan966x_port *port,
> > +                                   struct switchdev_brport_flags flags)
> > +{
> > +     u32 val = lan_rd(port->lan966x, ANA_PGID(PGID_MC));
> > +
> > +     val = ANA_PGID_PGID_GET(val);
> 
> Ideally you'd want to read PGID_MC only if you know that BR_MCAST_FLOOD
> is the flag getting changed. Otherwise you'd have to refactor this when
> you add support for more brport flags.

I can see your point. I will refactor this now, such that when new flags
are added this should not be changed.

> 
> > +
> > +     if (flags.mask & BR_MCAST_FLOOD) {
> > +             if (flags.val & BR_MCAST_FLOOD)
> > +                     val |= BIT(port->chip_port);
> > +             else
> > +                     val &= ~BIT(port->chip_port);
> > +     }
> > +
> > +     lan_rmw(ANA_PGID_PGID_SET(val),
> > +             ANA_PGID_PGID,
> > +             port->lan966x, ANA_PGID(PGID_MC));
> > +}
> > +
> > +static int lan966x_port_pre_bridge_flags(struct lan966x_port *port,
> > +                                      struct switchdev_brport_flags flags)
> > +{
> > +     if (flags.mask & ~BR_MCAST_FLOOD)
> > +             return -EINVAL;
> > +
> > +     return 0;
> > +}
> > +
> >  static void lan966x_update_fwd_mask(struct lan966x *lan966x)
> >  {
> >       int i;
> > @@ -67,6 +95,12 @@ static int lan966x_port_attr_set(struct net_device *dev, const void *ctx,
> >               return 0;
> >
> >       switch (attr->id) {
> > +     case SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS:
> > +             lan966x_port_bridge_flags(port, attr->u.brport_flags);
> > +             break;
> > +     case SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS:
> > +             err = lan966x_port_pre_bridge_flags(port, attr->u.brport_flags);
> > +             break;
> >       case SWITCHDEV_ATTR_ID_PORT_STP_STATE:
> >               lan966x_port_stp_state_set(port, attr->u.stp_state);
> >               break;
> > --
> > 2.33.0
> >
Horatiu Vultur Dec. 18, 2021, 12:56 p.m. UTC | #6
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
> >
Horatiu Vultur Dec. 18, 2021, 1 p.m. UTC | #7
The 12/17/2021 18:12, Vladimir Oltean wrote:
> 
> On Fri, Dec 17, 2021 at 04:53:53PM +0100, Horatiu Vultur wrote:
> > Extend lan966x driver with fdb support by implementing the switchdev
> > calls SWITCHDEV_FDB_ADD_TO_DEVICE and SWITCHDEV_FDB_DEL_TO_DEVICE.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> 
> Looks pretty good. Just one question, since I can't figure this out by
> looking at the code. Is the CPU port in the unknown unicast flood mask
> currently?

It is not. Because under a bridge can be only lan966x ports so the
HW will do already the flooding of the frames.

> 
> >  .../net/ethernet/microchip/lan966x/Makefile   |   2 +-
> >  .../ethernet/microchip/lan966x/lan966x_fdb.c  | 244 ++++++++++++++++++
> >  .../ethernet/microchip/lan966x/lan966x_main.c |   5 +
> >  .../ethernet/microchip/lan966x/lan966x_main.h |  14 +
> >  .../microchip/lan966x/lan966x_switchdev.c     |  21 ++
> >  .../ethernet/microchip/lan966x/lan966x_vlan.c |  15 +-
> >  6 files changed, 298 insertions(+), 3 deletions(-)
> >  create mode 100644 drivers/net/ethernet/microchip/lan966x/lan966x_fdb.c
> (...)
> > +static void lan966x_fdb_add_entry(struct lan966x *lan966x,
> > +                               struct switchdev_notifier_fdb_info *fdb_info)
> > +{
> > +     struct lan966x_fdb_entry *fdb_entry;
> > +
> > +     fdb_entry = lan966x_fdb_find_entry(lan966x, fdb_info);
> > +     if (fdb_entry) {
> > +             fdb_entry->references++;
> > +             return;
> > +     }
> > +
> > +     fdb_entry = kzalloc(sizeof(*fdb_entry), GFP_KERNEL);
> > +     if (!fdb_entry)
> > +             return;
> > +
> > +     memcpy(fdb_entry->mac, fdb_info->addr, ETH_ALEN);
> 
> ether_addr_copy
> 
> > +     fdb_entry->vid = fdb_info->vid;
> > +     fdb_entry->references = 1;
> > +     list_add_tail(&fdb_entry->list, &lan966x->fdb_entries);
> > +}
Horatiu Vultur Dec. 18, 2021, 1 p.m. UTC | #8
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.

> 
> >  }