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

[Salvatore Bonaccorso <carnil@debian.org>]

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