Messages in this thread | | | From | Andy Shevchenko <> | Date | Fri, 2 Apr 2021 14:12:20 +0300 | Subject | Re: [PATCH] platform/x86: intel_pmc_core: export platform global_reset via sysfs. |
| |
On Fri, Apr 2, 2021 at 12:32 AM Tomas Winkler <tomas.winkler@intel.com> wrote: > > From: Tamar Mashiah <tamar.mashiah@intel.com> > > During PCH manufacturing a global reset has to be induced in order > for configuration changes take affect upon following platform reset.
effect upon the following ?
> 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 register in MMIO space is defined for Cannon Lake and newer PCHs. > > Cc: David E Box <david.e.box@intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Hmm... okay, I forgot this, so my additional comments above and below.
...
> +static int set_global_reset(struct pmc_dev *pmcdev) > +{ > + const struct pmc_reg_map *map = pmcdev->map; > + u32 reg; > + int err; > + > + mutex_lock(&pmcdev->lock);
> + if (!map->etr3_offset) { > + err = -EOPNOTSUPP; > + goto out_unlock; > + }
Do we really need this check under the 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 */
Somewhere you use cf9 (small letters) I suggest to be consistent and use the capitalized version everywhere.
> + 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) == 0) {
Can be written in a form of !(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;
> + reg = pmc_core_reg_read(pmcdev, map->etr3_offset); > + reg &= ETR3_CF9GR | ETR3_CF9LOCK;
And why no lock here?
> + return sysfs_emit(buf, "0x%08x", reg); > +}
-- With Best Regards, Andy Shevchenko
| |