Re: [PATCH v2 net-next 7/9] net: dsa: microchip: add support for port mirror operations

From: Vladimir Oltean <olteanv@gmail.com>
To: Prasanna Vengateshan <prasanna.vengateshan@microchip.com>
Cc: andrew@lunn.ch, netdev@vger.kernel.org, robh+dt@kernel.org,
	UNGLinuxDriver@microchip.com, hkallweit1@gmail.com,
	linux@armlinux.org.uk, davem@davemloft.net, kuba@kernel.org,
	linux-kernel@vger.kernel.org, vivien.didelot@gmail.com,
	f.fainelli@gmail.com, devicetree@vger.kernel.org
Subject: Re: [PATCH v2 net-next 7/9] net: dsa: microchip: add support for port mirror operations
Date: Thu, 22 Apr 2021 23:11:19 +0300	[thread overview]
Message-ID: <20210422201119.kalmeag53yun2j5x@skbuf> (raw)
In-Reply-To: <20210422094257.1641396-8-prasanna.vengateshan@microchip.com>

On Thu, Apr 22, 2021 at 03:12:55PM +0530, Prasanna Vengateshan wrote:
> Added support for port_mirror_add() and port_mirror_del operations
> 
> Signed-off-by: Prasanna Vengateshan <prasanna.vengateshan@microchip.com>
> ---
>  drivers/net/dsa/microchip/lan937x_main.c | 50 ++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/net/dsa/microchip/lan937x_main.c b/drivers/net/dsa/microchip/lan937x_main.c
> index 573d2dd906f5..bfce5098ea69 100644
> --- a/drivers/net/dsa/microchip/lan937x_main.c
> +++ b/drivers/net/dsa/microchip/lan937x_main.c
> @@ -128,6 +128,54 @@ static void lan937x_port_stp_state_set(struct dsa_switch *ds, int port,
>  	mutex_unlock(&dev->dev_mutex);
>  }
>  
> +static int lan937x_port_mirror_add(struct dsa_switch *ds, int port,
> +				   struct dsa_mall_mirror_tc_entry *mirror,
> +				   bool ingress)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	int rc;
> +
> +	if (ingress)
> +		rc = lan937x_port_cfg(dev, port, P_MIRROR_CTRL, PORT_MIRROR_RX, true);
> +	else
> +		rc = lan937x_port_cfg(dev, port, P_MIRROR_CTRL, PORT_MIRROR_TX, true);
> +
> +	if (rc < 0)
> +		return rc;
> +
> +	rc = lan937x_port_cfg(dev, port, P_MIRROR_CTRL, PORT_MIRROR_SNIFFER, false);
> +	if (rc < 0)
> +		return rc;

This is odd, you shouldn't have to say 'the port which I'm sniffing is
not a sniffer'.

> +
> +	/* configure mirror port */
> +	rc = lan937x_port_cfg(dev, mirror->to_local_port, P_MIRROR_CTRL,
> +			      PORT_MIRROR_SNIFFER, true);
> +	if (rc < 0)
> +		return rc;
> +
> +	rc = lan937x_cfg(dev, S_MIRROR_CTRL, SW_MIRROR_RX_TX, false);
> +
> +	return rc;
> +}
> +
> +static void lan937x_port_mirror_del(struct dsa_switch *ds, int port,
> +				    struct dsa_mall_mirror_tc_entry *mirror)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	u8 data;
> +
> +	if (mirror->ingress)
> +		lan937x_port_cfg(dev, port, P_MIRROR_CTRL, PORT_MIRROR_RX, false);
> +	else
> +		lan937x_port_cfg(dev, port, P_MIRROR_CTRL, PORT_MIRROR_TX, false);
> +
> +	lan937x_pread8(dev, port, P_MIRROR_CTRL, &data);
> +
> +	if (!(data & (PORT_MIRROR_RX | PORT_MIRROR_TX)))
> +		lan937x_port_cfg(dev, mirror->to_local_port, P_MIRROR_CTRL,
> +				 PORT_MIRROR_SNIFFER, false);
> +}
> +

So let me understand. You have a PORT_MIRROR_SNIFFER setting per port.
Presumably the mirrored traffic inside the switch is sent to the ports
which have PORT_MIRROR_SNIFFER = true.
But this isn't the interpretation of the tc utility.

Instead, let's say you have the following sequence of commands:

tc filter add dev lan0 ingress matchall skip_sw action mirred egress mirror dev lan1
tc filter add dev lan2 ingress matchall skip_sw action mirred egress mirror dev lan3

What in your hardware configuration makes traffic from lan0 be mirrored
to lan1 but not to lan3?

>  static phy_interface_t lan937x_get_interface(struct ksz_device *dev, int port)
>  {
>  	phy_interface_t interface;
> @@ -396,6 +444,8 @@ const struct dsa_switch_ops lan937x_switch_ops = {
>  	.port_bridge_flags	= lan937x_port_bridge_flags,
>  	.port_stp_state_set	= lan937x_port_stp_state_set,
>  	.port_fast_age		= ksz_port_fast_age,
> +	.port_mirror_add	= lan937x_port_mirror_add,
> +	.port_mirror_del	= lan937x_port_mirror_del,
>  	.port_max_mtu		= lan937x_get_max_mtu,
>  	.port_change_mtu	= lan937x_change_mtu,
>  	.phylink_validate	= lan937x_phylink_validate,
> -- 
> 2.27.0
> 