| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200227213150.GA9372@plvision.eu>
Date: Thu, 27 Feb 2020 21:32:00 +0000
From: Vadym Kochan <vadym.kochan@...ision.eu>
To: Jiri Pirko <jiri@...nulli.us>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"David S . Miller" <davem@...emloft.net>,
Oleksandr Mazur <oleksandr.mazur@...ision.eu>,
Taras Chornyi <taras.chornyi@...ision.eu>,
Serhiy Boiko <serhiy.boiko@...ision.eu>,
Andrii Savka <andrii.savka@...ision.eu>,
Volodymyr Mytnyk <volodymyr.mytnyk@...ision.eu>,
Serhiy Pshyk <serhiy.pshyk@...ision.eu>
Subject: Re: [RFC net-next 1/3] net: marvell: prestera: Add Switchdev driver
for Prestera family ASIC device 98DX325x (AC3x)
Hi Jiri,
On Wed, Feb 26, 2020 at 04:54:23PM +0100, Jiri Pirko wrote:
> Tue, Feb 25, 2020 at 05:30:54PM CET, 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.
> >
> >This driver implementation includes only L1 & basic L2 support.
> >
> >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.
> >
> >The following Switchdev features are supported:
> >
> > - VLAN-aware bridge offloading
> > - VLAN-unaware bridge offloading
> > - FDB offloading (learning, ageing)
> > - Switchport configuration
> >
> >Signed-off-by: Vadym Kochan <vadym.kochan@...ision.eu>
> >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>
> >---
> > drivers/net/ethernet/marvell/Kconfig | 1 +
> > drivers/net/ethernet/marvell/Makefile | 1 +
> > drivers/net/ethernet/marvell/prestera/Kconfig | 13 +
> > .../net/ethernet/marvell/prestera/Makefile | 3 +
> > .../net/ethernet/marvell/prestera/prestera.c | 1502 +++++++++++++++++
> > .../net/ethernet/marvell/prestera/prestera.h | 244 +++
> > .../marvell/prestera/prestera_drv_ver.h | 23 +
> > .../ethernet/marvell/prestera/prestera_hw.c | 1094 ++++++++++++
> > .../ethernet/marvell/prestera/prestera_hw.h | 159 ++
> > .../marvell/prestera/prestera_switchdev.c | 1217 +++++++++++++
> > 10 files changed, 4257 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_drv_ver.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_switchdev.c
> >
> >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..d0b416dcb677
> >--- /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..9446298fb7f4
> >--- /dev/null
> >+++ b/drivers/net/ethernet/marvell/prestera/Makefile
> >@@ -0,0 +1,3 @@
> >+# SPDX-License-Identifier: GPL-2.0
> >+obj-$(CONFIG_PRESTERA) += prestera_sw.o
> >+prestera_sw-objs := prestera.o prestera_hw.o prestera_switchdev.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..12d0eb590bbb
> >--- /dev/null
> >+++ b/drivers/net/ethernet/marvell/prestera/prestera.c
> >@@ -0,0 +1,1502 @@
> >+/* 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/ethtool.h>
> >+#include <linux/jiffies.h>
> >+#include <net/switchdev.h>
> >+
> >+#include "prestera.h"
> >+#include "prestera_hw.h"
> >+#include "prestera_drv_ver.h"
> >+
> >+#define MVSW_PR_MTU_DEFAULT 1536
> >+
> >+#define PORT_STATS_CACHE_TIMEOUT_MS (msecs_to_jiffies(1000))
> >+#define PORT_STATS_CNT (sizeof(struct mvsw_pr_port_stats) / sizeof(u64))
>
> Keep the prefix for all defines withing the file. "PORT_STATS_CNT"
> looks way to generic on the first look.
>
>
> >+#define PORT_STATS_IDX(name) \
> >+ (offsetof(struct mvsw_pr_port_stats, name) / sizeof(u64))
> >+#define PORT_STATS_FIELD(name) \
> >+ [PORT_STATS_IDX(name)] = __stringify(name)
> >+
> >+static struct list_head switches_registered;
> >+
> >+static const char mvsw_driver_kind[] = "prestera_sw";
>
> Please be consistent. Make your prefixes, name, filenames the same.
> For example:
> prestera_driver_kind[] = "prestera";
>
> Applied to the whole code.
>
So you suggested to use prestera_ as a prefix, I dont see a problem
with that, but why not mvsw_pr_ ? So it has the vendor, device name parts
together as a key. Also it is necessary to apply prefix for the static
names ?
>
> >+static const char mvsw_driver_name[] = "mvsw_switchdev";
>
> Why is this different from kind?
>
> Also, don't mention "switchdev" anywhere.
>
>
> >+static const char mvsw_driver_version[] = PRESTERA_DRV_VER;
>
> [...]
>
>
> >+static void mvsw_pr_port_remote_cap_get(struct ethtool_link_ksettings *ecmd,
> >+ struct mvsw_pr_port *port)
> >+{
> >+ u64 bitmap;
> >+
> >+ if (!mvsw_pr_hw_port_remote_cap_get(port, &bitmap)) {
> >+ mvsw_modes_to_eth(ecmd->link_modes.lp_advertising,
> >+ bitmap, 0, MVSW_PORT_TYPE_NONE);
> >+ }
>
> Don't use {} for single statement. checkpatch.pl should warn you about
> this.
>
>
>
> >+}
> >+
> >+static void mvsw_pr_port_duplex_get(struct ethtool_link_ksettings *ecmd,
> >+ struct mvsw_pr_port *port)
> >+{
> >+ u8 duplex;
> >+
> >+ if (!mvsw_pr_hw_port_duplex_get(port, &duplex)) {
> >+ ecmd->base.duplex = duplex == MVSW_PORT_DUPLEX_FULL ?
> >+ DUPLEX_FULL : DUPLEX_HALF;
> >+ } else {
> >+ ecmd->base.duplex = DUPLEX_UNKNOWN;
> >+ }
>
> Same here.
>
>
> >+}
>
> [...]
>
>
> >+static void __exit mvsw_pr_module_exit(void)
> >+{
> >+ destroy_workqueue(mvsw_pr_wq);
> >+
> >+ pr_info("Unloading Marvell Prestera Switch Driver\n");
>
> No prints like this please.
>
>
>
> >+}
> >+
> >+module_init(mvsw_pr_module_init);
> >+module_exit(mvsw_pr_module_exit);
> >+
> >+MODULE_AUTHOR("Marvell Semi.");
>
> Does not look so :)
>
>
> >+MODULE_LICENSE("GPL");
> >+MODULE_DESCRIPTION("Marvell Prestera switch driver");
> >+MODULE_VERSION(PRESTERA_DRV_VER);
>
> [...]
Thank you for the comments and suggestions!
Regards,
Vadym Kochan
Powered by blists - more mailing lists