Date:   Wed, 10 Jun 2020 16:41:32 +0300
From:   Vadym Kochan <vadym.kochan@...ision.eu>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jiri Pirko <jiri@...lanox.com>,
        Ido Schimmel <idosch@...lanox.com>,
        Andrew Lunn <andrew@...n.ch>,
        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>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Mickey Rachamim <mickeyr@...vell.com>
Subject: Re: [net-next 1/6] net: marvell: prestera: Add driver for Prestera
 family ASIC devices

On Wed, Jun 03, 2020 at 04:16:32PM +0200, Jiri Pirko wrote:
> Thu, May 28, 2020 at 05:12:40PM CEST, vadym.kochan@...ision.eu wrote:
> 
> [...]
> 
> >+}
> >+
> >+int prestera_hw_port_info_get(const struct prestera_port *port,
> >+			      u16 *fp_id, u32 *hw_id, u32 *dev_id)
> 
> Please unify the ordering of "hw_id" and "dev_id" with the rest of the
> functions having the same args.
> 
OK, will do.

> 
> 
> >+{
> >+	struct prestera_msg_port_info_resp resp;
> >+	struct prestera_msg_port_info_req req = {
> >+		.port = port->id
> >+	};
> >+	int err;
> >+
> >+	err = prestera_cmd_ret(port->sw, PRESTERA_CMD_TYPE_PORT_INFO_GET,
> >+			       &req.cmd, sizeof(req), &resp.ret, sizeof(resp));
> >+	if (err)
> >+		return err;
> >+
> >+	*hw_id = resp.hw_id;
> >+	*dev_id = resp.dev_id;
> >+	*fp_id = resp.fp_id;
> >+
> >+	return 0;
> >+}
> >+
> >+int prestera_hw_switch_mac_set(struct prestera_switch *sw, char *mac)
> >+{
> >+	struct prestera_msg_switch_attr_req req = {
> >+		.attr = PRESTERA_CMD_SWITCH_ATTR_MAC,
> >+	};
> >+
> >+	memcpy(req.param.mac, mac, sizeof(req.param.mac));
> >+
> >+	return prestera_cmd(sw, PRESTERA_CMD_TYPE_SWITCH_ATTR_SET,
> >+			    &req.cmd, sizeof(req));
> >+}
> >+
> >+int prestera_hw_switch_init(struct prestera_switch *sw)
> >+{
> >+	struct prestera_msg_switch_init_resp resp;
> >+	struct prestera_msg_common_req req;
> >+	int err;
> >+
> >+	INIT_LIST_HEAD(&sw->event_handlers);
> >+
> >+	err = prestera_cmd_ret_wait(sw, PRESTERA_CMD_TYPE_SWITCH_INIT,
> >+				    &req.cmd, sizeof(req),
> >+				    &resp.ret, sizeof(resp),
> >+				    PRESTERA_SWITCH_INIT_TIMEOUT);
> >+	if (err)
> >+		return err;
> >+
> >+	sw->id = resp.switch_id;
> >+	sw->port_count = resp.port_count;
> >+	sw->mtu_min = PRESTERA_MIN_MTU;
> >+	sw->mtu_max = resp.mtu_max;
> >+	sw->dev->recv_msg = prestera_evt_recv;
> >+	sw->dev->recv_pkt = prestera_pkt_recv;
> >+
> >+	return 0;
> >+}
> >+
> >+int prestera_hw_port_state_set(const struct prestera_port *port,
> >+			       bool admin_state)
> >+{
> >+	struct prestera_msg_port_attr_req req = {
> >+		.attr = PRESTERA_CMD_PORT_ATTR_ADMIN_STATE,
> >+		.port = port->hw_id,
> >+		.dev = port->dev_id,
> >+		.param = {.admin_state = admin_state}
> >+	};
> >+
> >+	return prestera_cmd(port->sw, PRESTERA_CMD_TYPE_PORT_ATTR_SET,
> >+			    &req.cmd, sizeof(req));
> >+}
> >+
> >+int prestera_hw_port_mtu_set(const struct prestera_port *port, u32 mtu)
> >+{
> >+	struct prestera_msg_port_attr_req req = {
> >+		.attr = PRESTERA_CMD_PORT_ATTR_MTU,
> >+		.port = port->hw_id,
> >+		.dev = port->dev_id,
> >+		.param = {.mtu = mtu}
> >+	};
> >+
> >+	return prestera_cmd(port->sw, PRESTERA_CMD_TYPE_PORT_ATTR_SET,
> >+			    &req.cmd, sizeof(req));
> >+}
> >+
> >+int prestera_hw_port_mac_set(const struct prestera_port *port, char *mac)
> >+{
> >+	struct prestera_msg_port_attr_req req = {
> >+		.attr = PRESTERA_CMD_PORT_ATTR_MAC,
> >+		.port = port->hw_id,
> >+		.dev = port->dev_id
> >+	};
> >+	memcpy(&req.param.mac, mac, sizeof(req.param.mac));
> >+
> >+	return prestera_cmd(port->sw, PRESTERA_CMD_TYPE_PORT_ATTR_SET,
> >+			    &req.cmd, sizeof(req));
> >+}
> >+
> >+int prestera_hw_port_cap_get(const struct prestera_port *port,
> >+			     struct prestera_port_caps *caps)
> >+{
> >+	struct prestera_msg_port_attr_resp resp;
> >+	struct prestera_msg_port_attr_req req = {
> >+		.attr = PRESTERA_CMD_PORT_ATTR_CAPABILITY,
> >+		.port = port->hw_id,
> >+		.dev = port->dev_id
> >+	};
> >+	int err;
> >+
> >+	err = prestera_cmd_ret(port->sw, PRESTERA_CMD_TYPE_PORT_ATTR_GET,
> >+			       &req.cmd, sizeof(req), &resp.ret, sizeof(resp));
> >+	if (err)
> >+		return err;
> >+
> >+	caps->supp_link_modes = resp.param.cap.link_mode;
> >+	caps->supp_fec = resp.param.cap.fec;
> >+	caps->type = resp.param.cap.type;
> >+	caps->transceiver = resp.param.cap.transceiver;
> >+
> >+	return err;
> >+}
> >+
> >+int prestera_hw_port_autoneg_set(const struct prestera_port *port,
> >+				 bool autoneg, u64 link_modes, u8 fec)
> >+{
> >+	struct prestera_msg_port_attr_req req = {
> >+		.attr = PRESTERA_CMD_PORT_ATTR_AUTONEG,
> >+		.port = port->hw_id,
> >+		.dev = port->dev_id,
> >+		.param = {.autoneg = {.link_mode = link_modes,
> >+				      .enable = autoneg,
> >+				      .fec = fec}
> >+		}
> >+	};
> >+
> >+	return prestera_cmd(port->sw, PRESTERA_CMD_TYPE_PORT_ATTR_SET,
> >+			    &req.cmd, sizeof(req));
> >+}
> >+
> >+int prestera_hw_port_stats_get(const struct prestera_port *port,
> >+			       struct prestera_port_stats *st)
> >+{
> >+	struct prestera_msg_port_stats_resp resp;
> >+	struct prestera_msg_port_attr_req req = {
> >+		.attr = PRESTERA_CMD_PORT_ATTR_STATS,
> >+		.port = port->hw_id,
> >+		.dev = port->dev_id
> >+	};
> >+	u64 *hw = resp.stats;
> >+	int err;
> >+
> >+	err = prestera_cmd_ret(port->sw, PRESTERA_CMD_TYPE_PORT_ATTR_GET,
> >+			       &req.cmd, sizeof(req), &resp.ret, sizeof(resp));
> >+	if (err)
> >+		return err;
> >+
> >+	st->good_octets_received = hw[PRESTERA_PORT_GOOD_OCTETS_RCV_CNT];
> >+	st->bad_octets_received = hw[PRESTERA_PORT_BAD_OCTETS_RCV_CNT];
> >+	st->mac_trans_error = hw[PRESTERA_PORT_MAC_TRANSMIT_ERR_CNT];
> >+	st->broadcast_frames_received = hw[PRESTERA_PORT_BRDC_PKTS_RCV_CNT];
> >+	st->multicast_frames_received = hw[PRESTERA_PORT_MC_PKTS_RCV_CNT];
> >+	st->frames_64_octets = hw[PRESTERA_PORT_PKTS_64L_CNT];
> >+	st->frames_65_to_127_octets = hw[PRESTERA_PORT_PKTS_65TO127L_CNT];
> >+	st->frames_128_to_255_octets = hw[PRESTERA_PORT_PKTS_128TO255L_CNT];
> >+	st->frames_256_to_511_octets = hw[PRESTERA_PORT_PKTS_256TO511L_CNT];
> >+	st->frames_512_to_1023_octets = hw[PRESTERA_PORT_PKTS_512TO1023L_CNT];
> >+	st->frames_1024_to_max_octets = hw[PRESTERA_PORT_PKTS_1024TOMAXL_CNT];
> >+	st->excessive_collision = hw[PRESTERA_PORT_EXCESSIVE_COLLISIONS_CNT];
> >+	st->multicast_frames_sent = hw[PRESTERA_PORT_MC_PKTS_SENT_CNT];
> >+	st->broadcast_frames_sent = hw[PRESTERA_PORT_BRDC_PKTS_SENT_CNT];
> >+	st->fc_sent = hw[PRESTERA_PORT_FC_SENT_CNT];
> >+	st->fc_received = hw[PRESTERA_PORT_GOOD_FC_RCV_CNT];
> >+	st->buffer_overrun = hw[PRESTERA_PORT_DROP_EVENTS_CNT];
> >+	st->undersize = hw[PRESTERA_PORT_UNDERSIZE_PKTS_CNT];
> >+	st->fragments = hw[PRESTERA_PORT_FRAGMENTS_PKTS_CNT];
> >+	st->oversize = hw[PRESTERA_PORT_OVERSIZE_PKTS_CNT];
> >+	st->jabber = hw[PRESTERA_PORT_JABBER_PKTS_CNT];
> >+	st->rx_error_frame_received = hw[PRESTERA_PORT_MAC_RCV_ERROR_CNT];
> >+	st->bad_crc = hw[PRESTERA_PORT_BAD_CRC_CNT];
> >+	st->collisions = hw[PRESTERA_PORT_COLLISIONS_CNT];
> >+	st->late_collision = hw[PRESTERA_PORT_LATE_COLLISIONS_CNT];
> >+	st->unicast_frames_received = hw[PRESTERA_PORT_GOOD_UC_PKTS_RCV_CNT];
> >+	st->unicast_frames_sent = hw[PRESTERA_PORT_GOOD_UC_PKTS_SENT_CNT];
> >+	st->sent_multiple = hw[PRESTERA_PORT_MULTIPLE_PKTS_SENT_CNT];
> >+	st->sent_deferred = hw[PRESTERA_PORT_DEFERRED_PKTS_SENT_CNT];
> >+	st->good_octets_sent = hw[PRESTERA_PORT_GOOD_OCTETS_SENT_CNT];
> >+
> >+	return 0;
> >+}
> >+
> >+int prestera_hw_rxtx_init(struct prestera_switch *sw,
> >+			  struct prestera_rxtx_params *params)
> >+{
> >+	struct prestera_msg_rxtx_resp resp;
> >+	struct prestera_msg_rxtx_req req;
> >+	int err;
> >+
> >+	req.use_sdma = params->use_sdma;
> >+
> >+	err = prestera_cmd_ret(sw, PRESTERA_CMD_TYPE_RXTX_INIT,
> >+			       &req.cmd, sizeof(req), &resp.ret, sizeof(resp));
> >+	if (err)
> >+		return err;
> >+
> >+	params->map_addr = resp.map_addr;
> >+	return 0;
> >+}
> >+
> >+int prestera_hw_rxtx_port_init(struct prestera_port *port)
> >+{
> >+	struct prestera_msg_rxtx_port_req req = {
> >+		.port = port->hw_id,
> >+		.dev = port->dev_id,
> >+	};
> >+
> >+	return prestera_cmd(port->sw, PRESTERA_CMD_TYPE_RXTX_PORT_INIT,
> >+			    &req.cmd, sizeof(req));
> >+}
> >+
> >+int prestera_hw_event_handler_register(struct prestera_switch *sw,
> >+				       enum prestera_event_type type,
> >+				       prestera_event_cb_t fn,
> >+				       void *arg)
> >+{
> >+	struct prestera_fw_event_handler *eh;
> >+
> >+	eh = __find_event_handler(sw, type);
> >+	if (eh)
> >+		return -EEXIST;
> >+	eh = kmalloc(sizeof(*eh), GFP_KERNEL);
> >+	if (!eh)
> >+		return -ENOMEM;
> >+
> >+	eh->type = type;
> >+	eh->func = fn;
> >+	eh->arg = arg;
> >+
> >+	INIT_LIST_HEAD(&eh->list);
> >+
> >+	list_add_rcu(&eh->list, &sw->event_handlers);
> >+
> >+	return 0;
> >+}
> >+
> >+void prestera_hw_event_handler_unregister(struct prestera_switch *sw,
> >+					  enum prestera_event_type type,
> >+					  prestera_event_cb_t fn)
> >+{
> >+	struct prestera_fw_event_handler *eh;
> >+
> >+	eh = __find_event_handler(sw, type);
> >+	if (!eh)
> >+		return;
> >+
> >+	list_del_rcu(&eh->list);
> >+	synchronize_rcu();
> >+	kfree(eh);
> 
> Try to avoid use of synchronice rcu. You can rather do:
> kfree_rcu(eh, rcu);
Thanks for this.

> 
> 
> >+}
> >diff --git a/drivers/net/ethernet/marvell/prestera/prestera_hw.h b/drivers/net/ethernet/marvell/prestera/prestera_hw.h
> >new file mode 100644
> >index 000000000000..acb0e31d6684
> >--- /dev/null
> >+++ b/drivers/net/ethernet/marvell/prestera/prestera_hw.h
> >@@ -0,0 +1,71 @@
> >+/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> >+ *
> >+ * Copyright (c) 2019-2020 Marvell International Ltd. All rights reserved.
> >+ *
> >+ */
> >+
> >+#ifndef _PRESTERA_HW_H_
> >+#define _PRESTERA_HW_H_
> >+
> >+#include <linux/types.h>
> >+
> >+enum {
> >+	PRESTERA_PORT_TYPE_NONE,
> >+	PRESTERA_PORT_TYPE_TP,
> >+
> >+	PRESTERA_PORT_TYPE_MAX,
> >+};
> >+
> >+enum {
> >+	PRESTERA_PORT_FEC_OFF,
> >+
> >+	PRESTERA_PORT_FEC_MAX,
> >+};
> >+
> >+struct prestera_switch;
> >+struct prestera_port;
> >+struct prestera_port_stats;
> >+struct prestera_port_caps;
> >+enum prestera_event_type;
> >+struct prestera_event;
> >+
> >+typedef void (*prestera_event_cb_t)
> >+	(struct prestera_switch *sw, struct prestera_event *evt, void *arg);
> >+
> >+struct prestera_rxtx_params;
> >+
> >+/* Switch API */
> >+int prestera_hw_switch_init(struct prestera_switch *sw);
> >+int prestera_hw_switch_mac_set(struct prestera_switch *sw, char *mac);
> >+
> >+/* Port API */
> >+int prestera_hw_port_info_get(const struct prestera_port *port,
> >+			      u16 *fp_id, u32 *hw_id, u32 *dev_id);
> >+int prestera_hw_port_state_set(const struct prestera_port *port,
> >+			       bool admin_state);
> >+int prestera_hw_port_mtu_set(const struct prestera_port *port, u32 mtu);
> >+int prestera_hw_port_mtu_get(const struct prestera_port *port, u32 *mtu);
> >+int prestera_hw_port_mac_set(const struct prestera_port *port, char *mac);
> >+int prestera_hw_port_mac_get(const struct prestera_port *port, char *mac);
> >+int prestera_hw_port_cap_get(const struct prestera_port *port,
> >+			     struct prestera_port_caps *caps);
> >+int prestera_hw_port_autoneg_set(const struct prestera_port *port,
> >+				 bool autoneg, u64 link_modes, u8 fec);
> >+int prestera_hw_port_stats_get(const struct prestera_port *port,
> >+			       struct prestera_port_stats *stats);
> >+
> >+/* Event handlers */
> >+int prestera_hw_event_handler_register(struct prestera_switch *sw,
> >+				       enum prestera_event_type type,
> >+				       prestera_event_cb_t fn,
> >+				       void *arg);
> >+void prestera_hw_event_handler_unregister(struct prestera_switch *sw,
> >+					  enum prestera_event_type type,
> >+					  prestera_event_cb_t fn);
> >+
> >+/* RX/TX */
> >+int prestera_hw_rxtx_init(struct prestera_switch *sw,
> >+			  struct prestera_rxtx_params *params);
> >+int prestera_hw_rxtx_port_init(struct prestera_port *port);
> >+
> >+#endif /* _PRESTERA_HW_H_ */
> >diff --git a/drivers/net/ethernet/marvell/prestera/prestera_main.c b/drivers/net/ethernet/marvell/prestera/prestera_main.c
> >new file mode 100644
> >index 000000000000..b5241e9b784a
> >--- /dev/null
> >+++ b/drivers/net/ethernet/marvell/prestera/prestera_main.c
> >@@ -0,0 +1,506 @@
> >+// 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 <linux/of.h>
> >+#include <linux/of_net.h>
> >+
> >+#include "prestera.h"
> >+#include "prestera_hw.h"
> >+#include "prestera_rxtx.h"
> >+
> >+#define PRESTERA_MTU_DEFAULT 1536
> >+
> >+#define PRESTERA_STATS_DELAY_MS	msecs_to_jiffies(1000)
> >+
> >+static struct workqueue_struct *prestera_wq;
> >+
> >+struct prestera_port *prestera_port_find_by_hwid(struct prestera_switch *sw,
> >+						 u32 dev_id, u32 hw_id)
> 
> This is confusing. The called is calling this like:
> 	port = prestera_port_find_by_hwid(sdma->sw, hw_id, hw_port);
> 
> You are mixing hw_id and dev_id.
Yes, this is confusing, will fix it.

