Message-ID: <20200511112905.GH2245@nanopsycho>
Date:   Mon, 11 May 2020 13:29:05 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Vadym Kochan <vadym.kochan@...ision.eu>
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

Mon, May 11, 2020 at 01:11:34PM CEST, vadym.kochan@...ision.eu wrote:
>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 ?

If the HW is not capable holding the mac, I think that you can have it
in platform data. The usual way for such HW is to generate random mac.
module parameter is definitelly a no-go.


>
>> 
>> >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.

It is simple. Please make sure you have it included in the next version.


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

Great. Thanks! Just make sure you don't forget anything. It is
frustrating to see the same thing during review you commented the last
time ignored :/


>
>Regards,
>Vadym Kochan

Powered by blists - more mailing lists