diff mbox series

[RFC,net-next,3/3] net: dsa: ksz: moved ksz9477 port mirror to ksz_common.c

Message ID 20220427162343.18092-4-arun.ramadoss@microchip.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series net: dsa: ksz: generic port mirror function for ksz9477 based switch | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 10 of 10 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Arun Ramadoss April 27, 2022, 4:23 p.m. UTC 
Moved the port_mirror_add and port_mirror_del function from ksz9477 to
ksz_common, to make it generic function which can be used by KSZ9477
based switch.

Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
---
 drivers/net/dsa/microchip/ksz9477.c     | 74 +------------------------
 drivers/net/dsa/microchip/ksz9477_reg.h | 15 -----
 drivers/net/dsa/microchip/ksz_common.c  | 72 ++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz_common.h  |  5 ++
 drivers/net/dsa/microchip/ksz_reg.h     | 29 ++++++++++
 5 files changed, 108 insertions(+), 87 deletions(-)
 create mode 100644 drivers/net/dsa/microchip/ksz_reg.h

Comments

Vladimir Oltean April 27, 2022, 4:57 p.m. UTC | #1 
On Wed, Apr 27, 2022 at 09:53:43PM +0530, Arun Ramadoss wrote:
> Moved the port_mirror_add and port_mirror_del function from ksz9477 to

Present tense (move)

> ksz_common, to make it generic function which can be used by KSZ9477
> based switch.

