| Message ID | 20221203104348.1749811-5-horatiu.vultur@microchip.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 72df3489fb10324e30616d278ea9b37a8a801b6f |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: lan966x: Enable PTP on bridge interfaces | expand |
Hi Horatiu, > 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. -michael
Am 2022-12-08 10:25, schrieb Michael Walle: > Hi Horatiu, > >> 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] -michael
The 12/08/2022 10:27, Michael Walle wrote: > > Am 2022-12-08 10:25, schrieb Michael Walle: > > Hi Horatiu, Hi Michael, > > > > > 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! 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 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. > > -michael
Hi Horatiu, 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? > 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 > 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. -michael
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
Hi, Am 2022-12-09 10:29, schrieb Horatiu Vultur: > The 12/08/2022 14:18, Michael Walle wrote: >> 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. I guess I'd need to update my kernel config sometime. At the moment I just have a basic one, as there is still so much stuff missing for the lan9668. So I haven't come around testing anything else. As I said, I just noticed because my rootfs happens to have linuxptp started by default. >> > 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: Ok, I can say, I don't see the error message anymore. Haven't tested PTP though. I'd need to setup it up first. Does it also work out of the box with the following patch if the interface is part of a bridge or do you still have to do the tc magic from above? -michael > --- > 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
On Fri, Dec 09, 2022 at 01:58:57PM +0100, Horatiu Vultur wrote: > > Does it also work out of the box with the following patch if > > the interface is part of a bridge or do you still have to do > > the tc magic from above? > > You will still need to enable the TCAM using the tc command to have it > working when the interface is part of the bridge. FWIW, with ocelot (same VCAP mechanism), PTP traps work out of the box, no need to use tc. Same goes for ocelot-8021q, which also uses the VCAP. I wouldn't consider forcing the user to add any tc command in order for packet timestamping to work properly.
The 12/09/2022 13:10, Michael Walle wrote: > > Hi, Hi Michael, > > > > > 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. > > I guess I'd need to update my kernel config sometime. At the > moment I just have a basic one, as there is still so much stuff > missing for the lan9668. So I haven't come around testing anything > else. As I said, I just noticed because my rootfs happens to have > linuxptp started by default. I understand. > > > > > 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: > > Ok, I can say, I don't see the error message anymore. Haven't tested > PTP though. I'd need to setup it up first. Good, at least no more warnings and should not be any regression there. > > Does it also work out of the box with the following patch if > the interface is part of a bridge or do you still have to do > the tc magic from above? You will still need to enable the TCAM using the tc command to have it working when the interface is part of the bridge. > > -michael > > > --- > > 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
Am 2022-12-09 13:56, schrieb Vladimir Oltean: > On Fri, Dec 09, 2022 at 01:58:57PM +0100, Horatiu Vultur wrote: >> > Does it also work out of the box with the following patch if >> > the interface is part of a bridge or do you still have to do >> > the tc magic from above? >> >> You will still need to enable the TCAM using the tc command to have it >> working when the interface is part of the bridge. > > FWIW, with ocelot (same VCAP mechanism), PTP traps work out of the box, > no need to use tc. Same goes for ocelot-8021q, which also uses the > VCAP. > I wouldn't consider forcing the user to add any tc command in order for > packet timestamping to work properly. +1 Esp. because there is no warning. I.e. I tried this patch while the interface was added on a bridge and there was no error whatsoever. Also, you'd force the user to have that Kconfig option set. -michael
On Fri, Dec 09, 2022 at 03:05:01PM +0100, Michael Walle wrote: > Am 2022-12-09 13:56, schrieb Vladimir Oltean: > > On Fri, Dec 09, 2022 at 01:58:57PM +0100, Horatiu Vultur wrote: > > > > Does it also work out of the box with the following patch if > > > > the interface is part of a bridge or do you still have to do > > > > the tc magic from above? > > > > > > You will still need to enable the TCAM using the tc command to have it > > > working when the interface is part of the bridge. > > > > FWIW, with ocelot (same VCAP mechanism), PTP traps work out of the box, > > no need to use tc. Same goes for ocelot-8021q, which also uses the VCAP. > > I wouldn't consider forcing the user to add any tc command in order for > > packet timestamping to work properly. > > +1 > Esp. because there is no warning. I.e. I tried this patch while > the interface was added on a bridge and there was no error > whatsoever. Also, you'd force the user to have that Kconfig option > set. Yup. What I said about Ocelot is also applicable to MRP traps, which Horatiu added. No tc necessary there, either.
The 12/09/2022 15:05, Michael Walle wrote: > > Am 2022-12-09 13:56, schrieb Vladimir Oltean: > > On Fri, Dec 09, 2022 at 01:58:57PM +0100, Horatiu Vultur wrote: > > > > Does it also work out of the box with the following patch if > > > > the interface is part of a bridge or do you still have to do > > > > the tc magic from above? > > > > > > You will still need to enable the TCAM using the tc command to have it > > > working when the interface is part of the bridge. > > > > FWIW, with ocelot (same VCAP mechanism), PTP traps work out of the box, > > no need to use tc. Same goes for ocelot-8021q, which also uses the > > VCAP. > > I wouldn't consider forcing the user to add any tc command in order for > > packet timestamping to work properly. On ocelot, the vcap is enabled at port initialization, while on other platforms(lan966x and sparx5) you have the option to enable or disable. > > +1 > Esp. because there is no warning. I.e. I tried this patch while > the interface was added on a bridge and there was no error > whatsoever. What error/warning were you expecting to see here? > Also, you'd force the user to have that Kconfig option > set. > > -michael >
Am 2022-12-09 15:20, schrieb Horatiu Vultur: > The 12/09/2022 15:05, Michael Walle wrote: >> >> Am 2022-12-09 13:56, schrieb Vladimir Oltean: >> > On Fri, Dec 09, 2022 at 01:58:57PM +0100, Horatiu Vultur wrote: >> > > > Does it also work out of the box with the following patch if >> > > > the interface is part of a bridge or do you still have to do >> > > > the tc magic from above? >> > > >> > > You will still need to enable the TCAM using the tc command to have it >> > > working when the interface is part of the bridge. >> > >> > FWIW, with ocelot (same VCAP mechanism), PTP traps work out of the box, >> > no need to use tc. Same goes for ocelot-8021q, which also uses the >> > VCAP. >> > I wouldn't consider forcing the user to add any tc command in order for >> > packet timestamping to work properly. > > On ocelot, the vcap is enabled at port initialization, while on other > platforms(lan966x and sparx5) you have the option to enable or disable. > >> >> +1 >> Esp. because there is no warning. I.e. I tried this patch while >> the interface was added on a bridge and there was no error >> whatsoever. > > What error/warning were you expecting to see here? Scrap that. ptp4l is reporting an error in case the device is part of a bridge: Jan 1 02:33:04 buildroot user.info syslog: [9184.261] driver rejected most general HWTSTAMP filter Nevertheless, from a users POV I'd just expect it to work. How would I know what I need to do here? -michael
On Fri, Dec 09, 2022 at 03:20:58PM +0100, Horatiu Vultur wrote: > On ocelot, the vcap is enabled at port initialization, while on other > platforms(lan966x and sparx5) you have the option to enable or disable. Even if that wasn't the case, I'd still consider enabling/disabling VCAP lookups privately in the ocelot driver when there are non-tc users of traps, instead of requiring users to do anything with tc. It's simply too unfriendly for somebody who just wants PTP. You can use devlink traps to show which non-tc traps are active.
On Fri, Dec 09, 2022 at 04:43:28PM +0200, Vladimir Oltean wrote: > On Fri, Dec 09, 2022 at 03:20:58PM +0100, Horatiu Vultur wrote: > > On ocelot, the vcap is enabled at port initialization, while on other > > platforms(lan966x and sparx5) you have the option to enable or disable. > > Even if that wasn't the case, I'd still consider enabling/disabling VCAP > lookups privately in the ocelot driver when there are non-tc users of > traps, instead of requiring users to do anything with tc. It's simply > too unfriendly for somebody who just wants PTP. You can use devlink > traps to show which non-tc traps are active. To put it differently. Why do you even bother to make the driver auto-install PTP traps, and not let the user do that? It's the same argument as asking the user enable the VCAP lookups.
The 12/09/2022 15:23, Michael Walle wrote: > > Am 2022-12-09 15:20, schrieb Horatiu Vultur: > > The 12/09/2022 15:05, Michael Walle wrote: > > > > > > Am 2022-12-09 13:56, schrieb Vladimir Oltean: > > > > On Fri, Dec 09, 2022 at 01:58:57PM +0100, Horatiu Vultur wrote: > > > > > > Does it also work out of the box with the following patch if > > > > > > the interface is part of a bridge or do you still have to do > > > > > > the tc magic from above? > > > > > > > > > > You will still need to enable the TCAM using the tc command to have it > > > > > working when the interface is part of the bridge. > > > > > > > > FWIW, with ocelot (same VCAP mechanism), PTP traps work out of the box, > > > > no need to use tc. Same goes for ocelot-8021q, which also uses the > > > > VCAP. > > > > I wouldn't consider forcing the user to add any tc command in order for > > > > packet timestamping to work properly. > > > > On ocelot, the vcap is enabled at port initialization, while on other > > platforms(lan966x and sparx5) you have the option to enable or disable. > > > > > > > > +1 > > > Esp. because there is no warning. I.e. I tried this patch while > > > the interface was added on a bridge and there was no error > > > whatsoever. > > > > What error/warning were you expecting to see here? > > Scrap that. ptp4l is reporting an error in case the device is part > of a bridge: > Jan 1 02:33:04 buildroot user.info syslog: [9184.261] driver rejected > most general HWTSTAMP filter > > Nevertheless, from a users POV I'd just expect it to work. How > would I know what I need to do here? What about a warning in the driver? Say that the vcap needs to be enabled. > > -michael
On Fri, Dec 09, 2022 at 03:57:20PM +0100, Horatiu Vultur wrote: > The 12/09/2022 16:43, Vladimir Oltean wrote: > > > > On Fri, Dec 09, 2022 at 03:20:58PM +0100, Horatiu Vultur wrote: > > > On ocelot, the vcap is enabled at port initialization, while on other > > > platforms(lan966x and sparx5) you have the option to enable or disable. > > > > Even if that wasn't the case, I'd still consider enabling/disabling VCAP > > lookups privately in the ocelot driver when there are non-tc users of > > traps, instead of requiring users to do anything with tc. > > I was thinking also about this, such the ptp to enable the VCAP > privately. But then the issue would be if a user adds entries using tc > and then start ptp, then suddently the rules that were added using tc > could be hit. That is the reason why expected the user to enable the > tcam manually. I don't understand, tc rules which do what? Why would those rules only be hit after PTP is enabled and not before?
The 12/09/2022 16:43, Vladimir Oltean wrote: > > On Fri, Dec 09, 2022 at 03:20:58PM +0100, Horatiu Vultur wrote: > > On ocelot, the vcap is enabled at port initialization, while on other > > platforms(lan966x and sparx5) you have the option to enable or disable. > > Even if that wasn't the case, I'd still consider enabling/disabling VCAP > lookups privately in the ocelot driver when there are non-tc users of > traps, instead of requiring users to do anything with tc. I was thinking also about this, such the ptp to enable the VCAP privately. But then the issue would be if a user adds entries using tc and then start ptp, then suddently the rules that were added using tc could be hit. That is the reason why expected the user to enable the tcam manually. > It's simply too unfriendly for somebody who just wants PTP. You can use > devlink traps to show which non-tc traps are active.
On Fri, Dec 09, 2022 at 04:30:10PM +0100, Horatiu Vultur wrote: > For example this rule: > tc filter add dev eth0 ingress chain 8000000 prio 1 handle 1 protocol all > flower skip_sw dst_mac 00:11:22:33:44:55/ff:ff:ff:ff:ff:ff action trap > action goto chain 8100000 > > This will not be hit until you add this rule: > tc filter add dev eth0 ingress prio 1 handle 2 matchall skip_sw action goto chain 8000000 > > Because this rule will enable the HW. Just to aligned to a SW > implementation of the tc, we don't enable the vcap until there is a rule > in chain 0 that has an action to go to chain 8000000 were it resides > IS2 rules. > > So for example, on a fresh started lan966x the user will add the following > rule: > tc filter add dev eth0 ingress chain 8000000 prio 1 handle 1 protocol > all flower skip_sw dst_mac 00:11:22:33:44:55/ff:ff:ff:ff:ff:ff action > trap action goto chain 8100000 > > He expects this rule not to be hit as there is no rule in chain 0. Now if > PTP is started and it would enable vcap, then suddenly this rule may be > hit. Is it too restrictive to only allow adding offloaded filters to a chain that has a valid goto towards it, coming (perhaps indirectly) from chain 0?
The 12/09/2022 16:56, Vladimir Oltean wrote: > > On Fri, Dec 09, 2022 at 03:57:20PM +0100, Horatiu Vultur wrote: > > The 12/09/2022 16:43, Vladimir Oltean wrote: > > > > > > On Fri, Dec 09, 2022 at 03:20:58PM +0100, Horatiu Vultur wrote: > > > > On ocelot, the vcap is enabled at port initialization, while on other > > > > platforms(lan966x and sparx5) you have the option to enable or disable. > > > > > > Even if that wasn't the case, I'd still consider enabling/disabling VCAP > > > lookups privately in the ocelot driver when there are non-tc users of > > > traps, instead of requiring users to do anything with tc. > > > > I was thinking also about this, such the ptp to enable the VCAP > > privately. But then the issue would be if a user adds entries using tc > > and then start ptp, then suddently the rules that were added using tc > > could be hit. That is the reason why expected the user to enable the > > tcam manually. > > I don't understand, tc rules which do what? Why would those rules only > be hit after PTP is enabled and not before? Because you have not enabled the vcap. For example this rule: tc filter add dev eth0 ingress chain 8000000 prio 1 handle 1 protocol all flower skip_sw dst_mac 00:11:22:33:44:55/ff:ff:ff:ff:ff:ff action trap action goto chain 8100000 This will not be hit until you add this rule: tc filter add dev eth0 ingress prio 1 handle 2 matchall skip_sw action goto chain 8000000 Because this rule will enable the HW. Just to aligned to a SW implementation of the tc, we don't enable the vcap until there is a rule in chain 0 that has an action to go to chain 8000000 were it resides IS2 rules. So for example, on a fresh started lan966x the user will add the following rule: tc filter add dev eth0 ingress chain 8000000 prio 1 handle 1 protocol all flower skip_sw dst_mac 00:11:22:33:44:55/ff:ff:ff:ff:ff:ff action trap action goto chain 8100000 He expects this rule not to be hit as there is no rule in chain 0. Now if PTP is started and it would enable vcap, then suddenly this rule may be hit. I hope this helps a little bit.
On Fri, 9 Dec 2022 17:27:13 +0200 Vladimir Oltean wrote: > > So for example, on a fresh started lan966x the user will add the following > > rule: > > tc filter add dev eth0 ingress chain 8000000 prio 1 handle 1 protocol > > all flower skip_sw dst_mac 00:11:22:33:44:55/ff:ff:ff:ff:ff:ff action > > trap action goto chain 8100000 > > > > He expects this rule not to be hit as there is no rule in chain 0. Now if > > PTP is started and it would enable vcap, then suddenly this rule may be > > hit. > > Is it too restrictive to only allow adding offloaded filters to a chain > that has a valid goto towards it, coming (perhaps indirectly) from chain 0? Right, we fumbled the review and let the chain oddness in. Until recently the driver worked without any rules in chain 0 :( Maybe adding and offload of the rules can be separated? Only actually add the rules to the HW once the goto chain rule has been added?
The 12/09/2022 17:27, Vladimir Oltean wrote: > Sorry for late reply! > On Fri, Dec 09, 2022 at 04:30:10PM +0100, Horatiu Vultur wrote: > > For example this rule: > > tc filter add dev eth0 ingress chain 8000000 prio 1 handle 1 protocol all > > flower skip_sw dst_mac 00:11:22:33:44:55/ff:ff:ff:ff:ff:ff action trap > > action goto chain 8100000 > > > > This will not be hit until you add this rule: > > tc filter add dev eth0 ingress prio 1 handle 2 matchall skip_sw action goto chain 8000000 > > > > Because this rule will enable the HW. Just to aligned to a SW > > implementation of the tc, we don't enable the vcap until there is a rule > > in chain 0 that has an action to go to chain 8000000 were it resides > > IS2 rules. > > > > So for example, on a fresh started lan966x the user will add the following > > rule: > > tc filter add dev eth0 ingress chain 8000000 prio 1 handle 1 protocol > > all flower skip_sw dst_mac 00:11:22:33:44:55/ff:ff:ff:ff:ff:ff action > > trap action goto chain 8100000 > > > > He expects this rule not to be hit as there is no rule in chain 0. Now if > > PTP is started and it would enable vcap, then suddenly this rule may be > > hit. > > Is it too restrictive to only allow adding offloaded filters to a chain > that has a valid goto towards it, coming (perhaps indirectly) from chain 0? We were thinking to do something like this. With a small difference, to allow the user to add filters to a chain, but offload them in HW only when there is a valid goto towards. Instead of checking if there is a goto to that chain. But I think we need to spend a little bit more time on this. Any suggestion is more than welcome.
The 12/09/2022 15:03, Jakub Kicinski wrote: > > On Fri, 9 Dec 2022 17:27:13 +0200 Vladimir Oltean wrote: > > > So for example, on a fresh started lan966x the user will add the following > > > rule: > > > tc filter add dev eth0 ingress chain 8000000 prio 1 handle 1 protocol > > > all flower skip_sw dst_mac 00:11:22:33:44:55/ff:ff:ff:ff:ff:ff action > > > trap action goto chain 8100000 > > > > > > He expects this rule not to be hit as there is no rule in chain 0. Now if > > > PTP is started and it would enable vcap, then suddenly this rule may be > > > hit. > > > > Is it too restrictive to only allow adding offloaded filters to a chain > > that has a valid goto towards it, coming (perhaps indirectly) from chain 0? > > Right, we fumbled the review and let the chain oddness in. > Until recently the driver worked without any rules in chain 0 :( > > Maybe adding and offload of the rules can be separated? > Only actually add the rules to the HW once the goto chain rule > has been added? Yes, we would like to do something like this.
Hi, >> 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) Any news on this? Apart from the patches which would change the need to use some tc magic, this should be a separate fixes patch, right? -michael
The 01/05/2023 16:09, Michael Walle wrote: > > Hi, Hi Michael, > > > > 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) > > Any news on this? Apart from the patches which would change the > need to use some tc magic, this should be a separate fixes patch, > right? My colleague Steen, has sent a patch series here [1]. This allows to PTP rules in HW without any tc commands. [1] https://lore.kernel.org/lkml/20230105081335.1261636-1-steen.hegelund@microchip.com/T/ > > -michael
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c index f6092983d0281..cadde20505ba0 100644 --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c @@ -443,11 +443,22 @@ static int lan966x_port_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd) { struct lan966x_port *port = netdev_priv(dev); + int err; + + if (cmd == SIOCSHWTSTAMP) { + err = lan966x_ptp_setup_traps(port, ifr); + if (err) + return err; + } if (!phy_has_hwtstamp(dev->phydev) && port->lan966x->ptp) { switch (cmd) { case SIOCSHWTSTAMP: - return lan966x_ptp_hwtstamp_set(port, ifr); + err = lan966x_ptp_hwtstamp_set(port, ifr); + if (err) + lan966x_ptp_del_traps(port); + + return err; case SIOCGHWTSTAMP: return lan966x_ptp_hwtstamp_get(port, ifr); } @@ -456,7 +467,11 @@ static int lan966x_port_ioctl(struct net_device *dev, struct ifreq *ifr, if (!dev->phydev) return -ENODEV; - return phy_mii_ioctl(dev->phydev, ifr, cmd); + err = phy_mii_ioctl(dev->phydev, ifr, cmd); + if (err && cmd == SIOCSHWTSTAMP) + lan966x_ptp_del_traps(port); + + return err; } static const struct net_device_ops lan966x_port_netdev_ops = { diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h index f2e45da7ffd4f..3491f19618358 100644 --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h @@ -88,6 +88,10 @@ #define SE_IDX_QUEUE 0 /* 0-79 : Queue scheduler elements */ #define SE_IDX_PORT 80 /* 80-89 : Port schedular elements */ +#define LAN966X_VCAP_CID_IS2_L0 VCAP_CID_INGRESS_STAGE2_L0 /* IS2 lookup 0 */ +#define LAN966X_VCAP_CID_IS2_L1 VCAP_CID_INGRESS_STAGE2_L1 /* IS2 lookup 1 */ +#define LAN966X_VCAP_CID_IS2_MAX (VCAP_CID_INGRESS_STAGE2_L2 - 1) /* IS2 Max */ + /* MAC table entry types. * ENTRYTYPE_NORMAL is subject to aging. * ENTRYTYPE_LOCKED is not subject to aging. @@ -116,6 +120,14 @@ enum lan966x_fdma_action { FDMA_REDIRECT, }; +/* Controls how PORT_MASK is applied */ +enum LAN966X_PORT_MASK_MODE { + LAN966X_PMM_NO_ACTION, + LAN966X_PMM_REPLACE, + LAN966X_PMM_FORWARDING, + LAN966X_PMM_REDIRECT, +}; + struct lan966x_port; struct lan966x_db { @@ -473,6 +485,8 @@ irqreturn_t lan966x_ptp_irq_handler(int irq, void *args); irqreturn_t lan966x_ptp_ext_irq_handler(int irq, void *args); u32 lan966x_ptp_get_period_ps(void); int lan966x_ptp_gettime64(struct ptp_clock_info *ptp, struct timespec64 *ts); +int lan966x_ptp_setup_traps(struct lan966x_port *port, struct ifreq *ifr); +int lan966x_ptp_del_traps(struct lan966x_port *port); int lan966x_fdma_xmit(struct sk_buff *skb, __be32 *ifh, struct net_device *dev); int lan966x_fdma_xmit_xdpf(struct lan966x_port *port, diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c index e5a2bbe064f8f..300fe40059191 100644 --- a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c @@ -3,6 +3,8 @@ #include <linux/ptp_classify.h> #include "lan966x_main.h" +#include "vcap_api.h" +#include "vcap_api_client.h" #define LAN966X_MAX_PTP_ID 512 @@ -18,6 +20,17 @@ #define TOD_ACC_PIN 0x7 +/* This represents the base rule ID for the PTP rules that are added in the + * VCAP to trap frames to CPU. This number needs to be bigger than the maximum + * number of entries that can exist in the VCAP. + */ +#define LAN966X_VCAP_PTP_RULE_ID 1000000 +#define LAN966X_VCAP_L2_PTP_TRAP (LAN966X_VCAP_PTP_RULE_ID + 0) +#define LAN966X_VCAP_IPV4_EV_PTP_TRAP (LAN966X_VCAP_PTP_RULE_ID + 1) +#define LAN966X_VCAP_IPV4_GEN_PTP_TRAP (LAN966X_VCAP_PTP_RULE_ID + 2) +#define LAN966X_VCAP_IPV6_EV_PTP_TRAP (LAN966X_VCAP_PTP_RULE_ID + 3) +#define LAN966X_VCAP_IPV6_GEN_PTP_TRAP (LAN966X_VCAP_PTP_RULE_ID + 4) + enum { PTP_PIN_ACTION_IDLE = 0, PTP_PIN_ACTION_LOAD, @@ -35,19 +48,228 @@ static u64 lan966x_ptp_get_nominal_value(void) return 0x304d4873ecade305; } +static int lan966x_ptp_add_trap(struct lan966x_port *port, + int (*add_ptp_key)(struct vcap_rule *vrule, + struct lan966x_port*), + u32 rule_id, + u16 proto) +{ + struct lan966x *lan966x = port->lan966x; + struct vcap_rule *vrule; + int err; + + vrule = vcap_get_rule(lan966x->vcap_ctrl, rule_id); + if (vrule) { + u32 value, mask; + + /* Just modify the ingress port mask and exit */ + vcap_rule_get_key_u32(vrule, VCAP_KF_IF_IGR_PORT_MASK, + &value, &mask); + mask &= ~BIT(port->chip_port); + vcap_rule_mod_key_u32(vrule, VCAP_KF_IF_IGR_PORT_MASK, + value, mask); + + err = vcap_mod_rule(vrule); + goto free_rule; + } + + vrule = vcap_alloc_rule(lan966x->vcap_ctrl, port->dev, + LAN966X_VCAP_CID_IS2_L0, + VCAP_USER_PTP, 0, rule_id); + if (!vrule) + return -ENOMEM; + if (IS_ERR(vrule)) + return PTR_ERR(vrule); + + err = add_ptp_key(vrule, port); + if (err) + goto free_rule; + + err = vcap_set_rule_set_actionset(vrule, VCAP_AFS_BASE_TYPE); + err |= vcap_rule_add_action_bit(vrule, VCAP_AF_CPU_COPY_ENA, VCAP_BIT_1); + err |= vcap_rule_add_action_u32(vrule, VCAP_AF_MASK_MODE, LAN966X_PMM_REPLACE); + err |= vcap_val_rule(vrule, proto); + if (err) + goto free_rule; + + err = vcap_add_rule(vrule); + +free_rule: + /* Free the local copy of the rule */ + vcap_free_rule(vrule); + return err; +} + +static int lan966x_ptp_del_trap(struct lan966x_port *port, + u32 rule_id) +{ + struct lan966x *lan966x = port->lan966x; + struct vcap_rule *vrule; + u32 value, mask; + int err; + + vrule = vcap_get_rule(lan966x->vcap_ctrl, rule_id); + if (!vrule) + return -EEXIST; + + vcap_rule_get_key_u32(vrule, VCAP_KF_IF_IGR_PORT_MASK, &value, &mask); + mask |= BIT(port->chip_port); + + /* No other port requires this trap, so it is safe to remove it */ + if (mask == GENMASK(lan966x->num_phys_ports, 0)) { + err = vcap_del_rule(lan966x->vcap_ctrl, port->dev, rule_id); + goto free_rule; + } + + vcap_rule_mod_key_u32(vrule, VCAP_KF_IF_IGR_PORT_MASK, value, mask); + err = vcap_mod_rule(vrule); + +free_rule: + vcap_free_rule(vrule); + return err; +} + +static int lan966x_ptp_add_l2_key(struct vcap_rule *vrule, + struct lan966x_port *port) +{ + return vcap_rule_add_key_u32(vrule, VCAP_KF_ETYPE, ETH_P_1588, ~0); +} + +static int lan966x_ptp_add_ip_event_key(struct vcap_rule *vrule, + struct lan966x_port *port) +{ + return vcap_rule_add_key_u32(vrule, VCAP_KF_L4_DPORT, PTP_EV_PORT, ~0) || + vcap_rule_add_key_bit(vrule, VCAP_KF_TCP_IS, VCAP_BIT_0); +} + +static int lan966x_ptp_add_ip_general_key(struct vcap_rule *vrule, + struct lan966x_port *port) +{ + return vcap_rule_add_key_u32(vrule, VCAP_KF_L4_DPORT, PTP_GEN_PORT, ~0) || + vcap_rule_add_key_bit(vrule, VCAP_KF_TCP_IS, VCAP_BIT_0); +} + +static int lan966x_ptp_add_l2_rule(struct lan966x_port *port) +{ + return lan966x_ptp_add_trap(port, lan966x_ptp_add_l2_key, + LAN966X_VCAP_L2_PTP_TRAP, ETH_P_ALL); +} + +static int lan966x_ptp_add_ipv4_rules(struct lan966x_port *port) +{ + int err; + + err = lan966x_ptp_add_trap(port, lan966x_ptp_add_ip_event_key, + LAN966X_VCAP_IPV4_EV_PTP_TRAP, ETH_P_IP); + if (err) + return err; + + err = lan966x_ptp_add_trap(port, lan966x_ptp_add_ip_general_key, + LAN966X_VCAP_IPV4_GEN_PTP_TRAP, ETH_P_IP); + if (err) + lan966x_ptp_del_trap(port, LAN966X_VCAP_IPV4_EV_PTP_TRAP); + + return err; +} + +static int lan966x_ptp_add_ipv6_rules(struct lan966x_port *port) +{ + int err; + + err = lan966x_ptp_add_trap(port, lan966x_ptp_add_ip_event_key, + LAN966X_VCAP_IPV6_EV_PTP_TRAP, ETH_P_IPV6); + if (err) + return err; + + err = lan966x_ptp_add_trap(port, lan966x_ptp_add_ip_general_key, + LAN966X_VCAP_IPV6_GEN_PTP_TRAP, ETH_P_IPV6); + if (err) + lan966x_ptp_del_trap(port, LAN966X_VCAP_IPV6_EV_PTP_TRAP); + + return err; +} + +static int lan966x_ptp_del_l2_rule(struct lan966x_port *port) +{ + return lan966x_ptp_del_trap(port, LAN966X_VCAP_L2_PTP_TRAP); +} + +static int lan966x_ptp_del_ipv4_rules(struct lan966x_port *port) +{ + int err; + + err = lan966x_ptp_del_trap(port, LAN966X_VCAP_IPV4_EV_PTP_TRAP); + err |= lan966x_ptp_del_trap(port, LAN966X_VCAP_IPV4_GEN_PTP_TRAP); + + return err; +} + +static int lan966x_ptp_del_ipv6_rules(struct lan966x_port *port) +{ + int err; + + err = lan966x_ptp_del_trap(port, LAN966X_VCAP_IPV6_EV_PTP_TRAP); + err |= lan966x_ptp_del_trap(port, LAN966X_VCAP_IPV6_GEN_PTP_TRAP); + + return err; +} + +static int lan966x_ptp_add_traps(struct lan966x_port *port) +{ + int err; + + err = lan966x_ptp_add_l2_rule(port); + if (err) + goto err_l2; + + err = lan966x_ptp_add_ipv4_rules(port); + if (err) + goto err_ipv4; + + err = lan966x_ptp_add_ipv6_rules(port); + if (err) + goto err_ipv6; + + return err; + +err_ipv6: + lan966x_ptp_del_ipv4_rules(port); +err_ipv4: + lan966x_ptp_del_l2_rule(port); +err_l2: + return err; +} + +int lan966x_ptp_del_traps(struct lan966x_port *port) +{ + int err; + + err = lan966x_ptp_del_l2_rule(port); + err |= lan966x_ptp_del_ipv4_rules(port); + err |= lan966x_ptp_del_ipv6_rules(port); + + return err; +} + +int lan966x_ptp_setup_traps(struct lan966x_port *port, struct ifreq *ifr) +{ + struct hwtstamp_config cfg; + + if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg))) + return -EFAULT; + + if (cfg.rx_filter == HWTSTAMP_FILTER_NONE) + return lan966x_ptp_del_traps(port); + else + return lan966x_ptp_add_traps(port); +} + int lan966x_ptp_hwtstamp_set(struct lan966x_port *port, struct ifreq *ifr) { struct lan966x *lan966x = port->lan966x; struct hwtstamp_config cfg; struct lan966x_phc *phc; - /* For now don't allow to run ptp on ports that are part of a bridge, - * because in case of transparent clock the HW will still forward the - * frames, so there would be duplicate frames - */ - if (lan966x->bridge_mask & BIT(port->chip_port)) - return -EINVAL; - if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg))) return -EFAULT; diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c b/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c index 04a2afd683cca..ba3fa917d6b78 100644 --- a/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c @@ -4,14 +4,6 @@ #include "vcap_api.h" #include "vcap_api_client.h" -/* Controls how PORT_MASK is applied */ -enum LAN966X_PORT_MASK_MODE { - LAN966X_PMM_NO_ACTION, - LAN966X_PMM_REPLACE, - LAN966X_PMM_FORWARDING, - LAN966X_PMM_REDIRECT, -}; - struct lan966x_tc_flower_parse_usage { struct flow_cls_offload *f; struct flow_rule *frule; diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c b/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c index 44f40d9149470..d8dc9fbb81e1a 100644 --- a/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_vcap_impl.c @@ -5,10 +5,6 @@ #include "vcap_api.h" #include "vcap_api_client.h" -#define LAN966X_VCAP_CID_IS2_L0 VCAP_CID_INGRESS_STAGE2_L0 /* IS2 lookup 0 */ -#define LAN966X_VCAP_CID_IS2_L1 VCAP_CID_INGRESS_STAGE2_L1 /* IS2 lookup 1 */ -#define LAN966X_VCAP_CID_IS2_MAX (VCAP_CID_INGRESS_STAGE2_L2 - 1) /* IS2 Max */ - #define STREAMSIZE (64 * 4) #define LAN966X_IS2_LOOKUPS 2 @@ -219,9 +215,12 @@ static void lan966x_vcap_add_default_fields(struct net_device *dev, struct vcap_rule *rule) { struct lan966x_port *port = netdev_priv(dev); + u32 value, mask; - vcap_rule_add_key_u32(rule, VCAP_KF_IF_IGR_PORT_MASK, 0, - ~BIT(port->chip_port)); + if (vcap_rule_get_key_u32(rule, VCAP_KF_IF_IGR_PORT_MASK, + &value, &mask)) + vcap_rule_add_key_u32(rule, VCAP_KF_IF_IGR_PORT_MASK, 0, + ~BIT(port->chip_port)); if (lan966x_vcap_is_first_chain(rule)) vcap_rule_add_key_bit(rule, VCAP_KF_LOOKUP_FIRST_IS,
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. Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> --- .../ethernet/microchip/lan966x/lan966x_main.c | 19 +- .../ethernet/microchip/lan966x/lan966x_main.h | 14 ++ .../ethernet/microchip/lan966x/lan966x_ptp.c | 236 +++++++++++++++++- .../microchip/lan966x/lan966x_tc_flower.c | 8 - .../microchip/lan966x/lan966x_vcap_impl.c | 11 +- 5 files changed, 265 insertions(+), 23 deletions(-)