[2/3] cxl: Introduce module parameter 'enable_psltrace'

Message ID 20180209042535.16845-3-vaibhav@linux.vnet.ibm.com
State New
Headers show
Series
  • Provide ability to enable PSL traces on card probe
Related show

Commit Message

Vaibhav Jain Feb. 9, 2018, 4:25 a.m.
We introduce a new module parameter named 'enable_psltrace' which asks cxl
to start(by default) psl-traces on an adapter as soon as its probe is
finished. In case this default behavior is not needed then this
module parameter can be set to '0'.

Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
---
 drivers/misc/cxl/cxl.h  | 2 ++
 drivers/misc/cxl/main.c | 4 ++++
 drivers/misc/cxl/pci.c  | 3 +++
 3 files changed, 9 insertions(+)

Comments

Christophe Lombard Feb. 9, 2018, 1:14 p.m. | #1
Le 09/02/2018 à 05:25, Vaibhav Jain a écrit :
> We introduce a new module parameter named 'enable_psltrace' which asks cxl
> to start(by default) psl-traces on an adapter as soon as its probe is
> finished. In case this default behavior is not needed then this
> module parameter can be set to '0'.
> 
> Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
> ---
>   drivers/misc/cxl/cxl.h  | 2 ++
>   drivers/misc/cxl/main.c | 4 ++++
>   drivers/misc/cxl/pci.c  | 3 +++
>   3 files changed, 9 insertions(+)
> 
> diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
> index 81da307b60c0..1af66451cbb4 100644
> --- a/drivers/misc/cxl/cxl.h
> +++ b/drivers/misc/cxl/cxl.h
> @@ -28,6 +28,7 @@
>   #include <uapi/misc/cxl.h>
> 
>   extern uint cxl_verbose;
> +extern bool cxl_enable_psltrace;
> 
>   #define CXL_TIMEOUT 5
> 
> @@ -678,6 +679,7 @@ struct cxl_service_layer_ops {
>   	void (*psl_irq_dump_registers)(struct cxl_context *ctx);
>   	void (*err_irq_dump_registers)(struct cxl *adapter);
>   	void (*stop_psltrace)(struct cxl *adapter);
> +	void (*start_psltrace)(struct cxl *adapter);
>   	void (*write_timebase_ctrl)(struct cxl *adapter);
>   	u64 (*timebase_read)(struct cxl *adapter);
>   	int capi_mode;
> diff --git a/drivers/misc/cxl/main.c b/drivers/misc/cxl/main.c
> index c1ba0d42cbc8..593f2cdba3d8 100644
> --- a/drivers/misc/cxl/main.c
> +++ b/drivers/misc/cxl/main.c
> @@ -34,6 +34,10 @@ uint cxl_verbose;
>   module_param_named(verbose, cxl_verbose, uint, 0600);
>   MODULE_PARM_DESC(verbose, "Enable verbose dmesg output");
> 
> +bool cxl_enable_psltrace = true;
> +module_param_named(enable_psltrace, cxl_enable_psltrace, bool, 0600);
> +MODULE_PARM_DESC(enable_psltrace, "Set PSL traces on probe. default: on");
> +
I am not too agree to add a new parameter. This can cause doubts.
PSL team has confirmed that enabling traces has no impact.
Do you see any reason to disable the traces ?

>   const struct cxl_backend_ops *cxl_ops;
> 
>   int cxl_afu_slbia(struct cxl_afu *afu)
> diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
> index 926b13973b73..9e8b8525534c 100644
> --- a/drivers/misc/cxl/pci.c
> +++ b/drivers/misc/cxl/pci.c
> @@ -1726,6 +1726,9 @@ static int cxl_configure_adapter(struct cxl *adapter, struct pci_dev *dev)
>   	if ((rc = cxl_native_register_psl_err_irq(adapter)))
>   		goto err;
> 
> +	if (cxl_enable_psltrace && adapter->native->sl_ops->start_psltrace)
> +		adapter->native->sl_ops->start_psltrace(adapter);
> +
>   	return 0;
> 
>   err:
>
Vaibhav Jain Feb. 11, 2018, 5:10 p.m. | #2
Thanks for reviewing the patch Christophe,

christophe lombard <clombard@linux.vnet.ibm.com> writes:
>> +bool cxl_enable_psltrace = true;
>> +module_param_named(enable_psltrace, cxl_enable_psltrace, bool, 0600);
>> +MODULE_PARM_DESC(enable_psltrace, "Set PSL traces on probe. default: on");
>> +
> I am not too agree to add a new parameter. This can cause doubts.
> PSL team has confirmed that enabling traces has no impact.
> Do you see any reason to disable the traces ?

Traces on PSL follow a 'set and fetch' model. So once the trace buffer for
a specific array is full it will stop and switch to 'FIN' state and at
that point we need to fetch the trace-data and reinit the array to
re-arm it.

There might be some circumstances where this model may lead to confusion
specifically when AFU developers assume that the trace arrays are
already armed and dont re-arm it causing miss of trace data.

So this module param is a compromise to keep the old behaviour of traces
array intact where in the arming/disarming of the trace arrays is
controlled completely by userspace tooling and not by cxl.
Christophe Lombard Feb. 12, 2018, 10:46 a.m. | #3
Le 11/02/2018 à 18:10, Vaibhav Jain a écrit :
> Thanks for reviewing the patch Christophe,
> 
> christophe lombard <clombard@linux.vnet.ibm.com> writes:
>>> +bool cxl_enable_psltrace = true;
>>> +module_param_named(enable_psltrace, cxl_enable_psltrace, bool, 0600);
>>> +MODULE_PARM_DESC(enable_psltrace, "Set PSL traces on probe. default: on");
>>> +
>> I am not too agree to add a new parameter. This can cause doubts.
>> PSL team has confirmed that enabling traces has no impact.
>> Do you see any reason to disable the traces ?
> 
> Traces on PSL follow a 'set and fetch' model. So once the trace buffer for
> a specific array is full it will stop and switch to 'FIN' state and at
> that point we need to fetch the trace-data and reinit the array to
> re-arm it.
> 
> There might be some circumstances where this model may lead to confusion
> specifically when AFU developers assume that the trace arrays are
> already armed and dont re-arm it causing miss of trace data.
> 
> So this module param is a compromise to keep the old behaviour of traces
> array intact where in the arming/disarming of the trace arrays is
> controlled completely by userspace tooling and not by cxl.
> 
and about P8 ? This new parameter is only useful for P9. It will be 
confusing.
Frederic Barrat Feb. 12, 2018, 1:54 p.m. | #4
Le 11/02/2018 à 18:10, Vaibhav Jain a écrit :
> Thanks for reviewing the patch Christophe,
> 
> christophe lombard <clombard@linux.vnet.ibm.com> writes:
>>> +bool cxl_enable_psltrace = true;
>>> +module_param_named(enable_psltrace, cxl_enable_psltrace, bool, 0600);
>>> +MODULE_PARM_DESC(enable_psltrace, "Set PSL traces on probe. default: on");
>>> +
>> I am not too agree to add a new parameter. This can cause doubts.
>> PSL team has confirmed that enabling traces has no impact.
>> Do you see any reason to disable the traces ?
> 
> Traces on PSL follow a 'set and fetch' model. So once the trace buffer for
> a specific array is full it will stop and switch to 'FIN' state and at
> that point we need to fetch the trace-data and reinit the array to
> re-arm it.

If the PSL trace arrays don't wrap, is there anything to gain by 
enabling tracing by default instead of letting the developer handle it 
through sysfs? I was under the (now wrong) impression that the PSL would 
wrap.
I'm not a big fan of the module parameter. It seems we're giving a 
second way of activating traces on top of sysfs, more cumbersome and 
limited.

   Fred

> There might be some circumstances where this model may lead to confusion
> specifically when AFU developers assume that the trace arrays are
> already armed and dont re-arm it causing miss of trace data.
> 
> So this module param is a compromise to keep the old behaviour of traces
> array intact where in the arming/disarming of the trace arrays is
> controlled completely by userspace tooling and not by cxl.
>
Vaibhav Jain Feb. 13, 2018, 11:07 a.m. | #5
Frederic Barrat <fbarrat@linux.vnet.ibm.com> writes:

> Le 11/02/2018 à 18:10, Vaibhav Jain a écrit :
>> Thanks for reviewing the patch Christophe,
>> 
>> christophe lombard <clombard@linux.vnet.ibm.com> writes:
>>>> +bool cxl_enable_psltrace = true;
>>>> +module_param_named(enable_psltrace, cxl_enable_psltrace, bool, 0600);
>>>> +MODULE_PARM_DESC(enable_psltrace, "Set PSL traces on probe. default: on");
>>>> +
>>> I am not too agree to add a new parameter. This can cause doubts.
>>> PSL team has confirmed that enabling traces has no impact.
>>> Do you see any reason to disable the traces ?
>> 
>> Traces on PSL follow a 'set and fetch' model. So once the trace buffer for
>> a specific array is full it will stop and switch to 'FIN' state and at
>> that point we need to fetch the trace-data and reinit the array to
>> re-arm it.
>
> If the PSL trace arrays don't wrap, is there anything to gain by 
> enabling tracing by default instead of letting the developer handle it 
> through sysfs? I was under the (now wrong) impression that the PSL would 
> wrap.
Enabling the traces quickly enough should let AFU developers debug init
issues. Specifically AFU's that rely on cxl kernel-apis.

> I'm not a big fan of the module parameter. It seems we're giving a 
> second way of activating traces on top of sysfs, more cumbersome and 
> limited.
Yes, this indeed is providing a second way of activating traces on top
of sysfs. The way I see this that there are two ways PSL traces are
managed:

1. Let userspace handle state machine of the traces entirely via sysfs.
2. PSL trace machine is handled via cxl. It starts it when a card is
probed and stops it when the card is reset.

Patch

diff --git a/drivers/misc/cxl/cxl.h b/drivers/misc/cxl/cxl.h
index 81da307b60c0..1af66451cbb4 100644
--- a/drivers/misc/cxl/cxl.h
+++ b/drivers/misc/cxl/cxl.h
@@ -28,6 +28,7 @@ 
 #include <uapi/misc/cxl.h>
 
 extern uint cxl_verbose;
+extern bool cxl_enable_psltrace;
 
 #define CXL_TIMEOUT 5
 
@@ -678,6 +679,7 @@  struct cxl_service_layer_ops {
 	void (*psl_irq_dump_registers)(struct cxl_context *ctx);
 	void (*err_irq_dump_registers)(struct cxl *adapter);
 	void (*stop_psltrace)(struct cxl *adapter);
+	void (*start_psltrace)(struct cxl *adapter);
 	void (*write_timebase_ctrl)(struct cxl *adapter);
 	u64 (*timebase_read)(struct cxl *adapter);
 	int capi_mode;
diff --git a/drivers/misc/cxl/main.c b/drivers/misc/cxl/main.c
index c1ba0d42cbc8..593f2cdba3d8 100644
--- a/drivers/misc/cxl/main.c
+++ b/drivers/misc/cxl/main.c
@@ -34,6 +34,10 @@  uint cxl_verbose;
 module_param_named(verbose, cxl_verbose, uint, 0600);
 MODULE_PARM_DESC(verbose, "Enable verbose dmesg output");
 
+bool cxl_enable_psltrace = true;
+module_param_named(enable_psltrace, cxl_enable_psltrace, bool, 0600);
+MODULE_PARM_DESC(enable_psltrace, "Set PSL traces on probe. default: on");
+
 const struct cxl_backend_ops *cxl_ops;
 
 int cxl_afu_slbia(struct cxl_afu *afu)
diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
index 926b13973b73..9e8b8525534c 100644
--- a/drivers/misc/cxl/pci.c
+++ b/drivers/misc/cxl/pci.c
@@ -1726,6 +1726,9 @@  static int cxl_configure_adapter(struct cxl *adapter, struct pci_dev *dev)
 	if ((rc = cxl_native_register_psl_err_irq(adapter)))
 		goto err;
 
+	if (cxl_enable_psltrace && adapter->native->sl_ops->start_psltrace)
+		adapter->native->sl_ops->start_psltrace(adapter);
+
 	return 0;
 
 err: