| 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
| ||
|
Message-ID: <6a7b4b03-0bb4-492f-bcac-d4e9fd1a81ac@amd.com>
Date: Thu, 5 Oct 2023 13:56:27 -0500
From: Mario Limonciello <mario.limonciello@....com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: bhelgaas@...gle.com, lukas@...ner.de, linux-kernel@...r.kernel.org,
linux-pci@...r.kernel.org, mika.westerberg@...ux.intel.com
Subject: Re: [PATCH v2] PCI: Make d3cold_allowed sysfs attribute read only
On 10/5/2023 13:53, Bjorn Helgaas wrote:
> On Wed, Oct 04, 2023 at 09:47:31AM -0500, Mario Limonciello wrote:
>> Before d3cold was stable userspace was allowed to influence the kernel's
>> decision of whether to enable d3cold for a device by a sysfs file
>> `d3cold_allowed`. This potentially allows userspace to break the suspend
>> for the system.
>
> Is "Before d3cold was stable" referring to a "d3cold" read-only
> variable, or to Linux functionality of using D3cold, or ...?
I was referring to the previous thread's comments when I asked about the
history on it.
>
> In what sense does the `d3cold_allowed` sysfs file break suspend?
SoCs might not be able to get into their deepest sleep state if
userspace messes with it.
>
>> For debugging purposes `pci_port_pm=` can be used to control whether
>> a PCI port will go into D3cold and runtime PM can be turned off by
>> sysfs on PCI end points.
>
> I guess this should be "pcie_port_pm=", which affects *all* PCIe
> ports?
Yes.
>
> Which sysfs file turns off runtime PM for endpoints?
/sys/bus/pci/devices/*/power/control
>
>> Change the sysfs attribute to a noop that ignores the input when written
>> and shows a warning. Simplify the internal kernel logic to drop
>> `d3cold_allowed`.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
>> ---
>> v1->v2:
>> * Leave R/W and show a warning instead
>> * Mark deprecated in sysfs file
>> ---
>> Documentation/ABI/testing/sysfs-bus-pci | 4 ++--
>> drivers/pci/pci-acpi.c | 2 +-
>> drivers/pci/pci-sysfs.c | 14 ++------------
>> drivers/pci/pci.c | 3 +--
>> include/linux/pci.h | 1 -
>> 5 files changed, 6 insertions(+), 18 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
>> index ecf47559f495..b5db141dfee6 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-pci
>> +++ b/Documentation/ABI/testing/sysfs-bus-pci
>> @@ -283,8 +283,8 @@ Description:
>> device will never be put into D3Cold state. If it is set, the
>> device may be put into D3Cold state if other requirements are
>> satisfied too. Reading this attribute will show the current
>> - value of d3cold_allowed bit. Writing this attribute will set
>> - the value of d3cold_allowed bit.
>> + value of no_d3cold bit.
>> + Writing to this attribute is deprecated and will do nothing.
>>
>> What: /sys/bus/pci/devices/.../sriov_totalvfs
>> Date: November 2012
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index 05b7357bd258..a05350a4e49c 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -911,7 +911,7 @@ pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
>> {
>> int acpi_state, d_max;
>>
>> - if (pdev->no_d3cold || !pdev->d3cold_allowed)
>> + if (pdev->no_d3cold)
>> d_max = ACPI_STATE_D3_HOT;
>> else
>> d_max = ACPI_STATE_D3_COLD;
>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> index 5e741a05cf2c..52ed5a55a371 100644
>> --- a/drivers/pci/pci-sysfs.c
>> +++ b/drivers/pci/pci-sysfs.c
>> @@ -523,17 +523,7 @@ static ssize_t d3cold_allowed_store(struct device *dev,
>> struct device_attribute *attr,
>> const char *buf, size_t count)
>> {
>> - struct pci_dev *pdev = to_pci_dev(dev);
>> - unsigned long val;
>> -
>> - if (kstrtoul(buf, 0, &val) < 0)
>> - return -EINVAL;
>> -
>> - pdev->d3cold_allowed = !!val;
>> - pci_bridge_d3_update(pdev);
>> -
>> - pm_runtime_resume(dev);
>> -
>> + dev_warn_once(dev, "pci: writing to d3cold_allowed is deprecated\n");
>> return count;
>> }
>>
>> @@ -541,7 +531,7 @@ static ssize_t d3cold_allowed_show(struct device *dev,
>> struct device_attribute *attr, char *buf)
>> {
>> struct pci_dev *pdev = to_pci_dev(dev);
>> - return sysfs_emit(buf, "%u\n", pdev->d3cold_allowed);
>> + return sysfs_emit(buf, "%u\n", !pdev->no_d3cold);
>> }
>> static DEVICE_ATTR_RW(d3cold_allowed);
>> #endif
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 59c01d68c6d5..8c5a6f68f63d 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -3067,7 +3067,7 @@ static int pci_dev_check_d3cold(struct pci_dev *dev, void *data)
>> bool *d3cold_ok = data;
>>
>> if (/* The device needs to be allowed to go D3cold ... */
>> - dev->no_d3cold || !dev->d3cold_allowed ||
>> + dev->no_d3cold ||
>>
>> /* ... and if it is wakeup capable to do so from D3cold. */
>> (device_may_wakeup(&dev->dev) &&
>> @@ -3204,7 +3204,6 @@ void pci_pm_init(struct pci_dev *dev)
>> dev->d3hot_delay = PCI_PM_D3HOT_WAIT;
>> dev->d3cold_delay = PCI_PM_D3COLD_WAIT;
>> dev->bridge_d3 = pci_bridge_d3_possible(dev);
>> - dev->d3cold_allowed = true;
>>
>> dev->d1_support = false;
>> dev->d2_support = false;
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 8c7c2c3c6c65..5f4ed71d31f5 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -376,7 +376,6 @@ struct pci_dev {
>> unsigned int no_d1d2:1; /* D1 and D2 are forbidden */
>> unsigned int no_d3cold:1; /* D3cold is forbidden */
>> unsigned int bridge_d3:1; /* Allow D3 for bridge */
>> - unsigned int d3cold_allowed:1; /* D3cold is allowed by user */
>> unsigned int mmio_always_on:1; /* Disallow turning off io/mem
>> decoding during BAR sizing */
>> unsigned int wakeup_prepared:1;
>> --
>> 2.34.1
>>
Powered by blists - more mailing lists