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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGcaFA3c_rc6HVVqc2=UqOgu2c7zX_XTWNx7Yf-KL7W8LgEi7A@mail.gmail.com>
Date: Wed, 3 Jul 2024 00:41:26 +0200
From: Marek Maślanka <mmaslanka@...gle.com>
To: Hans de Goede <hdegoede@...hat.com>
Cc: LKML <linux-kernel@...r.kernel.org>, David E Box <david.e.box@...el.com>, 
	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, 
	Rajneesh Bhardwaj <irenic.rajneesh@...il.com>, platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH] platform/x86:intel/pmc: Enable the ACPI PM Timer to be
 turned off when suspended

Hi Hans,

On Tue, Jul 2, 2024 at 10:02 AM Hans de Goede <hdegoede@...hat.com> wrote:
>
> Hi Marek,
>
> On 7/2/24 12:25 AM, Marek Maslanka wrote:
> > Allow to disable ACPI PM Timer on suspend and enable on resume. A
> > disabled timer helps optimise power consumption when the system is
> > suspended. On resume the timer is only reactivated if it was activated
> > prior to suspend, so unless the ACPI PM timer is enabled in the BIOS,
> > this won't change anything.
> >
> > Signed-off-by: Marek Maslanka <mmaslanka@...gle.com>
>
> Thank you for your patch. I have not looked into it into too much
> detail (I expect the Intel maintainers of the driver will do that)
> but why is there a Kconfig option for this ?
>
> It seems to me that this is something which we simply always want
> to do and we don't need all the #ifdef-s ?

Yes, you're right, ifdef-s are not needed as there are conditions that check if
CPU support disables the timer. I'll remove it in the next patch.

Best,
Marek

