| 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: <dc012ad3-ec28-0959-426e-25ecc8b4f755@linux.intel.com>
Date: Thu, 10 Apr 2025 15:23:23 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Xi Pardee <xi.pardee@...ux.intel.com>
cc: irenic.rajneesh@...il.com, david.e.box@...ux.intel.com,
Hans de Goede <hdegoede@...hat.com>, platform-driver-x86@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>, linux-pm@...r.kernel.org
Subject: Re: [PATCH v3 1/8] platform/x86:intel/pmc: Move PMC Core related
functions
On Wed, 9 Apr 2025, Xi Pardee wrote:
> Move functions that implements PMC Core feature from core_ssram.c
> to core.c. This patch is a preparation step to introduce a new
> SSRAM Telemetry driver for the SSRAM device.
>
> Signed-off-by: Xi Pardee <xi.pardee@...ux.intel.com>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> ---
> drivers/platform/x86/intel/pmc/core.c | 168 +++++++++++++++++++
> drivers/platform/x86/intel/pmc/core.h | 9 +-
> drivers/platform/x86/intel/pmc/core_ssram.c | 175 --------------------
> 3 files changed, 176 insertions(+), 176 deletions(-)
>
> diff --git a/drivers/platform/x86/intel/pmc/core.c b/drivers/platform/x86/intel/pmc/core.c
> index 7a1d11f2914f..a42dc62d70da 100644
> --- a/drivers/platform/x86/intel/pmc/core.c
> +++ b/drivers/platform/x86/intel/pmc/core.c
> @@ -1345,6 +1345,173 @@ static void pmc_core_dbgfs_register(struct pmc_dev *pmcdev)
> }
> }
>
> +static u32 pmc_core_find_guid(struct pmc_info *list, const struct pmc_reg_map *map)
> +{
> + for (; list->map; ++list)
> + if (list->map == map)
> + return list->guid;
> +
> + return 0;
> +}
> +
> +static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc)
> +{
> + struct telem_endpoint *ep;
> + const u8 *lpm_indices;
> + int num_maps, mode_offset = 0;
> + int ret, mode;
> + int lpm_size;
> + u32 guid;
> +
> + lpm_indices = pmc->map->lpm_reg_index;
> + num_maps = pmc->map->lpm_num_maps;
> + lpm_size = LPM_MAX_NUM_MODES * num_maps;
> +
> + guid = pmc_core_find_guid(pmcdev->regmap_list, pmc->map);
> + if (!guid)
> + return -ENXIO;
> +
> + ep = pmt_telem_find_and_register_endpoint(pmcdev->ssram_pcidev, guid, 0);
> + if (IS_ERR(ep)) {
> + dev_dbg(&pmcdev->pdev->dev, "couldn't get telem endpoint %ld",
> + PTR_ERR(ep));
It seems I started to suggest changes into a move only patch (so those
suggestions below would be better to implement as separate patches).
Please use %pe.
> + return -EPROBE_DEFER;
> + }
> +
> + pmc->lpm_req_regs = devm_kzalloc(&pmcdev->pdev->dev,
> + lpm_size * sizeof(u32),
> + GFP_KERNEL);
> + if (!pmc->lpm_req_regs) {
> + ret = -ENOMEM;
> + goto unregister_ep;
> + }
> +
> + /*
> + * PMC Low Power Mode (LPM) table
> + *
> + * In telemetry space, the LPM table contains a 4 byte header followed
> + * by 8 consecutive mode blocks (one for each LPM mode). Each block
> + * has a 4 byte header followed by a set of registers that describe the
> + * IP state requirements for the given mode. The IP mapping is platform
> + * specific but the same for each block, making for easy analysis.
> + * Platforms only use a subset of the space to track the requirements
> + * for their IPs. Callers provide the requirement registers they use as
> + * a list of indices. Each requirement register is associated with an
> + * IP map that's maintained by the caller.
> + *
> + * Header
> + * +----+----------------------------+----------------------------+
> + * | 0 | REVISION | ENABLED MODES |
> + * +----+--------------+-------------+-------------+--------------+
> + *
> + * Low Power Mode 0 Block
> + * +----+--------------+-------------+-------------+--------------+
> + * | 1 | SUB ID | SIZE | MAJOR | MINOR |
> + * +----+--------------+-------------+-------------+--------------+
> + * | 2 | LPM0 Requirements 0 |
> + * +----+---------------------------------------------------------+
> + * | | ... |
> + * +----+---------------------------------------------------------+
> + * | 29 | LPM0 Requirements 27 |
> + * +----+---------------------------------------------------------+
> + *
> + * ...
> + *
> + * Low Power Mode 7 Block
> + * +----+--------------+-------------+-------------+--------------+
> + * | | SUB ID | SIZE | MAJOR | MINOR |
> + * +----+--------------+-------------+-------------+--------------+
> + * | 60 | LPM7 Requirements 0 |
> + * +----+---------------------------------------------------------+
> + * | | ... |
> + * +----+---------------------------------------------------------+
> + * | 87 | LPM7 Requirements 27 |
> + * +----+---------------------------------------------------------+
> + *
> + */
It would be better to have such a long comment above the function as now
it breaks the entire function badly in half.
> + mode_offset = LPM_HEADER_OFFSET + LPM_MODE_OFFSET;
> + pmc_for_each_mode(mode, pmcdev) {
> + u32 *req_offset = pmc->lpm_req_regs + (mode * num_maps);
> + int m;
> +
> + for (m = 0; m < num_maps; m++) {
> + u8 sample_id = lpm_indices[m] + mode_offset;
> +
> + ret = pmt_telem_read32(ep, sample_id, req_offset, 1);
> + if (ret) {
> + dev_err(&pmcdev->pdev->dev,
> + "couldn't read Low Power Mode requirements: %d\n", ret);
> + devm_kfree(&pmcdev->pdev->dev, pmc->lpm_req_regs);
Won't the error propagate and devm_ will take care of freeing? Why is it
done explictly here?
> + goto unregister_ep;
> + }
> + ++req_offset;
> + }
> + mode_offset += LPM_REG_COUNT + LPM_MODE_OFFSET;
> + }
> +
> +unregister_ep:
> + pmt_telem_unregister_endpoint(ep);
> +
> + return ret;
> +}
--
i.
> +
> +static int pmc_core_ssram_get_lpm_reqs(struct pmc_dev *pmcdev)
> +{
> + int ret, i;
> +
> + if (!pmcdev->ssram_pcidev)
> + return -ENODEV;
> +
> + for (i = 0; i < ARRAY_SIZE(pmcdev->pmcs); ++i) {
> + if (!pmcdev->pmcs[i])
> + continue;
> +
> + ret = pmc_core_get_lpm_req(pmcdev, pmcdev->pmcs[i]);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid)
> +{
> + for (; list->map; ++list)
> + if (devid == list->devid)
> + return list->map;
> +
> + return NULL;
> +}
> +
> +int pmc_core_pmc_add(struct pmc_dev *pmcdev, u64 pwrm_base,
> + const struct pmc_reg_map *reg_map, int pmc_index)
> +{
> + struct pmc *pmc = pmcdev->pmcs[pmc_index];
> +
> + if (!pwrm_base)
> + return -ENODEV;
> +
> + /* Memory for primary PMC has been allocated in core.c */
> + if (!pmc) {
> + pmc = devm_kzalloc(&pmcdev->pdev->dev, sizeof(*pmc), GFP_KERNEL);
> + if (!pmc)
> + return -ENOMEM;
> + }
> +
> + pmc->map = reg_map;
> + pmc->base_addr = pwrm_base;
> + pmc->regbase = ioremap(pmc->base_addr, pmc->map->regmap_length);
> +
> + if (!pmc->regbase) {
> + devm_kfree(&pmcdev->pdev->dev, pmc);
> + return -ENOMEM;
> + }
> +
> + pmcdev->pmcs[pmc_index] = pmc;
> +
> + return 0;
> +}
> +
> /*
> * When supported, ssram init is used to achieve all available PMCs.
> * If ssram init fails, this function uses legacy method to at least get the
> @@ -1719,5 +1886,6 @@ static struct platform_driver pmc_core_driver = {
>
> module_platform_driver(pmc_core_driver);
>
> +MODULE_IMPORT_NS("INTEL_PMT_TELEMETRY");
> MODULE_LICENSE("GPL v2");
> MODULE_DESCRIPTION("Intel PMC Core Driver");
> diff --git a/drivers/platform/x86/intel/pmc/core.h b/drivers/platform/x86/intel/pmc/core.h
> index 945a1c440cca..09aac6daabbd 100644
> --- a/drivers/platform/x86/intel/pmc/core.h
> +++ b/drivers/platform/x86/intel/pmc/core.h
> @@ -24,6 +24,11 @@ struct telem_endpoint;
> #define MAX_NUM_PMC 3
> #define S0IX_BLK_SIZE 4
>
> +/* PCH query */
> +#define LPM_HEADER_OFFSET 1
> +#define LPM_REG_COUNT 28
> +#define LPM_MODE_OFFSET 1
> +
> /* Sunrise Point Power Management Controller PCI Device ID */
> #define SPT_PMC_PCI_DEVICE_ID 0x9d21
> #define SPT_PMC_BASE_ADDR_OFFSET 0x48
> @@ -485,7 +490,6 @@ extern const struct pmc_reg_map mtl_socm_reg_map;
> extern const struct pmc_reg_map mtl_ioep_reg_map;
>
> void pmc_core_get_tgl_lpm_reqs(struct platform_device *pdev);
> -int pmc_core_ssram_get_lpm_reqs(struct pmc_dev *pmcdev);
> int pmc_core_send_ltr_ignore(struct pmc_dev *pmcdev, u32 value, int ignore);
>
> int pmc_core_resume_common(struct pmc_dev *pmcdev);
> @@ -497,6 +501,9 @@ void pmc_core_set_device_d3(unsigned int device);
> int pmc_core_ssram_init(struct pmc_dev *pmcdev, int func);
>
> int generic_core_init(struct pmc_dev *pmcdev, struct pmc_dev_info *pmc_dev_info);
> +const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid);
> +int pmc_core_pmc_add(struct pmc_dev *pmcdev, u64 pwrm_base,
> + const struct pmc_reg_map *reg_map, int pmc_index);
>
> extern struct pmc_dev_info spt_pmc_dev;
> extern struct pmc_dev_info cnp_pmc_dev;
> diff --git a/drivers/platform/x86/intel/pmc/core_ssram.c b/drivers/platform/x86/intel/pmc/core_ssram.c
> index 739569803017..e1a83425d802 100644
> --- a/drivers/platform/x86/intel/pmc/core_ssram.c
> +++ b/drivers/platform/x86/intel/pmc/core_ssram.c
> @@ -14,7 +14,6 @@
> #include <linux/io-64-nonatomic-lo-hi.h>
>
> #include "core.h"
> -#include "../pmt/telemetry.h"
>
> #define SSRAM_HDR_SIZE 0x100
> #define SSRAM_PWRM_OFFSET 0x14
> @@ -24,142 +23,8 @@
> #define SSRAM_IOE_OFFSET 0x68
> #define SSRAM_DEVID_OFFSET 0x70
>
> -/* PCH query */
> -#define LPM_HEADER_OFFSET 1
> -#define LPM_REG_COUNT 28
> -#define LPM_MODE_OFFSET 1
> -
> DEFINE_FREE(pmc_core_iounmap, void __iomem *, if (_T) iounmap(_T))
>
> -static u32 pmc_core_find_guid(struct pmc_info *list, const struct pmc_reg_map *map)
> -{
> - for (; list->map; ++list)
> - if (list->map == map)
> - return list->guid;
> -
> - return 0;
> -}
> -
> -static int pmc_core_get_lpm_req(struct pmc_dev *pmcdev, struct pmc *pmc)
> -{
> - struct telem_endpoint *ep;
> - const u8 *lpm_indices;
> - int num_maps, mode_offset = 0;
> - int ret, mode;
> - int lpm_size;
> - u32 guid;
> -
> - lpm_indices = pmc->map->lpm_reg_index;
> - num_maps = pmc->map->lpm_num_maps;
> - lpm_size = LPM_MAX_NUM_MODES * num_maps;
> -
> - guid = pmc_core_find_guid(pmcdev->regmap_list, pmc->map);
> - if (!guid)
> - return -ENXIO;
> -
> - ep = pmt_telem_find_and_register_endpoint(pmcdev->ssram_pcidev, guid, 0);
> - if (IS_ERR(ep)) {
> - dev_dbg(&pmcdev->pdev->dev, "couldn't get telem endpoint %ld",
> - PTR_ERR(ep));
> - return -EPROBE_DEFER;
> - }
> -
> - pmc->lpm_req_regs = devm_kzalloc(&pmcdev->pdev->dev,
> - lpm_size * sizeof(u32),
> - GFP_KERNEL);
> - if (!pmc->lpm_req_regs) {
> - ret = -ENOMEM;
> - goto unregister_ep;
> - }
> -
> - /*
> - * PMC Low Power Mode (LPM) table
> - *
> - * In telemetry space, the LPM table contains a 4 byte header followed
> - * by 8 consecutive mode blocks (one for each LPM mode). Each block
> - * has a 4 byte header followed by a set of registers that describe the
> - * IP state requirements for the given mode. The IP mapping is platform
> - * specific but the same for each block, making for easy analysis.
> - * Platforms only use a subset of the space to track the requirements
> - * for their IPs. Callers provide the requirement registers they use as
> - * a list of indices. Each requirement register is associated with an
> - * IP map that's maintained by the caller.
> - *
> - * Header
> - * +----+----------------------------+----------------------------+
> - * | 0 | REVISION | ENABLED MODES |
> - * +----+--------------+-------------+-------------+--------------+
> - *
> - * Low Power Mode 0 Block
> - * +----+--------------+-------------+-------------+--------------+
> - * | 1 | SUB ID | SIZE | MAJOR | MINOR |
> - * +----+--------------+-------------+-------------+--------------+
> - * | 2 | LPM0 Requirements 0 |
> - * +----+---------------------------------------------------------+
> - * | | ... |
> - * +----+---------------------------------------------------------+
> - * | 29 | LPM0 Requirements 27 |
> - * +----+---------------------------------------------------------+
> - *
> - * ...
> - *
> - * Low Power Mode 7 Block
> - * +----+--------------+-------------+-------------+--------------+
> - * | | SUB ID | SIZE | MAJOR | MINOR |
> - * +----+--------------+-------------+-------------+--------------+
> - * | 60 | LPM7 Requirements 0 |
> - * +----+---------------------------------------------------------+
> - * | | ... |
> - * +----+---------------------------------------------------------+
> - * | 87 | LPM7 Requirements 27 |
> - * +----+---------------------------------------------------------+
> - *
> - */
> - mode_offset = LPM_HEADER_OFFSET + LPM_MODE_OFFSET;
> - pmc_for_each_mode(mode, pmcdev) {
> - u32 *req_offset = pmc->lpm_req_regs + (mode * num_maps);
> - int m;
> -
> - for (m = 0; m < num_maps; m++) {
> - u8 sample_id = lpm_indices[m] + mode_offset;
> -
> - ret = pmt_telem_read32(ep, sample_id, req_offset, 1);
> - if (ret) {
> - dev_err(&pmcdev->pdev->dev,
> - "couldn't read Low Power Mode requirements: %d\n", ret);
> - devm_kfree(&pmcdev->pdev->dev, pmc->lpm_req_regs);
> - goto unregister_ep;
> - }
> - ++req_offset;
> - }
> - mode_offset += LPM_REG_COUNT + LPM_MODE_OFFSET;
> - }
> -
> -unregister_ep:
> - pmt_telem_unregister_endpoint(ep);
> -
> - return ret;
> -}
> -
> -int pmc_core_ssram_get_lpm_reqs(struct pmc_dev *pmcdev)
> -{
> - int ret, i;
> -
> - if (!pmcdev->ssram_pcidev)
> - return -ENODEV;
> -
> - for (i = 0; i < ARRAY_SIZE(pmcdev->pmcs); ++i) {
> - if (!pmcdev->pmcs[i])
> - continue;
> -
> - ret = pmc_core_get_lpm_req(pmcdev, pmcdev->pmcs[i]);
> - if (ret)
> - return ret;
> - }
> -
> - return 0;
> -}
> -
> static void
> pmc_add_pmt(struct pmc_dev *pmcdev, u64 ssram_base, void __iomem *ssram)
> {
> @@ -203,50 +68,11 @@ pmc_add_pmt(struct pmc_dev *pmcdev, u64 ssram_base, void __iomem *ssram)
> intel_vsec_register(pcidev, &info);
> }
>
> -static const struct pmc_reg_map *pmc_core_find_regmap(struct pmc_info *list, u16 devid)
> -{
> - for (; list->map; ++list)
> - if (devid == list->devid)
> - return list->map;
> -
> - return NULL;
> -}
> -
> static inline u64 get_base(void __iomem *addr, u32 offset)
> {
> return lo_hi_readq(addr + offset) & GENMASK_ULL(63, 3);
> }
>
> -static int
> -pmc_core_pmc_add(struct pmc_dev *pmcdev, u64 pwrm_base,
> - const struct pmc_reg_map *reg_map, int pmc_index)
> -{
> - struct pmc *pmc = pmcdev->pmcs[pmc_index];
> -
> - if (!pwrm_base)
> - return -ENODEV;
> -
> - /* Memory for primary PMC has been allocated in core.c */
> - if (!pmc) {
> - pmc = devm_kzalloc(&pmcdev->pdev->dev, sizeof(*pmc), GFP_KERNEL);
> - if (!pmc)
> - return -ENOMEM;
> - }
> -
> - pmc->map = reg_map;
> - pmc->base_addr = pwrm_base;
> - pmc->regbase = ioremap(pmc->base_addr, pmc->map->regmap_length);
> -
> - if (!pmc->regbase) {
> - devm_kfree(&pmcdev->pdev->dev, pmc);
> - return -ENOMEM;
> - }
> -
> - pmcdev->pmcs[pmc_index] = pmc;
> -
> - return 0;
> -}
> -
> static int
> pmc_core_ssram_get_pmc(struct pmc_dev *pmcdev, int pmc_idx, u32 offset)
> {
> @@ -329,4 +155,3 @@ int pmc_core_ssram_init(struct pmc_dev *pmcdev, int func)
> return ret;
> }
> MODULE_IMPORT_NS("INTEL_VSEC");
> -MODULE_IMPORT_NS("INTEL_PMT_TELEMETRY");
>
Powered by blists - more mailing lists