[v2, 0/4] ocelot: support PTP Ethernet frames trapping

[v2, 0/4] ocelot: support PTP Ethernet frames trapping

[Yangbo Lu]

This patch-set is to support PTP Ethernet frames trapping.
Before that, fix some issues and improve the ocelot_ace driver
for using.

---
Changes for v2:
	- Added PTP Ethernet frames trapping support patch.

Yangbo Lu (4):
  ocelot_ace: drop member port from ocelot_ace_rule structure
  ocelot_ace: fix ingress ports setting for rule
  ocelot_ace: fix action of trap
  ocelot: add VCAP IS2 rule to trap PTP Ethernet frames

 drivers/net/ethernet/mscc/ocelot.c        | 28 ++++++++++++++++++++++++++++
 drivers/net/ethernet/mscc/ocelot_ace.c    | 20 ++++++++++----------
 drivers/net/ethernet/mscc/ocelot_ace.h    |  4 ++--
 drivers/net/ethernet/mscc/ocelot_flower.c |  8 ++++----
 4 files changed, 44 insertions(+), 16 deletions(-)

-- 
2.7.4

[v2, 1/4] ocelot_ace: drop member port from ocelot_ace_rule structure

[Yangbo Lu]

The ocelot_ace_rule is not port specific. We don't need a member port
in ocelot_ace_rule structure. Drop it and use member ocelot instead.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---
Changes for v2:
	- None.
---
 drivers/net/ethernet/mscc/ocelot_ace.c    | 12 ++++++------
 drivers/net/ethernet/mscc/ocelot_ace.h    |  2 +-
 drivers/net/ethernet/mscc/ocelot_flower.c |  6 +++---
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_ace.c b/drivers/net/ethernet/mscc/ocelot_ace.c
index 39aca1a..5580a58 100644
--- a/drivers/net/ethernet/mscc/ocelot_ace.c
+++ b/drivers/net/ethernet/mscc/ocelot_ace.c
@@ -576,7 +576,7 @@ static void is2_entry_set(struct ocelot *ocelot, int ix,
 
 static void is2_entry_get(struct ocelot_ace_rule *rule, int ix)
 {
-	struct ocelot *op = rule->port->ocelot;
+	struct ocelot *op = rule->ocelot;
 	struct vcap_data data;
 	int row = (ix / 2);
 	u32 cnt;
@@ -655,11 +655,11 @@ int ocelot_ace_rule_offload_add(struct ocelot_ace_rule *rule)
 	/* Move down the rules to make place for the new rule */
 	for (i = acl_block->count - 1; i > index; i--) {
 		ace = ocelot_ace_rule_get_rule_index(acl_block, i);
-		is2_entry_set(rule->port->ocelot, i, ace);
+		is2_entry_set(rule->ocelot, i, ace);
 	}
 
 	/* Now insert the new rule */
-	is2_entry_set(rule->port->ocelot, index, rule);
+	is2_entry_set(rule->ocelot, index, rule);
 	return 0;
 }
 
@@ -697,11 +697,11 @@ int ocelot_ace_rule_offload_del(struct ocelot_ace_rule *rule)
 	/* Move up all the blocks over the deleted rule */
 	for (i = index; i < acl_block->count; i++) {
 		ace = ocelot_ace_rule_get_rule_index(acl_block, i);
-		is2_entry_set(rule->port->ocelot, i, ace);
+		is2_entry_set(rule->ocelot, i, ace);
 	}
 
 	/* Now delete the last rule, because it is duplicated */
-	is2_entry_set(rule->port->ocelot, acl_block->count, &del_ace);
+	is2_entry_set(rule->ocelot, acl_block->count, &del_ace);
 
 	return 0;
 }
@@ -717,7 +717,7 @@ int ocelot_ace_rule_stats_update(struct ocelot_ace_rule *rule)
 	/* After we get the result we need to clear the counters */
 	tmp = ocelot_ace_rule_get_rule_index(acl_block, index);
 	tmp->stats.pkts = 0;
