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

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

On Thu, 26 Jan 2023 10:56:20 -0800
Davidlohr Bueso <dave@stgolabs.net> wrote:

> On Thu, 26 Jan 2023, Jonathan Cameron wrote:
> 
> >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).  
> 
> A few questions (which I'm missing from the spec):
> 
> - How to verify if a ts was actually set? I would assume that if SET
> is unsupported, GET will also be, and since you set the ts at
> bootup, it should never occur in reality.

I don't think we care.  If a record comes back with the magic code (all
zeros) that is in the spec. Get can still be supported.

> 
> - Does "unsupported" return type mean the command cannot mean that
> flat out the device cannot handle timestamps or is it more that the
> device cannot handle nanosecond resolution?

Neither - it just means that it cannot be set by this means.
There is no provision for anything other than nanoseconds, but there is also
no statement on accuracy.  So if you only do seconds, multiply the number
by 1x10^9

The device might have another means of setting it. MCTP for example
or in theory might have an RTC.  The OS can only tell if that is true
when it sees a timestamp.



> 
> ...
> 
> >@@ -65,6 +65,7 @@ static struct cxl_mem_command cxl_mem_commands[CXL_MEM_COMMAND_ID_MAX] = {
> >	CXL_CMD(GET_SCAN_MEDIA_CAPS, 0x10, 0x4, 0),
> >	CXL_CMD(SCAN_MEDIA, 0x11, 0, 0),
> >	CXL_CMD(GET_SCAN_MEDIA, 0, CXL_VARIABLE_PAYLOAD, 0),
> >+	CXL_CMD(SET_TIMESTAMP, 8, 0, 0),
> > };
> >
> > /*
> >@@ -93,6 +94,7 @@ static u16 cxl_disabled_raw_commands[] = {
> >	CXL_MBOX_OP_SET_SHUTDOWN_STATE,
> >	CXL_MBOX_OP_SCAN_MEDIA,
> >	CXL_MBOX_OP_GET_SCAN_MEDIA,
> >+	CXL_MBOX_OP_SET_TIMESTAMP,  
> 
> Why no GET support?
> 
> > };
> >
> > /*
> >@@ -857,6 +859,29 @@ int cxl_mem_create_range_info(struct cxl_dev_state *cxlds)
> > }
> > EXPORT_SYMBOL_NS_GPL(cxl_mem_create_range_info, CXL);
> >
> >+int cxl_set_timestamp(struct cxl_dev_state *cxlds, u64 ts)
> >+{
> >+	struct cxl_mbox_cmd mbox_cmd;
> >+	struct cxl_mbox_set_timestamp_in pi;
> >+
> >+	/*
> >+	 * Command is optional and functionality should not be affected if
> >+	 * the command is not available.
> >+	 */
> >+	if (!test_bit(CXL_MEM_COMMAND_ID_SET_TIMESTAMP, cxlds->enabled_cmds))
> >+		return 0;
> >+
> >+	pi.timestamp = ts;  
> 
> Considering we always want "now", maybe just remove the parameter and just
> compute ktime_get_real_ns() here?

Sure. We could do that, and bring the parameter back if we want to
do anything clever in future.

> 
> >+	mbox_cmd = (struct cxl_mbox_cmd) {
> >+		.opcode = CXL_MBOX_OP_SET_TIMESTAMP,
> >+		.size_in = sizeof(pi),
> >+		.payload_in = &pi,
> >+	};
> >+
> >+	return cxl_internal_send_cmd(cxlds, &mbox_cmd);
> >+}  
> 
> Thanks,
> Davidlohr