RE: [PATCH] platform/x86: intel_pmc_core: export platform global_reset via sysfs.
> -Original Message- > From: Enrico Weigelt, metux IT consult > Sent: Friday, April 02, 2021 16:32 > To: Winkler, Tomas ; Rajneesh Bhardwaj > ; Box, David E ; Hans > de Goede ; Mark Gross > Cc: platform-driver-...@vger.kernel.org; linux-kernel@vger.kernel.org; > Mashiah, Tamar ; Andy Shevchenko > > Subject: Re: [PATCH] platform/x86: intel_pmc_core: export platform > global_reset via sysfs. > > On 01.04.21 23:31, Tomas Winkler wrote: > > Hi, > > > During PCH manufacturing a global reset has to be induced in order for > > configuration changes take affect 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. > > could you please define "manufacturing" ? The chip or board ? Board > > Is there any use for this, after the machine left the factory ? Refurbishing, mostly, the register is hw locked after that. > > > --mtx > > -- > --- > Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert > werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren > GPG/PGP-Schlüssel zu. > --- > Enrico Weigelt, metux IT consult > Free software and Linux embedded engineering i...@metux.net -- +49-151- > 27565287
Re: [PATCH] platform/x86: intel_pmc_core: export platform global_reset via sysfs.
On 01.04.21 23:31, Tomas Winkler wrote: Hi, During PCH manufacturing a global reset has to be induced in order for configuration changes take affect 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. could you please define "manufacturing" ? The chip or board ? Is there any use for this, after the machine left the factory ? --mtx -- --- Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren GPG/PGP-Schlüssel zu. --- Enrico Weigelt, metux IT consult Free software and Linux embedded engineering i...@metux.net -- +49-151-27565287
Re: [PATCH] platform/x86: intel_pmc_core: export platform global_reset via sysfs.
On Fri, Apr 2, 2021 at 12:32 AM Tomas Winkler wrote:
>
> From: Tamar Mashiah
>
> 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
> Reviewed-by: Andy Shevchenko
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(>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(>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
