[PATCH 0/3] ocelot_ace: fix and improve the driver

[PATCH 0/3] ocelot_ace: fix and improve the driver

[Yangbo Lu]

This patch-set is to fix some issues and improve the ocelot_ace driver
for using.

Yangbo Lu (3):
  ocelot_ace: drop member port from ocelot_ace_rule structure
  ocelot_ace: fix ingress ports setting for rule
  ocelot_ace: fix action of trap

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

-- 
2.7.4

[PATCH 1/3] 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>
---
 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

[PATCH 2/3] 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>
---
 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

[PATCH 3/3] 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>
---
 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

Re: [PATCH 3/3] ocelot_ace: fix action of trap

[Allan W. Nielsen]

The 08/12/2019 18:48, 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.

Are there any actions which do a "copy-to-cpu" and still forward the frame in
HW?

> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> ---
>  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);
This seems wrong. The policer is used to ensure that traffic are discarded, even
in the case where other users of the code has requested it to go to the CPU.

Are you sure this is working? If it is working, then I fear we have an issue
with the DROP action which uses this to discard frames.

>  		VCAP_ACT_SET(CPU_QU_NUM, 0x0);
>  		VCAP_ACT_SET(CPU_COPY_ENA, 0x1);
>  		break;
> -- 
> 2.7.4

-- 
/Allan

Re: [PATCH 2/3] ocelot_ace: fix ingress ports setting for rule

[Allan W. Nielsen]

The 08/12/2019 18:48, Yangbo Lu wrote:
> 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.
That is how the HW is working, and it would be nice if we could operate on a
port masks/lists instead. But how can this be used?

Can you please explain how/when this will make a difference?

> Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> ---
>  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;
>  }

-- Allan

RE: [PATCH 3/3] ocelot_ace: fix action of trap

[Y.b. Lu]

Hi Allan,

> -----Original Message-----
> From: Allan W. Nielsen <allan.nielsen@microchip.com>
> Sent: Monday, August 12, 2019 8:32 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: [PATCH 3/3] ocelot_ace: fix action of trap
> 
> The 08/12/2019 18:48, 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.
> 
> Are there any actions which do a "copy-to-cpu" and still forward the frame in
> HW?

[Y.b. Lu] We're using Felix switch whose code hadn't been accepted by upstream.
https://patchwork.ozlabs.org/project/netdev/list/?series=115399&state=*

I'd like to trap all IEEE 1588 PTP Ethernet frames to CPU through etype 0x88f7.
When I used current TRAP option, I found the frames were not only copied to CPU, but also forwarded to other ports.
So I just made the TRAP option same with DROP option except enabling CPU_COPY_ENA in the patch.

Thanks.

> 
> > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > ---
> >  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);
> This seems wrong. The policer is used to ensure that traffic are discarded, even
> in the case where other users of the code has requested it to go to the CPU.
> 
> Are you sure this is working? If it is working, then I fear we have an issue with
> the DROP action which uses this to discard frames.
> 
> >  		VCAP_ACT_SET(CPU_QU_NUM, 0x0);
> >  		VCAP_ACT_SET(CPU_COPY_ENA, 0x1);
> >  		break;
> > --
> > 2.7.4
> 
> --
> /Allan

RE: [PATCH 2/3] ocelot_ace: fix ingress ports setting for rule

[Y.b. Lu]

Hi Allan,

> -----Original Message-----
> From: Allan W. Nielsen <allan.nielsen@microchip.com>
> Sent: Monday, August 12, 2019 8:38 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: [PATCH 2/3] ocelot_ace: fix ingress ports setting for rule
> 
> The 08/12/2019 18:48, Yangbo Lu wrote:
> > 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.
> That is how the HW is working, and it would be nice if we could operate on a
> port masks/lists instead. But how can this be used?

[Y.b. Lu] Will the changes affect anything? Current usage in ocelot_flower.c will be converted as below.

-       rule->chip_port = block->port->chip_port;
+       rule->ingress_port = BIT(block->port->chip_port);


> 
> Can you please explain how/when this will make a difference?

[Y.b. Lu] Actually I have another internal patch based on this patch-set for setting rule of trapping IEEE 1588 PTP Ethernet frames.
For such rule which should be applied on several ingress ports, we can set it once when ocelot initialization I think.

