[PATCH net-next] net: dsa: mv88e6xxx: Add support for port mirroring

[PATCH net-next] net: dsa: mv88e6xxx: Add support for port mirroring

[Iwan R Timmer]

Add support for configuring port mirroring through the cls_matchall
classifier. We do a full ingress and/or egress capture towards the
capture port, configured with set_egress_port.

Signed-off-by: Iwan R Timmer <irtimmer@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 36 ++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/port.c | 21 +++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/port.h |  2 ++
 3 files changed, 59 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index d0a97eb73a37..3a48f9d07cbf 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -4595,6 +4595,40 @@ static int mv88e6xxx_port_mdb_del(struct dsa_switch *ds, int port,
 	return err;
 }
 
+static int mv88e6xxx_port_mirror_add(struct dsa_switch *ds, int port,
+				     struct dsa_mall_mirror_tc_entry *mirror,
+				     bool ingress)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	int err = -EOPNOTSUPP;
+
+	mutex_lock(&chip->reg_lock);
+	if (chip->info->ops->set_egress_port)
+		err = chip->info->ops->set_egress_port(chip,
+						       mirror->to_local_port);
+
+	if (err)
+		goto out;
+
+	err = mv88e6xxx_port_set_mirror(chip, port, true, ingress);
+out:
+	mutex_unlock(&chip->reg_lock);
+
+	return err;
+}
+
+static void mv88e6xxx_port_mirror_del(struct dsa_switch *ds, int port,
+				      struct dsa_mall_mirror_tc_entry *mirror)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+
+	mutex_lock(&chip->reg_lock);
+	if (mv88e6xxx_port_set_mirror(chip, port, false, false))
+		dev_err(ds->dev, "p%d: failed to disable mirroring\n", port);
+
+	mutex_unlock(&chip->reg_lock);
+}
+
 static int mv88e6xxx_port_egress_floods(struct dsa_switch *ds, int port,
 					 bool unicast, bool multicast)
 {
@@ -4647,6 +4681,8 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.port_mdb_prepare       = mv88e6xxx_port_mdb_prepare,
 	.port_mdb_add           = mv88e6xxx_port_mdb_add,
 	.port_mdb_del           = mv88e6xxx_port_mdb_del,
+	.port_mirror_add	= mv88e6xxx_port_mirror_add,
+	.port_mirror_del	= mv88e6xxx_port_mirror_del,
 	.crosschip_bridge_join	= mv88e6xxx_crosschip_bridge_join,
 	.crosschip_bridge_leave	= mv88e6xxx_crosschip_bridge_leave,
 	.port_hwtstamp_set	= mv88e6xxx_port_hwtstamp_set,
diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
index 04309ef0a1cc..301bf704c877 100644
--- a/drivers/net/dsa/mv88e6xxx/port.c
+++ b/drivers/net/dsa/mv88e6xxx/port.c
@@ -1086,6 +1086,27 @@ int mv88e6095_port_set_upstream_port(struct mv88e6xxx_chip *chip, int port,
 	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL2, reg);
 }
 
