Message-ID: <20200620125639.GA6911@plvision.eu>
Date:   Sat, 20 Jun 2020 15:56:39 +0300
From:   Vadym Kochan <vadym.kochan@...ision.eu>
To:     Ido Schimmel <idosch@...sch.org>
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

Hi Ido,

On Wed, Jun 03, 2020 at 12:23:58PM +0300, Ido Schimmel wrote:
> On Mon, Jun 01, 2020 at 01:50:13PM +0300, Vadym Kochan wrote:
> > Hi Ido,
> > 
> > On Sat, May 30, 2020 at 06:48:01PM +0300, Ido Schimmel wrote:
> > > On Thu, May 28, 2020 at 06:12:40PM +0300, Vadym Kochan wrote:
> > > 
> > 
> > [...]
> > 
> > > Nit: "From" ?
> > > 
> > > > +	PRESTERA_DSA_CMD_FROM_CPU,
> > > > +};
> > > > +
> > > > +struct prestera_dsa_vlan {
> > > > +	u16 vid;
> > > > +	u8 vpt;
> > > > +	u8 cfi_bit;
> > > > +	bool is_tagged;
> > > > +};
> > > > +
> > > > +struct prestera_dsa {
> > > > +	struct prestera_dsa_vlan vlan;
> > > > +	u32 hw_dev_num;
> > > > +	u32 port_num;
> > > > +};
> > > > +
> > > > +int prestera_dsa_parse(struct prestera_dsa *dsa, const u8 *dsa_buf);
> > > > +int prestera_dsa_build(const struct prestera_dsa *dsa, u8 *dsa_buf);
> > > > +
> > > > +#endif /* _PRESTERA_DSA_H_ */
> > > > diff --git a/drivers/net/ethernet/marvell/prestera/prestera_hw.c b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> > > > new file mode 100644
> > > > index 000000000000..3aa3974f957a
> > > > --- /dev/null
> > > > +++ b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> > > > @@ -0,0 +1,610 @@
> > > > +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> > > > +/* Copyright (c) 2019-2020 Marvell International Ltd. All rights reserved */
> > > > +
> > > > +#include <linux/etherdevice.h>
> > > > +#include <linux/ethtool.h>
> > > > +#include <linux/netdevice.h>
> > > > +#include <linux/list.h>
> > > > +
> > > > +#include "prestera.h"
> > > > +#include "prestera_hw.h"
> > > > +
> > > > +#define PRESTERA_SWITCH_INIT_TIMEOUT 30000000	/* 30sec */
> > > 
> > > Out of curiosity, how long does it actually take you to initialize the
> > > hardware?
It might be minimum 10-15 secs.