The internal patch I mentioned is for felix which had different ports number (VCAP_PORT_CNT). So I hadn't sent it out.
Let me just send v2 patch-set with the patch dropping VCAP_PORT_CNT changes for your reviewing.
Please feel free to provide suggestion.

Thanks a lot:)
> 
> > Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com>
> > ---
> >  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;
> >  }
> 
> -- Allan

Re: [PATCH 3/3] ocelot_ace: fix action of trap

[Allan W. Nielsen]

The 08/13/2019 02:12, Y.b. Lu wrote:
> > -----Original Message-----
> > From: Allan W. Nielsen <allan.nielsen@microchip.com>
> > Sent: Monday, August 12, 2019 8:32 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: [PATCH 3/3] ocelot_ace: fix action of trap
> > 
> > The 08/12/2019 18:48, 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.
> > 
> > Are there any actions which do a "copy-to-cpu" and still forward the frame in
> > HW?
> 
> [Y.b. Lu] We're using Felix switch whose code hadn't been accepted by upstream.
> https://patchwork.ozlabs.org/project/netdev/list/?series=115399&state=*
> 
> I'd like to trap all IEEE 1588 PTP Ethernet frames to CPU through etype 0x88f7.
> When I used current TRAP option, I found the frames were not only copied to CPU, but also forwarded to other ports.
> So I just made the TRAP option same with DROP option except enabling CPU_COPY_ENA in the patch.
This is still wrong to do - and it will not work for Ocelot (and I doubt it will
work for your Felix target).

The policer setting in the drop action ensure that the frame is dropped even if
other pipe-line steps in the switch has set the copy-to-cpu flag.

I think you can fix this patch my just clearing the port mask, and not set the
policer.

/Allan

Re: [PATCH 3/3] ocelot_ace: fix action of trap

[Andrew Lunn]

On Tue, Aug 13, 2019 at 02:12:47AM +0000, Y.b. Lu wrote:
> Hi Allan,
> 
> > -----Original Message-----
> > From: Allan W. Nielsen <allan.nielsen@microchip.com>
> > Sent: Monday, August 12, 2019 8:32 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: [PATCH 3/3] ocelot_ace: fix action of trap
> > 
> > The 08/12/2019 18:48, 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.
> > 
> > Are there any actions which do a "copy-to-cpu" and still forward the frame in
> > HW?
> 
> [Y.b. Lu] We're using Felix switch whose code hadn't been accepted by upstream.
> https://patchwork.ozlabs.org/project/netdev/list/?series=115399&state=*
> 
> I'd like to trap all IEEE 1588 PTP Ethernet frames to CPU through etype 0x88f7.

Is this the correct way to handle PTP for this switch? For other
switches we don't need such traps. The switch itself identifies PTP
frames and forwards them to the CPU so it can process them.

I'm just wondering if your general approach is wrong?

    Andrew

RE: [PATCH 3/3] ocelot_ace: fix action of trap

[Y.b. Lu]

Hi Allan,

> -----Original Message-----
> From: Allan W. Nielsen <allan.nielsen@microchip.com>
> Sent: Tuesday, August 13, 2019 2:16 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: [PATCH 3/3] ocelot_ace: fix action of trap
> 
> The 08/13/2019 02:12, Y.b. Lu wrote:
> > > -----Original Message-----
> > > From: Allan W. Nielsen <allan.nielsen@microchip.com>
> > > Sent: Monday, August 12, 2019 8:32 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: [PATCH 3/3] ocelot_ace: fix action of trap
> > >
> > > The 08/12/2019 18:48, 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.
> > >
> > > Are there any actions which do a "copy-to-cpu" and still forward the
> > > frame in HW?
> >
> > [Y.b. Lu] We're using Felix switch whose code hadn't been accepted by
> upstream.
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> >
> hwork.ozlabs.org%2Fproject%2Fnetdev%2Flist%2F%3Fseries%3D115399%26s
> tat
> >
> e%3D*&amp;data=02%7C01%7Cyangbo.lu%40nxp.com%7C42cd202cb17b45
> 69821708d
> >
> 71fb5c5de%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6370127
> 37899910
> >
> 736&amp;sdata=QnsDaWPHK9rb0XWg%2BduYEha6fuYSlv4YZdsu5f4kbfc%3D
> &amp;res
> > erved=0
> >
> > I'd like to trap all IEEE 1588 PTP Ethernet frames to CPU through etype
> 0x88f7.
> > When I used current TRAP option, I found the frames were not only copied to
> CPU, but also forwarded to other ports.
> > So I just made the TRAP option same with DROP option except enabling
> CPU_COPY_ENA in the patch.
> This is still wrong to do - and it will not work for Ocelot (and I doubt it will
> work for your Felix target).
> 
> The policer setting in the drop action ensure that the frame is dropped even if
> other pipe-line steps in the switch has set the copy-to-cpu flag.
> 
> I think you can fix this patch my just clearing the port mask, and not set the
> policer.

