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