> > > 
> > > Also, I find it useful to note the units in the name, so:
> > > 
> > > #define PRESTERA_SWITCH_INIT_TIMEOUT_US (30 * 1000 * 1000)
> > > 
> > > BTW, it says 30 seconds in comment, but the call chain where it is used
> > > is:
> > > 
> > > prestera_cmd_ret_wait(, PRESTERA_SWITCH_INIT_TIMEOUT)
> > >   __prestera_cmd_ret(..., wait)
> > >     prestera_fw_send_req(..., waitms)
> > >       prestera_fw_cmd_send(..., waitms)
> > >         prestera_fw_wait_reg32(..., waitms)
> > > 	  readl_poll_timeout(..., waitms * 1000)
> > > 
> > > So I think you should actually define it as:
> > > 
> > > #define PRESTERA_SWITCH_INIT_TIMEOUT_MS (30 * 1000)
> > > 
> > > And rename all these 'wait' arguments to 'waitms' so it's clearer which
> > > unit they expect.
> > > 
> > [...]
> > > > +static int __prestera_cmd_ret(struct prestera_switch *sw,
> > > > +			      enum prestera_cmd_type_t type,
> > > > +			      struct prestera_msg_cmd *cmd, size_t clen,
> > > > +			      struct prestera_msg_ret *ret, size_t rlen,
> > > > +			      int wait)
> > > > +{
> > > > +	struct prestera_device *dev = sw->dev;
> > > > +	int err;
> > > > +
> > > > +	cmd->type = type;
> > > > +
> > > > +	err = dev->send_req(dev, (u8 *)cmd, clen, (u8 *)ret, rlen, wait);
> > > > +	if (err)
> > > > +		return err;
> > > > +
> > > > +	if (ret->cmd.type != PRESTERA_CMD_TYPE_ACK)
> > > > +		return -EBADE;
> > > > +	if (ret->status != PRESTERA_CMD_ACK_OK)
> > > 
> > > You don't have more states here other than OK / FAIL ? It might help you
> > > in debugging if you include them. You might find trace_devlink_hwerr()
> > > useful.
> > Thanks, I will consider this.
> > 
> > > 
> > > > +		return -EINVAL;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int prestera_cmd_ret(struct prestera_switch *sw,
> > > > +			    enum prestera_cmd_type_t type,
> > > > +			    struct prestera_msg_cmd *cmd, size_t clen,
> > > > +			    struct prestera_msg_ret *ret, size_t rlen)
> > > > +{
> > > > +	return __prestera_cmd_ret(sw, type, cmd, clen, ret, rlen, 0);
> > > > +}
> > > > +
> > > > +static int prestera_cmd_ret_wait(struct prestera_switch *sw,
> > > > +				 enum prestera_cmd_type_t type,
> > > > +				 struct prestera_msg_cmd *cmd, size_t clen,
> > > > +				 struct prestera_msg_ret *ret, size_t rlen,
> > > > +				 int wait)
> > > > +{
> > > > +	return __prestera_cmd_ret(sw, type, cmd, clen, ret, rlen, wait);
> > > > +}
> > > > +
> > > > +static int prestera_cmd(struct prestera_switch *sw,
> > > > +			enum prestera_cmd_type_t type,
> > > > +			struct prestera_msg_cmd *cmd, size_t clen)
> > > > +{
> > > > +	struct prestera_msg_common_resp resp;
> > > > +
> > > > +	return prestera_cmd_ret(sw, type, cmd, clen, &resp.ret, sizeof(resp));
> > > > +}
> > > > +
> > > > +static int prestera_fw_parse_port_evt(u8 *msg, struct prestera_event *evt)
> > > > +{
> > > > +	struct prestera_msg_event_port *hw_evt;
> > > > +
> > > > +	hw_evt = (struct prestera_msg_event_port *)msg;
> > > > +
> > > > +	evt->port_evt.port_id = hw_evt->port_id;
> > > > +
> > > > +	if (evt->id == PRESTERA_PORT_EVENT_STATE_CHANGED)
> > > > +		evt->port_evt.data.oper_state = hw_evt->param.oper_state;
> > > > +	else
> > > > +		return -EINVAL;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static struct prestera_fw_evt_parser {
> > > > +	int (*func)(u8 *msg, struct prestera_event *evt);
> > > > +} fw_event_parsers[PRESTERA_EVENT_TYPE_MAX] = {
> > > > +	[PRESTERA_EVENT_TYPE_PORT] = {.func = prestera_fw_parse_port_evt},
> > > > +};
> > > > +
> > > > +static struct prestera_fw_event_handler *
> > > > +__find_event_handler(const struct prestera_switch *sw,
> > > > +		     enum prestera_event_type type)
> > > > +{
> > > > +	struct prestera_fw_event_handler *eh;
> > > > +
> > > > +	list_for_each_entry_rcu(eh, &sw->event_handlers, list) {
> > > 
> > > It does not look that this is always called under RCU which will result
> > > in various splats. For example in the following call path:
> > > 
> > > prestera_device_register()
> > >   prestera_switch_init()
> > >     prestera_event_handlers_register()
> > >       prestera_hw_event_handler_register()
> > >         __find_event_handler()
> > > 
> > > You want to make sure that you are testing with various debug options.
> > > For example:
> > So, right. Currently this prestera_hw_event_handler_register is called
> > synchronously and as I understand does not require additional locking
> > when use list rcu API. I will check with these options which you
> > suggested.
> > 
> > > 
> > > # Debug options
> > > ## General debug options
> > > config_enable CONFIG_PREEMPT
> > > config_enable CONFIG_DEBUG_PREEMPT
> > > config_enable CONFIG_DEBUG_INFO
> > > config_enable CONFIG_UNWINDER_ORC
> > > config_enable CONFIG_DYNAMIC_DEBUG
> > > config_enable CONFIG_DEBUG_NOTIFIERS
> > > ## Lock debugging
> > > config_enable CONFIG_LOCKDEP
> > > config_enable CONFIG_PROVE_LOCKING
> > > config_enable CONFIG_DEBUG_ATOMIC_SLEEP
> > > config_enable CONFIG_PROVE_RCU
> > > config_enable CONFIG_DEBUG_MUTEXES
> > > config_enable CONFIG_DEBUG_SPINLOCK
> > > config_enable CONFIG_LOCK_STAT
> > > ## Memory debugging
> > > config_enable CONFIG_DEBUG_VM
> > > config_enable CONFIG_FORTIFY_SOURCE
> > > config_enable CONFIG_KASAN
> > > config_enable CONFIG_KASAN_EXTRA
> > > config_enable CONFIG_KASAN_INLINE
> > > ## Reference counting debugging
> > > config_enable CONFIG_REFCOUNT_FULL
> > > ## Lockups debugging
> > > config_enable CONFIG_LOCKUP_DETECTOR
> > > config_enable CONFIG_SOFTLOCKUP_DETECTOR
> > > config_enable CONFIG_HARDLOCKUP_DETECTOR
> > > config_enable CONFIG_DETECT_HUNG_TASK
> > > config_enable CONFIG_WQ_WATCHDOG
> > > config_enable CONFIG_DETECT_HUNG_TASK
> > > config_set_val CONFIG_DEFAULT_HUNG_TASK_TIMEOUT 120
> > > ## Undefined behavior debugging
> > > config_enable CONFIG_UBSAN
> > > config_enable CONFIG_UBSAN_SANITIZE_ALL
> > > config_disable CONFIG_UBSAN_ALIGNMENT
> > > config_disable CONFIG_UBSAN_NULL
> > > ## Memory debugging
> > > config_enable CONFIG_SLUB_DEBUG
> > > config_enable CONFIG_SLUB_DEBUG_ON
> > > config_enable CONFIG_DEBUG_PAGEALLOC
> > > config_enable CONFIG_DEBUG_KMEMLEAK
> > > config_disable CONFIG_DEBUG_KMEMLEAK_DEFAULT_OFF
> > > config_set_val CONFIG_DEBUG_KMEMLEAK_EARLY_LOG_SIZE 8192
> > > config_enable CONFIG_DEBUG_STACKOVERFLOW
> > > config_enable CONFIG_DEBUG_LIST
> > > config_enable CONFIG_DEBUG_PER_CPU_MAPS
> > > config_set_val CONFIG_DEBUG_OBJECTS_ENABLE_DEFAULT 1
> > > config_enable CONFIG_DEBUG_OBJECTS
> > > config_enable CONFIG_DEBUG_OBJECTS_FREE
> > > config_enable CONFIG_DEBUG_OBJECTS_TIMERS
> > > config_enable CONFIG_DEBUG_OBJECTS_WORK
> > > config_enable CONFIG_DEBUG_OBJECTS_PERCPU_COUNTER
> > > config_enable CONFIG_DMA_API_DEBUG
> > > ## Lock debugging
> > > config_enable CONFIG_DEBUG_LOCK_ALLOC
> > > config_enable CONFIG_PROVE_LOCKING
> > > config_enable CONFIG_LOCK_STAT
> > > config_enable CONFIG_DEBUG_OBJECTS_RCU_HEAD
> > > config_enable CONFIG_SPARSE_RCU_POINTER
> > > 
> > > > +		if (eh->type == type)
> > > > +			return eh;
> > > > +	}
> > > > +
> > > > +	return NULL;
> > > > +}
> > > > +
> > > > +static int prestera_find_event_handler(const struct prestera_switch *sw,
> > > > +				       enum prestera_event_type type,
> > > > +				       struct prestera_fw_event_handler *eh)
> > > > +{
> > > > +	struct prestera_fw_event_handler *tmp;
> > > > +	int err = 0;
> > > > +
> > > > +	rcu_read_lock();
> > > > +	tmp = __find_event_handler(sw, type);
> > > > +	if (tmp)
> > > > +		*eh = *tmp;
> > > > +	else
> > > > +		err = -EEXIST;
> > > > +	rcu_read_unlock();
> > > > +
> > > > +	return err;
> > > > +}
> > > > +
> > > > +static int prestera_evt_recv(struct prestera_device *dev, u8 *buf, size_t size)
> > > > +{
> > > > +	struct prestera_msg_event *msg = (struct prestera_msg_event *)buf;
> > > > +	struct prestera_switch *sw = dev->priv;
> > > > +	struct prestera_fw_event_handler eh;
> > > > +	struct prestera_event evt;
> > > > +	int err;
> > > > +
> > > > +	if (msg->type >= PRESTERA_EVENT_TYPE_MAX)
> > > > +		return -EINVAL;
> > > > +
> > > > +	err = prestera_find_event_handler(sw, msg->type, &eh);
> > > > +
> > > > +	if (err || !fw_event_parsers[msg->type].func)
> > > > +		return 0;
> > > > +
> > > > +	evt.id = msg->id;
> > > > +
> > > > +	err = fw_event_parsers[msg->type].func(buf, &evt);
> > > > +	if (!err)
> > > > +		eh.func(sw, &evt, eh.arg);
> > > > +
> > > > +	return err;
> > > > +}
> > > > +
> > > > +static void prestera_pkt_recv(struct prestera_device *dev)
> > > > +{
> > > > +	struct prestera_switch *sw = dev->priv;
> > > > +	struct prestera_fw_event_handler eh;
> > > > +	struct prestera_event ev;
> > > > +	int err;
> > > > +
> > > > +	ev.id = PRESTERA_RXTX_EVENT_RCV_PKT;
> > > > +
> > > > +	err = prestera_find_event_handler(sw, PRESTERA_EVENT_TYPE_RXTX, &eh);
> > > > +	if (err)
> > > > +		return;
> > > > +
> > > > +	eh.func(sw, &ev, eh.arg);
> > > > +}
> > > > +
> > > > +int prestera_hw_port_info_get(const struct prestera_port *port,
> > > > +			      u16 *fp_id, u32 *hw_id, u32 *dev_id)
> > > > +{
> > > > +	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;
> > > > +}
> > > 
> > > Consider adding prestera_hw_switch_fini() that verifies that
> > > '&sw->event_handlers' is empty.
> > > 
> > As I see it can just warn on if list is not empty, right ?
> 
> Yes, something like:
> 
> WARN_ON(!list_empty(&sw->event_handlers));
> 
> > 
> > > > +
> > > > +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));
> > > > +}
> > > > +
> > 
> > [...]
> > 
> > > > +
> > > > +struct prestera_port *prestera_port_find_by_hwid(struct prestera_switch *sw,
> > > > +						 u32 dev_id, u32 hw_id)
> > > > +{
> > > > +	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) {
> > > > +			rcu_read_unlock();
> > > > +			return port;
> > > 
> > > This does not look correct. You call rcu_read_unlock(), but do not take
> > > a reference on the object, so nothing prevents it from being freed. 
> > > 
> > Currently there is no logic which can dynamically create/delete the
> > port, so how do you think may I just use synchronize_rcu() when freeing
> > the port instance on module unlolad ?
> 
> I don't understand what RCU is meant to protect here. You call
> rcu_read_unlock() and then return the port. Calling synchronize_rcu()
> before freeing the port will not prevent the caller of
> prestera_port_find_by_hwid() from accessing freed memory.
I just removed these rcu things, and looks like they does not
needed currently.

> 
> > 
> > > > +		}
> > > > +	}
> > > > +
> > > > +	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;
> > > 
> > > Same here.
> > > 
> > > > +}
> > > > +
> > > > +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 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);
> > > > +
> > > 
> > > I'm not sure what is the point of these blank lines. Best to remove
> > > them.
> > Will fix it.
> > 
> > > 
> > > > +	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);
> > > > +
> > > > +	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;
> > > 
> > > err_port_create ?
> > > 
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +
> > > > +err_ports_init:
> > > > +	prestera_destroy_ports(sw);
> > > 
> > > I'm not a fan of this construct. I find it best to always do proper
> > > rollback in the error path. Then you can always maintain init() being
> > > followed by fini() which is much easier to review.
> > As I understand you meant to move this destroy_ports() recovery to the
> > error path in xxx_switch_init() ?
> 
> No, I mean do proper rollback in this error path by calling
> prestera_port_destroy() for each port you created thus far.
> 
> Then move prestera_destroy_ports() after prestera_create_ports(),
> instead of before it.
But it will look same as prestera_destroy_ports(), do you think
this is not a problem to have a same logic doubled ?

