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