Presumably you mean "which can be used by other switches" (it can
already be used by ksz9477, so that can't be the argument for moving it)

> 
> Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> ---

Looks good, except for the spelling mistakes in the code that is being
moved (introduced in patch 1), which I expect you will update in the new
code as well.

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

> diff --git a/drivers/net/dsa/microchip/ksz_reg.h b/drivers/net/dsa/microchip/ksz_reg.h
> new file mode 100644
> index 000000000000..ccd4a6568e34
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/ksz_reg.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Microchip KSZ Switch register definitions
> + *
> + * Copyright (C) 2017-2022 Microchip Technology Inc.
> + */
> +
> +#ifndef __KSZ_REGS_H
> +#define __KSZ_REGS_H
> +
> +#define REG_SW_MRI_CTRL_0		0x0370
> +
> +#define SW_IGMP_SNOOP			BIT(6)
> +#define SW_IPV6_MLD_OPTION		BIT(3)
> +#define SW_IPV6_MLD_SNOOP		BIT(2)
> +#define SW_MIRROR_RX_TX			BIT(0)
> +
> +/* 8 - Classification and Policing */
> +#define REG_PORT_MRI_MIRROR_CTRL	0x0800
> +
> +#define PORT_MIRROR_RX			BIT(6)
> +#define PORT_MIRROR_TX			BIT(5)
> +#define PORT_MIRROR_SNIFFER		BIT(1)
> +
> +#define P_MIRROR_CTRL			REG_PORT_MRI_MIRROR_CTRL
> +
> +#define S_MIRROR_CTRL			REG_SW_MRI_CTRL_0

Small comment: if P_MIRROR_CTRL and S_MIRROR_CTRL are expected to be at
the same register offset for all switch families, why is there a macro
behind a macro for their addresses?

> +
> +#endif
> -- 
> 2.33.0
>
Arun Ramadoss April 28, 2022, 3:09 p.m. UTC | #2 
Hi Vladimir,
Thanks for the feedback.

On Wed, 2022-04-27 at 19:57 +0300, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Wed, Apr 27, 2022 at 09:53:43PM +0530, Arun Ramadoss wrote:
> > Moved the port_mirror_add and port_mirror_del function from ksz9477
> > to
> 
> Present tense (move)
> 
> > ksz_common, to make it generic function which can be used by
> > KSZ9477
> > based switch.
> 
> Presumably you mean "which can be used by other switches" (it can
> already be used by ksz9477, so that can't be the argument for moving
> it)
I will update the commit description.
> 
> > 
> > Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> > ---
> 
> Looks good, except for the spelling mistakes in the code that is
> being
> moved (introduced in patch 1), which I expect you will update in the
> new
> code as well.
Yes, I will update. 
> 
> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
> 
> > diff --git a/drivers/net/dsa/microchip/ksz_reg.h
> > b/drivers/net/dsa/microchip/ksz_reg.h
> > new file mode 100644
> > index 000000000000..ccd4a6568e34
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/ksz_reg.h
> > @@ -0,0 +1,29 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Microchip KSZ Switch register definitions
> > + *
> > + * Copyright (C) 2017-2022 Microchip Technology Inc.
> > + */
> > +
> > +#ifndef __KSZ_REGS_H
> > +#define __KSZ_REGS_H
> > +
> > +#define REG_SW_MRI_CTRL_0            0x0370
> > +
> > +#define SW_IGMP_SNOOP                        BIT(6)
> > +#define SW_IPV6_MLD_OPTION           BIT(3)
> > +#define SW_IPV6_MLD_SNOOP            BIT(2)
> > +#define SW_MIRROR_RX_TX                      BIT(0)
> > +
> > +/* 8 - Classification and Policing */
> > +#define REG_PORT_MRI_MIRROR_CTRL     0x0800
> > +
> > +#define PORT_MIRROR_RX                       BIT(6)
> > +#define PORT_MIRROR_TX                       BIT(5)
> > +#define PORT_MIRROR_SNIFFER          BIT(1)
> > +
> > +#define
> > P_MIRROR_CTRL                        REG_PORT_MRI_MIRROR_CTRL
> > +
> > +#define S_MIRROR_CTRL                        REG_SW_MRI_CTRL_0
> 
> Small comment: if P_MIRROR_CTRL and S_MIRROR_CTRL are expected to be
> at
> the same register offset for all switch families, why is there a
> macro
> behind a macro for their addresses?

ksz8795 and ksz9477 have different address/register for the
Mirror_ctrl. To make it common for the both, P_MIRROR_CTRL is defined
in ksz8795_reg.h and ksz9477_reg.h file.
I just carried forward to ksz_reg.h.

> 
> > +
> > +#endif
> > --
> > 2.33.0
> >
Vladimir Oltean April 28, 2022, 3:22 p.m. UTC | #3 
On Thu, Apr 28, 2022 at 03:09:50PM +0000, Arun.Ramadoss@microchip.com wrote:
> > > +#define
> > > P_MIRROR_CTRL                        REG_PORT_MRI_MIRROR_CTRL
> > > +
> > > +#define S_MIRROR_CTRL                        REG_SW_MRI_CTRL_0
> > 
> > Small comment: if P_MIRROR_CTRL and S_MIRROR_CTRL are expected to be
> > at
> > the same register offset for all switch families, why is there a
> > macro
> > behind a macro for their addresses?
> 
> ksz8795 and ksz9477 have different address/register for the
> Mirror_ctrl. To make it common for the both, P_MIRROR_CTRL is defined
> in ksz8795_reg.h and ksz9477_reg.h file.
> I just carried forward to ksz_reg.h.

So if P_MIRROR_CTRL has different values for ksz9477 and ksz8795, how
exactly do you plan to mask that difference away through the C preprocessor
at the level of ksz_reg.h included by ksz_common.c, depending on which
switch driver calls ksz_port_mirror_add()?

This can't work, you need to provide the offset of P_MIRROR_CTRL as
argument to the common function. What am I missing?
Arun Ramadoss April 28, 2022, 4:05 p.m. UTC | #4 
On Thu, 2022-04-28 at 18:22 +0300, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Thu, Apr 28, 2022 at 03:09:50PM +0000, Arun.Ramadoss@microchip.com
>  wrote:
> > > > +#define
> > > > P_MIRROR_CTRL                        REG_PORT_MRI_MIRROR_CTRL
> > > > +
> > > > +#define S_MIRROR_CTRL                        REG_SW_MRI_CTRL_0
> > > 
> > > Small comment: if P_MIRROR_CTRL and S_MIRROR_CTRL are expected to
> > > be
> > > at
> > > the same register offset for all switch families, why is there a
> > > macro
> > > behind a macro for their addresses?
> > 
> > ksz8795 and ksz9477 have different address/register for the
> > Mirror_ctrl. To make it common for the both, P_MIRROR_CTRL is
> > defined
> > in ksz8795_reg.h and ksz9477_reg.h file.
> > I just carried forward to ksz_reg.h.
> 
> So if P_MIRROR_CTRL has different values for ksz9477 and ksz8795, how
> exactly do you plan to mask that difference away through the C
> preprocessor
> at the level of ksz_reg.h included by ksz_common.c, depending on
> which
> switch driver calls ksz_port_mirror_add()?
> 
> This can't work, you need to provide the offset of P_MIRROR_CTRL as
> argument to the common function. What am I missing?
I compared the ksz8795 and ksz9447 mirror_add/del implementation, they
are different. Ksz9477 writes S_MIRROR_CTRL in addition to
P_MIRROL_CTRL. KSZ9477 and LAN937x have similar register set but
KSZ8795 has only limited registers/functionality.
Similar to port_mirror, few other functionality like mib counter, vlan
registers are same for both KSZ9477 & LAN937x.But ksz8795 has different
set of register implementation for that.
So I thought of not to disturb the existing ksz8795 implementation
except for any conflicts, just move the ksz9477 to ksz_common, and call
this for KSZ9477 and LAN937x dsa hooks.
diff mbox series

Patch

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index f762120ce3fd..d568ebfaf8c1 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -973,76 +973,6 @@  static int ksz9477_port_mdb_del(struct dsa_switch *ds, int port,
 	return ret;
 }
 
-static int ksz9477_port_mirror_add(struct dsa_switch *ds, int port,
-				   struct dsa_mall_mirror_tc_entry *mirror,
-				   bool ingress, struct netlink_ext_ack *extack)
-{
-	struct ksz_device *dev = ds->priv;
-	u8 data;
-	int p;
-
-	/* Limit to one sniffer port
-	 * Check if any of the port is already set for sniffing
-	 * If yes, instruct the user to remove the previous entry & exit
-	 */
-	for (p = 0; p < dev->port_cnt; p++) {
-		/* Skip the current sniffing port */
-		if (p == mirror->to_local_port)
-			continue;
-
-		ksz_pread8(dev, p, P_MIRROR_CTRL, &data);
-
-		if (data & PORT_MIRROR_SNIFFER) {
-			NL_SET_ERR_MSG_MOD(extack,
-					   "Sniffer port is already configured, delete existing rules & retry");
-			return -EBUSY;
-		}
-	}
-
-	if (ingress)
-		ksz_port_cfg(dev, port, P_MIRROR_CTRL, PORT_MIRROR_RX, true);
-	else
-		ksz_port_cfg(dev, port, P_MIRROR_CTRL, PORT_MIRROR_TX, true);
-
-	/* configure mirror port */
-	ksz_port_cfg(dev, mirror->to_local_port, P_MIRROR_CTRL,
-		     PORT_MIRROR_SNIFFER, true);
-
-	ksz_cfg(dev, S_MIRROR_CTRL, SW_MIRROR_RX_TX, false);
-
-	return 0;
-}
-
-static void ksz9477_port_mirror_del(struct dsa_switch *ds, int port,
-				    struct dsa_mall_mirror_tc_entry *mirror)
-{
-	struct ksz_device *dev = ds->priv;
-	bool in_use = false;
-	u8 data;
-	int p;
-
-	if (mirror->ingress)
-		ksz_port_cfg(dev, port, P_MIRROR_CTRL, PORT_MIRROR_RX, false);
-	else
-		ksz_port_cfg(dev, port, P_MIRROR_CTRL, PORT_MIRROR_TX, false);
-
-
-	/* Check if any of the port is still referring to sniffer port */
-	for (p = 0; p < dev->port_cnt; p++) {
-		ksz_pread8(dev, p, P_MIRROR_CTRL, &data);
-
-		if ((data & (PORT_MIRROR_RX | PORT_MIRROR_TX))) {
-			in_use = true;
-			break;
-		}
-	}
-
-	/* delete sniffing if there are no other mirroring rule exist */
-	if (!in_use)
-		ksz_port_cfg(dev, mirror->to_local_port, P_MIRROR_CTRL,
-			     PORT_MIRROR_SNIFFER, false);
-}
-
 static bool ksz9477_get_gbit(struct ksz_device *dev, u8 data)
 {
 	bool gbit;
@@ -1478,8 +1408,8 @@  static const struct dsa_switch_ops ksz9477_switch_ops = {
 	.port_fdb_del		= ksz9477_port_fdb_del,
 	.port_mdb_add           = ksz9477_port_mdb_add,
 	.port_mdb_del           = ksz9477_port_mdb_del,
-	.port_mirror_add	= ksz9477_port_mirror_add,
-	.port_mirror_del	= ksz9477_port_mirror_del,
+	.port_mirror_add	= ksz_port_mirror_add,
+	.port_mirror_del	= ksz_port_mirror_del,
 	.get_stats64		= ksz9477_get_stats64,
 	.port_change_mtu	= ksz9477_change_mtu,
 	.port_max_mtu		= ksz9477_max_mtu,
diff --git a/drivers/net/dsa/microchip/ksz9477_reg.h b/drivers/net/dsa/microchip/ksz9477_reg.h
index 7a2c8d4767af..abdd653a2f39 100644
--- a/drivers/net/dsa/microchip/ksz9477_reg.h
+++ b/drivers/net/dsa/microchip/ksz9477_reg.h
@@ -345,13 +345,6 @@ 
 #define REG_SW_MAC_TOS_PRIO_30		0x035E
 #define REG_SW_MAC_TOS_PRIO_31		0x035F
 
-#define REG_SW_MRI_CTRL_0		0x0370
-
-#define SW_IGMP_SNOOP			BIT(6)
-#define SW_IPV6_MLD_OPTION		BIT(3)
-#define SW_IPV6_MLD_SNOOP		BIT(2)
-#define SW_MIRROR_RX_TX			BIT(0)
-
 #define REG_SW_CLASS_D_IP_CTRL__4	0x0374
 
 #define SW_CLASS_D_IP_ENABLE		BIT(31)
@@ -1406,12 +1399,6 @@ 
 #define REG_PORT_ACL_CTRL_1		0x0613
 
 /* 8 - Classification and Policing */
-#define REG_PORT_MRI_MIRROR_CTRL	0x0800
-
-#define PORT_MIRROR_RX			BIT(6)
-#define PORT_MIRROR_TX			BIT(5)
-#define PORT_MIRROR_SNIFFER		BIT(1)
-
 #define REG_PORT_MRI_PRIO_CTRL		0x0801
 
 #define PORT_HIGHEST_PRIO		BIT(7)
@@ -1628,7 +1615,6 @@ 
 
 #define P_BCAST_STORM_CTRL		REG_PORT_MAC_CTRL_0
 #define P_PRIO_CTRL			REG_PORT_MRI_PRIO_CTRL
-#define P_MIRROR_CTRL			REG_PORT_MRI_MIRROR_CTRL
 #define P_STP_CTRL			REG_PORT_LUE_MSTP_STATE
 #define P_PHY_CTRL			REG_PORT_PHY_CTRL
 #define P_NEG_RESTART_CTRL		REG_PORT_PHY_CTRL
@@ -1637,7 +1623,6 @@ 
 #define P_RATE_LIMIT_CTRL		REG_PORT_MAC_IN_RATE_LIMIT
 
 #define S_LINK_AGING_CTRL		REG_SW_LUE_CTRL_1
-#define S_MIRROR_CTRL			REG_SW_MRI_CTRL_0
 #define S_REPLACE_VID_CTRL		REG_SW_MAC_CTRL_2
 #define S_802_1P_PRIO_CTRL		REG_SW_MAC_802_1P_MAP_0
 #define S_TOS_PRIO_CTRL			REG_SW_MAC_TOS_PRIO_0
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 9b9f570ebb0b..ec5759b017d9 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -19,6 +19,78 @@ 
 #include <net/switchdev.h>
 
 #include "ksz_common.h"
+#include "ksz_reg.h"
+
+int ksz_port_mirror_add(struct dsa_switch *ds, int port,
+			struct dsa_mall_mirror_tc_entry *mirror,
+			bool ingress, struct netlink_ext_ack *extack)
+{
+	struct ksz_device *dev = ds->priv;
+	u8 data;
+	int p;
+
+	/* Limit to one sniffer port
+	 * Check if any of the port is already set for sniffing
+	 * If yes, instruct the user to remove the previous entry & exit
+	 */
+	for (p = 0; p < dev->port_cnt; p++) {
+		/* Skip the current sniffing port */
+		if (p == mirror->to_local_port)
+			continue;
+
+		ksz_pread8(dev, p, P_MIRROR_CTRL, &data);
+
+		if (data & PORT_MIRROR_SNIFFER) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Sniffer port is already configured, delete existing rules & retry");
+			return -EBUSY;
+		}
+	}
+
+	if (ingress)
+		ksz_port_cfg(dev, port, P_MIRROR_CTRL, PORT_MIRROR_RX, true);
+	else
+		ksz_port_cfg(dev, port, P_MIRROR_CTRL, PORT_MIRROR_TX, true);
+
+	/* configure mirror port */
+	ksz_port_cfg(dev, mirror->to_local_port, P_MIRROR_CTRL,
+		     PORT_MIRROR_SNIFFER, true);
+
+	ksz_cfg(dev, S_MIRROR_CTRL, SW_MIRROR_RX_TX, false);
+
+	return 0;
+}
+EXPORT_SYMBOL(ksz_port_mirror_add);
+
+void ksz_port_mirror_del(struct dsa_switch *ds, int port,
+			 struct dsa_mall_mirror_tc_entry *mirror)
+{
+	struct ksz_device *dev = ds->priv;
+	bool in_use = false;
+	u8 data;
+	int p;
+
+	if (mirror->ingress)
+		ksz_port_cfg(dev, port, P_MIRROR_CTRL, PORT_MIRROR_RX, false);
+	else
+		ksz_port_cfg(dev, port, P_MIRROR_CTRL, PORT_MIRROR_TX, false);
+
+	/* Check if any of the port is still referring to sniffer port */
+	for (p = 0; p < dev->port_cnt; p++) {
+		ksz_pread8(dev, p, P_MIRROR_CTRL, &data);
+
+		if ((data & (PORT_MIRROR_RX | PORT_MIRROR_TX))) {
+			in_use = true;
+			break;
+		}
+	}
+
+	/* delete sniffing if there are no other mirroring rule exist */
+	if (!in_use)
+		ksz_port_cfg(dev, mirror->to_local_port, P_MIRROR_CTRL,
+			     PORT_MIRROR_SNIFFER, false);
+}
+EXPORT_SYMBOL(ksz_port_mirror_del);
 
 void ksz_update_port_member(struct ksz_device *dev, int port)
 {
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 4f049e9d8952..e8eeafc03bf7 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -177,6 +177,11 @@  int ksz_port_mdb_del(struct dsa_switch *ds, int port,
 		     const struct switchdev_obj_port_mdb *mdb,
 		     struct dsa_db db);
 int ksz_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy);
+int ksz_port_mirror_add(struct dsa_switch *ds, int port,
+			struct dsa_mall_mirror_tc_entry *mirror,
+			bool ingress, struct netlink_ext_ack *extack);
+void ksz_port_mirror_del(struct dsa_switch *ds, int port,
+			 struct dsa_mall_mirror_tc_entry *mirror);
 
 /* Common register access functions */
 
diff --git a/drivers/net/dsa/microchip/ksz_reg.h b/drivers/net/dsa/microchip/ksz_reg.h
new file mode 100644
index 000000000000..ccd4a6568e34
--- /dev/null
+++ b/drivers/net/dsa/microchip/ksz_reg.h
@@ -0,0 +1,29 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Microchip KSZ Switch register definitions
+ *
+ * Copyright (C) 2017-2022 Microchip Technology Inc.
+ */
+
+#ifndef __KSZ_REGS_H
+#define __KSZ_REGS_H
+
+#define REG_SW_MRI_CTRL_0		0x0370
+
+#define SW_IGMP_SNOOP			BIT(6)
+#define SW_IPV6_MLD_OPTION		BIT(3)
+#define SW_IPV6_MLD_SNOOP		BIT(2)
+#define SW_MIRROR_RX_TX			BIT(0)
+
+/* 8 - Classification and Policing */
+#define REG_PORT_MRI_MIRROR_CTRL	0x0800
+
+#define PORT_MIRROR_RX			BIT(6)
+#define PORT_MIRROR_TX			BIT(5)
+#define PORT_MIRROR_SNIFFER		BIT(1)
+
+#define P_MIRROR_CTRL			REG_PORT_MRI_MIRROR_CTRL
+
+#define S_MIRROR_CTRL			REG_SW_MRI_CTRL_0
+
+#endif