[Y.b. Lu] Sorry. I missed your previous comments on the TRAP action.
With my configuration in the patch, it indeed worked. Maybe it was because "the CPU port is not touched by MASK_MODE" which I saw in RM.

I will try your suggestion too. It sound more proper.
Thanks.

> 
> /Allan

RE: [PATCH 3/3] ocelot_ace: fix action of trap

[Y.b. Lu]

Hi Andrew and Allan,

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Tuesday, August 13, 2019 9:43 PM
> To: Y.b. Lu <yangbo.lu@nxp.com>
> Cc: Allan W. Nielsen <allan.nielsen@microchip.com>; 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: [PATCH 3/3] ocelot_ace: fix action of trap
> 
> On Tue, Aug 13, 2019 at 02:12:47AM +0000, Y.b. Lu wrote:
> > Hi Allan,
> >
> > > -----Original Message-----
> > > From: Allan W. Nielsen <allan.nielsen@microchip.com>
> > > Sent: Monday, August 12, 2019 8:32 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: [PATCH 3/3] ocelot_ace: fix action of trap
> > >
> > > The 08/12/2019 18:48, 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.
> > >
> > > Are there any actions which do a "copy-to-cpu" and still forward the
> > > frame in HW?
> >
> > [Y.b. Lu] We're using Felix switch whose code hadn't been accepted by
> upstream.
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> >
> hwork.ozlabs.org%2Fproject%2Fnetdev%2Flist%2F%3Fseries%3D115399%26s
> tat
> >
> e%3D*&amp;data=02%7C01%7Cyangbo.lu%40nxp.com%7Cfbf7f74803d040f1
> b55608d
> >
> 71ff41b17%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6370130
> 05597107
> >
> 485&amp;sdata=xPGDbm2XtDI0L7F5A2xLhDDtctbeqB0MFByCAlgAtJ4%3D&a
> mp;reser
> > ved=0
> >
> > I'd like to trap all IEEE 1588 PTP Ethernet frames to CPU through etype
> 0x88f7.
> 
> Is this the correct way to handle PTP for this switch? For other switches we
> don't need such traps. The switch itself identifies PTP frames and forwards
> them to the CPU so it can process them.
> 
> I'm just wondering if your general approach is wrong?

[Y.b. Lu] PTP messages over Ethernet will use two multicast addresses.
01-80-C2-00-00-0E for peer delay messages.
01-1B-19-00-00-00 for other messages.

But only 01-80-C2-00-00-0E could be handled by hardware filter for BPDU frames (01-80-C2-00-00-0x).
For PTP messages handling, trapping them to CPU through VCAP IS2 is the suggested way by Ocelot/Felix.

I have a question since you are experts.
For other switches, whether they are always trapping PTP messages to CPU?
Is there any common method in linux to configure switch to select trapping or just forwarding PTP messages?

Like Allan's comments in new version patch. I have no idea.
https://patchwork.ozlabs.org/patch/1145988/

Thanks.

> 
>     Andrew

Re: [PATCH 3/3] ocelot_ace: fix action of trap

[Allan W. Nielsen]

