Date:   Mon, 11 May 2020 14:11:34 +0300
From:   Vadym Kochan <vadym.kochan@...ision.eu>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Oleksandr Mazur <oleksandr.mazur@...ision.eu>,
        Serhiy Boiko <serhiy.boiko@...ision.eu>,
        Serhiy Pshyk <serhiy.pshyk@...ision.eu>,
        Volodymyr Mytnyk <volodymyr.mytnyk@...ision.eu>,
        Taras Chornyi <taras.chornyi@...ision.eu>,
        Andrii Savka <andrii.savka@...ision.eu>,
        Jiri Pirko <jiri@...lanox.com>,
        Ido Schimmel <idosch@...lanox.com>,
        Andrew Lunn <andrew@...n.ch>,
        Chris Packham <Chris.Packham@...iedtelesis.co.nz>
Subject: Re: [RFC next-next v2 1/5] net: marvell: prestera: Add driver for
 Prestera family ASIC devices

Hi Jiri,

On Mon, May 11, 2020 at 12:32:22PM +0200, Jiri Pirko wrote:
> Fri, May 01, 2020 at 01:20:48AM CEST, vadym.kochan@...ision.eu wrote:
> >Marvell Prestera 98DX326x integrates up to 24 ports of 1GbE with 8
> >ports of 10GbE uplinks or 2 ports of 40Gbps stacking for a largely
> >wireless SMB deployment.
> >
> >The current implementation supports only boards designed for the Marvell
> >Switchdev solution and requires special firmware.
> >
> >The core Prestera switching logic is implemented in prestera.c, there is
> >an intermediate hw layer between core logic and firmware. It is
> >implemented in prestera_hw.c, the purpose of it is to encapsulate hw
> >related logic, in future there is a plan to support more devices with
> >different HW related configurations.
> >
> >This patch contains only basic switch initialization and RX/TX support
> >over SDMA mechanism.
> >
> >Currently supported devices have DMA access range <= 32bit and require
> >ZONE_DMA to be enabled, for such cases SDMA driver checks if the skb
> >allocated in proper range supported by the Prestera device.
> >
> >Also meanwhile there is no TX interrupt support in current firmware
> >version so recycling work is sheduled on each xmit.
> >
> >It is required to specify 'base_mac' module parameter which is used for
> 
> No module parameter please.
> 
I understand this is not good, but currently this is simple solution to
handle base MAC configuration for different boards which has this PP,
otherwise it needs to by supported by platform drivers. Is there some
generic way to handle this ?

