linux-kernel - Re: [PATCH v1 1/5] platform/x86: intel_pmc

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]

Date:   Fri, 17 Nov 2017 14:49:25 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Souvik Kumar Chakravarty <souvik.k.chakravarty@...el.com>
Cc:     Platform Driver <platform-driver-x86@...r.kernel.org>,
        "dvhart@...radead.org" <dvhart@...radead.org>,
        Andy Shevchenko <andy@...radead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Rajneesh Bhardwaj <rajneesh.bhardwaj@...el.com>
Subject: Re: [PATCH v1 1/5] platform/x86: intel_pmc_ipc: Fix register names

On Thu, Nov 16, 2017 at 12:18 PM, Souvik Kumar Chakravarty
<souvik.k.chakravarty@...el.com> wrote:
> GCR TELEM register names have been fixed as per the External Design
> Sepcification (EDS) of Apollolake and GeminiLake. This makes it
> possible to fetch the complete 64-bit S0ix counter using exported APIs.
>
> This patch also fixes some alignment issues in the macro definition
> section.

>  /* Commands */
>  #define PMC_IPC_PMIC_ACCESS            0xFF
> -#define                PMC_IPC_PMIC_ACCESS_READ        0x0
> -#define                PMC_IPC_PMIC_ACCESS_WRITE       0x1
> +#define        PMC_IPC_PMIC_ACCESS_READ        0x0
> +#define        PMC_IPC_PMIC_ACCESS_WRITE       0x1

I don't see any usefulness of this hunk, honestly.


>  /* GCR reg offsets from gcr base*/
>  #define PMC_GCR_PMC_CFG_REG            0x08
> -#define PMC_GCR_TELEM_DEEP_S0IX_REG    0x78
> -#define PMC_GCR_TELEM_SHLW_S0IX_REG    0x80
> +#define PMC_GCR_TELEM_DEEP_S0IX_LO_REG 0x78
> +#define PMC_GCR_TELEM_DEEP_S0IX_HI_REG 0x7C
> +#define PMC_GCR_TELEM_SHLW_S0IX_LO_REG 0x80
> +#define PMC_GCR_TELEM_SHLW_S0IX_HI_REG 0x84

> -       deep = gcr_data_readq(PMC_GCR_TELEM_DEEP_S0IX_REG);
> -       shlw = gcr_data_readq(PMC_GCR_TELEM_SHLW_S0IX_REG);
> +       deep = gcr_data_readq(PMC_GCR_TELEM_DEEP_S0IX_LO_REG);
> +       shlw = gcr_data_readq(PMC_GCR_TELEM_SHLW_S0IX_LO_REG);

I didn't get how this helps to what commit message states.
Moreover, it's a common practice to name 64-bit registers without
LO/HI split (in case we access them using readq() / writeq() or alike
functions).

-- 
With Best Regards,
Andy Shevchenko