> > -----Original Message-----
> > From: Allan W. Nielsen <allan.nielsen@microchip.com>
> > Sent: Tuesday, August 13, 2019 2:16 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: [PATCH 3/3] ocelot_ace: fix action of trap
> > 
> > The 08/13/2019 02:12, Y.b. Lu wrote:
> > > > -----Original Message-----
> > > > From: Allan W. Nielsen <allan.nielsen@microchip.com>
> > > > Sent: Monday, August 12, 2019 8:32 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: [PATCH 3/3] ocelot_ace: fix action of trap
> > > >
> > > > The 08/12/2019 18:48, 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.
> > > >
> > > > Are there any actions which do a "copy-to-cpu" and still forward the
> > > > frame in HW?
> > >
> > > [Y.b. Lu] We're using Felix switch whose code hadn't been accepted by
> > upstream.
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > >
> > hwork.ozlabs.org%2Fproject%2Fnetdev%2Flist%2F%3Fseries%3D115399%26s
> > tat
> > >
> > e%3D*&amp;data=02%7C01%7Cyangbo.lu%40nxp.com%7C42cd202cb17b45
> > 69821708d
> > >
> > 71fb5c5de%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6370127
> > 37899910
> > >
> > 736&amp;sdata=QnsDaWPHK9rb0XWg%2BduYEha6fuYSlv4YZdsu5f4kbfc%3D
> > &amp;res
> > > erved=0
> > >
> > > I'd like to trap all IEEE 1588 PTP Ethernet frames to CPU through etype
> > 0x88f7.
> > > When I used current TRAP option, I found the frames were not only copied to
> > CPU, but also forwarded to other ports.
> > > So I just made the TRAP option same with DROP option except enabling
> > CPU_COPY_ENA in the patch.
> > This is still wrong to do - and it will not work for Ocelot (and I doubt it will
> > work for your Felix target).
> > 
> > The policer setting in the drop action ensure that the frame is dropped even if
> > other pipe-line steps in the switch has set the copy-to-cpu flag.
> > 
> > I think you can fix this patch my just clearing the port mask, and not set the
> > policer.
> 
> [Y.b. Lu] Sorry. I missed your previous comments on the TRAP action.
> With my configuration in the patch, it indeed worked. Maybe it was because "the CPU port is not touched by MASK_MODE" which I saw in RM.
Okay. If this is working, then you should properly test and see if the DROP
action is working on your target. I do not have access to the SoC which incldues
Felix, so I cannot check.

> I will try your suggestion too. It sound more proper.
Sounds good - thanks

/Allan

Re: [PATCH 3/3] ocelot_ace: fix action of trap

[Allan W. Nielsen]

Hi Y.b. and Andrew,

The 08/14/2019 04:28, Y.b. Lu wrote:
> > > I'd like to trap all IEEE 1588 PTP Ethernet frames to CPU through etype
> > 0x88f7.
> > 
> > Is this the correct way to handle PTP for this switch? For other switches we
> > don't need such traps. The switch itself identifies PTP frames and forwards
> > them to the CPU so it can process them.
> > 
> > I'm just wondering if your general approach is wrong?
> 
> [Y.b. Lu] PTP messages over Ethernet will use two multicast addresses.
> 01-80-C2-00-00-0E for peer delay messages.
Yes, and as you write, this is a BPDU which must not be forwarded (and they are
not).

> 01-1B-19-00-00-00 for other messages.
Yes, this is a normal L2 multicast address, which by default are broadcastet.

> But only 01-80-C2-00-00-0E could be handled by hardware filter for BPDU frames
> (01-80-C2-00-00-0x).  For PTP messages handling, trapping them to CPU through
> VCAP IS2 is the suggested way by Ocelot/Felix.
As I see it there are at least 3 scenarios which needs to be considered and
ideally supported:

1) Operate as a PTP-unaware switch. This means that Peer-delays messages are
   trapped to the CPU and not handled. End-to-End PTP sessions can still run on
   the network as 01-1B-19-00-00-00 frames are forwarded normally.

   This is what we have by default today.

2) A "passive" PTP switch (in MSCC/MCHP we call this an end-to-end transparent
   clock) where 01-80-C2-00-00-0E frames are still trapped to the CPU (and not
   handled), 01-1B-19-00-00-00 frames are forwarded, but we use the TCAM to add
   the residence time to the correction field in Sync and Delay request
   messages.

   This is a simple mechanism which allow end-to-end PTP sessions to synchronize
   their time, and compensate for the variable delay caused by the switch.

   Compared to implement a complete boundary clock, this is much simpler, and
   cause a much lower work load on the CPU (even small switches may be serving
   many many PTP sessions).

3) Full PTP aware switch. In this mode we need all PTP frames trapped (on the
   ports where PTP are running) to the CPU, and we need a PTP daemon in
   user-space to process them in-order for things to work.

   I guess this is what you are trying to achieve.