> 
> >generation of initial port's mac address, as currently there is no
> >some generic way to set it because base mac can be stored on different
> >storage places.
> >
> >Signed-off-by: Andrii Savka <andrii.savka@...ision.eu>
> >Signed-off-by: Oleksandr Mazur <oleksandr.mazur@...ision.eu>
> >Signed-off-by: Serhiy Boiko <serhiy.boiko@...ision.eu>
> >Signed-off-by: Serhiy Pshyk <serhiy.pshyk@...ision.eu>
> >Signed-off-by: Taras Chornyi <taras.chornyi@...ision.eu>
> >Signed-off-by: Volodymyr Mytnyk <volodymyr.mytnyk@...ision.eu>
> >Signed-off-by: Vadym Kochan <vadym.kochan@...ision.eu>
> >---
> > drivers/net/ethernet/marvell/Kconfig          |   1 +
> > drivers/net/ethernet/marvell/Makefile         |   1 +
> > drivers/net/ethernet/marvell/prestera/Kconfig |  13 +
> > .../net/ethernet/marvell/prestera/Makefile    |   4 +
> > .../net/ethernet/marvell/prestera/prestera.c  | 530 +++++++++++
> > .../net/ethernet/marvell/prestera/prestera.h  | 172 ++++
> > .../ethernet/marvell/prestera/prestera_dsa.c  | 134 +++
> > .../ethernet/marvell/prestera/prestera_dsa.h  |  37 +
> > .../ethernet/marvell/prestera/prestera_hw.c   | 614 +++++++++++++
> > .../ethernet/marvell/prestera/prestera_hw.h   |  71 ++
> > .../ethernet/marvell/prestera/prestera_rxtx.c | 825 ++++++++++++++++++
> > .../ethernet/marvell/prestera/prestera_rxtx.h |  21 +
> > 12 files changed, 2423 insertions(+)
> > create mode 100644 drivers/net/ethernet/marvell/prestera/Kconfig
> > create mode 100644 drivers/net/ethernet/marvell/prestera/Makefile
> > create mode 100644 drivers/net/ethernet/marvell/prestera/prestera.c
> > create mode 100644 drivers/net/ethernet/marvell/prestera/prestera.h
> > create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_dsa.c
> > create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_dsa.h
> > create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_hw.c
> > create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_hw.h
> > create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_rxtx.c
> > create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_rxtx.h
> >
> >diff --git a/drivers/net/ethernet/marvell/Kconfig b/drivers/net/ethernet/marvell/Kconfig
> >index 3d5caea096fb..74313d9e1fc0 100644
> >--- a/drivers/net/ethernet/marvell/Kconfig
> >+++ b/drivers/net/ethernet/marvell/Kconfig
> >@@ -171,5 +171,6 @@ config SKY2_DEBUG
> > 
> > 
> > source "drivers/net/ethernet/marvell/octeontx2/Kconfig"
> >+source "drivers/net/ethernet/marvell/prestera/Kconfig"
> > 
> > endif # NET_VENDOR_MARVELL
> >diff --git a/drivers/net/ethernet/marvell/Makefile b/drivers/net/ethernet/marvell/Makefile
> >index 89dea7284d5b..9f88fe822555 100644
> >--- a/drivers/net/ethernet/marvell/Makefile
> >+++ b/drivers/net/ethernet/marvell/Makefile
> >@@ -12,3 +12,4 @@ obj-$(CONFIG_PXA168_ETH) += pxa168_eth.o
> > obj-$(CONFIG_SKGE) += skge.o
> > obj-$(CONFIG_SKY2) += sky2.o
> > obj-y		+= octeontx2/
> >+obj-y		+= prestera/
> >diff --git a/drivers/net/ethernet/marvell/prestera/Kconfig b/drivers/net/ethernet/marvell/prestera/Kconfig
> >new file mode 100644
> >index 000000000000..0eddbc2e5901
> >--- /dev/null
> >+++ b/drivers/net/ethernet/marvell/prestera/Kconfig
> >@@ -0,0 +1,13 @@
> >+# SPDX-License-Identifier: GPL-2.0-only
> >+#
> >+# Marvell Prestera drivers configuration
> >+#
> >+
> >+config PRESTERA
> >+	tristate "Marvell Prestera Switch ASICs support"
> >+	depends on NET_SWITCHDEV && VLAN_8021Q
> >+	help
> >+	  This driver supports Marvell Prestera Switch ASICs family.
> >+
> >+	  To compile this driver as a module, choose M here: the
> >+	  module will be called prestera_sw.
> >diff --git a/drivers/net/ethernet/marvell/prestera/Makefile b/drivers/net/ethernet/marvell/prestera/Makefile
> >new file mode 100644
> >index 000000000000..2c35c498339e
> >--- /dev/null
> >+++ b/drivers/net/ethernet/marvell/prestera/Makefile
> >@@ -0,0 +1,4 @@
> >+# SPDX-License-Identifier: GPL-2.0
> >+obj-$(CONFIG_PRESTERA)	+= prestera_sw.o
> >+prestera_sw-objs	:= prestera.o prestera_hw.o prestera_dsa.o \
> 
> Everything else is "prestera". Let the module name be "prestera" too.
> Don't forget to rename this in Kconfig as well.
> 
> 
> >+			   prestera_rxtx.o
> >diff --git a/drivers/net/ethernet/marvell/prestera/prestera.c b/drivers/net/ethernet/marvell/prestera/prestera.c
> >new file mode 100644
> >index 000000000000..e2cccd9db742
> >--- /dev/null
> >+++ b/drivers/net/ethernet/marvell/prestera/prestera.c
> >@@ -0,0 +1,530 @@
> >+// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> >+/* Copyright (c) 2019-2020 Marvell International Ltd. All rights reserved */
> >+
> >+#include <linux/kernel.h>
> >+#include <linux/module.h>
> >+#include <linux/list.h>
> >+#include <linux/netdevice.h>
> >+#include <linux/netdev_features.h>
> >+#include <linux/etherdevice.h>
> >+#include <linux/jiffies.h>
> >+
> >+#include "prestera.h"
> >+#include "prestera_hw.h"
> >+#include "prestera_rxtx.h"
> >+
> >+static char base_mac_addr[ETH_ALEN];
> >+static char *base_mac;
> >+
> >+#define PRESTERA_MTU_DEFAULT 1536
> >+
> >+#define PRESTERA_STATS_DELAY_MS	(msecs_to_jiffies(1000))
> 
> Drop the ()s
> 
> 
> >+
> >+static struct prestera_switch *registered_switch;
> 
> Please remove this global variable.
> 
> 
> >+static struct workqueue_struct *prestera_wq;
> >+
> >+struct prestera_port *prestera_port_find_by_hwid(u32 dev_id, u32 hw_id)
> >+{
> >+	struct prestera_port *port;
> >+
> >+	rcu_read_lock();
> >+
> >+	list_for_each_entry_rcu(port, &registered_switch->port_list, list) {
> >+		if (port->dev_id == dev_id && port->hw_id == hw_id) {
> >+			rcu_read_unlock();
> >+			return port;
> >+		}
> >+	}
> >+
> >+	rcu_read_unlock();
> >+
> >+	return NULL;
> >+}
> >+
> >+static struct prestera_port *prestera_find_port(struct prestera_switch *sw,
> >+						u32 port_id)
> >+{
> >+	struct prestera_port *port;
> >+
> >+	rcu_read_lock();
> >+
> >+	list_for_each_entry_rcu(port, &sw->port_list, list) {
> >+		if (port->id == port_id) {
> >+			rcu_read_unlock();
> 
> In cases like this is good to have one unlock in the function.
> 
> 
> >+			return port;
> >+		}
> >+	}
> >+
> >+	rcu_read_unlock();
> >+
> >+	return NULL;
> >+}
> >+
> >+static int prestera_port_state_set(struct net_device *dev, bool is_up)
> >+{
> >+	struct prestera_port *port = netdev_priv(dev);
> >+	int err;
> >+
> >+	if (!is_up)
> >+		netif_stop_queue(dev);
> >+
> >+	err = prestera_hw_port_state_set(port, is_up);
> >+
> >+	if (is_up && !err)
> >+		netif_start_queue(dev);
> >+
> >+	return err;
> >+}
> >+
> >+static int prestera_port_get_port_parent_id(struct net_device *dev,
> >+					    struct netdev_phys_item_id *ppid)
> >+{
> >+	const struct prestera_port *port = netdev_priv(dev);
> >+
> >+	ppid->id_len = sizeof(port->sw->id);
> >+
> >+	memcpy(&ppid->id, &port->sw->id, ppid->id_len);
> >+	return 0;
> >+}
> >+
> >+static int prestera_port_get_phys_port_name(struct net_device *dev,
> >+					    char *buf, size_t len)
> >+{
> 
> Hmm, in my previous patch version review I wrote:
> "Don't implement this please. Just implement basic devlink and devlink
>  port support, devlink is going to take care of the netdevice names."
> 
> Why did you chose to ignore my comment? :(
Sorry, I really still keep it in mind) I was mostly focused on things
originated from the original patch and was a bit stressed with some
re-implementations and global renamings. Of course I will add support
for the devlink interface, currently I am not familiar with this
interface so I need some investigation.
> 
> 
> >+	const struct prestera_port *port = netdev_priv(dev);
> >+
> >+	snprintf(buf, len, "%u", port->fp_id);
> >+	return 0;
> >+}
> >+
> >+static int prestera_port_open(struct net_device *dev)
> >+{
> >+	return prestera_port_state_set(dev, true);
> >+}
> >+
> >+static int prestera_port_close(struct net_device *dev)
> >+{
> >+	return prestera_port_state_set(dev, false);
> >+}
> >+
> >+static netdev_tx_t prestera_port_xmit(struct sk_buff *skb,
> >+				      struct net_device *dev)
> >+{
> >+	return prestera_rxtx_xmit(netdev_priv(dev), skb);
> >+}
> >+
> >+static int prestera_is_valid_mac_addr(struct prestera_port *port, u8 *addr)
> >+{
> >+	if (!is_valid_ether_addr(addr))
> >+		return -EADDRNOTAVAIL;
> >+
> >+	if (memcmp(port->sw->base_mac, addr, ETH_ALEN - 1))
> >+		return -EINVAL;
> >+
> >+	return 0;
> >+}
> >+
> >+static int prestera_port_set_mac_address(struct net_device *dev, void *p)
> >+{
> >+	struct prestera_port *port = netdev_priv(dev);
> >+	struct sockaddr *addr = p;
> >+	int err;
> >+
> >+	err = prestera_is_valid_mac_addr(port, addr->sa_data);
> >+	if (err)
> >+		return err;
> >+
> >+	err = prestera_hw_port_mac_set(port, addr->sa_data);
> >+	if (err)
> >+		return err;
> >+
> >+	memcpy(dev->dev_addr, addr->sa_data, dev->addr_len);
> >+	return 0;
> >+}
> >+
> >+static int prestera_port_change_mtu(struct net_device *dev, int mtu)
> >+{
> >+	struct prestera_port *port = netdev_priv(dev);
> >+	int err;
> >+
> >+	err = prestera_hw_port_mtu_set(port, mtu);
> >+	if (err)
> >+		return err;
> >+
> >+	dev->mtu = mtu;
> >+	return 0;
> >+}
> >+
> >+static void prestera_port_get_stats64(struct net_device *dev,
> >+				      struct rtnl_link_stats64 *stats)
> >+{
> >+	struct prestera_port *port = netdev_priv(dev);
> >+	struct prestera_port_stats *port_stats = &port->cached_hw_stats.stats;
> >+
> >+	stats->rx_packets = port_stats->broadcast_frames_received +
> >+				port_stats->multicast_frames_received +
> >+				port_stats->unicast_frames_received;
> >+
> >+	stats->tx_packets = port_stats->broadcast_frames_sent +
> >+				port_stats->multicast_frames_sent +
> >+				port_stats->unicast_frames_sent;
> >+
> >+	stats->rx_bytes = port_stats->good_octets_received;
> >+
> >+	stats->tx_bytes = port_stats->good_octets_sent;
> >+
> >+	stats->rx_errors = port_stats->rx_error_frame_received;
> >+	stats->tx_errors = port_stats->mac_trans_error;
> >+
> >+	stats->rx_dropped = port_stats->buffer_overrun;
> >+	stats->tx_dropped = 0;
> >+
> >+	stats->multicast = port_stats->multicast_frames_received;
> >+	stats->collisions = port_stats->excessive_collision;
> >+
> >+	stats->rx_crc_errors = port_stats->bad_crc;
> >+}
> >+
> >+static void prestera_port_get_hw_stats(struct prestera_port *port)
> >+{
> >+	prestera_hw_port_stats_get(port, &port->cached_hw_stats.stats);
> >+}
> >+
> >+static void prestera_port_stats_update(struct work_struct *work)
> >+{
> >+	struct prestera_port *port =
> >+		container_of(work, struct prestera_port,
> >+			     cached_hw_stats.caching_dw.work);
> >+
> >+	prestera_port_get_hw_stats(port);
> >+
> >+	queue_delayed_work(prestera_wq, &port->cached_hw_stats.caching_dw,
> >+			   PRESTERA_STATS_DELAY_MS);
> >+}
> >+
> >+static const struct net_device_ops netdev_ops = {
> >+	.ndo_open = prestera_port_open,
> >+	.ndo_stop = prestera_port_close,
> >+	.ndo_start_xmit = prestera_port_xmit,
> >+	.ndo_change_mtu = prestera_port_change_mtu,
> >+	.ndo_get_stats64 = prestera_port_get_stats64,
> >+	.ndo_set_mac_address = prestera_port_set_mac_address,
> >+	.ndo_get_phys_port_name = prestera_port_get_phys_port_name,
> >+	.ndo_get_port_parent_id = prestera_port_get_port_parent_id
> >+};
> >+
> >+static int prestera_port_autoneg_set(struct prestera_port *port, bool enable,
> >+				     u64 link_modes, u8 fec)
> >+{
> >+	bool refresh = false;
> >+	int err = 0;
> >+
> >+	if (port->caps.type != PRESTERA_PORT_TYPE_TP)
> >+		return enable ? -EINVAL : 0;
> >+
> >+	if (port->adver_link_modes != link_modes || port->adver_fec != fec) {
> >+		port->adver_fec = fec ?: BIT(PRESTERA_PORT_FEC_OFF);
> >+		port->adver_link_modes = link_modes;
> >+		refresh = true;
> >+	}
> >+
> >+	if (port->autoneg == enable && !(port->autoneg && refresh))
> >+		return 0;
> >+
> >+	err = prestera_hw_port_autoneg_set(port, enable, port->adver_link_modes,
> >+					   port->adver_fec);
> >+	if (err)
> >+		return -EINVAL;
> >+
> >+	port->autoneg = enable;
> >+	return 0;
> >+}
> >+
> >+static int prestera_port_create(struct prestera_switch *sw, u32 id)
> >+{
> >+	struct prestera_port *port;
> >+	struct net_device *dev;
> >+	int err;
> >+
> >+	dev = alloc_etherdev(sizeof(*port));
> >+	if (!dev)
> >+		return -ENOMEM;
> >+
> >+	port = netdev_priv(dev);
> >+
> >+	port->dev = dev;
> >+	port->id = id;
> >+	port->sw = sw;
> >+
> >+	err = prestera_hw_port_info_get(port, &port->fp_id,
> >+					&port->hw_id, &port->dev_id);
> >+	if (err) {
> >+		dev_err(prestera_dev(sw), "Failed to get port(%u) info\n", id);
> >+		goto err_port_init;
> >+	}
> >+
> >+	dev->features |= NETIF_F_NETNS_LOCAL;
> >+	dev->netdev_ops = &netdev_ops;
> >+
> >+	netif_carrier_off(dev);
> >+
> >+	dev->mtu = min_t(unsigned int, sw->mtu_max, PRESTERA_MTU_DEFAULT);
> >+	dev->min_mtu = sw->mtu_min;
> >+	dev->max_mtu = sw->mtu_max;
> >+
> >+	err = prestera_hw_port_mtu_set(port, dev->mtu);
> >+	if (err) {
> >+		dev_err(prestera_dev(sw), "Failed to set port(%u) mtu(%d)\n",
> >+			id, dev->mtu);
> >+		goto err_port_init;
> >+	}
> >+
> >+	/* Only 0xFF mac addrs are supported */
> >+	if (port->fp_id >= 0xFF)
> >+		goto err_port_init;
> >+
> >+	memcpy(dev->dev_addr, sw->base_mac, dev->addr_len - 1);
> >+	dev->dev_addr[dev->addr_len - 1] = (char)port->fp_id;
> >+
> >+	err = prestera_hw_port_mac_set(port, dev->dev_addr);
> >+	if (err) {
> >+		dev_err(prestera_dev(sw), "Failed to set port(%u) mac addr\n", id);
> >+		goto err_port_init;
> >+	}
> >+
> >+	err = prestera_hw_port_cap_get(port, &port->caps);
> >+	if (err) {
> >+		dev_err(prestera_dev(sw), "Failed to get port(%u) caps\n", id);
> >+		goto err_port_init;
> >+	}
> >+
> >+	port->adver_fec = BIT(PRESTERA_PORT_FEC_OFF);
> >+	prestera_port_autoneg_set(port, true, port->caps.supp_link_modes,
> >+				  port->caps.supp_fec);
> >+
> >+	err = prestera_hw_port_state_set(port, false);
> >+	if (err) {
> >+		dev_err(prestera_dev(sw), "Failed to set port(%u) down\n", id);
> >+		goto err_port_init;
> >+	}
> >+
> >+	err = prestera_rxtx_port_init(port);
> >+	if (err)
> >+		goto err_port_init;
> >+
> >+	INIT_DELAYED_WORK(&port->cached_hw_stats.caching_dw,
> >+			  &prestera_port_stats_update);
> >+
> >+	spin_lock(&sw->ports_lock);
> >+	list_add(&port->list, &sw->port_list);
> 
> This is RCU list. Treat it accordingly.
> 
> 
> >+	spin_unlock(&sw->ports_lock);
> 
> I don't follow, why do you need to protect the list by spinlock here?
> More to that, why do you need the port_list reader-writer
> protected (by rcu)? Is is possible that you add/remove port in the same
> time packets are flying in?
> 
> If yes, you need to ensure the structs are in the memory (free_rcu,
> synchronize_rcu). But I believe that you should disable that from
> happening in HW.
Probably you are right, may be this is too much for the current
implementation)

