net: dsa: microchip: issues when using PTP between KSZ9567 devices

Commit Message

Martin Whitaker Aug. 13, 2024, 10:04 a.m. UTC

Three issues. The first two look like hardware bugs. The third is a
driver bug.

I have an embedded processor board running Linux that incorporates a
KSZ9567 ethernet switch. The first port of the switch is used as a WAN
port. The second and third ports are used as two LAN ports. The aim is
to daisy-chain multiple boards via the LAN ports and synchronise their
clocks using PTP, with one board acting as the PTP grand master.
Currently I am testing this with only two boards and one active LAN
connection. My basic linuxptp configuration is

   [global]
   gmCapable               1
   network_transport       L2
   delay_mechanism         P2P
   time_stamping           p2p1step

   [lan1]

   [lan2]


Issue 1
-------
PTP messages sent to address 01:80:C2:00:00:0E are not being received.
tshark displays the messages on the transmitting device, but not on the
receiving device. I don't know how close to the wire tshark gets in the
DSA architecture, but this suggests that the hardware is filtering the
messages.

I can work around this issue by adding

   p2p_dst_mac            01:1B:19:00:00:00

to the linuxptp configuration.


Issue 2
-------
The source port ID field in the peer delay request messages is being
modified. linuxptp sets it to 1 for the first port it uses (lan1 in my
example) and 2 for the second port (lan2). tshark shows these IDs on the
transmitting device, but on the receiving device shows the IDs have been
changed to the switch physical port number (so 2 and 3 in my case).
Again this suggests the hardware is changing the IDs on the fly. This
only happens for peer delay request messages - the other PTP messages
retain the linuxptp source port IDs.

I have checked that the "Enable IEEE 802.1AS Mode" bit is being set in
the KSZ9567 "Global PTP Message Config 1" register. According to the
datasheet

   When this mode is enabled, it modifies the IEEE 1588
   mode behavior. Primarily it causes all PTP messages to
   be forwarded to the host port, and the switch will not
   modify PTP message headers.

so if the hardware is responsible, both this and issue 1 look to be
device bugs.

I am currently working round this issue by patching linuxptp to use the
physical port numbers as the source port IDs. I can't think of a general
solution to this issue.

Issue 3
-------
When performing the port_hwtstamp_set operation, ptp_schedule_worker()
will be called if hardware timestamoing is enabled on any of the ports.
When using multiple ports for PTP, port_hwtstamp_set is executed for
each port. When called for the first time ptp_schedule_worker() returns
0. On subsequent calls it returns 1, indicating the worker is already
scheduled. Currently the ksz_ptp module treats 1 as an error and fails
to complete the port_hwtstamp_set operation, thus leaving the
timestamping configuration for those ports unchanged.

(note that the documentation of ptp_schedule_worker refers you to
kthread_queue_delayed_work rather than documenting the return values,
but kthread_queue_delayed_work returns a bool, not an int)

I fixed this issue by


CC'ing the authors of the ksz_ptp module as well as the ksz9477 driver
maintainers.

Comments

Andrew Lunn Aug. 13, 2024, 7:55 p.m. UTC | #1

