[RFC patch net-next 0/3] net: dsa: ksz: generic port mirror function for ksz9477 based switch

[RFC patch net-next 0/3] net: dsa: ksz: generic port mirror function for ksz9477 based switch

[Arun Ramadoss]

This patch series updates the ksz9477 port_mirror_add and port_mirror_del as
per the LAN937x patch submission. It allows only one port to be sniffing port.
Then moves the function to ksz_common.c file, to have it referenced by LAN937x
based switch. And add new header file ksz_reg.h which has common register
address between them.

Arun Ramadoss (3):
  net: dsa: ksz9477: port mirror sniffing limited to one port
  net: dsa: ksz: remove duplicate ksz_cfg and ksz_port_cfg
  net: dsa: ksz: moved ksz9477 port mirror to ksz_common.c

 drivers/net/dsa/microchip/ksz8795.c     | 12 -----
 drivers/net/dsa/microchip/ksz9477.c     | 56 +------------------
 drivers/net/dsa/microchip/ksz9477_reg.h | 15 ------
 drivers/net/dsa/microchip/ksz_common.c  | 72 +++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz_common.h  | 18 +++++++
 drivers/net/dsa/microchip/ksz_reg.h     | 29 ++++++++++
 6 files changed, 121 insertions(+), 81 deletions(-)
 create mode 100644 drivers/net/dsa/microchip/ksz_reg.h


base-commit: 03fa8fc93e443e6caa485cc741328a1386c63630
-- 
2.33.0

[RFC patch net-next 1/3] net: dsa: ksz9477: port mirror sniffing limited to one port

[Arun Ramadoss]

This patch limits the sniffing to only one port during the mirror add.
And during the mirror_del it checks for all the ports using the sniff,
if and only if no other ports are referring, sniffing is disabled.
The code is updated based on the review comments of LAN937x port mirror
patch.
https://patchwork.kernel.org/project/netdevbpf/patch/20210422094257.1641396-8-prasanna.vengateshan@microchip.com/

Signed-off-by: Prasanna Vengateshan <prasanna.vengateshan@microchip.com>
Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
---
 drivers/net/dsa/microchip/ksz9477.c | 38 ++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 4f617fee9a4e..90ce789107eb 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -990,14 +990,32 @@ static int ksz9477_port_mirror_add(struct dsa_switch *ds, int port,
 				   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);
 
-	ksz_port_cfg(dev, port, P_MIRROR_CTRL, PORT_MIRROR_SNIFFER, false);
-
 	/* configure mirror port */
 	ksz_port_cfg(dev, mirror->to_local_port, P_MIRROR_CTRL,
 		     PORT_MIRROR_SNIFFER, true);
@@ -1011,16 +1029,28 @@ 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);
 
-	ksz_pread8(dev, port, P_MIRROR_CTRL, &data);
 
-	if (!(data & (PORT_MIRROR_RX | PORT_MIRROR_TX)))
+	/* 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);
 }
-- 
2.33.0

Re: [RFC patch net-next 1/3] net: dsa: ksz9477: port mirror sniffing limited to one port

[Vladimir Oltean]

On Wed, Apr 27, 2022 at 09:53:41PM +0530, Arun Ramadoss wrote:
> This patch limits the sniffing to only one port during the mirror add.
> And during the mirror_del it checks for all the ports using the sniff,
> if and only if no other ports are referring, sniffing is disabled.
> The code is updated based on the review comments of LAN937x port mirror
> patch.
> https://patchwork.kernel.org/project/netdevbpf/patch/20210422094257.1641396-8-prasanna.vengateshan@microchip.com/
> 
> Signed-off-by: Prasanna Vengateshan <prasanna.vengateshan@microchip.com>
> Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> ---

This probably needs:

Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")

with the mention that it probably won't be backported too far due to the
dependency on 0148bb50b8fd ("net: dsa: pass extack to dsa_switch_ops ::
port_mirror_add()"). But this doesn't change what you should do.

You should send it towards the "net" tree (probably right now), wait
until the "net" pull request for this gets sent out, then "net" gets
merged back into "net-next", then you can continue your work with the
other patches.

>  drivers/net/dsa/microchip/ksz9477.c | 38 ++++++++++++++++++++++++++---
>  1 file changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index 4f617fee9a4e..90ce789107eb 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -990,14 +990,32 @@ static int ksz9477_port_mirror_add(struct dsa_switch *ds, int port,
>  				   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);
>  
> -	ksz_port_cfg(dev, port, P_MIRROR_CTRL, PORT_MIRROR_SNIFFER, false);
> -
>  	/* configure mirror port */
>  	ksz_port_cfg(dev, mirror->to_local_port, P_MIRROR_CTRL,
>  		     PORT_MIRROR_SNIFFER, true);
> @@ -1011,16 +1029,28 @@ 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);
>  
> -	ksz_pread8(dev, port, P_MIRROR_CTRL, &data);
>  
> -	if (!(data & (PORT_MIRROR_RX | PORT_MIRROR_TX)))
> +	/* 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 */

Either "there are no other mirroring rules", or "no other mirroring rule
exists".

