Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.debian.kernel > #79128 > unrolled thread

Bug#1036744: PTP in combination with vclocks partially broken on Debian kernels

Back to article view | Back to linux.debian.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Bug#1036744: PTP in combination with vclocks partially broken on Debian kernels Salvatore Bonaccorso <carnil@debian.org> - 2023-05-25 10:10 +0200
    Bug#1036744: PTP in combination with vclocks partially broken on Debian kernels Salvatore Bonaccorso <carnil@debian.org> - 2023-05-25 10:40 +0200
      Bug#1036744: PTP in combination with vclocks partially broken on Debian kernels Ben Hutchings <benh@debian.org> - 2023-05-25 15:10 +0200
        Bug#1036744: PTP in combination with vclocks partially broken on Debian kernels Salvatore Bonaccorso <carnil@debian.org> - 2023-05-25 16:00 +0200
          Bug#1036744: PTP in combination with vclocks partially broken on Debian kernels Salvatore Bonaccorso <carnil@debian.org> - 2023-07-24 14:10 +0200
    Bug#1036744: PTP in combination with vclocks partially broken on Debian kernels Florian Bezdeka <florian.bezdeka@siemens.com> - 2023-05-25 10:40 +0200

#79128 — Bug#1036744: PTP in combination with vclocks partially broken on Debian kernels

Control: tags -1 + moreinfo

On Thu, May 25, 2023 at 07:21:41AM +0000, Bezdeka, Florian wrote:
> Package: linux-image-amd64
> Version: 6.1.27-1
> 
> Hi all,
> 
> we did some investigations regarding time synchronization on Debian. 
> Background is industrial communication on Linux in general. 
> 
> We found out that the PTP subsystem is only partially usable in
> combination with virtual clocks on Debian. While the setup was running
> fine on other distributions (like Ubuntu and Fedora) we had problems to
> bring it up and running on Debian.
> 
> When starting ptp4l in combination with a vclock, the vclock could be
> created but not attached to a physical clock:
> 
> ptp4l[127.805]: selected /dev/ptp4 as PTP clock
> ptp4l[127.805]: port 1 (enptp): PHC device mismatch
> ptp4l[127.805]: port 1 (enptp): /dev/ptp4 requested, ptp0 attached
> ptp4l[127.805]: failed to open port enptp
> failed to create a clock
> 
> It turned out that CONFIG_PTP_1588_CLOCK makes the difference. When
> enabled (built-in, =y) our setup works, when set to "m" we run into
> problems. The "problematic" code can be found at [1].
> 
> Might I request to set CONFIG_PTP_1588_CLOCK to y in the Debian kernel
> configuration?