> 
> 
> 
> >+{
> >+	struct prestera_port *port;
> >+
> >+	rcu_read_lock();
> >+
> >+	list_for_each_entry_rcu(port, &sw->port_list, list) {
> >+		if (port->dev_id == dev_id && port->hw_id == hw_id) {
> 
> Note this is the fast path. I'm not sure what the values of dev_id or
> hw_id are, but didn't you consider having the port pointers in 2 dim
> array? Or, if the values are totally arbitrary, at least a hash table
> would be nice here.
The hash table may help.

> 
> 
> >+			rcu_read_unlock();
> >+			return port;
> 
> As Ido already pointed out, this is invalid use of rcu read.
> If you really need to do rcu read lock, the caller should hold it while
> calling this function and until it finisher work with port struct.
> 
> 
> >+		}
> >+	}
> >+
> >+	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)
> >+			break;
> >+	}
> >+
> >+	rcu_read_unlock();
> >+
> >+	return port;
> >+}
> >+
> >+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_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,
> >+};
> >+
> >+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);
> >+
> >+	list_add_rcu(&port->list, &sw->port_list);
> 
> I still am not sure I fully follow. We discussed this before. Can one
> of the following cases happen?
> 
> 1) a packet is RXed while adding ports
> 2) a packet is RXed while removing ports
> 
> If yes, the rcu makes sense here. If no, you are okay with a simple
> list.
Well, I was afraid about the _fini part when ports are destroying and
the packets are flying at the same time. And referring to the previous
comment regarding rcu_read_lock()/..._unlock() - it needs only to
protect the ports list (while destroying/adding the ports) and using
just "rcu add/del list" looks enough because there is only 1 writer (or
use simply spinlock).

