RE: [PATCH v4] platform/x86: intel_pmc_core: export platform global_reset via sysfs.

From: Winkler, Tomas
Date: Wed Apr 07 2021 - 05:31:53 EST



> Hi,
>
> On 4/7/21 8:51 AM, Winkler, Tomas wrote:
> >>>
> >>> During PCH (platform/board) manufacturing process a global reset has
> >>> to be induced in order for configuration changes take the effect
> >>> upon following platform reset.
> >>> This setting was commonly done by accessing PMC registers via
> >>> /dev/mem but due to security concern /dev/mem access is much
> >>> restricted, hence the reason for exposing this setting via dedicated sysfs
> interface.
> >>> To prevent post manufacturing abuse the register is protected by
> >>> hardware locking.
> >>
> >> The purpose of this reset functionality is not entirely clear to me.
> >>
> >> Is this only used during production of a board? Or is this also
> >> something which a user/reseller may use as part of a factory-reset
> procedure?
> >
> > Board production and refurbishing of the board. I can try to rephrase but I
> thought all the info is in the commit message.
> > As a runtime feature a user can check that her/his platform is correctly
> sealed.
>
> Manufacturing is clear, refurbishing is very much not clear, do you mean
> actually desoldering the chip and replacing it with a new one ?
>
> >> If this is only used once during production, then I'm not sure if
> >> introducing a sysfs file for this is desirable.
> >
> > What do you suggest, than? I'm just guessing is where are you heading
> > so the answer is that the manufacturing is often already run on the
> production OS installation, w/o going into details swapping or reconfiguring
> the OS is not always an option.
> > The manufacturer is also a user of ours.
>
> Ok, so lets compromise here, please make use of the visibility sysfs attribute
> callback, which returns a umask and make the file read-only at the umask
> level if it has been sealed, to make it clear to users that they cannot write to
> it, the -EACCES error means 'Permission denied' so if the user is already root
> they are going to get mightily confused if ls -l shows the file is writable.
Okay, it seems a better solution if the file is the global reset,
but maybe this path should not be taken if we rename it to extended_test_mode_register3, than it's better to get EACCESS on a specific bit.
>
> Also on set you are checking that the written value is bit 20, and on show you
> are showing the contents of the "Extended Test Mode Register 3" in hex, or
> at least those bits you are willing to show.

The intention was to left the user space behave same as with direct register access (/dev/mem)

> So in essence what you are doing here is giving userspace (some) access to
> the "Extended Test Mode Register 3", I would prefer to spell that out
> explicitly. The global_reset sysfs file name to me too much hints at something
> which the user can trigger / use while it is not intended for user usage.

Yeah, Global reset is maybe too ambiguous name in a general context, this is not the standard platform reset.

I've left it in register form in order to keep the user space as it has access to the register (/dev/mem)
>
> Also the Documentation/ABI/testing/sysfs-platform-intel-pm file pretty much
> describes this as direct register access rather then as some reset mechanism.
>
> So I think it would be better to call the new file
> extended_test_mode_register3, this will also be useful if we need to provide
> access to other bits in the same register later; and this will be a good
> template to follow if we need to provide some access to other registers later
> too.

Need to sync with David on that he pointed just ow, that he plans to expose some more bits.

