| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 15 Jun 2015 19:10:25 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>,
"H. Peter Anvin" <hpa@...or.com>,
"x86@...nel.org" <x86@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Paul Bolle <pebolle@...cali.nl>
Subject: Re: [PATCH v4] x86: punit_atom: punit device state debug driver
On Wed, May 6, 2015 at 10:15 PM, Srinivas Pandruvada
<srinivas.pandruvada@...ux.intel.com> wrote:
> The patch adds a debug driver, which dumps the power states
> of all the North complex (NC) devices. This debug interface is
> useful to figure out the devices, which blocks the S0ix
> transitions on the platform. This is extremely useful during
> enabling PM on customer platforms and derivatives.
>
> This submission is based on the submission from
> Kumar P, Mahesh <mahesh.kumar.p.intel.com>.
> https://lkml.org/lkml/2014/11/5/367
> The changes done on top of the above submission:
> - Dropped changes to config for PMC_ATOM, as PMC_ATOM
> is not just a debug driver as suggested by the change. It has
> additional power off interface also.
> - Instead of just using nc ("North complex") use punit_..
> similar to south complex PMC.
> - Removed pmc_config structure, as we don't need to predefine
> number of register, we want to dump. This way new register
> can be added without changing NC_NUM_DEVICES.
> - prefixed function with punit_
> - The debugfs directory will be punit_atom, which is NC equivalent
> of pmc_atom, which we already exposed by pmc_atom driver.
> - Added explanation for register and shift defines
> - Will not create debugfs if the cpuid doesn't match
> - Formatting changes to match compliance to coding convention
> - Address review comments
Sorry for late review. Hope my following comments will be addressed.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
> ---
> arch/x86/Kconfig.debug | 11 ++
> arch/x86/platform/Makefile | 1 +
> arch/x86/platform/atom/Makefile | 1 +
> arch/x86/platform/atom/punit_atom_debug.c | 183 ++++++++++++++++++++++++++++++
Should be pmc_atom.c moved here as well?
> 4 files changed, 196 insertions(+)
> create mode 100644 arch/x86/platform/atom/Makefile
> create mode 100644 arch/x86/platform/atom/punit_atom_debug.c
>
> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
> index 20028da..b525d86 100644
> --- a/arch/x86/Kconfig.debug
> +++ b/arch/x86/Kconfig.debug
> @@ -336,4 +336,15 @@ config X86_DEBUG_STATIC_CPU_HAS
>
> If unsure, say N.
>
> +config PUNIT_ATOM_DEBUG
> + tristate "ATOM Punit debug driver"
"Intel Atom Punit debug driver"
> + select DEBUG_FS
> + select IOSF_MBI
> + ---help---
> + This is a debug driver, which gets the power states
> + of all Punit North Complex devices. The power states of
> + each device is exposed as part of the debugfs interface.
> + The current power state can be read from
> + /sys/kernel/debug/punit_atom/dev_power_state
I hope you may use wider lines here.
> +
> endmenu
> diff --git a/arch/x86/platform/Makefile b/arch/x86/platform/Makefile
> index a62e0be..f1a6c8e 100644
> --- a/arch/x86/platform/Makefile
> +++ b/arch/x86/platform/Makefile
> @@ -1,4 +1,5 @@
> # Platform specific code goes here
> +obj-y += atom/
> obj-y += ce4100/
> obj-y += efi/
> obj-y += geode/
> diff --git a/arch/x86/platform/atom/Makefile b/arch/x86/platform/atom/Makefile
> new file mode 100644
> index 0000000..0a3a40c
> --- /dev/null
> +++ b/arch/x86/platform/atom/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_PUNIT_ATOM_DEBUG) += punit_atom_debug.o
> diff --git a/arch/x86/platform/atom/punit_atom_debug.c b/arch/x86/platform/atom/punit_atom_debug.c
> new file mode 100644
> index 0000000..5ca8ead
> --- /dev/null
> +++ b/arch/x86/platform/atom/punit_atom_debug.c
> @@ -0,0 +1,183 @@
> +/*
> + * Intel SOC Punit device state debug driver
SoC
And perhaps empty line to the next paragraph.
> + * Punit controls power management for North Complex devices (Graphics
> + * blocks, Image Signal Processing, video processing, display, DSP etc.)
* Punit controls power management for North Complex devices: Graphics
* blocks, Image Signal Processing, video processing, display, DSP, etc.
> + *
> + * Copyright (c) 2015, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
Do we need the above paragraph at all?
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/debugfs.h>
> +#include <linux/seq_file.h>
> +#include <linux/io.h>
> +#include <asm/cpu_device_id.h>
> +#include <asm/iosf_mbi.h>
> +
> +/* Side band Interface port */
> +#define PUNIT_PORT 0x04
+ empty line ?
> +/* Power gate status reg */
reg -> register
> +#define PWRGT_STATUS 0x61
Should be sorted regarding to the offset?
> +/* Subsystem config/status Video processor */
> +#define VED_SS_PM0 0x32
> +/* Subsystem config/status ISP (Image Signal Processor) */
> +#define ISP_SS_PM0 0x39
> +/* Subsystem config/status Input/output controller */
> +#define MIO_SS_PM 0x3B
+ empty line ?
> +/* Shift bits for getting status for video, isp and i/o */
> +#define SSS_SHIFT 24
> +/* Shift bits for getting status for graphics rendering */
> +#define RENDER_POS 0
> +/* Shift bits for getting status for media control */
> +#define MEDIA_POS 2
> +/* Shift bits for getting status for Valley View/Baytrail display */
I would prefer to see BYT prefix and BayTrail in the comment.
> +#define VLV_DISPLAY_POS 6
Should all shift bits be sorted by number?
> +/* Subsystem config/status display for Cherry Trail SOC */
> +#define CHT_DSP_SSS 0x36
This is part of above group.
I think better naming scheme is something like:
PUNIT_SS_…[_MACHINE], where MACHINE might be CHT, BYT, and so on if needed.
Here I can't see necessity of suffix at all. At least right now in the
current implementation.
> +/* Shift bits for getting status for display */
> +#define CHT_DSP_SSS_POS 16
> +
> +struct punit_device {
punit_island if to be correct, isn't it?
> + char *name;
> + int reg;
u32 ? Since IOSF interface take it as u32.
> + int sss_pos;
unsigned int shift; ?
> +};
> +
> +static const struct punit_device punit_device_byt[] = {
> + { "GFX RENDER", PWRGT_STATUS, RENDER_POS },
> + { "GFX MEDIA", PWRGT_STATUS, MEDIA_POS },
> + { "DISPLAY", PWRGT_STATUS, VLV_DISPLAY_POS },
> + { "VED", VED_SS_PM0, SSS_SHIFT },
> + { "ISP", ISP_SS_PM0, SSS_SHIFT },
> + { "MIO", MIO_SS_PM, SSS_SHIFT },
> + { NULL }
> +};
> +
> +static const struct punit_device punit_device_cht[] = {
> + { "GFX RENDER", PWRGT_STATUS, RENDER_POS },
> + { "GFX MEDIA", PWRGT_STATUS, MEDIA_POS },
> + { "DISPLAY", CHT_DSP_SSS, CHT_DSP_SSS_POS },
> + { "VED", VED_SS_PM0, SSS_SHIFT },
> + { "ISP", ISP_SS_PM0, SSS_SHIFT },
> + { "MIO", MIO_SS_PM, SSS_SHIFT },
> + { NULL }
> +};
> +
> +static const char * const dstates[] = {"D0", "D0i1", "D0i2", "D0i3"};
> +
> +static int punit_dev_state_show(struct seq_file *seq_file, void *unused)
> +{
> + u32 punit_pwr_status;
> + struct punit_device *punit_devp = seq_file->private;
punit_devp -> island (including change punit_device -> punit_island).
> + int index;
> + int status;
> +
> + seq_puts(seq_file, "\n\nPUNIT NORTH COMPLEX DEVICES :\n");
Maybe
"Punit North Complex Islands:\n" ?
> + while (punit_devp->name) {
> + status = iosf_mbi_read(PUNIT_PORT, BT_MBI_PMC_READ,
> + punit_devp->reg,
> + &punit_pwr_status);
> + if (status) {
> + seq_printf(seq_file, "%9s : Read Failed\n",
> + punit_devp->name);
> + } else {
Indentation?
> + index = (punit_pwr_status >> punit_devp->sss_pos) & 3;
3 is a magic number.
> + seq_printf(seq_file, "%9s : %s\n", punit_devp->name,
> + dstates[index]);
> + }
> + punit_devp++;
> + }
> +
> + return 0;
> +}
> +
> +static int punit_dev_state_open(struct inode *inode, struct file *file)
> +{
> + return single_open(file, punit_dev_state_show, inode->i_private);
> +}
> +
> +static const struct file_operations punit_dev_state_ops = {
> + .open = punit_dev_state_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = single_release,
> +};
> +
> +static struct dentry *punit_dbg_file;
Perhaps
static struct dentry *punit_debugfs;
since it's a folder.
> +
> +static int punit_dbgfs_register(struct punit_device *punit_device)
> +{
> + static struct dentry *dev_state;
Should it be static?
dev_state -> state ?
> +
> + punit_dbg_file = debugfs_create_dir("punit_atom", NULL);
> + if (!punit_dbg_file)
> + return -ENXIO;
> +
> + dev_state = debugfs_create_file("dev_power_state", S_IFREG | S_IRUGO,
dev_power_state -> state ?
> + punit_dbg_file, punit_device,
> + &punit_dev_state_ops);
> + if (!dev_state) {
> + pr_err("punit_dev_state register failed\n");
> + debugfs_remove(punit_dbg_file);
> + return -ENXIO;
> + }
> +
> + return 0;
> +}
> +
> +static void punit_dbgfs_unregister(void)
> +{
> + debugfs_remove_recursive(punit_dbg_file);
> +}
> +
> +#define ICPU(model, drv_data) \
> + { X86_VENDOR_INTEL, 6, model, X86_FEATURE_MWAIT,\
> + (kernel_ulong_t)&drv_data }
> +
> +static const struct x86_cpu_id intel_punit_cpu_ids[] = {
> + ICPU(55, punit_device_byt), /* Valleyview, Bay Trail */
> + ICPU(76, punit_device_cht), /* Braswell, Cherry Trail */
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(x86cpu, intel_punit_cpu_ids);
> +
> +static int __init punit_atom_debug_init(void)
> +{
> + const struct x86_cpu_id *id;
> + int ret;
> +
> + id = x86_match_cpu(intel_punit_cpu_ids);
> + if (!id)
> + return -ENODEV;
> +
> + ret = punit_dbgfs_register((struct punit_device *)id->driver_data);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static void __exit punit_atom_debug_exit(void)
> +{
> + punit_dbgfs_unregister();
> +}
> +
> +module_init(punit_atom_debug_init);
> +module_exit(punit_atom_debug_exit);
> +
> +MODULE_AUTHOR("Kumar P, Mahesh <mahesh.kumar.p@...el.com>");
> +MODULE_AUTHOR("Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>");
> +MODULE_DESCRIPTION("Driver for Punit devices states debugging");
> +MODULE_LICENSE("GPL v2");
Just noticed it's already in the tip tree.
Though, please, comment my mail I can do patch by myself if there is
no objections.
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists