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

[Andy Shevchenko]

On Fri, Apr 2, 2021 at 12:32 AM Tomas Winkler <tomas.winkler@xxxxxxxxx> wrote:
>
> From: Tamar Mashiah <tamar.mashiah@xxxxxxxxx>
>
> 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@xxxxxxxxx>

> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

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