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: <e1ed82cb-6d20-4ca8-b047-4a02dde115a8@gmail.com>
Date: Tue, 9 Jul 2024 00:15:48 +0200
From: Heiner Kallweit <hkallweit1@...il.com>
To: George-Daniel Matei <danielgeorgem@...omium.org>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, linux-kernel@...r.kernel.org,
 linux-pci@...r.kernel.org, nic_swsd@...ltek.com, netdev@...r.kernel.org,
 Bjorn Helgaas <helgaas@...nel.org>
Subject: Re: [PATCH] PCI: r8169: add suspend/resume aspm quirk

On 08.07.2024 19:23, Bjorn Helgaas wrote:
> [+cc r8169 folks]
> 
> On Mon, Jul 08, 2024 at 03:38:15PM +0000, George-Daniel Matei wrote:
>> Added aspm suspend/resume hooks that run
>> before and after suspend and resume to change
>> the ASPM states of the PCI bus in order to allow
>> the system suspend while trying to prevent card hangs
> 
> Why is this needed?  Is there a r8169 defect we're working around?
> A BIOS defect?  Is there a problem report you can reference here?
> 

Basically the same question from my side. Apparently such a workaround
isn't needed on any other system. And Realtek NICs can be found on more
or less every consumer system. What's the root cause of the issue?
A silicon bug on the host side?

What is the RTL8168 chip version used on these systems?

ASPM L1 is disabled per default in r8169. So why is the patch needed
at all?

> s/Added/Add/
> 
> s/aspm/ASPM/ above
> 
> s/PCI bus/device and parent/
> 
> Add period at end of sentence.
> 
> Rewrap to fill 75 columns.
> 
>> Signed-off-by: George-Daniel Matei <danielgeorgem@...omium.org>
>> ---
>>  drivers/pci/quirks.c | 142 +++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 142 insertions(+)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index dc12d4a06e21..aa3dba2211d3 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -6189,6 +6189,148 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b0, aspm_l1_acceptable_latency
>>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b1, aspm_l1_acceptable_latency);
>>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c0, aspm_l1_acceptable_latency);
>>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c1, aspm_l1_acceptable_latency);
>> +
>> +static const struct dmi_system_id chromebox_match_table[] = {
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Brask"),
>> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
>> +		}
>> +	},
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Aurash"),
>> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
>> +		}
>> +	},
>> +		{
>> +		.matches = {
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Bujia"),
>> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
>> +		}
>> +	},
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Gaelin"),
>> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
>> +		}
>> +	},
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Gladios"),
>> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
>> +		}
>> +	},
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Hahn"),
>> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
>> +		}
>> +	},
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Jeev"),
>> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
>> +		}
>> +	},
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Kinox"),
>> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
>> +		}
>> +	},
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Kuldax"),
>> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
>> +		}
>> +	},
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Lisbon"),
>> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
>> +		}
>> +	},
>> +	{
>> +			.matches = {
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Moli"),
>> +			DMI_MATCH(DMI_BIOS_VENDOR, "coreboot"),
>> +		}
>> +	},
>> +	{ }
>> +};
>> +
>> +static void rtl8169_suspend_aspm_settings(struct pci_dev *dev)
>> +{
>> +	u16 val = 0;
>> +
>> +	if (dmi_check_system(chromebox_match_table)) {
>> +		//configure parent
>> +		pcie_capability_clear_and_set_word(dev->bus->self,
>> +						   PCI_EXP_LNKCTL,
>> +						   PCI_EXP_LNKCTL_ASPMC,
>> +						   PCI_EXP_LNKCTL_ASPM_L1);
>> +
>> +		pci_read_config_word(dev->bus->self,
>> +				     dev->bus->self->l1ss + PCI_L1SS_CTL1,
>> +				     &val);
>> +		val = (val & ~PCI_L1SS_CTL1_L1SS_MASK) |
>> +		      PCI_L1SS_CTL1_PCIPM_L1_2 | PCI_L1SS_CTL1_PCIPM_L1_2 |
>> +		      PCI_L1SS_CTL1_ASPM_L1_1;
>> +		pci_write_config_word(dev->bus->self,
>> +				      dev->bus->self->l1ss + PCI_L1SS_CTL1,
>> +				      val);
>> +
>> +		//configure device
>> +		pcie_capability_clear_and_set_word(dev, PCI_EXP_LNKCTL,
>> +						   PCI_EXP_LNKCTL_ASPMC,
>> +						   PCI_EXP_LNKCTL_ASPM_L1);
>> +
>> +		pci_read_config_word(dev, dev->l1ss + PCI_L1SS_CTL1, &val);
>> +		val = (val & ~PCI_L1SS_CTL1_L1SS_MASK) |
>> +		      PCI_L1SS_CTL1_PCIPM_L1_2 | PCI_L1SS_CTL1_PCIPM_L1_2 |
>> +		      PCI_L1SS_CTL1_ASPM_L1_1;
>> +		pci_write_config_word(dev, dev->l1ss + PCI_L1SS_CTL1, val);
>> +	}
>> +}
>> +
>> +DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_REALTEK, 0x8168,
>> +			  rtl8169_suspend_aspm_settings);
>> +
>> +static void rtl8169_resume_aspm_settings(struct pci_dev *dev)
>> +{
>> +	u16 val = 0;
>> +
>> +	if (dmi_check_system(chromebox_match_table)) {
>> +		//configure device
>> +		pcie_capability_clear_and_set_word(dev, PCI_EXP_LNKCTL,
>> +						   PCI_EXP_LNKCTL_ASPMC, 0);
>> +
>> +		pci_read_config_word(dev->bus->self,
>> +				     dev->bus->self->l1ss + PCI_L1SS_CTL1,
>> +				     &val);
>> +		val = val & ~PCI_L1SS_CTL1_L1SS_MASK;
>> +		pci_write_config_word(dev->bus->self,
>> +				      dev->bus->self->l1ss + PCI_L1SS_CTL1,
>> +				      val);
>> +
>> +		//configure parent
>> +		pcie_capability_clear_and_set_word(dev->bus->self,
>> +						   PCI_EXP_LNKCTL,
>> +						   PCI_EXP_LNKCTL_ASPMC, 0);
>> +
>> +		pci_read_config_word(dev->bus->self,
>> +				     dev->bus->self->l1ss + PCI_L1SS_CTL1,
>> +				     &val);
>> +		val = val & ~PCI_L1SS_CTL1_L1SS_MASK;
>> +		pci_write_config_word(dev->bus->self,
>> +				      dev->bus->self->l1ss + PCI_L1SS_CTL1,
>> +				      val);
> 
> Updates the parent (dev->bus->self) twice; was the first one supposed
> to update the device (dev)?
> 
> This doesn't restore the state as it existed before suspend.  Does
> this rely on other parts of restore to do that?
> 
>> +	}
>> +}
>> +
>> +DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_REALTEK, 0x8168,
>> +			 rtl8169_resume_aspm_settings);
>>  #endif
>>  
>>  #ifdef CONFIG_PCIE_DPC
>> -- 
>> 2.45.2.803.g4e1b14247a-goog
>>


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