[3/3] ocelot_ace: fix action of trap
diff mbox series

Message ID 20190812104827.5935-4-yangbo.lu@nxp.com
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • ocelot_ace: fix and improve the driver
Related show

Commit Message

Yangbo Lu Aug. 12, 2019, 10:48 a.m. UTC
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(-)

Comments

Allan W. Nielsen Aug. 12, 2019, 12:31 p.m. UTC | #1
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
Yangbo Lu Aug. 13, 2019, 2:12 a.m. UTC | #2
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
Allan W. Nielsen Aug. 13, 2019, 6:16 a.m. UTC | #3
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
Andrew Lunn Aug. 13, 2019, 1:42 p.m. UTC | #4
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
Yangbo Lu Aug. 14, 2019, 4:03 a.m. UTC | #5
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
Yangbo Lu Aug. 14, 2019, 4:28 a.m. UTC | #6
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
Allan W. Nielsen Aug. 14, 2019, 6:45 a.m. UTC | #7
> > -----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
Allan W. Nielsen Aug. 14, 2019, 8:57 a.m. UTC | #8
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
Andrew Lunn Aug. 14, 2019, 1:42 p.m. UTC | #9
> [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
Andrew Lunn Aug. 14, 2019, 1:52 p.m. UTC | #10
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
Yangbo Lu Aug. 15, 2019, 12:39 p.m. UTC | #11
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

Patch
diff mbox series

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;