linux-kernel - Re: [RESEND PATCH v3 2/2] PCI/PTM: fix to maintain pci_dev->ptm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220830162529.GA106073@bhelgaas>
Date:   Tue, 30 Aug 2022 11:25:29 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Rajvi Jingar <rajvi.jingar@...ux.intel.com>
Cc:     rafael.j.wysocki@...el.com, bhelgaas@...gle.com,
        david.e.box@...ux.intel.com, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        Kai-Heng Feng <kai.heng.feng@...onical.com>
Subject: Re: [RESEND PATCH v3 2/2] PCI/PTM: fix to maintain
 pci_dev->ptm_enabled

[+cc Kai-Heng]

On Tue, Aug 30, 2022 at 03:49:13AM -0700, Rajvi Jingar wrote:
> pci_dev->ptm_enabled needs to be maintained to reflect the current PTM
> state of the device. In pci_ptm_disable(), clear ptm_enabled from
> 'struct pci_dev' on disabling PTM state for the device.
> In pci_restore_ptm_state(), set dev->ptm_enabled based on the restored
> PTM state of the device.
> 
> In pci_ptm_disable(), perform ptm_enabled check to avoid config space
> access in case if PTM is already disabled for the device. ptm_enabled
> won't be set for non-PCIe devices so pci_is_pcie(dev) check is not
> needed anymore.

This one sounds like it's supposed to fix something, but I'm not clear
exactly what.

I have a vague memory of config accesses messing up a low power state.
But this is still completely magical and unmaintainable since AFAIK
there is nothing in the PCIe spec about avoiding config accesses when
PTM is disabled.

At the very least, we would need more details in the commit log and
a hint in the code about this.

> Signed-off-by: Rajvi Jingar <rajvi.jingar@...ux.intel.com>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> ---
>  v1->v2:
>    - add ptm_enabled check in pci_ptm_disable().
>    - set the dev->ptm_enabled value in pci_restore_ptm_state().
>  v2->v3:
>    - remove pci_is_pcie(dev) check in pci_ptm_disable().
>    - add Reviewed-by tag in commit message
> ---
>  drivers/pci/pcie/ptm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
> index 368a254e3124..1ce241d4538f 100644
> --- a/drivers/pci/pcie/ptm.c
> +++ b/drivers/pci/pcie/ptm.c
> @@ -34,7 +34,7 @@ void pci_disable_ptm(struct pci_dev *dev)
>  	int ptm;
>  	u16 ctrl;
>  
> -	if (!pci_is_pcie(dev))
> +	if (!dev->ptm_enabled)
>  		return;

This will conflict with a change Kai-Heng Feng and I have been working
on, but I can resolve it when applying.

>  	ptm = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_PTM);
> @@ -44,6 +44,7 @@ void pci_disable_ptm(struct pci_dev *dev)
>  	pci_read_config_word(dev, ptm + PCI_PTM_CTRL, &ctrl);
>  	ctrl &= ~(PCI_PTM_CTRL_ENABLE | PCI_PTM_CTRL_ROOT);
>  	pci_write_config_word(dev, ptm + PCI_PTM_CTRL, ctrl);
> +	dev->ptm_enabled = 0;
>  }
>  
>  void pci_save_ptm_state(struct pci_dev *dev)
> @@ -83,6 +84,7 @@ void pci_restore_ptm_state(struct pci_dev *dev)
>  
>  	cap = (u16 *)&save_state->cap.data[0];
>  	pci_write_config_word(dev, ptm + PCI_PTM_CTRL, *cap);
> +	dev->ptm_enabled = !!(*cap & PCI_PTM_CTRL_ENABLE);
>  }
>  
>  void pci_ptm_init(struct pci_dev *dev)
> -- 
> 2.25.1
>