Eventually, I hope we can get to a point where all (and maybe more) scenarios
are supported.

Lets consider them case by case:

1) This is what we have today.

2) To support this, we need a SW implementation of this, and then we can add
   hooks to offload this in HW.

   We would certainly be interested in supporting this in both SW and HW.

3) It can be done via 'tc' using the trap action, but I do not know if this is
   the desired way of doing it. Ocelot will be using a TCAM rule to do this,
   which align nicely with the 'tc' approach, but other chips may be have
   dedicated HW for doing this.

   Also, in the current implementation we will be using a rule per port, and
   ideally we could have done it with a single rule (this is what Y.B. prepared
   in this patch series).

I'm very much against configuring option 3 in the driver initialization, as it
will prevent us from having a conforming switch if a PTP daemon is not running
in user-space, and it give us very little room for supporting other ways in the
future without breaking backwards compatibility.

> I have a question since you are experts.
I'm not really an expert on this, but I have access to some good guidance from
collages knowing PTP very well :-D

> For other switches, whether they are always trapping PTP messages to CPU?
Good question, I could not find anything in the SW bridge forcing option 3.

My understanding is that the SW bridge is implementing option 1, but I could be
wrong.

> Is there any common method in linux to configure switch to select trapping or
> just forwarding PTP messages?
You should be able to use TC for this. But due to the port vs port-mask
limitation you will need to install a rule per port.

I do not know if this is what others are doing, but would like to learn about
that.

/Allan

Re: [PATCH 3/3] ocelot_ace: fix action of trap

[Andrew Lunn]

> [Y.b. Lu] PTP messages over Ethernet will use two multicast addresses.
> 01-80-C2-00-00-0E for peer delay messages.
> 01-1B-19-00-00-00 for other messages.
> 
> But only 01-80-C2-00-00-0E could be handled by hardware filter for BPDU frames (01-80-C2-00-00-0x).
> For PTP messages handling, trapping them to CPU through VCAP IS2 is the suggested way by Ocelot/Felix.
> 
> I have a question since you are experts.

There real expert is Richard Cochran <richardcochran@gmail.com>. He
implemented PTP support for the mv88e6xxx devices, maintains to core
code, and the linuxptp daemon. Any ptp support your post will be
reviewed by him.

> For other switches, whether they are always trapping PTP messages to CPU?

For the mv88e6xxx family, there is a per port bit which enabled
PTP. When enabled, PTP frames are recognised by the hardware and
forwarded to the CPU.

> Is there any common method in linux to configure switch to select
> trapping or just forwarding PTP messages?

The best answer to that is to look at other switch driver which
implement ptp. The ptp core expects a ptp_clock_info structure, and
one of its members is 'enable'.

    Andrew

Re: [PATCH 3/3] ocelot_ace: fix action of trap

[Andrew Lunn]

On Wed, Aug 14, 2019 at 10:57:12AM +0200, Allan W. Nielsen wrote:
> Hi Y.b. and Andrew,
> 
> The 08/14/2019 04:28, Y.b. Lu wrote:
> > > > I'd like to trap all IEEE 1588 PTP Ethernet frames to CPU through etype
> > > 0x88f7.
> > > 
> > > Is this the correct way to handle PTP for this switch? For other switches we
> > > don't need such traps. The switch itself identifies PTP frames and forwards
> > > them to the CPU so it can process them.
> > > 
> > > I'm just wondering if your general approach is wrong?
> > 
> > [Y.b. Lu] PTP messages over Ethernet will use two multicast addresses.
> > 01-80-C2-00-00-0E for peer delay messages.
> Yes, and as you write, this is a BPDU which must not be forwarded (and they are
> not).
> 
> > 01-1B-19-00-00-00 for other messages.
> Yes, this is a normal L2 multicast address, which by default are broadcastet.
> 
> > But only 01-80-C2-00-00-0E could be handled by hardware filter for BPDU frames
> > (01-80-C2-00-00-0x).  For PTP messages handling, trapping them to CPU through
> > VCAP IS2 is the suggested way by Ocelot/Felix.

Hi Allan

The typical userspace for this is linuxptp. It implements Boundary
Clock (BC), Ordinary Clock (OC) and Transparent Clock (TC). On
switches, it works great for L2 PTP. But it has architectural issues
for L3 PTP when used with a bridge. I've no idea if Richard is fixing
this.

