Re: [PATCH net-next v3 4/4] net: lan966x: Add ptp trap rules

[Horatiu Vultur <horatiu.vultur@microchip.com>]

The 12/08/2022 14:18, Michael Walle wrote:
> 
> Hi Horatiu,

Hi Michael,

> 
> Am 2022-12-08 14:04, schrieb Horatiu Vultur:
> > > > > Currently lan966x, doesn't allow to run PTP over interfaces that are
> > > > > part of the bridge. The reason is when the lan966x was receiving a
> > > > > PTP frame (regardless if L2/IPv4/IPv6) the HW it would flood this
> > > > > frame.
> > > > > Now that it is possible to add VCAP rules to the HW, such to trap
> > > > > these
> > > > > frames to the CPU, it is possible to run PTP also over interfaces that
> > > > > are part of the bridge.
> > > >
> > > > This gives me:
> > > >
> > > > # /etc/init.d/S65ptp4l start
> > > > Starting linuxptp daemon: OK
> > > > [   44.136870] vcap_val_rule:1678: keyset was not updated: -22
> > > > [   44.140196] vcap_val_rule:1678: keyset was not updated: -22
> > > > #
> > > >
> > > > # ptp4l -v
> > > > 3.1.1
> > > > # uname -a
> > > > Linux buildroot 6.1.0-rc8-next-20221208+ #924 SMP Thu Dec  8 10:08:58
> > > > CET 2022 armv7l GNU/Linux
> > > >
> > > > I don't know whats going on, but I'm happy to help with debugging with
> > > > some
> > > > guidance.
> > > 
> > > Oh, and linuxptp is running on eth0, no bridges are set up. linuxptp
> > > is started with "/usr/sbin/ptp4l -f /etc/linuxptp.cfg"
> > > 
> > > # cat /etc/linuxptp.cfg
> > > # LinuxPTP configuration file for synchronizing the system clock to
> > > # a remote PTP master in slave-only mode.
> > > #
> > > # By default synchronize time in slave-only mode using UDP and
> > > hardware
> > > time
> > > # stamps on eth0. If the difference to master is >1.0 second correct
> > > by
> > > # stepping the clock instead of adjusting the frequency.
> > > #
> > > # If you change the configuration don't forget to update the phc2sys
> > > # parameters accordingly in linuxptp-system-clock.service (systemd)
> > > # or the linuxptp SysV init script.
> > > 
> > > [global]
> > > slaveOnly               1
> > > delay_mechanism         Auto
> > > network_transport       UDPv4
> > > time_stamping           hardware
> > > step_threshold          1.0
> > > 
> > > [eth0]
> > 
> > Thanks for trying this!
> 
> Actually I was just booting my board which happens to have linuxptp
> started by default. And the error messages were new. But I'm not so
> sure anymore if PTP was really working. I'm still puzzled by reading
> your commit message. Was it already working for interfaces which aren't
> part of a bridge and this commit will make it work even for interfaces
> which are part of a bridge?

Exactly!
This worked on interfaces that were not part of the bridge. And with
this commit will make it work even on interfaces that are part of the
bridge.

> 
> > The issue is because you have not enabled the TCAM lookups per
> > port. They can be enabled using this commands:
> > 
> > tc qdisc add dev eth0 clsact
> 
> This gives me the following error, might be a missing kconfig option:
> 
> # tc qdisc add dev eth0 clsact
> RTNETLINK answers: Operation not supported

Yes that should be the case, I think you are missing:
CONFIG_NET_SCHED
But may be others when you try to add the next rule.

> 
> > tc filter add dev eth0 ingress prio 5 handle 5 matchall skip_sw action
> > goto chain 8000000
> > 
> > This will enable the lookup and then you should be able to start again
> > the ptp4l. Sorry for not mention this, at least I should have written
> > it
> > somewhere that this is required.
> > 
> > I was not sure if lan966x should or not enable tcam lookups
> > automatically when a ptp trap action is added. I am open to suggestion
> > here.
> 
> IMHO, from a user point of view this should just work. For a user
> there is no connection between running linuxptp and some filtering
> stuff with 'tc'.
> 
> Also, if the answer to my question above is yes, and ptp should
> have worked on eth0 before, this is a regression then.

OK, I can see your point.
With the following diff, you should see the same behaviour as before:
---
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c b/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
index 904f5a3f636d3..538f4b76cf97a 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c
@@ -91,8 +91,6 @@ lan966x_vcap_is2_get_port_keysets(struct net_device *dev, int lookup,

        /* Check if the port keyset selection is enabled */
        val = lan_rd(lan966x, ANA_VCAP_S2_CFG(port->chip_port));
-       if (!ANA_VCAP_S2_CFG_ENA_GET(val))
-               return -ENOENT;

        /* Collect all keysets for the port in a list */
        if (l3_proto == ETH_P_ALL)
---

> 
> -michael

-- 
/Horatiu