Question: Is this a bug, why does the code needs to expect it to be
builtin? (I'm not meaning what is present actually in the code, but
why it *needs* to be builtin on upstream side)? AFAICS the requirement
is added with e5f31552674e ("ethernet: fix PTP_1588_CLOCK
dependencies") in 5.15-rc1. As explained in the commit in the
following:

|     However, the two recently added ptp_get_vclocks_index() and
|     ptp_convert_timestamp() interfaces are only called from builtin code with
|     ethtool and socket timestamps, so keep the current behavior by stubbing
|     those out completely when PTP is in a loadable module. This should be
|     addressed properly in a follow-up.
| 
|     As Richard suggested, we may want to actually turn PTP support into a
|     'bool' option later on, preventing it from being a loadable module
|     altogether, which would be one way to solve the problem with the ethtool
|     interface.

Ben, would you agree on make the PTP support.

> It would be possible that we provide a merge request on
> salsa.debian.org. Just tell me the correct target branch. We would be
> very happy to have it in 6.1 (bookworm) and of course in all kernel
> flavors/feature sets.

I do not plan do accept any further bookworm targetting updates *right
now*, mabye later for bookworm point releases as we are now in full
freeze. There is no further upload planned for the 6.1.y series to
unstable, following to be migrated in testing before the bookworm
release. We can first apply to master and experimental upload if
agreed on the following change above and then consider it later for
bookworm. 

Hope this helps already

Regards,
Salvatore

[toc] | [next] | [standalone]

#79129

Hi Florian,

[dropping a typoed mail from my to not cause further bounces]

On Thu, May 25, 2023 at 10:18:46AM +0200, Florian Bezdeka wrote:
> On Thu, 2023-05-25 at 10:03 +0200, Salvatore Bonaccorso wrote:
> > Control: tags -1 + moreinfo
> > 
> > On Thu, May 25, 2023 at 07:21:41AM +0000, Bezdeka, Florian wrote:
> > > Package: linux-image-amd64
> > > Version: 6.1.27-1
> > > 
> > > Hi all,
> > > 
> > > we did some investigations regarding time synchronization on Debian. 
> > > Background is industrial communication on Linux in general. 
> > > 
> > > We found out that the PTP subsystem is only partially usable in
> > > combination with virtual clocks on Debian. While the setup was running
> > > fine on other distributions (like Ubuntu and Fedora) we had problems to
> > > bring it up and running on Debian.
> > > 
> > > When starting ptp4l in combination with a vclock, the vclock could be
> > > created but not attached to a physical clock:
> > > 
> > > ptp4l[127.805]: selected /dev/ptp4 as PTP clock
> > > ptp4l[127.805]: port 1 (enptp): PHC device mismatch
> > > ptp4l[127.805]: port 1 (enptp): /dev/ptp4 requested, ptp0 attached
> > > ptp4l[127.805]: failed to open port enptp
> > > failed to create a clock
> > > 
> > > It turned out that CONFIG_PTP_1588_CLOCK makes the difference. When
> > > enabled (built-in, =y) our setup works, when set to "m" we run into
> > > problems. The "problematic" code can be found at [1].
> > > 
> > > Might I request to set CONFIG_PTP_1588_CLOCK to y in the Debian kernel
> > > configuration?
> > 
> > Question: Is this a bug, why does the code needs to expect it to be
> > builtin? (I'm not meaning what is present actually in the code, but
> > why it *needs* to be builtin on upstream side)? AFAICS the requirement
> > is added with e5f31552674e ("ethernet: fix PTP_1588_CLOCK
> > dependencies") in 5.15-rc1. As explained in the commit in the
> > following:
> > 
> > >     However, the two recently added ptp_get_vclocks_index() and
> > >     ptp_convert_timestamp() interfaces are only called from builtin code with
> > >     ethtool and socket timestamps, so keep the current behavior by stubbing
> > >     those out completely when PTP is in a loadable module. This should be
> > >     addressed properly in a follow-up.
> > > 
> > >     As Richard suggested, we may want to actually turn PTP support into a
> > >     'bool' option later on, preventing it from being a loadable module
> > >     altogether, which would be one way to solve the problem with the ethtool
> > >     interface.
> > 
> 
> I re-visited the changes from Arnd as well. Seems this "follow up"
> never happened. 

Ok great for confirming.

> 
> > Ben, would you agree on make the PTP support.
> > 
> > > It would be possible that we provide a merge request on
> > > salsa.debian.org. Just tell me the correct target branch. We would be
> > > very happy to have it in 6.1 (bookworm) and of course in all kernel
> > > flavors/feature sets.
> > 
> > I do not plan do accept any further bookworm targetting updates *right
> > now*, mabye later for bookworm point releases as we are now in full
> > freeze. There is no further upload planned for the 6.1.y series to
> > unstable, following to be migrated in testing before the bookworm
> > release. We can first apply to master and experimental upload if
> > agreed on the following change above and then consider it later for
> > bookworm. 
> 
> I think a point release would be acceptable for us. Sounds like a plan!
> Thanks!

Let's see if there are comments from Ben or Arnd on the proposed
change.

Regards,
Salvatore

[toc] | [prev] | [next] | [standalone]

#79135

On Thu, 2023-05-25 at 10:37 +0200, Salvatore Bonaccorso wrote:
> Hi Florian,
> 
> [dropping a typoed mail from my to not cause further bounces]
> 
> On Thu, May 25, 2023 at 10:18:46AM +0200, Florian Bezdeka wrote:
> > On Thu, 2023-05-25 at 10:03 +0200, Salvatore Bonaccorso wrote:
[...]
> > 
> > > Ben, would you agree on make the PTP support.
> > > 
> > > > It would be possible that we provide a merge request on
> > > > salsa.debian.org. Just tell me the correct target branch. We would be
> > > > very happy to have it in 6.1 (bookworm) and of course in all kernel
> > > > flavors/feature sets.
> > > 
> > > I do not plan do accept any further bookworm targetting updates *right
> > > now*, mabye later for bookworm point releases as we are now in full
> > > freeze. There is no further upload planned for the 6.1.y series to
> > > unstable, following to be migrated in testing before the bookworm
> > > release. We can first apply to master and experimental upload if
> > > agreed on the following change above and then consider it later for
> > > bookworm. 
> > 
> > I think a point release would be acceptable for us. Sounds like a plan!
> > Thanks!
> 
> Let's see if there are comments from Ben or Arnd on the proposed
> change.

I agree we should make this built-in (except on armel/marvell).

Ben.

-- 
Ben Hutchings - Debian developer, member of kernel, installer and LTS
teams

[toc] | [prev] | [next] | [standalone]

#79137

Control: tags -1 - moreinfo
Control: tags -1 + confirmed

Hi Ben,

On Thu, May 25, 2023 at 03:07:24PM +0200, Ben Hutchings wrote:
> On Thu, 2023-05-25 at 10:37 +0200, Salvatore Bonaccorso wrote:
> > Hi Florian,
> > 
> > [dropping a typoed mail from my to not cause further bounces]
> > 
> > On Thu, May 25, 2023 at 10:18:46AM +0200, Florian Bezdeka wrote:
> > > On Thu, 2023-05-25 at 10:03 +0200, Salvatore Bonaccorso wrote:
> [...]
> > > 
> > > > Ben, would you agree on make the PTP support.
> > > > 
> > > > > It would be possible that we provide a merge request on
> > > > > salsa.debian.org. Just tell me the correct target branch. We would be
> > > > > very happy to have it in 6.1 (bookworm) and of course in all kernel
> > > > > flavors/feature sets.
> > > > 
> > > > I do not plan do accept any further bookworm targetting updates *right
> > > > now*, mabye later for bookworm point releases as we are now in full
> > > > freeze. There is no further upload planned for the 6.1.y series to
> > > > unstable, following to be migrated in testing before the bookworm
> > > > release. We can first apply to master and experimental upload if
> > > > agreed on the following change above and then consider it later for
> > > > bookworm. 
> > > 
> > > I think a point release would be acceptable for us. Sounds like a plan!
> > > Thanks!
> > 
> > Let's see if there are comments from Ben or Arnd on the proposed
> > change.
> 
> I agree we should make this built-in (except on armel/marvell).

Thanks for confirming, so this change can go into the master branch
first.

Regards,
Salvatore

[toc] | [prev] | [next] | [standalone]

#79744

Hi Florian,

On Mon, Jul 24, 2023 at 01:08:16PM +0200, Florian Bezdeka wrote:
> On Thu, 2023-05-25 at 15:55 +0200, Salvatore Bonaccorso wrote:
> 
> > 
> > Thanks for confirming, so this change can go into the master branch
> > first.
> 
> I expected to have this bug fixed in the next point release, which was
> released recently. The "version graph" visible at [1] (upper right
> corner) mentions the 6.1.38 release as well but it seems the kernel
> configuration file stayed unmodified.
> 
> Any additional action expected from my side?

Right, now that the change is in testing, we can pick it up as well
for bookworm, which got almost forgotten, thanks for your ping.

I picked it up
https://salsa.debian.org/kernel-team/linux/-/commit/81db00d5a849c316aeb02311f15dbd0640ca3302
which means if no surprises arises, it will be in 12.2.

Regards,
Salvatore

[toc] | [prev] | [next] | [standalone]

#79130

On Thu, 2023-05-25 at 10:03 +0200, Salvatore Bonaccorso wrote:
> Control: tags -1 + moreinfo
> 
> On Thu, May 25, 2023 at 07:21:41AM +0000, Bezdeka, Florian wrote:
> > Package: linux-image-amd64
> > Version: 6.1.27-1
> > 
> > Hi all,
> > 
> > we did some investigations regarding time synchronization on Debian. 
> > Background is industrial communication on Linux in general. 
> > 
> > We found out that the PTP subsystem is only partially usable in
> > combination with virtual clocks on Debian. While the setup was running
> > fine on other distributions (like Ubuntu and Fedora) we had problems to
> > bring it up and running on Debian.
> > 
> > When starting ptp4l in combination with a vclock, the vclock could be
> > created but not attached to a physical clock:
> > 
> > ptp4l[127.805]: selected /dev/ptp4 as PTP clock
> > ptp4l[127.805]: port 1 (enptp): PHC device mismatch
> > ptp4l[127.805]: port 1 (enptp): /dev/ptp4 requested, ptp0 attached
> > ptp4l[127.805]: failed to open port enptp
> > failed to create a clock
> > 
> > It turned out that CONFIG_PTP_1588_CLOCK makes the difference. When
> > enabled (built-in, =y) our setup works, when set to "m" we run into
> > problems. The "problematic" code can be found at [1].
> > 
> > Might I request to set CONFIG_PTP_1588_CLOCK to y in the Debian kernel
> > configuration?
> 
> Question: Is this a bug, why does the code needs to expect it to be
> builtin? (I'm not meaning what is present actually in the code, but
> why it *needs* to be builtin on upstream side)? AFAICS the requirement
> is added with e5f31552674e ("ethernet: fix PTP_1588_CLOCK
> dependencies") in 5.15-rc1. As explained in the commit in the
> following:
> 
> >     However, the two recently added ptp_get_vclocks_index() and
> >     ptp_convert_timestamp() interfaces are only called from builtin code with
> >     ethtool and socket timestamps, so keep the current behavior by stubbing
> >     those out completely when PTP is in a loadable module. This should be
> >     addressed properly in a follow-up.
> > 
> >     As Richard suggested, we may want to actually turn PTP support into a
> >     'bool' option later on, preventing it from being a loadable module
> >     altogether, which would be one way to solve the problem with the ethtool
> >     interface.
> 

I re-visited the changes from Arnd as well. Seems this "follow up"
never happened. 


> Ben, would you agree on make the PTP support.
> 
> > It would be possible that we provide a merge request on
> > salsa.debian.org. Just tell me the correct target branch. We would be
> > very happy to have it in 6.1 (bookworm) and of course in all kernel
> > flavors/feature sets.
> 
> I do not plan do accept any further bookworm targetting updates *right
> now*, mabye later for bookworm point releases as we are now in full
> freeze. There is no further upload planned for the 6.1.y series to
> unstable, following to be migrated in testing before the bookworm
> release. We can first apply to master and experimental upload if
> agreed on the following change above and then consider it later for
> bookworm. 

I think a point release would be acceptable for us. Sounds like a plan!
Thanks!

> 
> Hope this helps already
> 
> Regards,
> Salvatore

[toc] | [prev] | [standalone]

Back to top | Article view | linux.debian.kernel

csiph-web