| Message ID | 20180209042535.16845-3-vaibhav@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Provide ability to enable PSL traces on card probe | expand |
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: >
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.
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.
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. >
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.
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:
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(+)