> 
> > 
> > > 
> > > > +	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);
> > > > +}
> > > 
> > > Please reverse the order so that register() is first.
> > > 
> > > > +
> > > > +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_set_base_mac_addr(struct prestera_switch *sw)
> > > > +{
> > > > +	struct device_node *base_mac_np;
> > > > +	struct device_node *np;
> > > > +
> > > > +	np = of_find_compatible_node(NULL, NULL, "marvell,prestera");
> > > > +	if (np) {
> > > > +		base_mac_np = of_parse_phandle(np, "base-mac-provider", 0);
> > > > +		if (base_mac_np) {
> > > > +			const char *base_mac;
> > > > +
> > > > +			base_mac = of_get_mac_address(base_mac_np);
> > > > +			of_node_put(base_mac_np);
> > > > +			if (!IS_ERR(base_mac))
> > > > +				ether_addr_copy(sw->base_mac, base_mac);
> > > > +		}
> > > > +	}
> > > > +
> > > > +	if (!is_valid_ether_addr(sw->base_mac)) {
> > > > +		eth_random_addr(sw->base_mac);
> > > > +		dev_info(sw->dev->dev, "using random base mac address\n");
> > > > +	}
> > > > +
> > > > +	return prestera_hw_switch_mac_set(sw, sw->base_mac);
> > > > +}
> > > > +
> > > > +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;
> > > > +	}
> > > > +
> > > > +	INIT_LIST_HEAD(&sw->port_list);
> > > > +
> > > > +	err = prestera_switch_set_base_mac_addr(sw);
> > > > +	if (err)
> > > > +		return err;
> > > > +
> > > > +	err = prestera_rxtx_switch_init(sw);
> > > > +	if (err)
> > > > +		return err;
> > > > +
> > > > +	err = prestera_event_handlers_register(sw);
> > > > +	if (err)
> > > > +		return err;
> > > > +
> > > > +	err = prestera_create_ports(sw);
> > > > +	if (err)
> > > > +		goto err_ports_create;
> > > > +
> > > > +	return 0;
> > > > +
> > > > +err_ports_create:
> > > > +	prestera_event_handlers_unregister(sw);
> > > > +
> > > 
> > > You are missing prestera_rxtx_switch_fini() here... With init() always
> > > followed by fini() you can easily tell that the error path is not
> > > symmetric with fini().
> > Yeah, for some reason I mixed this fix with Switchdev patch, I will fix
> > this.
> > 
> > > 
> > > > +	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;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(prestera_device_register);
> > > > +
> > > > +void prestera_device_unregister(struct prestera_device *dev)
> > > > +{
> > > > +	struct prestera_switch *sw = dev->priv;
> > > > +
> > > > +	prestera_switch_fini(sw);
> > > > +	kfree(sw);
> > > > +}
> > 
> > [...]
> > 
> > > > +int prestera_rxtx_port_init(struct prestera_port *port)
> > > > +{
> > > > +	int err;
> > > > +
> > > > +	err = prestera_hw_rxtx_port_init(port);
> > > > +	if (err)
> > > > +		return err;
> > > > +
> > > > +	port->dev->needed_headroom = PRESTERA_DSA_HLEN + ETH_FCS_LEN;
> > > 
> > > Why do you need headroom for FCS?
> > > 
> > I had issue when the SKB did not have additional bytes for the FCS, so I
> > thought to added this to the needed_headroom to be sure.
> 
> But FCS is at the end of the frame. 'needed_headroom' is for headers you
> need to prepend to each frame.
> 
> Which issues did you have? In mlxsw FCS is computed in hardware and we
> pass the length of the frame without the FCS when posting the frame to
> hardware.
Looks like it really not needed.

> 
> > 
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +netdev_tx_t prestera_rxtx_xmit(struct prestera_port *port, struct sk_buff *skb)
> > > > +{
> > > > +	struct prestera_dsa dsa;
> > > > +
> > > > +	dsa.hw_dev_num = port->dev_id;
> > > > +	dsa.port_num = port->hw_id;
> > > > +
> > > > +	if (skb_cow_head(skb, PRESTERA_DSA_HLEN) < 0)
> > > > +		return NET_XMIT_DROP;
> > > > +
> > > > +	skb_push(skb, PRESTERA_DSA_HLEN);
> > > > +	memmove(skb->data, skb->data + PRESTERA_DSA_HLEN, 2 * ETH_ALEN);
> > > > +
> > > > +	if (prestera_dsa_build(&dsa, skb->data + 2 * ETH_ALEN) != 0)
> > > > +		return NET_XMIT_DROP;
> > > > +
> > > > +	return prestera_sdma_xmit(&port->sw->rxtx->sdma, skb);
> > > > +}
> > > > diff --git a/drivers/net/ethernet/marvell/prestera/prestera_rxtx.h b/drivers/net/ethernet/marvell/prestera/prestera_rxtx.h
> > > > new file mode 100644
> > > > index 000000000000..bbbadfa5accf
> > > > --- /dev/null
> > > > +++ b/drivers/net/ethernet/marvell/prestera/prestera_rxtx.h
> > > > @@ -0,0 +1,21 @@
> > > > +/* SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> > > > + *
> > > > + * Copyright (c) 2019-2020 Marvell International Ltd. All rights reserved.
> > > > + *
> > > > + */
> > > > +
> > > > +#ifndef _PRESTERA_RXTX_H_
> > > > +#define _PRESTERA_RXTX_H_
> > > > +
> > > > +#include <linux/netdevice.h>
> > > > +
> > > > +#include "prestera.h"
> > > > +
> > > > +int prestera_rxtx_switch_init(struct prestera_switch *sw);
> > > > +void prestera_rxtx_switch_fini(struct prestera_switch *sw);
> > > > +
> > > > +int prestera_rxtx_port_init(struct prestera_port *port);
> > > > +
> > > > +netdev_tx_t prestera_rxtx_xmit(struct prestera_port *port, struct sk_buff *skb);
> > > > +
> > > > +#endif /* _PRESTERA_RXTX_H_ */
> > > > -- 
> > > > 2.17.1
> > > > 
> > 
> > Thank you for review.

Powered by blists - more mailing lists