> +	if (!in_use)
>  		ksz_port_cfg(dev, mirror->to_local_port, P_MIRROR_CTRL,
>  			     PORT_MIRROR_SNIFFER, false);
>  }
> -- 
> 2.33.0
> 

[RFC patch net-next 2/3] net: dsa: ksz: remove duplicate ksz_cfg and ksz_port_cfg

[Arun Ramadoss]

ksz8795.c and ksz9477.c has individual ksz_cfg and ksz_port_cfg
function, both are same. Hence moving it to ksz_common.c. And removed
the individual references.

Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
---
 drivers/net/dsa/microchip/ksz8795.c    | 12 ------------
 drivers/net/dsa/microchip/ksz9477.c    | 12 ------------
 drivers/net/dsa/microchip/ksz_common.h | 13 +++++++++++++
 3 files changed, 13 insertions(+), 24 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index f91deea9368e..33453060fa71 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -211,18 +211,6 @@ static bool ksz_is_ksz88x3(struct ksz_device *dev)
 	return dev->chip_id == 0x8830;
 }
 
-static void ksz_cfg(struct ksz_device *dev, u32 addr, u8 bits, bool set)
-{
-	regmap_update_bits(dev->regmap[0], addr, bits, set ? bits : 0);
-}
-
-static void ksz_port_cfg(struct ksz_device *dev, int port, int offset, u8 bits,
-			 bool set)
-{
-	regmap_update_bits(dev->regmap[0], PORT_CTRL_ADDR(port, offset),
-			   bits, set ? bits : 0);
-}
-
 static int ksz8_ind_write8(struct ksz_device *dev, u8 table, u16 addr, u8 data)
 {
 	struct ksz8 *ksz8 = dev->priv;
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 90ce789107eb..f762120ce3fd 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -159,18 +159,6 @@ static void ksz9477_get_stats64(struct dsa_switch *ds, int port,
 	spin_unlock(&mib->stats64_lock);
 }
 
-static void ksz_cfg(struct ksz_device *dev, u32 addr, u8 bits, bool set)
-{
-	regmap_update_bits(dev->regmap[0], addr, bits, set ? bits : 0);
-}
-
-static void ksz_port_cfg(struct ksz_device *dev, int port, int offset, u8 bits,
-			 bool set)
-{
-	regmap_update_bits(dev->regmap[0], PORT_CTRL_ADDR(port, offset),
-			   bits, set ? bits : 0);
-}
-
 static void ksz9477_cfg32(struct ksz_device *dev, u32 addr, u32 bits, bool set)
 {
 	regmap_update_bits(dev->regmap[2], addr, bits, set ? bits : 0);
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 4d978832c448..4f049e9d8952 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -246,6 +246,11 @@ static inline int ksz_write64(struct ksz_device *dev, u32 reg, u64 value)
 	return regmap_bulk_write(dev->regmap[2], reg, val, 2);
 }
 
+static inline void ksz_cfg(struct ksz_device *dev, u32 addr, u8 bits, bool set)
+{
+	regmap_update_bits(dev->regmap[0], addr, bits, set ? bits : 0);
+}
+
 static inline void ksz_pread8(struct ksz_device *dev, int port, int offset,
 			      u8 *data)
 {
@@ -282,6 +287,14 @@ static inline void ksz_pwrite32(struct ksz_device *dev, int port, int offset,
 	ksz_write32(dev, dev->dev_ops->get_port_addr(port, offset), data);
 }
 
+static inline void ksz_port_cfg(struct ksz_device *dev, int port, int offset,
+				u8 bits, bool set)
+{
+	regmap_update_bits(dev->regmap[0],
+			   dev->dev_ops->get_port_addr(port, offset),
+			   bits, set ? bits : 0);
+}
+
 static inline void ksz_regmap_lock(void *__mtx)
 {
 	struct mutex *mtx = __mtx;
-- 
2.33.0

Re: [RFC patch net-next 2/3] net: dsa: ksz: remove duplicate ksz_cfg and ksz_port_cfg

[Vladimir Oltean]

On Wed, Apr 27, 2022 at 09:53:42PM +0530, Arun Ramadoss wrote:
> ksz8795.c and ksz9477.c has individual ksz_cfg and ksz_port_cfg

Plural (have).

> function, both are same. Hence moving it to ksz_common.c. And removed

Present tense (remove).

> the individual references.
> 

Small hint for writing commit messages. You should describe the entire
change, and walk the reviewer through the thought process.

Here, something which you are not mentioning is that the chip-local
implementation for ksz_port_cfg() that ksz8795 and ksz9477 have uses the
PORT_CTRL_ADDR() macro. Whereas the newly added generic ksz_port_cfg()
uses dev->dev_ops->get_port_addr(). The transformation is safe because
both ksz8795 and ksz9477 provide a get_port_addr() implementation.

I didn't know that, so I had to check.

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

[Arun Ramadoss]

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

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

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

[Vladimir Oltean]

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
> 

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

[Arun.Ramadoss]

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

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

[Vladimir Oltean]

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?

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

[Arun.Ramadoss]

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.