| 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: Wed, 7 Apr 2021 16:37:14 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: "David E. Box" <david.e.box@...ux.intel.com>,
irenic.rajneesh@...il.com, mgross@...ux.intel.com,
gayatri.kammela@...el.com
Cc: platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 7/9] platform/x86: intel_pmc_core: Add option to set/clear
LPM mode
Hi,
On 4/1/21 5:05 AM, David E. Box wrote:
> By default the Low Power Mode (LPM or sub-state) status registers will
> latch condition status on every entry into Package C10. This is
> configurable in the PMC to allow latching on any achievable sub-state. Add
> a debugfs file to support this.
>
> Also add the option to clear the status registers to 0. Clearing the status
> registers before testing removes ambiguity around when the current values
> were set.
>
> The new file, latch_lpm_mode, looks like this:
>
> [c10] S0i2.0 S0i3.0 S0i2.1 S0i3.1 S0i3.2 clear
>
> Signed-off-by: David E. Box <david.e.box@...ux.intel.com>
> ---
> drivers/platform/x86/intel_pmc_core.c | 94 +++++++++++++++++++++++++++
> drivers/platform/x86/intel_pmc_core.h | 20 ++++++
> 2 files changed, 114 insertions(+)
>
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index 0b47a1da5f49..458c0056e7a1 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -584,6 +584,8 @@ static const struct pmc_reg_map tgl_reg_map = {
> .ltr_ignore_max = TGL_NUM_IP_IGN_ALLOWED,
> .lpm_num_maps = TGL_LPM_NUM_MAPS,
> .lpm_res_counter_step_x2 = TGL_PMC_LPM_RES_COUNTER_STEP_X2,
> + .etr3_offset = TGL_ETR3_OFFSET,
> + .lpm_sts_latch_en_offset = TGL_LPM_STS_LATCH_EN_OFFSET,
> .lpm_en_offset = TGL_LPM_EN_OFFSET,
> .lpm_priority_offset = TGL_LPM_PRI_OFFSET,
> .lpm_residency_offset = TGL_LPM_RESIDENCY_OFFSET,
> @@ -1202,6 +1204,95 @@ static int pmc_core_substate_req_regs_show(struct seq_file *s, void *unused)
> }
> DEFINE_SHOW_ATTRIBUTE(pmc_core_substate_req_regs);
>
> +static int pmc_core_lpm_latch_mode_show(struct seq_file *s, void *unused)
> +{
> + struct pmc_dev *pmcdev = s->private;
> + bool c10;
> + u32 reg;
> + int idx, mode;
> +
> + reg = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_sts_latch_en_offset);
> + if (reg & BIT(LPM_STS_LATCH_MODE_BIT)) {
> + seq_puts(s, "c10");
> + c10 = false;
> + } else {
> + seq_puts(s, "[c10]");
> + c10 = true;
> + }
> +
> + pmc_for_each_mode(idx, mode, pmcdev) {
> + if ((BIT(mode) & reg) && !c10)
> + seq_printf(s, " [%s]", pmc_lpm_modes[mode]);
> + else
> + seq_printf(s, " %s", pmc_lpm_modes[mode]);
> + }
So this either shows [c10] selected, or it shows which s0ix modes
are selected, that is a bit weird.
I realize this is a debugfs interface, but can we still get some docs
in this, at a minimum some more explanation in the commit message ?
> +
> + seq_puts(s, " clear\n");
> +
> + return 0;
> +}
> +
> +static ssize_t pmc_core_lpm_latch_mode_write(struct file *file,
> + const char __user *userbuf,
> + size_t count, loff_t *ppos)
> +{
> + struct seq_file *s = file->private_data;
> + struct pmc_dev *pmcdev = s->private;
> + bool clear = false, c10 = false;
> + unsigned char buf[10] = {0};
> + size_t ret;
> + int mode;
> + u32 reg;
> +
> + ret = simple_write_to_buffer(buf, 10, ppos, userbuf, count);
> + if (ret < 0)
> + return ret;
You are using C-string functions on buf below, so you must guarantee
that it is 0 terminated. To guarantee the buffer's size must be 1 larger
then the size which you pass to simple_write_to_buffer()
> +
> + mode = sysfs_match_string(pmc_lpm_modes, buf);
> + if (mode < 0) {
> + if (strncmp("clear", buf, 5) == 0)
> + clear = true;
> + else if (strncmp("c10", buf, 3) == 0)
> + c10 = true;
Ugh, no. Now it will not just accept "clear" and "clear\n" but
also "clear foobar everything else I write now does not matter"
as "clear" string. This misuse of strncmp for sysfs / debugfs write
functions seems to be some sort of anti-pattern in the kernel.
Please use sysfs_streq() here so that only "clear" and "clear\n"
are accepted (and the same for the "c10" check).
> + else
> + return mode;
> + }
> +
> + if (clear) {
> + mutex_lock(&pmcdev->lock);
> +
> + reg = pmc_core_reg_read(pmcdev, pmcdev->map->etr3_offset);
> + reg |= BIT(ETR3_CLEAR_LPM_EVENTS_BIT);
> + pmc_core_reg_write(pmcdev, pmcdev->map->etr3_offset, reg);
> +
> + mutex_unlock(&pmcdev->lock);
> +
> + return count;
> + }
> +
> + if (c10) {
> + mutex_lock(&pmcdev->lock);
> +
> + reg = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_sts_latch_en_offset);
> + reg &= ~BIT(LPM_STS_LATCH_MODE_BIT);
> + pmc_core_reg_write(pmcdev, pmcdev->map->lpm_sts_latch_en_offset, reg);
> +
> + mutex_unlock(&pmcdev->lock);
> +
> + return count;
> + }
> +
> + /*
> + * For LPM mode latching we set the latch enable bit and selected mode
> + * and clear everything else.
> + */
> + reg = BIT(LPM_STS_LATCH_MODE_BIT) | BIT(mode);
> + pmc_core_reg_write(pmcdev, pmcdev->map->lpm_sts_latch_en_offset, reg);
> +
> + return count;
> +}
> +DEFINE_PMC_CORE_ATTR_WRITE(pmc_core_lpm_latch_mode);
> +
> static int pmc_core_pkgc_show(struct seq_file *s, void *unused)
> {
> struct pmc_dev *pmcdev = s->private;
> @@ -1320,6 +1411,9 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
> debugfs_create_file("substate_live_status_registers", 0444,
> pmcdev->dbgfs_dir, pmcdev,
> &pmc_core_substate_l_sts_regs_fops);
> + debugfs_create_file("lpm_latch_mode", 0644,
> + pmcdev->dbgfs_dir, pmcdev,
> + &pmc_core_lpm_latch_mode_fops);
> }
>
> if (pmcdev->lpm_req_regs) {
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index 81d797feed33..f41f61aa7008 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -189,6 +189,8 @@ enum ppfear_regs {
>
> #define LPM_MAX_NUM_MODES 8
> #define GET_X2_COUNTER(v) ((v) >> 1)
> +#define ETR3_CLEAR_LPM_EVENTS_BIT 28
> +#define LPM_STS_LATCH_MODE_BIT 31
>
> #define TGL_NUM_IP_IGN_ALLOWED 22
> #define TGL_PMC_SLP_S0_RES_COUNTER_STEP 0x7A
> @@ -197,6 +199,8 @@ enum ppfear_regs {
> /*
> * Tigerlake Power Management Controller register offsets
> */
> +#define TGL_ETR3_OFFSET 0x1048
> +#define TGL_LPM_STS_LATCH_EN_OFFSET 0x1C34
> #define TGL_LPM_EN_OFFSET 0x1C78
> #define TGL_LPM_RESIDENCY_OFFSET 0x1C80
>
> @@ -266,6 +270,8 @@ struct pmc_reg_map {
> /* Low Power Mode registers */
> const int lpm_num_maps;
> const int lpm_res_counter_step_x2;
> + const u32 etr3_offset;
> + const u32 lpm_sts_latch_en_offset;
> const u32 lpm_en_offset;
> const u32 lpm_priority_offset;
> const u32 lpm_residency_offset;
> @@ -313,4 +319,18 @@ struct pmc_dev {
> i < pmcdev->num_modes; \
> i++, mode = pmcdev->lpm_en_modes[i])
>
> +#define DEFINE_PMC_CORE_ATTR_WRITE(__name) \
> +static int __name ## _open(struct inode *inode, struct file *file) \
> +{ \
> + return single_open(file, __name ## _show, inode->i_private); \
> +} \
> + \
> +static const struct file_operations __name ## _fops = { \
> + .owner = THIS_MODULE, \
> + .open = __name ## _open, \
> + .read = seq_read, \
> + .write = __name ## _write, \
> + .release = single_release, \
> +}
> +
> #endif /* PMC_CORE_H */
>
Regards,
Hans
Powered by blists - more mailing lists