| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20211218125632.uz4vb55l3t5i43kd@soft-dev3-1.localhost>
Date: Sat, 18 Dec 2021 13:56:32 +0100
From: Horatiu Vultur <horatiu.vultur@...rochip.com>
To: Vladimir Oltean <vladimir.oltean@....com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"davem@...emloft.net" <davem@...emloft.net>,
"kuba@...nel.org" <kuba@...nel.org>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
"linux@...linux.org.uk" <linux@...linux.org.uk>,
"f.fainelli@...il.com" <f.fainelli@...il.com>,
"vivien.didelot@...il.com" <vivien.didelot@...il.com>,
"andrew@...n.ch" <andrew@...n.ch>
Subject: Re: [PATCH net-next v7 6/9] net: lan966x: Add support to offload the
forwarding.
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@...rochip.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@...rochip.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
Powered by blists - more mailing lists