> Regards,
>
> Hans
>
>
>
> >
> >>
> >> Can you please provide a new version where the purpsoe of the newly
> >> introduced sysfs file is made more clear, both in the commit-msg as
> >> well as in
> >> the:
> > Okay I can do that.
> >>
> >> Documentation/ABI/testing/sysfs-platform-intel-pmc
> >>
> >> File ?
> >>
> >> Regards,
> >>
> >> Hans
> >>
> >>
> >>
> >>>
> >>> The register in MMIO space is defined for Cannon Lake and newer PCHs.
> >>>
> >>> Cc: David E Box <david.e.box@xxxxxxxxx>
> >>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> >>> Signed-off-by: Tamar Mashiah <tamar.mashiah@xxxxxxxxx>
> >>> Signed-off-by: Tomas Winkler <tomas.winkler@xxxxxxxxx>
> >>> ---
> >>> 2:
> >>> 1. Add locking for reading the ET3 register (Andy) 2. Fix few style
> >>> issues (Andy)
> >>> V3:
> >>> 1. Resend
> >>> v4:
> >>> 1. Fix return statement (Andy)
> >>> 2. Specify manufacturing process (Enrico)
> >>>
> >>> .../ABI/testing/sysfs-platform-intel-pmc | 11 +++
> >>> MAINTAINERS | 1 +
> >>> drivers/platform/x86/intel_pmc_core.c | 97 +++++++++++++++++++
> >>> drivers/platform/x86/intel_pmc_core.h | 6 ++
> >>> 4 files changed, 115 insertions(+)
> >>> create mode 100644
> >>> Documentation/ABI/testing/sysfs-platform-intel-pmc
> >>>
> >>> diff --git a/Documentation/ABI/testing/sysfs-platform-intel-pmc
> >>> b/Documentation/ABI/testing/sysfs-platform-intel-pmc
> >>> new file mode 100644
> >>> index 000000000000..7ce00e77fbcd
> >>> --- /dev/null
> >>> +++ b/Documentation/ABI/testing/sysfs-platform-intel-pmc
> >>> @@ -0,0 +1,11 @@
> >>> +What: /sys/devices/platform/<platform>/global_reset
> >>> +Date: Apr 2021
> >>> +KernelVersion: 5.13
> >>> +Contact: "Tomas Winkler" <tomas.winkler@xxxxxxxxx>
> >>> +Description:
> >>> + Display global reset setting bits for PMC.
> >>> + * bit 31 - global reset is locked
> >>> + * bit 20 - global reset is set
> >>> + Writing bit 20 value to the global_reset will induce
> >>> + a platform global reset upon consequent platform reset.
> >>> + in case the register is not locked.
> >>> diff --git a/MAINTAINERS b/MAINTAINERS index
> >>> 04f68e0cda64..618676eba8c8 100644
> >>> --- a/MAINTAINERS
> >>> +++ b/MAINTAINERS
> >>> @@ -9166,6 +9166,7 @@ M: Rajneesh Bhardwaj
> >> <irenic.rajneesh@xxxxxxxxx>
> >>> M: David E Box <david.e.box@xxxxxxxxx>
> >>> L: platform-driver-x86@xxxxxxxxxxxxxxx
> >>> S: Maintained
> >>> +F: Documentation/ABI/testing/sysfs-platform-intel-pmc
> >>> F: drivers/platform/x86/intel_pmc_core*
> >>>
> >>> INTEL PMIC GPIO DRIVERS
> >>> diff --git a/drivers/platform/x86/intel_pmc_core.c
> >>> b/drivers/platform/x86/intel_pmc_core.c
> >>> index ee2f757515b0..8afc198550a4 100644
> >>> --- a/drivers/platform/x86/intel_pmc_core.c
> >>> +++ b/drivers/platform/x86/intel_pmc_core.c
> >>> @@ -401,6 +401,7 @@ static const struct pmc_reg_map cnp_reg_map =
> {
> >>> .pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET,
> >>> .pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
> >>> .ltr_ignore_max = CNP_NUM_IP_IGN_ALLOWED,
> >>> + .etr3_offset = ETR3_OFFSET,
> >>> };
> >>>
> >>> static const struct pmc_reg_map icl_reg_map = { @@ -418,6 +419,7 @@
> >>> static const struct pmc_reg_map icl_reg_map = {
> >>> .pm_cfg_offset = CNP_PMC_PM_CFG_OFFSET,
> >>> .pm_read_disable_bit = CNP_PMC_READ_DISABLE_BIT,
> >>> .ltr_ignore_max = ICL_NUM_IP_IGN_ALLOWED,
> >>> + .etr3_offset = ETR3_OFFSET,
> >>> };
> >>>
> >>> static const struct pmc_bit_map tgl_clocksource_status_map[] = { @@
> >>> -585,6 +587,7 @@ static const struct pmc_reg_map tgl_reg_map = {
> >>> .lpm_sts = tgl_lpm_maps,
> >>> .lpm_status_offset = TGL_LPM_STATUS_OFFSET,
> >>> .lpm_live_status_offset = TGL_LPM_LIVE_STATUS_OFFSET,
> >>> + .etr3_offset = ETR3_OFFSET,
> >>> };
> >>>
> >>> static inline u32 pmc_core_reg_read(struct pmc_dev *pmcdev, int
> >>> reg_offset) @@ -603,6 +606,99 @@ static inline u64
> >> pmc_core_adjust_slp_s0_step(struct pmc_dev *pmcdev, u32 value)
> >>> return (u64)value * pmcdev->map->slp_s0_res_counter_step;
> >>> }
> >>>
> >>> +static int set_global_reset(struct pmc_dev *pmcdev) {
> >>> + const struct pmc_reg_map *map = pmcdev->map;
> >>> + u32 reg;
> >>> + int err;
> >>> +
> >>> + if (!map->etr3_offset)
> >>> + return -EOPNOTSUPP;
> >>> +
> >>> + mutex_lock(&pmcdev->lock);
> >>> +
> >>> + /* check if CF9 is locked */
> >>> + reg = pmc_core_reg_read(pmcdev, map->etr3_offset);
> >>> + if (reg & ETR3_CF9LOCK) {
> >>> + err = -EACCES;
> >>> + goto out_unlock;
> >>> + }
> >>> +
> >>> + /* write CF9 global reset bit */
> >>> + reg |= ETR3_CF9GR;
> >>> + pmc_core_reg_write(pmcdev, map->etr3_offset, reg);
> >>> +
> >>> + reg = pmc_core_reg_read(pmcdev, map->etr3_offset);
> >>> + if (!(reg & ETR3_CF9GR)) {
> >>> + err = -EIO;
> >>> + goto out_unlock;
> >>> + }
> >>> +
> >>> + err = 0;
> >>> +
> >>> +out_unlock:
> >>> + mutex_unlock(&pmcdev->lock);
> >>> + return err;
> >>> +}
> >>> +
> >>> +static ssize_t global_reset_show(struct device *dev,
> >>> + struct device_attribute *attr, char *buf) {
> >>> + struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> >>> + const struct pmc_reg_map *map = pmcdev->map;
> >>> + u32 reg;
> >>> +
> >>> + if (!map->etr3_offset)
> >>> + return -EOPNOTSUPP;
> >>> +
> >>> + mutex_lock(&pmcdev->lock);
> >>> +
> >>> + reg = pmc_core_reg_read(pmcdev, map->etr3_offset);
> >>> + reg &= ETR3_CF9GR | ETR3_CF9LOCK;
> >>> +
> >>> + mutex_unlock(&pmcdev->lock);
> >>> +
> >>> + return sysfs_emit(buf, "0x%08x", reg); }
> >>> +
> >>> +static ssize_t global_reset_store(struct device *dev,
> >>> + struct device_attribute *attr,
> >>> + const char *buf, size_t len)
> >>> +{
> >>> + struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> >>> + int err;
> >>> + u32 reg;
> >>> +
> >>> + err = kstrtouint(buf, 16, &reg);
> >>> + if (err)
> >>> + return err;
> >>> +
> >>> + /* allow only CF9 writes */
> >>> + if (reg != ETR3_CF9GR)
> >>> + return -EINVAL;
> >>> +
> >>> + err = set_global_reset(pmcdev);
> >>> + if (err)
> >>> + return err;
> >>> +
> >>> + return len;
> >>> +}
> >>> +static DEVICE_ATTR_RW(global_reset);
> >>> +
> >>> +static struct attribute *pmc_attrs[] = {
> >>> + &dev_attr_global_reset.attr,
> >>> + NULL
> >>> +};
> >>> +
> >>> +static const struct attribute_group pmc_attr_group = {
> >>> + .attrs = pmc_attrs,
> >>> +};
> >>> +
> >>> +static const struct attribute_group *pmc_dev_groups[] = {
> >>> + &pmc_attr_group,
> >>> + NULL
> >>> +};
> >>> +
> >>> static int pmc_core_dev_state_get(void *data, u64 *val) {
> >>> struct pmc_dev *pmcdev = data;
> >>> @@ -1364,6 +1460,7 @@ static struct platform_driver pmc_core_driver
> = {
> >>> .name = "intel_pmc_core",
> >>> .acpi_match_table = ACPI_PTR(pmc_core_acpi_ids),
> >>> .pm = &pmc_core_pm_ops,
> >>> + .dev_groups = pmc_dev_groups,
> >>> },
> >>> .probe = pmc_core_probe,
> >>> .remove = pmc_core_remove,
> >>> diff --git a/drivers/platform/x86/intel_pmc_core.h
> >>> b/drivers/platform/x86/intel_pmc_core.h
> >>> index f33cd2c34835..98ebdfe57138 100644
> >>> --- a/drivers/platform/x86/intel_pmc_core.h
> >>> +++ b/drivers/platform/x86/intel_pmc_core.h
> >>> @@ -200,6 +200,11 @@ enum ppfear_regs {
> >>> #define TGL_LPM_STATUS_OFFSET 0x1C3C
> >>> #define TGL_LPM_LIVE_STATUS_OFFSET 0x1C5C
> >>>
> >>> +/* Extended Test Mode Register 3 (CNL and later) */
> >>> +#define ETR3_OFFSET 0x1048
> >>> +#define ETR3_CF9GR BIT(20)
> >>> +#define ETR3_CF9LOCK BIT(31)
> >>> +
> >>> const char *tgl_lpm_modes[] = {
> >>> "S0i2.0",
> >>> "S0i2.1",
> >>> @@ -263,6 +268,7 @@ struct pmc_reg_map {
> >>> const u32 lpm_residency_offset;
> >>> const u32 lpm_status_offset;
> >>> const u32 lpm_live_status_offset;
> >>> + const u32 etr3_offset;
> >>> };
> >>>
> >>> /**
> >>>
> >