> 
> 
> >+
> >+	err = register_netdev(dev);
> >+	if (err)
> >+		goto err_register_netdev;
> >+
> >+	return 0;
> >+
> >+err_register_netdev:
> >+	list_del_rcu(&port->list);
> >+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);
> >+
> >+	list_del_rcu(&port->list);
> >+
> >+	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);
> >+
> >+	list_splice_init(&sw->port_list, &remove_list);
> 
> Why do you need a separate remove list? Why don't you iterate sw->port_list
> directly?
> 
> 
> >+
> >+	list_for_each_entry_safe(port, tmp, &remove_list, list)
> >+		prestera_port_destroy(port);
> >+}
> >+
> 
> [...]
> 
> 
> >+static int prestera_rxtx_process_skb(struct prestera_sdma *sdma,
> >+				     struct sk_buff *skb)
> >+{
> >+	const struct prestera_port *port;
> >+	struct prestera_dsa dsa;
> 
> What "DSA" stands for? Anything to do with net/dsa/ ?
Well, it uses special Marvell DSA tag which is used to extract
additional meta information like port number, etc (it differs from the
one which is supported in net/dsa, bacuse the latter are part of the DSA
infra).

> 
> 
> >+	u32 hw_port, hw_id;
> >+	int err;
> >+
> >+	skb_pull(skb, ETH_HLEN);
> >+
> >+	/* ethertype field is part of the dsa header */
> >+	err = prestera_dsa_parse(&dsa, skb->data - ETH_TLEN);
> >+	if (err)
> >+		return err;
> >+
> >+	hw_port = dsa.port_num;
> >+	hw_id = dsa.hw_dev_num;
> >+
> >+	port = prestera_port_find_by_hwid(sdma->sw, hw_id, hw_port);
> >+	if (unlikely(!port)) {
> >+		pr_warn_ratelimited("prestera: received pkt for non-existent port(%u, %u)\n",
> 
> Drop the "prestera: " prefix.
Okay.

> 
> 
> >+				    hw_id, hw_port);
> >+		return -EEXIST;
> >+	}
> >+
> 
> [...]

Powered by blists - more mailing lists