Re: [PATCH v2] cxl/pci: Set the device timestamp

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Wed, 22 Feb 2023 21:55:09 -0600
Nabeel M Mohamed <nabeelmd.linux@gmail.com> wrote:

> On Wed, Feb 22, 2023 at 04:27:00PM +0000, Jonathan Cameron wrote:
> > On Wed, 22 Feb 2023 09:34:14 -0600
> > Nabeel M Mohamed <nabeelmd.linux@gmail.com> wrote:
> >   
> > > On Mon, Jan 30, 2023 at 03:13:27PM +0000, Jonathan Cameron wrote:  
> > > > CXL r3.0 section 8.2.9.4.2 "Set Timestamp" recommends that the host sets
> > > > the timestamp after every Conventional or CXL Reset to ensure accurate
> > > > timestamps. This should include on initial boot up. The time base that
> > > > is being set is used by a device for the poison list overflow timestamp
> > > > and all event timestamps.  Note that the command is optional and if
> > > > not supported and the device cannot return accurate timestamps it will
> > > > fill the fields in with an appropriate marker (see the specification
> > > > description of each timestamp).
> > > > 
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > >    
> > > 
> > > Hi Jonathan,
> > > 
> > > Could you please educate me on the reason for not supporting a
> > > "Get Timestamp" mailbox command at this time?
> > > 
> > > A use-case that I can think of for having "Get Timestamp" in the UAPI
> > > is to enable userspace to better correlate the timestamp in the device
> > > logs with the wall time by querying the current timestamp on the device.
> > > This could be useful if there's some inaccuracy or skew in the device
> > > timestamp.
> > > 
> > > Would that be a valid use-case or am I misunderstanding something here?
> > > 
> > > I can see why "Set Timestamp" is a kernel-internal command but fail to
> > > see why "Get Timestamp" cannot be in the UAPI.  
> >   
> 
> > Hi Nabeel,
> > 
> > I'd be fine with a patch adding the support, it just didn't solve my
> > immediate problem so I didn't want to make this patch more complex.
> > 
> > Jonathan  
> 
> Great, thanks for clarifying Jonathan!
> 
> I'll be happy to post a patch which adds support for "Get Timestamp".
> 
> Btw, this will be my first patch to the CXL driver.
> So, I'm wondering if there's any checklist listing the prerequisites
> for submitting a patch to the CXL driver - for instance, requirements
> on adding new tests for coverage, any existing CXL regression suites
> that the patch must pass, etc.
> 
> Any pointers on the above would be greatly appreciated!

As we are talking about a userspace exposed feature, support on the ndctl
/cxl tools should probably follow soon after the kernel side of things.

Jonathan

> 
> Thanks,
> Nabeel