lkml.org 
[lkml]   [2021]   [Apr]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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

\
 
 \ /
  Last update: 2021-04-02 13:13    [W:0.064 / U:0.280 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site