Re: [PATCH v2 1/3] PCI: Add sysfs support for exposing PTM context

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Apr 07, 2025 at 01:14:56PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Mar 24, 2025 at 11:28:54AM -0500, Bjorn Helgaas wrote:
> > On Mon, Mar 24, 2025 at 03:34:35PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > 
> > > Precision Time Management (PTM) mechanism defined in PCIe spec
> > > r6.0, sec 6.22 allows precise coordination of timing information
> > > across multiple components in a PCIe hierarchy with independent
> > > local time clocks.
> > > 
> > > PCI core already supports enabling PTM in the root port and
> > > endpoint devices through PTM Extended Capability registers. But
> > > the PTM context supported by the PTM capable components such as
> > > Root Complex (RC) and Endpoint (EP) controllers were not exposed
> > > as of now.
> > > 
> > > Hence, add the sysfs support to expose the PTM context to
> > > userspace from both PCIe RC and EP controllers. Controller
> > > drivers are expected to call pcie_ptm_create_sysfs() to create
> > > the sysfs attributes for the PTM context and call
> > > pcie_ptm_destroy_sysfs() to destroy them. The drivers should
> > > also populate the relevant callbacks in the 'struct
> > > pcie_ptm_ops' structure based on the controller implementation.
> > 
> > Can we include some motivation here, e.g., what is the value of
> > exposing this information?  Is this for debugging or bringup
> > purposes?  Can users or administrators use this for something?
> > Obviously they can read and update some internal PTM state, but it
> > would be nice to know what that's good for.
> 
> This was a request from one of the Qualcomm customers, but they
> didn't share how they are using these context. They just said that
> they want to collect the PTM timestamps for comparing with PTP
> timestamps from a different PCIe switch. That was not a worth of
> information to be mentioned in the cover letter, so I skipped it
> intentionally.

I think it is important to include a reason for merging a change.  The
mere fact that information exists is not enough reason to expose it
as a sysfs ABI.

> >  Consequently this probably can't be done by generic drivers like
> >  ACPI, and maybe this is a candidate for debugfs instead of sysfs.
> 
> Well, we can still create sysfs ABI for vendor specific features.
> Problem with debugfs is that the customers cannot use debugfs in a
> production environment.  Moreover, I cannot strictly classify PTM
> context as a debugging information.

I'm not convinced about making sysfs ABI for vendor-specific features.
I see that we do have a few existing things like this:

  Documentation/ABI/testing/sysfs-bus-pci-drivers-janz-cmodio
  Documentation/ABI/testing/sysfs-bus-pci-devices-cciss
  Documentation/ABI/testing/sysfs-bus-pci-devices-avs

but I'm a bit hesitant to extend this model.

Bjorn