> 
> 
> >+
> >+	err = register_netdev(dev);
> >+	if (err)
> >+		goto err_register_netdev;
> >+
> >+	return 0;
> >+
> >+err_register_netdev:
> >+	spin_lock(&sw->ports_lock);
> >+	list_del_rcu(&port->list);
> >+	spin_unlock(&sw->ports_lock);
> >+err_port_init:
> >+	free_netdev(dev);
> >+	return err;
> >+}
> >+
> >+static void prestera_port_destroy(struct prestera_port *port)
> >+{
> >+	struct net_device *dev = port->dev;
> >+
> >+	cancel_delayed_work_sync(&port->cached_hw_stats.caching_dw);
> >+	unregister_netdev(dev);
> >+
> >+	spin_lock(&port->sw->ports_lock);
> >+	list_del_rcu(&port->list);
> >+	spin_unlock(&port->sw->ports_lock);
> >+
> >+	free_netdev(dev);
> >+}
> >+
> >+static void prestera_destroy_ports(struct prestera_switch *sw)
> >+{
> >+	struct prestera_port *port, *tmp;
> >+	struct list_head remove_list;
> >+
> >+	INIT_LIST_HEAD(&remove_list);
> >+
> >+	spin_lock(&sw->ports_lock);
> >+	list_splice_init(&sw->port_list, &remove_list);
> >+	spin_unlock(&sw->ports_lock);
> >+
> >+	list_for_each_entry_safe(port, tmp, &remove_list, list)
> >+		prestera_port_destroy(port);
> >+}
> >+
> >+static int prestera_create_ports(struct prestera_switch *sw)
> >+{
> >+	u32 port;
> >+	int err;
> >+
> >+	for (port = 0; port < sw->port_count; port++) {
> >+		err = prestera_port_create(sw, port);
> >+		if (err)
> >+			goto err_ports_init;
> >+	}
> >+
> >+	return 0;
> >+
> >+err_ports_init:
> >+	prestera_destroy_ports(sw);
> >+	return err;
> >+}
> >+
> >+static void prestera_port_handle_event(struct prestera_switch *sw,
> >+				       struct prestera_event *evt, void *arg)
> >+{
> >+	struct delayed_work *caching_dw;
> >+	struct prestera_port *port;
> >+
> >+	port = prestera_find_port(sw, evt->port_evt.port_id);
> >+	if (!port)
> >+		return;
> >+
> >+	caching_dw = &port->cached_hw_stats.caching_dw;
> >+
> >+	if (evt->id == PRESTERA_PORT_EVENT_STATE_CHANGED) {
> >+		if (evt->port_evt.data.oper_state) {
> >+			netif_carrier_on(port->dev);
> >+			if (!delayed_work_pending(caching_dw))
> >+				queue_delayed_work(prestera_wq, caching_dw, 0);
> >+		} else {
> >+			netif_carrier_off(port->dev);
> >+			if (delayed_work_pending(caching_dw))
> >+				cancel_delayed_work(caching_dw);
> >+		}
> >+	}
> >+}
> >+
> >+static void prestera_event_handlers_unregister(struct prestera_switch *sw)
> >+{
> >+	prestera_hw_event_handler_unregister(sw, PRESTERA_EVENT_TYPE_PORT,
> >+					     prestera_port_handle_event);
> >+}
> >+
> >+static int prestera_event_handlers_register(struct prestera_switch *sw)
> >+{
> >+	return prestera_hw_event_handler_register(sw, PRESTERA_EVENT_TYPE_PORT,
> >+						  prestera_port_handle_event,
> >+						  NULL);
> >+}
> >+
> >+static int prestera_switch_init(struct prestera_switch *sw)
> >+{
> >+	int err;
> >+
> >+	err = prestera_hw_switch_init(sw);
> >+	if (err) {
> >+		dev_err(prestera_dev(sw), "Failed to init Switch device\n");
> >+		return err;
> >+	}
> >+
> >+	memcpy(sw->base_mac, base_mac_addr, sizeof(sw->base_mac));
> >+	spin_lock_init(&sw->ports_lock);
> >+	INIT_LIST_HEAD(&sw->port_list);
> >+
> >+	err = prestera_hw_switch_mac_set(sw, sw->base_mac);
> >+	if (err)
> >+		return err;
> >+
> >+	err = prestera_rxtx_switch_init(sw);
> >+	if (err)
> >+		return err;
> >+
> >+	err = prestera_event_handlers_register(sw);
> >+	if (err)
> >+		goto err_evt_handlers;
> >+
> >+	err = prestera_create_ports(sw);
> >+	if (err)
> >+		goto err_ports_create;
> >+
> >+	return 0;
> >+
> >+err_ports_create:
> 
> You are missing prestera_event_handlers_unregister(sw); call here.
> 
> 
> >+err_evt_handlers:
> >+	prestera_rxtx_switch_fini(sw);
> >+
> >+	return err;
> >+}
> >+
> >+static void prestera_switch_fini(struct prestera_switch *sw)
> >+{
> >+	prestera_destroy_ports(sw);
> >+	prestera_event_handlers_unregister(sw);
> >+	prestera_rxtx_switch_fini(sw);
> >+}
> >+
> >+int prestera_device_register(struct prestera_device *dev)
> >+{
> >+	struct prestera_switch *sw;
> >+	int err;
> >+
> >+	sw = kzalloc(sizeof(*sw), GFP_KERNEL);
> >+	if (!sw)
> >+		return -ENOMEM;
> >+
> >+	dev->priv = sw;
> >+	sw->dev = dev;
> >+
> >+	err = prestera_switch_init(sw);
> >+	if (err) {
> >+		kfree(sw);
> >+		return err;
> >+	}
> >+
> >+	registered_switch = sw;
> >+	return 0;
> >+}
> >+EXPORT_SYMBOL(prestera_device_register);
> >+
> >+void prestera_device_unregister(struct prestera_device *dev)
> >+{
> >+	struct prestera_switch *sw = dev->priv;
> >+
> >+	registered_switch = NULL;
> >+	prestera_switch_fini(sw);
> >+	kfree(sw);
> >+}
> >+EXPORT_SYMBOL(prestera_device_unregister);
> >+
> >+static int __init prestera_module_init(void)
> >+{
> >+	if (!base_mac) {
> >+		pr_err("[base_mac] parameter must be specified\n");
> >+		return -EINVAL;
> >+	}
> >+	if (!mac_pton(base_mac, base_mac_addr)) {
> >+		pr_err("[base_mac] parameter has invalid format\n");
> >+		return -EINVAL;
> >+	}
> >+
> >+	prestera_wq = alloc_workqueue("prestera", 0, 0);
> >+	if (!prestera_wq)
> >+		return -ENOMEM;
> >+
> >+	return 0;
> >+}
> >+
> >+static void __exit prestera_module_exit(void)
> >+{
> >+	destroy_workqueue(prestera_wq);
> >+}
> >+
> >+module_init(prestera_module_init);
> >+module_exit(prestera_module_exit);
> >+
> >+MODULE_AUTHOR("Marvell Semi.");
> >+MODULE_LICENSE("Dual BSD/GPL");
> >+MODULE_DESCRIPTION("Marvell Prestera switch driver");
> >+
> >+module_param(base_mac, charp, 0);
> 
> No please.
> 
> 
> [..]
> 

Thanks for review!

I still keep in todo list some things from Ido (regarding default pvid)
and Jakub regarding module author and of course the devlink, I will put
them on next version, I assume it can be as PATCH version ?

Regards,
Vadym Kochan

Powered by blists - more mailing lists