| 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
| ||
|
Date: Tue, 6 Jun 2023 14:40:45 -0500
From: "Limonciello, Mario" <mario.limonciello@....com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: "Rafael J . Wysocki" <rafael@...nel.org>,
linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
Len Brown <lenb@...nel.org>, linux-acpi@...r.kernel.org
Subject: Re: [PATCH v2] PCI: Call _REG when saving/restoring PCI state
On 6/6/2023 2:23 PM, Bjorn Helgaas wrote:
> On Tue, Jun 06, 2023 at 11:23:21AM -0500, Mario Limonciello wrote:
>> ASMedia PCIe GPIO controllers fail functional tests after returning from
>> suspend (S3 or s2idle). This is because the BIOS checks whether the
>> OSPM has called the `_REG` method to determine whether it can interact with
>> the OperationRegion assigned to the device.
>>
>> As described in 6.5.4 in the APCI spec, `_REG` is used to inform the AML
>> code on the availability of an operation region.
>>
>> To fix this issue, call acpi_evaluate_reg() when saving and restoring the
>> state of PCI devices.
>>
>> Link: https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/06_Device_Configuration/Device_Configuration.html#reg-region
>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
>> ---
>> v1->v2:
>> * Handle case of no CONFIG_ACPI
>> * Rename function
>> * Update commit message
>> * Move ACPI calling code into pci-acpi.c instead
>> * Cite the ACPI spec
> Thanks for the spec reference (s/APCI/ACPI/ and add the revision if
> you rev this (r6.5 is the latest, AFAIK) if you rev this).
>
> I don't see text in that section that connects S3 with _REG. If it's
> there, you might have to quote the relevant sentence or two in the
> commit log.
I don't think there is anything the spec connecting this
with S3. At least from my perspective S3 is the reason
this was exposed but there is a deficiency that exists
that _REG is not being called by Linux.
I intend to re-word the commit message something to the
effect of explaining what _REG does and why _REG should be
called, along with citations.
Then in another paragraph "Fixing this resolves an issue ...".
>
> You mentioned _REG being sort of a mutex to synchronize OSPM vs
> platform access; if there's spec language to that effect, let's cite
> it.
That sentence I included was cited from the spec.
> Ideally we should have been able to read the PCI and ACPI specs and
> implement this without tripping over problem on this particular
> hardware. I'm looking for the text that enables that "clean-room"
> implementation. If the spec doesn't have that text, it's either a
> hole in the spec or a BIOS defect that depends on something the spec
> doesn't require.
IMO both the spec and BIOS are correct, it's a Linux
issue that _REG wasn't used.
Hopefully disconnecting the issue as just an example
will achieve what you're looking for.
>
> Doing this in pci_save_state() still seems wrong to me. For example,
> e1000_probe() calls pci_save_state(), but this is not part of suspend.
> IIUC, this patch will disconnect the opregion when we probe an e1000
> NIC. Is that what you intend?
Thanks for pointing this one out. I was narrowly focused
on callers in PCI core. This was a caller I wasn't
aware of; I agree it doesn't make sense.
I think pci_set_power_state() might be another good
candidate to use. What do you think of this?
Or can you suggest another call site? I'm hesitant
to put this into PCI suspend/resume code, because
I think this same issue could happen at runtime too.
>> ---
>> drivers/pci/pci-acpi.c | 10 ++++++++++
>> drivers/pci/pci.c | 14 ++++++++++++++
>> drivers/pci/pci.h | 2 ++
>> 3 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>> index 1698205dd73c..abc8bcfc2c71 100644
>> --- a/drivers/pci/pci-acpi.c
>> +++ b/drivers/pci/pci-acpi.c
>> @@ -1209,6 +1209,16 @@ void acpi_pci_remove_bus(struct pci_bus *bus)
>> acpi_pci_slot_remove(bus);
>> }
>>
>> +void acpi_pci_set_register_access(struct pci_dev *dev, bool enable)
>> +{
>> + int val = enable ? ACPI_REG_CONNECT : ACPI_REG_DISCONNECT;
>> + int ret = acpi_evaluate_reg(ACPI_HANDLE(&dev->dev),
>> + ACPI_ADR_SPACE_PCI_CONFIG, val);
>> + if (ret)
>> + pci_dbg(dev, "ACPI _REG %s evaluation failed (%d)\n",
>> + val ? "connect" : "disconnect", ret);
>> +}
>> +
>> /* ACPI bus type */
>>
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index e38c2f6eebd4..b2f1f603ec62 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -1068,6 +1068,14 @@ static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
>> return acpi_pci_bridge_d3(dev);
>> }
>>
>> +static inline void platform_set_register_access(struct pci_dev *dev, bool en)
>> +{
>> + if (pci_use_mid_pm())
>> + return;
>> +
>> + acpi_pci_set_register_access(dev, en);
>> +}
>> +
>> /**
>> * pci_update_current_state - Read power state of given device and cache it
>> * @dev: PCI device to handle.
>> @@ -1645,6 +1653,9 @@ static void pci_restore_ltr_state(struct pci_dev *dev)
>> int pci_save_state(struct pci_dev *dev)
>> {
>> int i;
>> +
>> + platform_set_register_access(dev, false);
>> +
>> /* XXX: 100% dword access ok here? */
>> for (i = 0; i < 16; i++) {
>> pci_read_config_dword(dev, i * 4, &dev->saved_config_space[i]);
>> @@ -1790,6 +1801,8 @@ void pci_restore_state(struct pci_dev *dev)
>> pci_enable_acs(dev);
>> pci_restore_iov_state(dev);
>>
>> + platform_set_register_access(dev, true);
>> +
>> dev->state_saved = false;
>> }
>> EXPORT_SYMBOL(pci_restore_state);
>> @@ -3203,6 +3216,7 @@ void pci_pm_init(struct pci_dev *dev)
>> pci_read_config_word(dev, PCI_STATUS, &status);
>> if (status & PCI_STATUS_IMM_READY)
>> dev->imm_ready = 1;
>> + platform_set_register_access(dev, true);
>> }
>>
>> static unsigned long pci_ea_flags(struct pci_dev *dev, u8 prop)
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index ffccb03933e2..78961505aae2 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -703,6 +703,7 @@ void acpi_pci_refresh_power_state(struct pci_dev *dev);
>> int acpi_pci_wakeup(struct pci_dev *dev, bool enable);
>> bool acpi_pci_need_resume(struct pci_dev *dev);
>> pci_power_t acpi_pci_choose_state(struct pci_dev *pdev);
>> +void acpi_pci_set_register_access(struct pci_dev *dev, bool enable);
>> #else
>> static inline int pci_dev_acpi_reset(struct pci_dev *dev, bool probe)
>> {
>> @@ -742,6 +743,7 @@ static inline pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)
>> {
>> return PCI_POWER_ERROR;
>> }
>> +static inline void acpi_pci_set_register_access(struct pci_dev *dev, bool enable) {}
>> #endif
>>
>> #ifdef CONFIG_PCIEASPM
>> --
>> 2.34.1
>>
Powered by blists - more mailing lists