-	is2_entry_set(rule->port->ocelot, index, tmp);
+	is2_entry_set(rule->ocelot, index, tmp);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/mscc/ocelot_ace.h b/drivers/net/ethernet/mscc/ocelot_ace.h
index e98944c..ce72f02 100644
--- a/drivers/net/ethernet/mscc/ocelot_ace.h
+++ b/drivers/net/ethernet/mscc/ocelot_ace.h
@@ -186,7 +186,7 @@ struct ocelot_ace_stats {
 
 struct ocelot_ace_rule {
 	struct list_head list;
-	struct ocelot_port *port;
+	struct ocelot *ocelot;
 
 	u16 prio;
 	u32 id;
diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
index 59487d4..7c60e8c 100644
--- a/drivers/net/ethernet/mscc/ocelot_flower.c
+++ b/drivers/net/ethernet/mscc/ocelot_flower.c
@@ -183,7 +183,7 @@ struct ocelot_ace_rule *ocelot_ace_rule_create(struct flow_cls_offload *f,
 	if (!rule)
 		return NULL;
 
-	rule->port = block->port;
+	rule->ocelot = block->port->ocelot;
 	rule->chip_port = block->port->chip_port;
 	return rule;
 }
@@ -219,7 +219,7 @@ static int ocelot_flower_destroy(struct flow_cls_offload *f,
 	int ret;
 
 	rule.prio = get_prio(f->common.prio);
-	rule.port = port_block->port;
+	rule.ocelot = port_block->port->ocelot;
 	rule.id = f->cookie;
 
 	ret = ocelot_ace_rule_offload_del(&rule);
@@ -237,7 +237,7 @@ static int ocelot_flower_stats_update(struct flow_cls_offload *f,
 	int ret;
 
 	rule.prio = get_prio(f->common.prio);
-	rule.port = port_block->port;
+	rule.ocelot = port_block->port->ocelot;
 	rule.id = f->cookie;
 	ret = ocelot_ace_rule_stats_update(&rule);
 	if (ret)
-- 
2.7.4

[v2, 2/4] ocelot_ace: fix ingress ports setting for rule

[Yangbo Lu]

The ingress ports setting of rule should support covering all ports.
This patch is to use u16 ingress_port for ingress port mask setting
for ace rule. One bit corresponds one port.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---
Changes for v2:
	- None.
---
 drivers/net/ethernet/mscc/ocelot_ace.c    | 2 +-
 drivers/net/ethernet/mscc/ocelot_ace.h    | 2 +-
 drivers/net/ethernet/mscc/ocelot_flower.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_ace.c b/drivers/net/ethernet/mscc/ocelot_ace.c
index 5580a58..91250f3 100644
--- a/drivers/net/ethernet/mscc/ocelot_ace.c
+++ b/drivers/net/ethernet/mscc/ocelot_ace.c
@@ -352,7 +352,7 @@ static void is2_entry_set(struct ocelot *ocelot, int ix,
 	data.type = IS2_ACTION_TYPE_NORMAL;
 
 	VCAP_KEY_ANY_SET(PAG);
-	VCAP_KEY_SET(IGR_PORT_MASK, 0, ~BIT(ace->chip_port));
+	VCAP_KEY_SET(IGR_PORT_MASK, 0, ~ace->ingress_port);
 	VCAP_KEY_BIT_SET(FIRST, OCELOT_VCAP_BIT_1);
 	VCAP_KEY_BIT_SET(HOST_MATCH, OCELOT_VCAP_BIT_ANY);
 	VCAP_KEY_BIT_SET(L2_MC, ace->dmac_mc);
diff --git a/drivers/net/ethernet/mscc/ocelot_ace.h b/drivers/net/ethernet/mscc/ocelot_ace.h
index ce72f02..0fe23e0 100644
--- a/drivers/net/ethernet/mscc/ocelot_ace.h
+++ b/drivers/net/ethernet/mscc/ocelot_ace.h
@@ -193,7 +193,7 @@ struct ocelot_ace_rule {
 
 	enum ocelot_ace_action action;
 	struct ocelot_ace_stats stats;
-	int chip_port;
+	u16 ingress_port;
 
 	enum ocelot_vcap_bit dmac_mc;
 	enum ocelot_vcap_bit dmac_bc;
diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c
index 7c60e8c..bfddc50 100644
--- a/drivers/net/ethernet/mscc/ocelot_flower.c
+++ b/drivers/net/ethernet/mscc/ocelot_flower.c
@@ -184,7 +184,7 @@ struct ocelot_ace_rule *ocelot_ace_rule_create(struct flow_cls_offload *f,
 		return NULL;
 
 	rule->ocelot = block->port->ocelot;
-	rule->chip_port = block->port->chip_port;
+	rule->ingress_port = BIT(block->port->chip_port);
 	return rule;
 }
 
-- 
2.7.4

[v2, 3/4] ocelot_ace: fix action of trap

[Yangbo Lu]

The trap action should be copying the frame to CPU and
dropping it for forwarding, but current setting was just
copying frame to CPU.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---
Changes for v2:
	- None.
---
 drivers/net/ethernet/mscc/ocelot_ace.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot_ace.c b/drivers/net/ethernet/mscc/ocelot_ace.c
index 91250f3..59ad590 100644
--- a/drivers/net/ethernet/mscc/ocelot_ace.c
+++ b/drivers/net/ethernet/mscc/ocelot_ace.c
@@ -317,9 +317,9 @@ static void is2_action_set(struct vcap_data *data,
 		break;
 	case OCELOT_ACL_ACTION_TRAP:
 		VCAP_ACT_SET(PORT_MASK, 0x0);
-		VCAP_ACT_SET(MASK_MODE, 0x0);
-		VCAP_ACT_SET(POLICE_ENA, 0x0);
-		VCAP_ACT_SET(POLICE_IDX, 0x0);
+		VCAP_ACT_SET(MASK_MODE, 0x1);
+		VCAP_ACT_SET(POLICE_ENA, 0x1);
+		VCAP_ACT_SET(POLICE_IDX, OCELOT_POLICER_DISCARD);
 		VCAP_ACT_SET(CPU_QU_NUM, 0x0);
 		VCAP_ACT_SET(CPU_COPY_ENA, 0x1);
 		break;
-- 
2.7.4

[v2, 4/4] ocelot: add VCAP IS2 rule to trap PTP Ethernet frames

[Yangbo Lu]

All the PTP messages over Ethernet have etype 0x88f7 on them.
Use etype as the key to trap PTP messages.

Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
---
Changes for v2:
	- Added this patch.
---
 drivers/net/ethernet/mscc/ocelot.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 6932e61..40f4e0d 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -1681,6 +1681,33 @@ int ocelot_probe_port(struct ocelot *ocelot, u8 port,
 }
 EXPORT_SYMBOL(ocelot_probe_port);
 
+static int ocelot_ace_add_ptp_rule(struct ocelot *ocelot)
+{
+	struct ocelot_ace_rule *rule;
+
+	rule = kzalloc(sizeof(*rule), GFP_KERNEL);
+	if (!rule)
+		return -ENOMEM;
+
+	/* Entry for PTP over Ethernet (etype 0x88f7)
+	 * Action: trap to CPU port
+	 */
+	rule->ocelot = ocelot;
+	rule->prio = 1;
+	rule->type = OCELOT_ACE_TYPE_ETYPE;
+	/* Available on all ingress port except CPU port */
+	rule->ingress_port = ~BIT(ocelot->num_phys_ports);
+	rule->dmac_mc = OCELOT_VCAP_BIT_1;
+	rule->frame.etype.etype.value[0] = 0x88;
+	rule->frame.etype.etype.value[1] = 0xf7;
+	rule->frame.etype.etype.mask[0] = 0xff;
+	rule->frame.etype.etype.mask[1] = 0xff;
+	rule->action = OCELOT_ACL_ACTION_TRAP;
+
+	ocelot_ace_rule_offload_add(rule);
+	return 0;
+}
+
 int ocelot_init(struct ocelot *ocelot)
 {
 	u32 port;
@@ -1708,6 +1735,7 @@ int ocelot_init(struct ocelot *ocelot)
 	ocelot_mact_init(ocelot);
 	ocelot_vlan_init(ocelot);
 	ocelot_ace_init(ocelot);
+	ocelot_ace_add_ptp_rule(ocelot);
 
 	for (port = 0; port < ocelot->num_phys_ports; port++) {
 		/* Clear all counters (5 groups) */
-- 
2.7.4

Re: [v2, 3/4] ocelot_ace: fix action of trap

[Allan W . Nielsen]

The 08/13/2019 10:52, Yangbo Lu wrote:
> The trap action should be copying the frame to CPU and
> dropping it for forwarding, but current setting was just
> copying frame to CPU.
> 
> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> ---
> Changes for v2:
> 	- None.
> ---
>  drivers/net/ethernet/mscc/ocelot_ace.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot_ace.c b/drivers/net/ethernet/mscc/ocelot_ace.c
> index 91250f3..59ad590 100644
> --- a/drivers/net/ethernet/mscc/ocelot_ace.c
> +++ b/drivers/net/ethernet/mscc/ocelot_ace.c
> @@ -317,9 +317,9 @@ static void is2_action_set(struct vcap_data *data,
>  		break;
>  	case OCELOT_ACL_ACTION_TRAP:
>  		VCAP_ACT_SET(PORT_MASK, 0x0);
> -		VCAP_ACT_SET(MASK_MODE, 0x0);
> -		VCAP_ACT_SET(POLICE_ENA, 0x0);
> -		VCAP_ACT_SET(POLICE_IDX, 0x0);
> +		VCAP_ACT_SET(MASK_MODE, 0x1);
> +		VCAP_ACT_SET(POLICE_ENA, 0x1);
> +		VCAP_ACT_SET(POLICE_IDX, OCELOT_POLICER_DISCARD);
>  		VCAP_ACT_SET(CPU_QU_NUM, 0x0);
>  		VCAP_ACT_SET(CPU_COPY_ENA, 0x1);
>  		break;

This is still wrong, please see the comments provided the first time you
submitted this.

/Allan

Re: [v2, 4/4] ocelot: add VCAP IS2 rule to trap PTP Ethernet frames

[Allan W . Nielsen]

The 08/13/2019 10:52, Yangbo Lu wrote:
> All the PTP messages over Ethernet have etype 0x88f7 on them.
> Use etype as the key to trap PTP messages.
> 
> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> ---
> Changes for v2:
> 	- Added this patch.
> ---
>  drivers/net/ethernet/mscc/ocelot.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 6932e61..40f4e0d 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -1681,6 +1681,33 @@ int ocelot_probe_port(struct ocelot *ocelot, u8 port,
>  }
>  EXPORT_SYMBOL(ocelot_probe_port);
>  
> +static int ocelot_ace_add_ptp_rule(struct ocelot *ocelot)
> +{
> +	struct ocelot_ace_rule *rule;
> +
> +	rule = kzalloc(sizeof(*rule), GFP_KERNEL);
> +	if (!rule)
> +		return -ENOMEM;
> +
> +	/* Entry for PTP over Ethernet (etype 0x88f7)
> +	 * Action: trap to CPU port
> +	 */
> +	rule->ocelot = ocelot;
> +	rule->prio = 1;
> +	rule->type = OCELOT_ACE_TYPE_ETYPE;
> +	/* Available on all ingress port except CPU port */
> +	rule->ingress_port = ~BIT(ocelot->num_phys_ports);
> +	rule->dmac_mc = OCELOT_VCAP_BIT_1;
> +	rule->frame.etype.etype.value[0] = 0x88;
> +	rule->frame.etype.etype.value[1] = 0xf7;
> +	rule->frame.etype.etype.mask[0] = 0xff;
> +	rule->frame.etype.etype.mask[1] = 0xff;
> +	rule->action = OCELOT_ACL_ACTION_TRAP;
> +
> +	ocelot_ace_rule_offload_add(rule);
> +	return 0;
> +}
> +
>  int ocelot_init(struct ocelot *ocelot)
>  {
>  	u32 port;
> @@ -1708,6 +1735,7 @@ int ocelot_init(struct ocelot *ocelot)
>  	ocelot_mact_init(ocelot);
>  	ocelot_vlan_init(ocelot);
>  	ocelot_ace_init(ocelot);
> +	ocelot_ace_add_ptp_rule(ocelot);
>  
>  	for (port = 0; port < ocelot->num_phys_ports; port++) {
>  		/* Clear all counters (5 groups) */
This seems really wrong to me, and much too hard-coded...

What if I want to forward the PTP frames to be forwarded like a normal non-aware
PTP switch?

What if do not want this on all ports?

If you do not have an application behind this implementing a boundary or
transparent clock, then you are breaking PTP on the network.

/Allan

Re: [v2, 3/4] ocelot_ace: fix action of trap

[Allan W . Nielsen]

The 08/13/2019 08:16, Allan W . Nielsen wrote:
> The 08/13/2019 10:52, Yangbo Lu wrote:
> > The trap action should be copying the frame to CPU and
> > dropping it for forwarding, but current setting was just
> > copying frame to CPU.
> > 
> > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > ---
> > Changes for v2:
> > 	- None.
> > ---
> >  drivers/net/ethernet/mscc/ocelot_ace.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/mscc/ocelot_ace.c b/drivers/net/ethernet/mscc/ocelot_ace.c
> > index 91250f3..59ad590 100644
> > --- a/drivers/net/ethernet/mscc/ocelot_ace.c
> > +++ b/drivers/net/ethernet/mscc/ocelot_ace.c
> > @@ -317,9 +317,9 @@ static void is2_action_set(struct vcap_data *data,
> >  		break;
> >  	case OCELOT_ACL_ACTION_TRAP:
> >  		VCAP_ACT_SET(PORT_MASK, 0x0);
> > -		VCAP_ACT_SET(MASK_MODE, 0x0);
> > -		VCAP_ACT_SET(POLICE_ENA, 0x0);
> > -		VCAP_ACT_SET(POLICE_IDX, 0x0);
> > +		VCAP_ACT_SET(MASK_MODE, 0x1);
> > +		VCAP_ACT_SET(POLICE_ENA, 0x1);
> > +		VCAP_ACT_SET(POLICE_IDX, OCELOT_POLICER_DISCARD);
> >  		VCAP_ACT_SET(CPU_QU_NUM, 0x0);
> >  		VCAP_ACT_SET(CPU_COPY_ENA, 0x1);
> >  		break;
> 
> This is still wrong, please see the comments provided the first time you
> submitted this.
> 
> /Allan

I believe this will make it work - but I have not tested it:

 	case OCELOT_ACL_ACTION_TRAP:
 		VCAP_ACT_SET(PORT_MASK, 0x0);
-		VCAP_ACT_SET(MASK_MODE, 0x0);
+		VCAP_ACT_SET(MASK_MODE, 0x1);
 		VCAP_ACT_SET(CPU_QU_NUM, 0x0);
 		VCAP_ACT_SET(CPU_COPY_ENA, 0x1);
 		break;

-- 
/Allan

RE: [v2, 3/4] ocelot_ace: fix action of trap

[Y.b. Lu]

Hi Allan,

> -----Original Message-----
> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> Behalf Of Allan W . Nielsen
> Sent: Tuesday, August 13, 2019 2:30 PM
> To: Y.b. Lu <yangbo.lu@nxp.com>
> Cc: netdev@vger.kernel.org; David S . Miller <davem@davemloft.net>;
> Alexandre Belloni <alexandre.belloni@bootlin.com>; Microchip Linux Driver
> Support <UNGLinuxDriver@microchip.com>
> Subject: Re: [v2, 3/4] ocelot_ace: fix action of trap
> 
> The 08/13/2019 08:16, Allan W . Nielsen wrote:
> > The 08/13/2019 10:52, Yangbo Lu wrote:
> > > The trap action should be copying the frame to CPU and dropping it
> > > for forwarding, but current setting was just copying frame to CPU.
> > >
> > > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > > ---
> > > Changes for v2:
> > > 	- None.
> > > ---
> > >  drivers/net/ethernet/mscc/ocelot_ace.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/mscc/ocelot_ace.c
> > > b/drivers/net/ethernet/mscc/ocelot_ace.c
> > > index 91250f3..59ad590 100644
> > > --- a/drivers/net/ethernet/mscc/ocelot_ace.c
> > > +++ b/drivers/net/ethernet/mscc/ocelot_ace.c
> > > @@ -317,9 +317,9 @@ static void is2_action_set(struct vcap_data *data,
> > >  		break;
> > >  	case OCELOT_ACL_ACTION_TRAP:
> > >  		VCAP_ACT_SET(PORT_MASK, 0x0);
> > > -		VCAP_ACT_SET(MASK_MODE, 0x0);
> > > -		VCAP_ACT_SET(POLICE_ENA, 0x0);
> > > -		VCAP_ACT_SET(POLICE_IDX, 0x0);
> > > +		VCAP_ACT_SET(MASK_MODE, 0x1);
> > > +		VCAP_ACT_SET(POLICE_ENA, 0x1);
> > > +		VCAP_ACT_SET(POLICE_IDX, OCELOT_POLICER_DISCARD);
> > >  		VCAP_ACT_SET(CPU_QU_NUM, 0x0);
> > >  		VCAP_ACT_SET(CPU_COPY_ENA, 0x1);
> > >  		break;
> >
> > This is still wrong, please see the comments provided the first time
> > you submitted this.
> >
> > /Allan
> 
> I believe this will make it work - but I have not tested it:
> 
>  	case OCELOT_ACL_ACTION_TRAP:
>  		VCAP_ACT_SET(PORT_MASK, 0x0);
> -		VCAP_ACT_SET(MASK_MODE, 0x0);
> +		VCAP_ACT_SET(MASK_MODE, 0x1);
>  		VCAP_ACT_SET(CPU_QU_NUM, 0x0);
>  		VCAP_ACT_SET(CPU_COPY_ENA, 0x1);
>  		break;
> 

[Y.b. Lu] I will have a try. It seems more proper.
Thanks.

> --
> /Allan

RE: [v2, 4/4] ocelot: add VCAP IS2 rule to trap PTP Ethernet frames

[Y.b. Lu]

Hi Allan,

> -----Original Message-----
> From: Allan W . Nielsen <allan.nielsen@microchip.com>
> Sent: Tuesday, August 13, 2019 2:25 PM
> To: Y.b. Lu <yangbo.lu@nxp.com>
> Cc: netdev@vger.kernel.org; David S . Miller <davem@davemloft.net>;
> Alexandre Belloni <alexandre.belloni@bootlin.com>; Microchip Linux Driver
> Support <UNGLinuxDriver@microchip.com>
> Subject: Re: [v2, 4/4] ocelot: add VCAP IS2 rule to trap PTP Ethernet frames
> 
> The 08/13/2019 10:52, Yangbo Lu wrote:
> > All the PTP messages over Ethernet have etype 0x88f7 on them.
> > Use etype as the key to trap PTP messages.
> >
> > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > ---
> > Changes for v2:
> > 	- Added this patch.
> > ---
> >  drivers/net/ethernet/mscc/ocelot.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/mscc/ocelot.c
> > b/drivers/net/ethernet/mscc/ocelot.c
> > index 6932e61..40f4e0d 100644
> > --- a/drivers/net/ethernet/mscc/ocelot.c
> > +++ b/drivers/net/ethernet/mscc/ocelot.c
> > @@ -1681,6 +1681,33 @@ int ocelot_probe_port(struct ocelot *ocelot, u8
> > port,  }  EXPORT_SYMBOL(ocelot_probe_port);
> >
> > +static int ocelot_ace_add_ptp_rule(struct ocelot *ocelot) {
> > +	struct ocelot_ace_rule *rule;
> > +
> > +	rule = kzalloc(sizeof(*rule), GFP_KERNEL);
> > +	if (!rule)
> > +		return -ENOMEM;
> > +
> > +	/* Entry for PTP over Ethernet (etype 0x88f7)
> > +	 * Action: trap to CPU port
> > +	 */
> > +	rule->ocelot = ocelot;
> > +	rule->prio = 1;
> > +	rule->type = OCELOT_ACE_TYPE_ETYPE;
> > +	/* Available on all ingress port except CPU port */
> > +	rule->ingress_port = ~BIT(ocelot->num_phys_ports);
> > +	rule->dmac_mc = OCELOT_VCAP_BIT_1;
> > +	rule->frame.etype.etype.value[0] = 0x88;
> > +	rule->frame.etype.etype.value[1] = 0xf7;
> > +	rule->frame.etype.etype.mask[0] = 0xff;
> > +	rule->frame.etype.etype.mask[1] = 0xff;
> > +	rule->action = OCELOT_ACL_ACTION_TRAP;
> > +
> > +	ocelot_ace_rule_offload_add(rule);
> > +	return 0;
> > +}
> > +
> >  int ocelot_init(struct ocelot *ocelot)  {
> >  	u32 port;
> > @@ -1708,6 +1735,7 @@ int ocelot_init(struct ocelot *ocelot)
> >  	ocelot_mact_init(ocelot);
> >  	ocelot_vlan_init(ocelot);
> >  	ocelot_ace_init(ocelot);
> > +	ocelot_ace_add_ptp_rule(ocelot);
> >
> >  	for (port = 0; port < ocelot->num_phys_ports; port++) {
> >  		/* Clear all counters (5 groups) */
> This seems really wrong to me, and much too hard-coded...
> 
> What if I want to forward the PTP frames to be forwarded like a normal
> non-aware PTP switch?

[Y.b. Lu] As Andrew said, other switches could identify PTP messages and forward to CPU for processing.
https://patchwork.ozlabs.org/patch/1145627/

I'm also wondering whether there is common method in linux to address your questions.
If no, I think trapping all PTP messages on all ports to CPU could be used for now.
If users require PTP synchronization, they actually don’t want a non-aware PTP switch.

I once see other ocelot code configure ptp trap rules in ioctl timestamping setting. But I don’t think it's proper either.
Enable timestamping doesn’t mean we want to trap PTP messages.

> 
> What if do not want this on all ports?

[Y.b. Lu] Actually I don’t think there should be difference of handling PTP messages on each port.
You don’t need to run PTP protocol application on the specific port if you don’t want.

> 
> If you do not have an application behind this implementing a boundary or
> transparent clock, then you are breaking PTP on the network.

[Y.b. Lu] You're right. But actually for PTP network, all PTP devices should run PTP protocol on it.
Of course, it's better to have a way to configure it as non-aware PTP switch.

> 
> /Allan

Re: [v2, 4/4] ocelot: add VCAP IS2 rule to trap PTP Ethernet frames

[Allan W . Nielsen]

The 08/14/2019 04:56, Y.b. Lu wrote:
> > -----Original Message-----
> > From: Allan W . Nielsen <allan.nielsen@microchip.com>
> > Sent: Tuesday, August 13, 2019 2:25 PM
> > To: Y.b. Lu <yangbo.lu@nxp.com>
> > Cc: netdev@vger.kernel.org; David S . Miller <davem@davemloft.net>;
> > Alexandre Belloni <alexandre.belloni@bootlin.com>; Microchip Linux Driver
> > Support <UNGLinuxDriver@microchip.com>
> > Subject: Re: [v2, 4/4] ocelot: add VCAP IS2 rule to trap PTP Ethernet frames
> > 
> > The 08/13/2019 10:52, Yangbo Lu wrote:
> > > All the PTP messages over Ethernet have etype 0x88f7 on them.
> > > Use etype as the key to trap PTP messages.
> > >
> > > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > > ---
> > > Changes for v2:
> > > 	- Added this patch.
> > > ---
> > >  drivers/net/ethernet/mscc/ocelot.c | 28 ++++++++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > >
> > > diff --git a/drivers/net/ethernet/mscc/ocelot.c
> > > b/drivers/net/ethernet/mscc/ocelot.c
> > > index 6932e61..40f4e0d 100644
> > > --- a/drivers/net/ethernet/mscc/ocelot.c
> > > +++ b/drivers/net/ethernet/mscc/ocelot.c
> > > @@ -1681,6 +1681,33 @@ int ocelot_probe_port(struct ocelot *ocelot, u8
> > > port,  }  EXPORT_SYMBOL(ocelot_probe_port);
> > >
> > > +static int ocelot_ace_add_ptp_rule(struct ocelot *ocelot) {
> > > +	struct ocelot_ace_rule *rule;
> > > +
> > > +	rule = kzalloc(sizeof(*rule), GFP_KERNEL);
> > > +	if (!rule)
> > > +		return -ENOMEM;
> > > +
> > > +	/* Entry for PTP over Ethernet (etype 0x88f7)
> > > +	 * Action: trap to CPU port
> > > +	 */
> > > +	rule->ocelot = ocelot;
> > > +	rule->prio = 1;
> > > +	rule->type = OCELOT_ACE_TYPE_ETYPE;
> > > +	/* Available on all ingress port except CPU port */
> > > +	rule->ingress_port = ~BIT(ocelot->num_phys_ports);
> > > +	rule->dmac_mc = OCELOT_VCAP_BIT_1;
> > > +	rule->frame.etype.etype.value[0] = 0x88;
> > > +	rule->frame.etype.etype.value[1] = 0xf7;
> > > +	rule->frame.etype.etype.mask[0] = 0xff;
> > > +	rule->frame.etype.etype.mask[1] = 0xff;
> > > +	rule->action = OCELOT_ACL_ACTION_TRAP;
> > > +
> > > +	ocelot_ace_rule_offload_add(rule);
> > > +	return 0;
> > > +}
> > > +
> > >  int ocelot_init(struct ocelot *ocelot)  {
> > >  	u32 port;
> > > @@ -1708,6 +1735,7 @@ int ocelot_init(struct ocelot *ocelot)
> > >  	ocelot_mact_init(ocelot);
> > >  	ocelot_vlan_init(ocelot);
> > >  	ocelot_ace_init(ocelot);
> > > +	ocelot_ace_add_ptp_rule(ocelot);
> > >
> > >  	for (port = 0; port < ocelot->num_phys_ports; port++) {
> > >  		/* Clear all counters (5 groups) */
> > This seems really wrong to me, and much too hard-coded...
> > 
> > What if I want to forward the PTP frames to be forwarded like a normal
> > non-aware PTP switch?
> 
> [Y.b. Lu] As Andrew said, other switches could identify PTP messages and forward to CPU for processing.
> https://patchwork.ozlabs.org/patch/1145627/
Yes, it would be good to see some exampels to understand this better.

> I'm also wondering whether there is common method in linux to address your questions.
Me too.

> If no, I think trapping all PTP messages on all ports to CPU could be used for now.
> If users require PTP synchronization, they actually don’t want a non-aware PTP switch.
Can we continue this discussion in the other thread where I listed the 3
scenarios?

> I once see other ocelot code configure ptp trap rules in ioctl timestamping
> setting. But I don’t think it's proper either.  Enable timestamping doesn’t
> mean we want to trap PTP messages.
Where did you see this?

The effort in [1] is just about the time-stamping and does not really consider
the bridge part of it, and it should not be installing any TCAM rules (I believe
it did in earlier versions, but this has been changed).

[1] https://patchwork.ozlabs.org/patch/1145777/

> > What if do not want this on all ports?
> [Y.b. Lu] Actually I don’t think there should be difference of handling PTP messages on each port.
> You don’t need to run PTP protocol application on the specific port if you don’t want.
What if you want some vlans or some ports to be PTP unaware, and other to be PTP
aware.

> > If you do not have an application behind this implementing a boundary or
> > transparent clock, then you are breaking PTP on the network.
> [Y.b. Lu] You're right. But actually for PTP network, all PTP devices should run PTP protocol on it.
> Of course, it's better to have a way to configure it as non-aware PTP switch.
I think we agree.

In my point of view, it is the PTP daemon who should configure frames to be
trapped. Then the switch will be PTP unaware until the PTP daemon starts up and
is ready to make it aware.

If we put it in the init function, then it will be of PTP broken until the PTP
daemon starts.

/Allan

RE: [v2, 4/4] ocelot: add VCAP IS2 rule to trap PTP Ethernet frames

[Y.b. Lu]

Hi Allan,

> -----Original Message-----
> From: Allan W . Nielsen <allan.nielsen@microchip.com>
> Sent: Wednesday, August 14, 2019 5:17 PM
> To: Y.b. Lu <yangbo.lu@nxp.com>
> Cc: Andrew Lunn <andrew@lunn.ch>; netdev@vger.kernel.org; David S . Miller
> <davem@davemloft.net>; Alexandre Belloni <alexandre.belloni@bootlin.com>;
> Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
> Subject: Re: [v2, 4/4] ocelot: add VCAP IS2 rule to trap PTP Ethernet frames
> 
> The 08/14/2019 04:56, Y.b. Lu wrote:
> > > -----Original Message-----
> > > From: Allan W . Nielsen <allan.nielsen@microchip.com>
> > > Sent: Tuesday, August 13, 2019 2:25 PM
> > > To: Y.b. Lu <yangbo.lu@nxp.com>
> > > Cc: netdev@vger.kernel.org; David S . Miller <davem@davemloft.net>;
> > > Alexandre Belloni <alexandre.belloni@bootlin.com>; Microchip Linux
> > > Driver Support <UNGLinuxDriver@microchip.com>
> > > Subject: Re: [v2, 4/4] ocelot: add VCAP IS2 rule to trap PTP
> > > Ethernet frames
> > >
> > > The 08/13/2019 10:52, Yangbo Lu wrote:
> > > > All the PTP messages over Ethernet have etype 0x88f7 on them.
> > > > Use etype as the key to trap PTP messages.
> > > >
> > > > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
[...]
> Can we continue this discussion in the other thread where I listed the 3
> scenarios?

[Y.b. Lu] Sure. Let's discuss in that thread.
https://patchwork.ozlabs.org/patch/1145627/

[...]
> > > What if do not want this on all ports?
> > [Y.b. Lu] Actually I don’t think there should be difference of handling PTP
> messages on each port.
> > You don’t need to run PTP protocol application on the specific port if you
> don’t want.
> What if you want some vlans or some ports to be PTP unaware, and other to
> be PTP aware.

[Y.b. Lu] Actually I couldn’t find reasons why make some ports PTP unaware, if there is software stack for PTP aware...

> 
> > > If you do not have an application behind this implementing a
> > > boundary or transparent clock, then you are breaking PTP on the network.
> > [Y.b. Lu] You're right. But actually for PTP network, all PTP devices should run
> PTP protocol on it.
> > Of course, it's better to have a way to configure it as non-aware PTP switch.
> I think we agree.
> 
> In my point of view, it is the PTP daemon who should configure frames to be
> trapped. Then the switch will be PTP unaware until the PTP daemon starts up
> and is ready to make it aware.
> 
> If we put it in the init function, then it will be of PTP broken until the PTP
> daemon starts.
> 
> /Allan

Re: [v2, 4/4] ocelot: add VCAP IS2 rule to trap PTP Ethernet frames

[Andrew Lunn]

> [Y.b. Lu] Actually I couldn’t find reasons why make some ports PTP
> unaware, if there is software stack for PTP aware...

Maybe because i have not yet done

apt-get install linuxptp
$EDITOR /etc/defaults/ptp4l
systemctl restart ptp4l

Just because it exists does not mean it is installed.

     Andrew

RE: [v2, 3/4] ocelot_ace: fix action of trap

[Y.b. Lu]

Hi,

> -----Original Message-----
> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> Behalf Of Allan W . Nielsen
> Sent: Tuesday, August 13, 2019 2:30 PM
> To: Y.b. Lu <yangbo.lu@nxp.com>
> Cc: netdev@vger.kernel.org; David S . Miller <davem@davemloft.net>;
> Alexandre Belloni <alexandre.belloni@bootlin.com>; Microchip Linux Driver
> Support <UNGLinuxDriver@microchip.com>
> Subject: Re: [v2, 3/4] ocelot_ace: fix action of trap
> 
> The 08/13/2019 08:16, Allan W . Nielsen wrote:
> > The 08/13/2019 10:52, Yangbo Lu wrote:
> > > The trap action should be copying the frame to CPU and dropping it
> > > for forwarding, but current setting was just copying frame to CPU.
> > >
> > > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > > ---
> > > Changes for v2:
> > > 	- None.
> > > ---
> > >  drivers/net/ethernet/mscc/ocelot_ace.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/mscc/ocelot_ace.c
> > > b/drivers/net/ethernet/mscc/ocelot_ace.c
> > > index 91250f3..59ad590 100644
> > > --- a/drivers/net/ethernet/mscc/ocelot_ace.c
> > > +++ b/drivers/net/ethernet/mscc/ocelot_ace.c
> > > @@ -317,9 +317,9 @@ static void is2_action_set(struct vcap_data *data,
> > >  		break;
> > >  	case OCELOT_ACL_ACTION_TRAP:
> > >  		VCAP_ACT_SET(PORT_MASK, 0x0);
> > > -		VCAP_ACT_SET(MASK_MODE, 0x0);
> > > -		VCAP_ACT_SET(POLICE_ENA, 0x0);
> > > -		VCAP_ACT_SET(POLICE_IDX, 0x0);
> > > +		VCAP_ACT_SET(MASK_MODE, 0x1);
> > > +		VCAP_ACT_SET(POLICE_ENA, 0x1);
> > > +		VCAP_ACT_SET(POLICE_IDX, OCELOT_POLICER_DISCARD);
> > >  		VCAP_ACT_SET(CPU_QU_NUM, 0x0);
> > >  		VCAP_ACT_SET(CPU_COPY_ENA, 0x1);
> > >  		break;
> >
> > This is still wrong, please see the comments provided the first time
> > you submitted this.
> >
> > /Allan
> 
> I believe this will make it work - but I have not tested it:
> 
>  	case OCELOT_ACL_ACTION_TRAP:
>  		VCAP_ACT_SET(PORT_MASK, 0x0);
> -		VCAP_ACT_SET(MASK_MODE, 0x0);
> +		VCAP_ACT_SET(MASK_MODE, 0x1);
>  		VCAP_ACT_SET(CPU_QU_NUM, 0x0);
>  		VCAP_ACT_SET(CPU_COPY_ENA, 0x1);
>  		break;
> 

[Y.b. Lu] It makes sense. And it worked.
I have sent out v3 which only included this one patch. I'd like to send the other patches once Felix driver is accepted, but I'd like to collect the suggestions :)
Thanks.

Sorry, I missed to add change logs for v3 patch...
https://patchwork.ozlabs.org/patch/1149770/


> --
> /Allan