> 3) It can be done via 'tc' using the trap action, but I do not know if this is
>    the desired way of doing it.

No, it is not. It could be the way you the implement
ptp_clock_info.enable() does the same as what TC could do, but TC
itself is not used, it should all be internal to the driver. And you
might also want to consider hiding such rules from TC, otherwise the
user might remove them and things break.

     Andrew

RE: [PATCH 3/3] ocelot_ace: fix action of trap

[Y.b. Lu]

Hi Andrew and Allan,

I add Richard in email for help and some suggestions.
And please see my comments inline.
Thanks.

Hi Richard,

We are discussing problem of PTP message trapping to CPU on switch. Hope for your suggestions since you are the expert.
Here are the two versions patch-set.
V2: https://patchwork.ozlabs.org/project/netdev/list/?series=124713&state=*
V1: https://patchwork.ozlabs.org/project/netdev/list/?series=124563&state=*

Thanks in advance :)

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, August 14, 2019 9:52 PM
> To: Allan W. Nielsen <allan.nielsen@microchip.com>
> Cc: Y.b. Lu <yangbo.lu@nxp.com>; 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: [PATCH 3/3] ocelot_ace: fix action of trap
> 
> On Wed, Aug 14, 2019 at 10:57:12AM +0200, Allan W. Nielsen wrote:
> > Hi Y.b. and Andrew,
> >
> > The 08/14/2019 04:28, Y.b. Lu wrote:
> > > > > I'd like to trap all IEEE 1588 PTP Ethernet frames to CPU
> > > > > through etype
> > > > 0x88f7.
> > > >
> > > > Is this the correct way to handle PTP for this switch? For other
> > > > switches we don't need such traps. The switch itself identifies
> > > > PTP frames and forwards them to the CPU so it can process them.
> > > >
> > > > I'm just wondering if your general approach is wrong?
> > >
> > > [Y.b. Lu] PTP messages over Ethernet will use two multicast addresses.
> > > 01-80-C2-00-00-0E for peer delay messages.
> > Yes, and as you write, this is a BPDU which must not be forwarded (and
> > they are not).
> >
> > > 01-1B-19-00-00-00 for other messages.
> > Yes, this is a normal L2 multicast address, which by default are broadcastet.
> >
> > > But only 01-80-C2-00-00-0E could be handled by hardware filter for
> > > BPDU frames (01-80-C2-00-00-0x).  For PTP messages handling,
> > > trapping them to CPU through VCAP IS2 is the suggested way by
> Ocelot/Felix.
> 
> Hi Allan
> 
> The typical userspace for this is linuxptp. It implements Boundary Clock (BC),
> Ordinary Clock (OC) and Transparent Clock (TC). On switches, it works great for
> L2 PTP. But it has architectural issues for L3 PTP when used with a bridge. I've
> no idea if Richard is fixing this.

[Y.b. Lu] Right.
For the 3 scenarios Allan listed, actually I think usually the first and the second wouldn't be used if user needs 1588 synchronization.
#1 scenario, since it's PTP unaware, asymmetric delay will be introduced and it couldn't ensure sync performance.
#2 scenario, this has too much limitation. The switch hardware could only be configured as two-step end-to-end transparent clock in hardware.
For other clock types, it requires CPU handling and ptp software. In addition, ptp software takes very few resources.

So the desired scenario for 1588 synchronization should be #3 scenario.

> 
> > 3) It can be done via 'tc' using the trap action, but I do not know if this is
> >    the desired way of doing it.
> 
> No, it is not. It could be the way you the implement
> ptp_clock_info.enable() does the same as what TC could do, but TC itself is not
> used, it should all be internal to the driver. And you might also want to
> consider hiding such rules from TC, otherwise the user might remove them and
> things break.

[Y.b. Lu] ptp_clock_info.enable() ?
As I understand, PTP clock driver is only for ptp clock operations, not for networking.

I would have intended to send PTP trapping rule patch for discussion after Felix driver is ready on upstream.
Let me just send TRAP option fix-up patch in next version. Will rework and send PTP trapping rule patch once Felix driver is accepted by upstream.
But I'd like to gather suggestions on that.

Thanks a lot.

> 
>      Andrew