Re: [PATCH v2 4/5] platform:x86: Add Intel Telemetry Debugfs interfaces

[Darren Hart <dvhart@infradead.org>]

On Wed, Dec 23, 2015 at 04:14:16PM +0530, Souvik Kumar Chakravarty wrote:
> This implements debugfs interfaces for reading the telemetry
> samples from SSRAM and configuring firmware trace verbosity.
> Interface created under /sys/kernel/debug/telemetry

What was the rationale for placing this under /sys/kernel/debug/telemetry,
instead of /sys/power/telemetry ?

Rafael, would you like to weigh in on this series? This builds on top of
Qipeng's punit driver queued in my tree for 4.5.

> soc_states: SoC Device and Low Power States
> pss_info: Info from the Primary SubSystem
> ioss_info: Info from IO SubSusytem
> pss_trace_verbosity: Read/Modify PSS F/W trace verbosity
> ioss_trace_verbosity: Read/Modify IOSS F/W trace verbosity.
> 
> Signed-off-by: Souvik Kumar Chakravarty <souvik.k.chakravarty@intel.com>

Hi Souvik,

This particular patche's legibility suffers from a number of very long variables
- or struct paths to variables. A couple of ideas below.

What is the plan for supporting future SoCs here? You have APL incorporated into
this file. Do you intend to add each generation to this file?

> 
> ---
> Changes in v2:
> * Fix issues in code style, indentation & comments
> * Changed Banner to include "All Rights Reserved"
> * Group all ApolloLake specific #defines under banner
> * Change to MODULE_LICENSE to GPL
> ---
>  drivers/platform/x86/intel_telemetry_debugfs.c | 1094 ++++++++++++++++++++++++
>  1 file changed, 1094 insertions(+)
>  create mode 100644 drivers/platform/x86/intel_telemetry_debugfs.c
> 
> diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c b/drivers/platform/x86/intel_telemetry_debugfs.c
> new file mode 100644
> index 0000000..5b79899
> --- /dev/null
> +++ b/drivers/platform/x86/intel_telemetry_debugfs.c
...
> +static u8 suspend_prep_ok;
> +static u32 suspend_shlw_ctr_temp, suspend_deep_ctr_temp;
> +static u64 suspend_shlw_res_temp, suspend_deep_res_temp;
> +
> +struct telemetry_susp_stats {
> +	u32 suspend_shlw_swake_ctr;
> +	u32 suspend_deep_swake_ctr;
> +	u64 suspend_shlw_swake_res;
> +	u64 suspend_deep_swake_res;
> +	u32 suspend_shlw_ctr;
> +	u32 suspend_deep_ctr;
> +	u64 suspend_shlw_res;
> +	u64 suspend_deep_res;
> +};

The suspend_ prefix to every variable seems redundant with the fact that they
are part of a telemetry_suspend_stats structure. Eliminating this would help
legibility considerably below.

...

> +
> +static int telem_soc_states_show(struct seq_file *s, void *unused)
> +{
> +	u32 d3_sts[TELEM_IOSS_DX_D0IX_EVTS], d0ix_sts[TELEM_IOSS_DX_D0IX_EVTS];
> +	u32 pg_sts[TELEM_IOSS_PG_EVTS], pss_idle[TELEM_PSS_IDLE_EVTS];
> +	struct telemetry_evtlog evtlog[TELEM_MAX_OS_ALLOCATED_EVENTS];
> +	u32 s0ix_total_ctr = 0, s0ix_shlw_ctr = 0, s0ix_deep_ctr = 0;
> +	u64 s0ix_total_res = 0, s0ix_shlw_res = 0, s0ix_deep_res = 0;
> +	struct pci_dev *dev = NULL;
> +	int index, idx, ret;
> +	u32 d3_state;
> +	u16 pmcsr;
> +
> +	ret = telemetry_read_eventlog(TELEM_IOSS, evtlog,
> +				      TELEM_MAX_OS_ALLOCATED_EVENTS);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (index = 0; index < ret; index++) {
> +		if (evtlog[index].telem_evtid == debugfs_conf->ioss_d3_id) {
> +			for (idx = 0; idx < debugfs_conf->ioss_d0ix_evts;
> +			     idx++) {

The above can be on one line with a shorter name for debugfs_conf, even if just
a local alias.

> +				d3_sts[idx] =
> +				(evtlog[index].telem_evtlog >>
> +				debugfs_conf->ioss_d0ix_data[idx].bit_pos) &
> +				TELEM_MASK_BIT;

By the time you have indented 4, and really should be 5, the preference is to
determine if this can be refactored into a shallower nesting structure.

Perhaps a macro of some sort as these all seem fairly repetitive.

...

This needs more time for review than I can give today, I'll have to get back to
this one. Hopefully tomorrow.

-- 
Darren Hart
Intel Open Source Technology Center