+int mv88e6xxx_port_set_mirror(struct mv88e6xxx_chip *chip, int port,
+			      bool mirror, bool ingress)
+{
+	u16 reg;
+	u16 bit;
+	int err;
+
+	err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_CTL2, &reg);
+	if (err)
+		return err;
+
+	bit = ingress ? MV88E6XXX_PORT_CTL2_INGRESS_MONITOR :
+			MV88E6XXX_PORT_CTL2_EGRESS_MONITOR;
+	reg &= ~bit;
+
+	if (mirror)
+		reg |= bit;
+
+	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_CTL2, reg);
+}
+
 int mv88e6xxx_port_set_8021q_mode(struct mv88e6xxx_chip *chip, int port,
 				  u16 mode)
 {
diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index 8d5a6cd6fb19..40ed60a2099b 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -347,6 +347,8 @@ int mv88e6352_port_link_state(struct mv88e6xxx_chip *chip, int port,
 int mv88e6xxx_port_set_map_da(struct mv88e6xxx_chip *chip, int port);
 int mv88e6095_port_set_upstream_port(struct mv88e6xxx_chip *chip, int port,
 				     int upstream_port);
+int mv88e6xxx_port_set_mirror(struct mv88e6xxx_chip *chip, int port,
+			      bool mirror, bool ingress);
 
 int mv88e6xxx_port_disable_learn_limit(struct mv88e6xxx_chip *chip, int port);
 int mv88e6xxx_port_disable_pri_override(struct mv88e6xxx_chip *chip, int port);
-- 
2.23.0

Re: [PATCH net-next] net: dsa: mv88e6xxx: Add support for port mirroring

[Andrew Lunn]

On Tue, Sep 17, 2019 at 10:23:01PM +0200, Iwan R Timmer wrote:
> Add support for configuring port mirroring through the cls_matchall
> classifier. We do a full ingress and/or egress capture towards the
> capture port, configured with set_egress_port.

Hi Iwan

This looks good as far as it goes.

Have you tried adding/deleting multiple port mirrors? Do we need to
limit how many are added. A quick look at the datasheet, you can
define one egress mirror port and one ingress mirror port. I think you
can have multiple ports mirroring ingress to that one ingress mirror
port. And you can have multiple port mirroring egress to the one
egress mirror port. We should add code to check this, and return
-EBUSY if the existing configuration prevents a new mirror being
configured.

Thanks
	Andrew

Re: [PATCH net-next] net: dsa: mv88e6xxx: Add support for port mirroring

[Iwan R Timmer]

On Tue, Sep 17, 2019 at 10:55:05PM +0200, Andrew Lunn wrote:
> On Tue, Sep 17, 2019 at 10:23:01PM +0200, Iwan R Timmer wrote:
> > Add support for configuring port mirroring through the cls_matchall
> > classifier. We do a full ingress and/or egress capture towards the
> > capture port, configured with set_egress_port.
> 
> Hi Iwan
> 
> This looks good as far as it goes.
> 
> Have you tried adding/deleting multiple port mirrors? Do we need to
> limit how many are added. A quick look at the datasheet, you can
> define one egress mirror port and one ingress mirror port. I think you
> can have multiple ports mirroring ingress to that one ingress mirror
> port. And you can have multiple port mirroring egress to the one
> egress mirror port. We should add code to check this, and return
> -EBUSY if the existing configuration prevents a new mirror being
> configured.
> 
> Thanks
> 	Andrew

Hi Andrew,

I only own a simple 5 ports switch (88E6176) which has no problem of
mirroring the other ports to a single port. Except for a bandwith
shortage ofcourse. While I thought I checked adding and removing ports,
I seemed to forgot to check removing ingress traffic as it will now
disable mirroring egress traffic. Searching for how I can distinct
ingress from egress mirroring in port_mirror_del, I saw there is a
variable in the mirror struct called ingress. Which seems strange,
because why is it a seperate argument to the port_mirror_add function?

Origally I planned to be able to set the egress and ingress mirror
seperatly. But in my laziness when I saw there already was a function
to configure the destination port this functionality was lost.

Because the other drivers which implemented the port_mirror_add (b53 and
ksz9477) also lacks additional checks to prevent new mirror filters from
breaking previous ones I assumed they were not necessary.

At least I will soon sent a new version with at least the issue of
removing mirror ingress traffic fixed and the ability to define a 
seperate ingress and egress port.

Regards,
Iwan

Re: [PATCH net-next] net: dsa: mv88e6xxx: Add support for port mirroring

[Florian Fainelli]

On 9/17/19 3:32 PM, Iwan R Timmer wrote:
> On Tue, Sep 17, 2019 at 10:55:05PM +0200, Andrew Lunn wrote:
>> On Tue, Sep 17, 2019 at 10:23:01PM +0200, Iwan R Timmer wrote:
>>> Add support for configuring port mirroring through the cls_matchall
>>> classifier. We do a full ingress and/or egress capture towards the
>>> capture port, configured with set_egress_port.
>>
>> Hi Iwan
>>
>> This looks good as far as it goes.
>>
>> Have you tried adding/deleting multiple port mirrors? Do we need to
>> limit how many are added. A quick look at the datasheet, you can
>> define one egress mirror port and one ingress mirror port. I think you
>> can have multiple ports mirroring ingress to that one ingress mirror
>> port. And you can have multiple port mirroring egress to the one
>> egress mirror port. We should add code to check this, and return
>> -EBUSY if the existing configuration prevents a new mirror being
>> configured.
>>
>> Thanks
>> 	Andrew
> 
> Hi Andrew,
> 
> I only own a simple 5 ports switch (88E6176) which has no problem of
> mirroring the other ports to a single port. Except for a bandwith
> shortage ofcourse. While I thought I checked adding and removing ports,
> I seemed to forgot to check removing ingress traffic as it will now
> disable mirroring egress traffic. Searching for how I can distinct
> ingress from egress mirroring in port_mirror_del, I saw there is a
> variable in the mirror struct called ingress. Which seems strange,
> because why is it a seperate argument to the port_mirror_add function?
> 
> Origally I planned to be able to set the egress and ingress mirror
> seperatly. But in my laziness when I saw there already was a function
> to configure the destination port this functionality was lost.
> 
> Because the other drivers which implemented the port_mirror_add (b53 and
> ksz9477) also lacks additional checks to prevent new mirror filters from
> breaking previous ones I assumed they were not necessary.

That does sound like a bug indeed, I just looked at b53_mirror_add()
again and clearing the MIRROR_MASK before setting up the new port
clearly sounds wrong, at least on ingress.

> 
> At least I will soon sent a new version with at least the issue of
> removing mirror ingress traffic fixed and the ability to define a 
> seperate ingress and egress port.
> 
> Regards,
> Iwan
> 


-- 
Florian

Re: [PATCH net-next] net: dsa: mv88e6xxx: Add support for port mirroring

[Florian Fainelli]

On 9/19/19 2:30 PM, Jason Cobham wrote:
> Hi Iwan,
> 
>> Hi Andrew,
>>
>> I only own a simple 5 ports switch (88E6176) which has no problem of mirroring the other ports to a single port. Except for a bandwith shortage ofcourse. While I thought I checked adding and removing ports, I seemed to forgot to check removing ingress traffic as it will now >disable mirroring egress traffic. Searching for how I can distinct ingress from egress mirroring in port_mirror_del, I saw there is a variable in the mirror struct called ingress. Which seems strange, because why is it a seperate argument to the port_mirror_add function?
>>
>> Origally I planned to be able to set the egress and ingress mirror seperatly. But in my laziness when I saw there already was a function to configure the destination port this functionality was lost.
>>
>> Because the other drivers which implemented the port_mirror_add (b53 and
>> ksz9477) also lacks additional checks to prevent new mirror filters from breaking previous ones I assumed they were not necessary.
>>
>> At least I will soon sent a new version with at least the issue of removing mirror ingress traffic fixed and the ability to define a seperate ingress and egress port.
>>
>> Regards,
>> Iwan
> 
> I have a similar patch set for port mirror from a few years ago. I'd
> also like to see this functionality in mainline. One issue I ran into
> is when doing port mirror in a cross-chip dsa configuration. If the
> ingress and egress ports are on different chips, the ingress chip
> needs to set the egress to the cross-chip dsa port and the cross-chip
> egress port needs to be set appropriately. I also had the
> functionality to mirror egress from a port to a destination port.
> 
> Is it appropriate to send my patch to the mailing list for review or
> should we work on this off-line?

Given that the net-next tree is closed at the moment, working offline
and posting a combined version of a patch that supports port mirroring
for cross chip configurations as well as standalone sounds good to me.

Thanks!
-- 
Florian

Re: [PATCH net-next] net: dsa: mv88e6xxx: Add support for port mirroring

[Jason Cobham]

Hi Iwan,

>Hi Andrew,
>
>I only own a simple 5 ports switch (88E6176) which has no problem of mirroring the other ports to a single port. Except for a bandwith shortage ofcourse. While I thought I checked adding and removing ports, I seemed to forgot to check removing ingress traffic as it will now >disable mirroring egress traffic. Searching for how I can distinct ingress from egress mirroring in port_mirror_del, I saw there is a variable in the mirror struct called ingress. Which seems strange, because why is it a seperate argument to the port_mirror_add function?
>
>Origally I planned to be able to set the egress and ingress mirror seperatly. But in my laziness when I saw there already was a function to configure the destination port this functionality was lost.
>
>Because the other drivers which implemented the port_mirror_add (b53 and
>ksz9477) also lacks additional checks to prevent new mirror filters from breaking previous ones I assumed they were not necessary.
>
>At least I will soon sent a new version with at least the issue of removing mirror ingress traffic fixed and the ability to define a seperate ingress and egress port.
>
>Regards,
>Iwan

I have a similar patch set for port mirror from a few years ago. I'd
also like to see this functionality in mainline. One issue I ran into
is when doing port mirror in a cross-chip dsa configuration. If the
ingress and egress ports are on different chips, the ingress chip
needs to set the egress to the cross-chip dsa port and the cross-chip
egress port needs to be set appropriately. I also had the
functionality to mirror egress from a port to a destination port.

Is it appropriate to send my patch to the mailing list for review or
should we work on this off-line?

Thanks,
Jason