> Issue 3
> -------
> When performing the port_hwtstamp_set operation, ptp_schedule_worker()
> will be called if hardware timestamoing is enabled on any of the ports.
> When using multiple ports for PTP, port_hwtstamp_set is executed for
> each port. When called for the first time ptp_schedule_worker() returns
> 0. On subsequent calls it returns 1, indicating the worker is already
> scheduled. Currently the ksz_ptp module treats 1 as an error and fails
> to complete the port_hwtstamp_set operation, thus leaving the
> timestamping configuration for those ports unchanged.
> 
> (note that the documentation of ptp_schedule_worker refers you to
> kthread_queue_delayed_work rather than documenting the return values,
> but kthread_queue_delayed_work returns a bool, not an int)
> 
> I fixed this issue by
> 
> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c
> b/drivers/net/dsa/microchip/ksz_ptp.c
> index 4e22a695a64c..7ef5fac69657 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> @@ -266,7 +266,6 @@ static int ksz_ptp_enable_mode(struct ksz_device *dev)
>         struct ksz_port *prt;
>         struct dsa_port *dp;
>         bool tag_en = false;
> -       int ret;
> 
>         dsa_switch_for_each_user_port(dp, dev->ds) {
>                 prt = &dev->ports[dp->index];
> @@ -277,9 +276,7 @@ static int ksz_ptp_enable_mode(struct ksz_device *dev)
>         }
> 
>         if (tag_en) {
> -               ret = ptp_schedule_worker(ptp_data->clock, 0);
> -               if (ret)
> -                       return ret;
> +               ptp_schedule_worker(ptp_data->clock, 0);

This looks correct. Please could you submit a formal patch?

https://docs.kernel.org/process/submitting-patches.html
https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#netdev-faq

	Andrew

Christian Eggers Aug. 20, 2024, 3:19 p.m. UTC | #2

Hi Martin,

On Tuesday, 13 August 2024, 12:04:59 CEST, Martin Whitaker wrote:
> Three issues. The first two look like hardware bugs. The third is a
> driver bug.
> 
> I have an embedded processor board running Linux that incorporates a
> KSZ9567 ethernet switch. The first port of the switch is used as a WAN
> port. The second and third ports are used as two LAN ports. The aim is
> to daisy-chain multiple boards via the LAN ports and synchronise their
> clocks using PTP, with one board acting as the PTP grand master.
> Currently I am testing this with only two boards and one active LAN
> connection. My basic linuxptp configuration is
> 
>    [global]
>    gmCapable               1
>    network_transport       L2
>    delay_mechanism         P2P
>    time_stamping           p2p1step
> 
>    [lan1]
> 
>    [lan2]
> 
> 
> Issue 1
> -------
> PTP messages sent to address 01:80:C2:00:00:0E are not being received.
> tshark displays the messages on the transmitting device, but not on the
> receiving device. I don't know how close to the wire tshark gets in the
> DSA architecture, but this suggests that the hardware is filtering the
> messages.
> 
> I can work around this issue by adding
> 
>    p2p_dst_mac            01:1B:19:00:00:00
> 
> to the linuxptp configuration.

I use a KSZ9563 in a similar (daisy-chained) configuration. As non-PTP messages
must be forwarded (like a non-DSA switch would do), I have configured a bridge
above lan1 and lan2. If you also have such a bridged setup, you may need to add
extra ebtables rules, so that PTP traffic received on the lan1/lan2 ports is not
internally forwarded to the bridge interface. Otherwise ptp4l can probably not
receive any messages.

> Issue 2
> -------
> The source port ID field in the peer delay request messages is being
> modified. linuxptp sets it to 1 for the first port it uses (lan1 in my
> example) and 2 for the second port (lan2). tshark shows these IDs on the
> transmitting device, but on the receiving device shows the IDs have been
> changed to the switch physical port number (so 2 and 3 in my case).
> Again this suggests the hardware is changing the IDs on the fly. This
> only happens for peer delay request messages - the other PTP messages
> retain the linuxptp source port IDs.

I also remember that I saw such a behavior when I developed the initial PTP
support. In my case this wasn't a big issue as the port IDs from ptp4l matched
the physical port numbers of the switch. I guess that the hw developers of the
switch consider this as a feature and not a bug.

> 
> I have checked that the "Enable IEEE 802.1AS Mode" bit is being set in
> the KSZ9567 "Global PTP Message Config 1" register. According to the
> datasheet
> 
>    When this mode is enabled, it modifies the IEEE 1588
>    mode behavior. Primarily it causes all PTP messages to
>    be forwarded to the host port, and the switch will not
>    modify PTP message headers.
> 
> so if the hardware is responsible, both this and issue 1 look to be
> device bugs.
> 
> I am currently working round this issue by patching linuxptp to use the
> physical port numbers as the source port IDs. I can't think of a general
> solution to this issue.
> 
> Issue 3
> -------
> When performing the port_hwtstamp_set operation, ptp_schedule_worker()
> will be called if hardware timestamoing is enabled on any of the ports.
> When using multiple ports for PTP, port_hwtstamp_set is executed for
> each port. When called for the first time ptp_schedule_worker() returns
> 0. On subsequent calls it returns 1, indicating the worker is already
> scheduled. Currently the ksz_ptp module treats 1 as an error and fails
> to complete the port_hwtstamp_set operation, thus leaving the
> timestamping configuration for those ports unchanged.
> 
> (note that the documentation of ptp_schedule_worker refers you to
> kthread_queue_delayed_work rather than documenting the return values,
> but kthread_queue_delayed_work returns a bool, not an int)
> 
> I fixed this issue by
> 
> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c
> b/drivers/net/dsa/microchip/ksz_ptp.c
> index 4e22a695a64c..7ef5fac69657 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> @@ -266,7 +266,6 @@ static int ksz_ptp_enable_mode(struct ksz_device *dev)
>          struct ksz_port *prt;
>          struct dsa_port *dp;
>          bool tag_en = false;
> -       int ret;
> 
>          dsa_switch_for_each_user_port(dp, dev->ds) {
>                  prt = &dev->ports[dp->index];
> @@ -277,9 +276,7 @@ static int ksz_ptp_enable_mode(struct ksz_device *dev)
>          }
> 
>          if (tag_en) {
> -               ret = ptp_schedule_worker(ptp_data->clock, 0);
> -               if (ret)
> -                       return ret;
> +               ptp_schedule_worker(ptp_data->clock, 0);
>          } else {
>                  ptp_cancel_worker_sync(ptp_data->clock);
>          }
> 
> CC'ing the authors of the ksz_ptp module as well as the ksz9477 driver
> maintainers.
> 

regards,
Christian

Patch

diff --git a/drivers/net/dsa/microchip/ksz_ptp.c
b/drivers/net/dsa/microchip/ksz_ptp.c
index 4e22a695a64c..7ef5fac69657 100644
--- a/drivers/net/dsa/microchip/ksz_ptp.c
+++ b/drivers/net/dsa/microchip/ksz_ptp.c
@@ -266,7 +266,6 @@  static int ksz_ptp_enable_mode(struct ksz_device *dev)
         struct ksz_port *prt;
         struct dsa_port *dp;
         bool tag_en = false;
-       int ret;

         dsa_switch_for_each_user_port(dp, dev->ds) {
                 prt = &dev->ports[dp->index];
@@ -277,9 +276,7 @@  static int ksz_ptp_enable_mode(struct ksz_device *dev)
         }

         if (tag_en) {
-               ret = ptp_schedule_worker(ptp_data->clock, 0);
-               if (ret)
-                       return ret;
+               ptp_schedule_worker(ptp_data->clock, 0);
         } else {
                 ptp_cancel_worker_sync(ptp_data->clock);
         }