>
> Regards,
>
> Hans
>
>
>
> > ---
> >
> >  drivers/platform/x86/intel/pmc/Kconfig | 13 ++++++++
> >  drivers/platform/x86/intel/pmc/adl.c   |  4 +++
> >  drivers/platform/x86/intel/pmc/cnp.c   |  4 +++
> >  drivers/platform/x86/intel/pmc/core.c  | 43 ++++++++++++++++++++++++++
> >  drivers/platform/x86/intel/pmc/core.h  | 14 +++++++++
> >  drivers/platform/x86/intel/pmc/icl.c   |  4 +++
> >  drivers/platform/x86/intel/pmc/mtl.c   |  4 +++
> >  drivers/platform/x86/intel/pmc/spt.c   |  4 +++
> >  drivers/platform/x86/intel/pmc/tgl.c   |  4 +++
> >  9 files changed, 94 insertions(+)
> >
> > diff --git a/drivers/platform/x86/intel/pmc/Kconfig b/drivers/platform/x86/intel/pmc/Kconfig
> > index d2f651fbec2cf..3a563db8eba6a 100644
> > --- a/drivers/platform/x86/intel/pmc/Kconfig
> > +++ b/drivers/platform/x86/intel/pmc/Kconfig
> > @@ -24,3 +24,16 @@ config INTEL_PMC_CORE
> >               - SLPS0 Debug registers (Cannonlake/Icelake PCH)
> >               - Low Power Mode registers (Tigerlake and beyond)
> >               - PMC quirks as needed to enable SLPS0/S0ix
> > +
> > +config DISABLE_ACPI_PM_TIMER_ON_SUSPEND
> > +     bool "Disable ACPI PM Timer on suspend"
> > +     default n
> > +     depends on INTEL_PMC_CORE
> > +     help
> > +       Disable ACPI Power Management Timer on entering to suspend and enable it
> > +       on resume. This helps optimize energy consumption while the system is
> > +       suspend.
> > +
> > +       This is only applicable if the ACPI PM timer is enabled by the BIOS.
> > +
> > +       Say N if unsure.
> > diff --git a/drivers/platform/x86/intel/pmc/adl.c b/drivers/platform/x86/intel/pmc/adl.c
> > index e7878558fd909..8859e0d275288 100644
> > --- a/drivers/platform/x86/intel/pmc/adl.c
> > +++ b/drivers/platform/x86/intel/pmc/adl.c
> > @@ -295,6 +295,10 @@ const struct pmc_reg_map adl_reg_map = {
> >       .ppfear_buckets = CNP_PPFEAR_NUM_ENTRIES,
> >       .pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET,
> >       .pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
> > +#ifdef CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND
> > +     .acpi_pm_tmr_ctl_offset = SPT_PMC_ACPI_PM_TMR_CTL_OFFSET,
> > +     .acpi_pm_tmr_disable_bit = SPT_PMC_BIT_ACPI_PM_TMR_DISABLE,
> > +#endif /* CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND */
> >       .ltr_ignore_max = ADL_NUM_IP_IGN_ALLOWED,
> >       .lpm_num_modes = ADL_LPM_NUM_MODES,
> >       .lpm_num_maps = ADL_LPM_NUM_MAPS,
> > diff --git a/drivers/platform/x86/intel/pmc/cnp.c b/drivers/platform/x86/intel/pmc/cnp.c
> > index dd72974bf71e2..e92157aa3c9f1 100644
> > --- a/drivers/platform/x86/intel/pmc/cnp.c
> > +++ b/drivers/platform/x86/intel/pmc/cnp.c
> > @@ -200,6 +200,10 @@ const struct pmc_reg_map cnp_reg_map = {
> >       .ppfear_buckets = CNP_PPFEAR_NUM_ENTRIES,
> >       .pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET,
> >       .pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
> > +#ifdef CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND
> > +     .acpi_pm_tmr_ctl_offset = SPT_PMC_ACPI_PM_TMR_CTL_OFFSET,
> > +     .acpi_pm_tmr_disable_bit = SPT_PMC_BIT_ACPI_PM_TMR_DISABLE,
> > +#endif /* CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND */
> >       .ltr_ignore_max = CNP_NUM_IP_IGN_ALLOWED,
> >       .etr3_offset = ETR3_OFFSET,
> >  };
> > diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
> > index 10c96c1a850af..a3e56c524308f 100644
> > --- a/drivers/platform/x86/intel/pmc/core.c
> > +++ b/drivers/platform/x86/intel/pmc/core.c
> > @@ -1171,6 +1171,37 @@ static bool pmc_core_is_pson_residency_enabled(struct pmc_dev *pmcdev)
> >       return val == 1;
> >  }
> >
> > +#ifdef CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND
> > +/*
> > + * Enable or disable APCI PM Timer
> > + *
> > + * @return: Previous APCI PM Timer enabled state
> > + */
> > +static bool pmc_core_enable_apci_pm_timer(struct pmc_dev *pmcdev, bool enable)
> > +{
> > +     struct pmc *pmc = pmcdev->pmcs[PMC_IDX_MAIN];
> > +     const struct pmc_reg_map *map = pmc->map;
> > +     bool state;
> > +     u32 reg;
> > +
> > +     if (!map->acpi_pm_tmr_ctl_offset)
> > +             return false;
> > +
> > +     mutex_lock(&pmcdev->lock);
> > +
> > +     reg = pmc_core_reg_read(pmc, map->acpi_pm_tmr_ctl_offset);
> > +     state = !(reg & map->acpi_pm_tmr_disable_bit);
> > +     if (enable)
> > +             reg &= ~map->acpi_pm_tmr_disable_bit;
> > +     else
> > +             reg |= map->acpi_pm_tmr_disable_bit;
> > +     pmc_core_reg_write(pmc, map->acpi_pm_tmr_ctl_offset, reg);
> > +
> > +     mutex_unlock(&pmcdev->lock);
> > +
> > +     return state;
> > +}
> > +#endif /* CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND */
> >
> >  static void pmc_core_dbgfs_unregister(struct pmc_dev *pmcdev)
> >  {
> > @@ -1446,6 +1477,12 @@ static __maybe_unused int pmc_core_suspend(struct device *dev)
> >       if (pmcdev->suspend)
> >               pmcdev->suspend(pmcdev);
> >
> > +#ifdef CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND
> > +     /* Disable APCI PM Timer */
> > +     pmcdev->enable_acpi_pm_timer_on_resume =
> > +             pmc_core_enable_apci_pm_timer(pmcdev, false);
> > +#endif /* CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND */
> > +
> >       /* Check if the syspend will actually use S0ix */
> >       if (pm_suspend_via_firmware())
> >               return 0;
> > @@ -1500,6 +1537,12 @@ int pmc_core_resume_common(struct pmc_dev *pmcdev)
> >       int offset = pmc->map->lpm_status_offset;
> >       int i;
> >
> > +#ifdef CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND
> > +     /* Enable APCI PM Timer */
> > +     if (pmcdev->enable_acpi_pm_timer_on_resume)
> > +             pmc_core_enable_apci_pm_timer(pmcdev, true);
> > +#endif /* CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND */
> > +
> >       /* Check if the syspend used S0ix */
> >       if (pm_suspend_via_firmware())
> >               return 0;
> > diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
> > index 83504c49a0e31..4d5983d741433 100644
> > --- a/drivers/platform/x86/intel/pmc/core.h
> > +++ b/drivers/platform/x86/intel/pmc/core.h
> > @@ -67,6 +67,10 @@ struct telem_endpoint;
> >  #define SPT_PMC_LTR_SCC                              0x3A0
> >  #define SPT_PMC_LTR_ISH                              0x3A4
> >
> > +#ifdef CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND
> > +#define SPT_PMC_ACPI_PM_TMR_CTL_OFFSET               0x18FC
> > +#endif /* CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND */
> > +
> >  /* Sunrise Point: PGD PFET Enable Ack Status Registers */
> >  enum ppfear_regs {
> >       SPT_PMC_XRAM_PPFEAR0A = 0x590,
> > @@ -147,6 +151,10 @@ enum ppfear_regs {
> >  #define SPT_PMC_VRIC1_SLPS0LVEN                      BIT(13)
> >  #define SPT_PMC_VRIC1_XTALSDQDIS             BIT(22)
> >
> > +#ifdef CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND
> > +#define SPT_PMC_BIT_ACPI_PM_TMR_DISABLE              BIT(1)
> > +#endif /* CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND */
> > +
> >  /* Cannonlake Power Management Controller register offsets */
> >  #define CNP_PMC_SLPS0_DBG_OFFSET             0x10B4
> >  #define CNP_PMC_PM_CFG_OFFSET                        0x1818
> > @@ -344,6 +352,8 @@ struct pmc_reg_map {
> >       const u8  *lpm_reg_index;
> >       const u32 pson_residency_offset;
> >       const u32 pson_residency_counter_step;
> > +     const u32 acpi_pm_tmr_ctl_offset;
> > +     const u32 acpi_pm_tmr_disable_bit;
> >  };
> >
> >  /**
> > @@ -417,6 +427,10 @@ struct pmc_dev {
> >       u32 die_c6_offset;
> >       struct telem_endpoint *punit_ep;
> >       struct pmc_info *regmap_list;
> > +
> > +#ifdef CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND
> > +     bool enable_acpi_pm_timer_on_resume;
> > +#endif /* CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND */
> >  };
> >
> >  enum pmc_index {
> > diff --git a/drivers/platform/x86/intel/pmc/icl.c b/drivers/platform/x86/intel/pmc/icl.c
> > index 71b0fd6cb7d84..8b5c782e71ebd 100644
> > --- a/drivers/platform/x86/intel/pmc/icl.c
> > +++ b/drivers/platform/x86/intel/pmc/icl.c
> > @@ -46,6 +46,10 @@ const struct pmc_reg_map icl_reg_map = {
> >       .ppfear_buckets = ICL_PPFEAR_NUM_ENTRIES,
> >       .pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET,
> >       .pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
> > +#ifdef CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND
> > +     .acpi_pm_tmr_ctl_offset = SPT_PMC_ACPI_PM_TMR_CTL_OFFSET,
> > +     .acpi_pm_tmr_disable_bit = SPT_PMC_BIT_ACPI_PM_TMR_DISABLE,
> > +#endif /* CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND */
> >       .ltr_ignore_max = ICL_NUM_IP_IGN_ALLOWED,
> >       .etr3_offset = ETR3_OFFSET,
> >  };
> > diff --git a/drivers/platform/x86/intel/pmc/mtl.c b/drivers/platform/x86/intel/pmc/mtl.c
> > index c7d15d864039d..c726ef8f1d5a9 100644
> > --- a/drivers/platform/x86/intel/pmc/mtl.c
> > +++ b/drivers/platform/x86/intel/pmc/mtl.c
> > @@ -462,6 +462,10 @@ const struct pmc_reg_map mtl_socm_reg_map = {
> >       .ppfear_buckets = MTL_SOCM_PPFEAR_NUM_ENTRIES,
> >       .pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET,
> >       .pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
> > +#ifdef CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND
> > +     .acpi_pm_tmr_ctl_offset = SPT_PMC_ACPI_PM_TMR_CTL_OFFSET,
> > +     .acpi_pm_tmr_disable_bit = SPT_PMC_BIT_ACPI_PM_TMR_DISABLE,
> > +#endif /* CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND */
> >       .lpm_num_maps = ADL_LPM_NUM_MAPS,
> >       .ltr_ignore_max = MTL_SOCM_NUM_IP_IGN_ALLOWED,
> >       .lpm_res_counter_step_x2 = TGL_PMC_LPM_RES_COUNTER_STEP_X2,
> > diff --git a/drivers/platform/x86/intel/pmc/spt.c b/drivers/platform/x86/intel/pmc/spt.c
> > index ab993a69e33ee..4832e953d0403 100644
> > --- a/drivers/platform/x86/intel/pmc/spt.c
> > +++ b/drivers/platform/x86/intel/pmc/spt.c
> > @@ -130,6 +130,10 @@ const struct pmc_reg_map spt_reg_map = {
> >       .ppfear_buckets = SPT_PPFEAR_NUM_ENTRIES,
> >       .pm_cfg_offset = SPT_PMC_PM_CFG_OFFSET,
> >       .pm_read_disable_bit = SPT_PMC_READ_DISABLE_BIT,
> > +#ifdef CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND
> > +     .acpi_pm_tmr_ctl_offset = SPT_PMC_ACPI_PM_TMR_CTL_OFFSET,
> > +     .acpi_pm_tmr_disable_bit = SPT_PMC_BIT_ACPI_PM_TMR_DISABLE,
> > +#endif /* CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND */
> >       .ltr_ignore_max = SPT_NUM_IP_IGN_ALLOWED,
> >       .pm_vric1_offset = SPT_PMC_VRIC1_OFFSET,
> >  };
> > diff --git a/drivers/platform/x86/intel/pmc/tgl.c b/drivers/platform/x86/intel/pmc/tgl.c
> > index e0580de180773..4742b84fe226e 100644
> > --- a/drivers/platform/x86/intel/pmc/tgl.c
> > +++ b/drivers/platform/x86/intel/pmc/tgl.c
> > @@ -197,6 +197,10 @@ const struct pmc_reg_map tgl_reg_map = {
> >       .ppfear_buckets = ICL_PPFEAR_NUM_ENTRIES,
> >       .pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET,
> >       .pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
> > +#ifdef CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND
> > +     .acpi_pm_tmr_ctl_offset = SPT_PMC_ACPI_PM_TMR_CTL_OFFSET,
> > +     .acpi_pm_tmr_disable_bit = SPT_PMC_BIT_ACPI_PM_TMR_DISABLE,
> > +#endif /* CONFIG_DISABLE_ACPI_PM_TIMER_ON_SUSPEND */
> >       .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,
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