| 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
| ||
|
Message-ID: <1430763447.5506.23.camel@spandruv-DESK3.jf.intel.com>
Date: Mon, 04 May 2015 11:17:27 -0700
From: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
To: Ingo Molnar <mingo@...nel.org>
Cc: tglx@...utronix.de, mingo@...hat.com, hpa@...or.com,
x86@...nel.org, linux-kernel@...r.kernel.org, pebolle@...cali.nl,
Borislav Petkov <bp@...en8.de>
Subject: Re: [PATCH v1] x86: punit_atom: punit device state debug driver
On Sat, 2015-05-02 at 08:00 +0200, Ingo Molnar wrote:
Thanks for the review
> * 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 NC IPs which blocks the S0ix
> > transitions on the platform. This is extremely useful during
> > enabling PM on customer platforms and derivatives.
>
> Looks useful.
>
> > This submission not a complete rewrite from the original
> > submission from Kumar P, Mahesh <mahesh.kumar.p.intel.com>.
> > https://lkml.org/lkml/2014/11/5/367
>
> This sentence does not parse. Did you mean 'is a complete rewrite'?
Updated the commit message, Meant "updated the submission based on the
previous submission"
>
> > +config PUNIT_ATOM_DEBUG
> > + tristate "ATOM Punit debug driver"
> > + depends on 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 IP is exposed as part of the debugfs interface.
>
> What is an 'IP'?
Changed to device.
>
> Also:
>
> s/devices.The
> /devices. The
Done.
>
> > +
> > endmenu
> > diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> > index cdb1b70..2ecbd55 100644
> > --- a/arch/x86/kernel/Makefile
> > +++ b/arch/x86/kernel/Makefile
> > @@ -110,6 +110,7 @@ obj-$(CONFIG_PERF_EVENTS) += perf_regs.o
> > obj-$(CONFIG_TRACING) += tracepoint.o
> > obj-$(CONFIG_IOSF_MBI) += iosf_mbi.o
> > obj-$(CONFIG_PMC_ATOM) += pmc_atom.o
> > +obj-$(CONFIG_PUNIT_ATOM_DEBUG) += punit_atom_debug.o
> >
> > ###
> > # 64 bit specific files
> > diff --git a/arch/x86/kernel/punit_atom_debug.c b/arch/x86/kernel/punit_atom_debug.c
>
> Please put this somewhere suitable in arch/x86/platform/.
Moved to arch/x86/platform/intel-mid
>
> > +/*
> > + * Intel SOC Punit device state debug driver
> > + * Copyright (c) 2015, Intel Corporation.
>
> Would be nice to add a blurb about what 'Intel SOC Punit' systems are.
> There doesn't seem to be anything in the kernel source about it, so
> you have a golden opportunity here.
Done
>
> > +#define PUNIT_PORT 0x04
> > +#define PWRGT_STATUS 0x61 /* Power gate status reg */
> > +#define VED_SS_PM0 0x32 /* Subsystem config/status Video */
> > +#define ISP_SS_PM0 0x39 /* Subsystem config/status ISP */
> > +#define MIO_SS_PM 0x3B /* Subsystem config/status MIO */
> > +#define SSS_SHIFT 24
> > +#define RENDER_POS 0
> > +#define MEDIA_POS 2
> > +#define VLV_DISPLAY_POS 6
> > +#define CHT_DSP_SSS 0x36 /* Subsystem config/status DSP */
> > +#define CHT_DSP_SSS_POS 16
>
> All the magic constants need some explanation for humans, not just
> some.
Added for missing defines
>
> > +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 },
> > + { "DSP", 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}
> > +};
>
> Please align such initialization tables vertically, like you did here:
>
Aligned.
> > +static const struct file_operations punit_dev_state_ops = {
> > + .open = punit_dev_state_open,
> > + .read = seq_read,
> > + .llseek = seq_lseek,
> > + .release = single_release,
> > +};
>
>
> > + punit_dbg_file = debugfs_create_dir("punit_atom", NULL);
> > + if (!punit_dbg_file)
> > + return -ENXIO;
> > +
> > + dev_state = debugfs_create_file("dev_state", S_IFREG | S_IRUGO,
> > + punit_dbg_file, NULL,
> > + &punit_dev_state_ops);
>
> So why isn't something that is declaradly a power state dump, called
> "power_state" or "dev_power_state"?
>
Changed to dev_power_state
> > +MODULE_AUTHOR("Kumar P, Mahesh <mahesh.kumar.p.intel.com>");
> > +MODULE_DESCRIPTION("Driver for Punit devices states debugging");
> > +MODULE_LICENSE("GPL v2");
>
> Btw., you can have multiple MODULE_AUTHOR() lines, so you can credit
> yourself as well. Take a look at drivers/net/wireless/at76c50x-usb.c's
> MODULE_AUTHOR() for grins.
Added myself .
Thanks,
Srinivas
>
> Thanks,
>
> Ingo
--
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