| Message ID | 20240222221521.32159-1-helgaas@kernel.org |
|---|---|
| Headers | show |
| Series | PCI/DPC: Clean up DPC vs AER/EDR ownership and Kconfig | expand |
On 2/23/2024 6:15 AM, Bjorn Helgaas wrote: > From: Bjorn Helgaas <bhelgaas@google.com> > > Previously we could request control of DPC without AER, which is illegal > per spec. Also, we could enable CONFIG_PCIE_DPC without CONFIG_PCIE_EDR, > which is also illegal. This series addresses both. I have a question here, how to understand the relationship EDR & AER ? somewhere EDR touches AER status without checking _OSC granted bits, such as pci_aer_raw_clear_status(edev); sometimes EDR calling AER with host->native_aer checked, like pcie_do_recovery() { ... if (host->native_aer || pcie_ports_native) { pcie_clear_device_status(dev); pci_aer_clear_nonfatal_status(dev); } ... } That is really confusing. could we do some cleanup to eliminate it ? such as seperate AER code into common code and runtime part. Thanks, Ethan > > Bjorn Helgaas (3): > PCI/DPC: Request DPC only if also requesting AER > PCI/DPC: Remove CONFIG_PCIE_EDR > PCI/DPC: Encapsulate pci_acpi_add_edr_notifier() > > drivers/acpi/pci_root.c | 22 ++++++++++++---------- > drivers/pci/pci.h | 4 ++++ > drivers/pci/pcie/Kconfig | 14 ++++---------- > drivers/pci/pcie/Makefile | 5 ++++- > drivers/pci/pcie/dpc.c | 10 ---------- > include/linux/pci-acpi.h | 8 -------- > 6 files changed, 24 insertions(+), 39 deletions(-) >
Hi, On 2/26/24 10:18 PM, Ethan Zhao wrote: > On 2/23/2024 6:15 AM, Bjorn Helgaas wrote: >> From: Bjorn Helgaas <bhelgaas@google.com> >> >> Previously we could request control of DPC without AER, which is illegal >> per spec. Also, we could enable CONFIG_PCIE_DPC without CONFIG_PCIE_EDR, >> which is also illegal. This series addresses both. > > I have a question here, how to understand the relationship EDR & AER ? > somewhere EDR touches AER status without checking _OSC granted bits, > such as > pci_aer_raw_clear_status(edev); Which_OSC bits? EDR code will only get triggered if OS advertises the EDR support (which also means OS supports AER and DPC), and both AER and DPC is owned by the firmware. During the EDR notification, the OS is allowed to touch AER and DPC registers. So there is no problem with EDR code using AER routines. > > sometimes EDR calling AER with host->native_aer checked, like > > pcie_do_recovery() > { > ... > if (host->native_aer || pcie_ports_native) { > pcie_clear_device_status(dev); > pci_aer_clear_nonfatal_status(dev); > } > ... > } > > That is really confusing. could we do some cleanup to eliminate it ? > such as seperate AER code into common code and runtime part. > > > Thanks, > Ethan > > >> >> Bjorn Helgaas (3): >> PCI/DPC: Request DPC only if also requesting AER >> PCI/DPC: Remove CONFIG_PCIE_EDR >> PCI/DPC: Encapsulate pci_acpi_add_edr_notifier() >> >> drivers/acpi/pci_root.c | 22 ++++++++++++---------- >> drivers/pci/pci.h | 4 ++++ >> drivers/pci/pcie/Kconfig | 14 ++++---------- >> drivers/pci/pcie/Makefile | 5 ++++- >> drivers/pci/pcie/dpc.c | 10 ---------- >> include/linux/pci-acpi.h | 8 -------- >> 6 files changed, 24 insertions(+), 39 deletions(-) >>
On 2/27/2024 2:35 PM, Kuppuswamy Sathyanarayanan wrote: > Hi, > > On 2/26/24 10:18 PM, Ethan Zhao wrote: >> On 2/23/2024 6:15 AM, Bjorn Helgaas wrote: >>> From: Bjorn Helgaas <bhelgaas@google.com> >>> >>> Previously we could request control of DPC without AER, which is illegal >>> per spec. Also, we could enable CONFIG_PCIE_DPC without CONFIG_PCIE_EDR, >>> which is also illegal. This series addresses both. >> I have a question here, how to understand the relationship EDR & AER ? >> somewhere EDR touches AER status without checking _OSC granted bits, >> such as >> pci_aer_raw_clear_status(edev); > > Which_OSC bits? > > EDR code will only get triggered if OS advertises the EDR support (which > also means OS supports AER and DPC), and both AER and DPC is owned by > the firmware. During the EDR notification, the OS is allowed to touch AER Means no need to check if host->native_aer ? why checked in pcie_do_recovery() ? Thanks, Ethan > and DPC registers. So there is no problem with EDR code using AER routines. > > >> sometimes EDR calling AER with host->native_aer checked, like >> >> pcie_do_recovery() >> { >> ... >> if (host->native_aer || pcie_ports_native) { >> pcie_clear_device_status(dev); >> pci_aer_clear_nonfatal_status(dev); >> } >> ... >> } >> >> That is really confusing. could we do some cleanup to eliminate it ? >> such as seperate AER code into common code and runtime part. >> >> >> Thanks, >> Ethan >> >> >>> Bjorn Helgaas (3): >>> PCI/DPC: Request DPC only if also requesting AER >>> PCI/DPC: Remove CONFIG_PCIE_EDR >>> PCI/DPC: Encapsulate pci_acpi_add_edr_notifier() >>> >>> drivers/acpi/pci_root.c | 22 ++++++++++++---------- >>> drivers/pci/pci.h | 4 ++++ >>> drivers/pci/pcie/Kconfig | 14 ++++---------- >>> drivers/pci/pcie/Makefile | 5 ++++- >>> drivers/pci/pcie/dpc.c | 10 ---------- >>> include/linux/pci-acpi.h | 8 -------- >>> 6 files changed, 24 insertions(+), 39 deletions(-) >>>
On 2/26/24 11:12 PM, Ethan Zhao wrote: > On 2/27/2024 2:35 PM, Kuppuswamy Sathyanarayanan wrote: >> Hi, >> >> On 2/26/24 10:18 PM, Ethan Zhao wrote: >>> On 2/23/2024 6:15 AM, Bjorn Helgaas wrote: >>>> From: Bjorn Helgaas <bhelgaas@google.com> >>>> >>>> Previously we could request control of DPC without AER, which is illegal >>>> per spec. Also, we could enable CONFIG_PCIE_DPC without CONFIG_PCIE_EDR, >>>> which is also illegal. This series addresses both. >>> I have a question here, how to understand the relationship EDR & AER ? >>> somewhere EDR touches AER status without checking _OSC granted bits, >>> such as >>> pci_aer_raw_clear_status(edev); >> >> Which_OSC bits? >> >> EDR code will only get triggered if OS advertises the EDR support (which >> also means OS supports AER and DPC), and both AER and DPC is owned by >> the firmware. During the EDR notification, the OS is allowed to touch AER > > Means no need to check if host->native_aer ? why checked in > pcie_do_recovery() ? pcie_do_recovery() can be called form EDR (FF) or native path. That check is used to skip device status clearing in FF mode. You can find details about it in https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/pci/pcie/err.c?id=068c29a248b6ddbfdf7bb150b547569759620d36 > > Thanks, > Ethan > >> and DPC registers. So there is no problem with EDR code using AER routines. >> >> >>> sometimes EDR calling AER with host->native_aer checked, like >>> >>> pcie_do_recovery() >>> { >>> ... >>> if (host->native_aer || pcie_ports_native) { >>> pcie_clear_device_status(dev); >>> pci_aer_clear_nonfatal_status(dev); >>> } >>> ... >>> } >>> >>> That is really confusing. could we do some cleanup to eliminate it ? >>> such as seperate AER code into common code and runtime part. >>> >>> >>> Thanks, >>> Ethan >>> >>>> Bjorn Helgaas (3): >>>> PCI/DPC: Request DPC only if also requesting AER >>>> PCI/DPC: Remove CONFIG_PCIE_EDR >>>> PCI/DPC: Encapsulate pci_acpi_add_edr_notifier() >>>> >>>> drivers/acpi/pci_root.c | 22 ++++++++++++---------- >>>> drivers/pci/pci.h | 4 ++++ >>>> drivers/pci/pcie/Kconfig | 14 ++++---------- >>>> drivers/pci/pcie/Makefile | 5 ++++- >>>> drivers/pci/pcie/dpc.c | 10 ---------- >>>> include/linux/pci-acpi.h | 8 -------- >>>> 6 files changed, 24 insertions(+), 39 deletions(-) >>>> >
From: Bjorn Helgaas <bhelgaas@google.com> Previously we could request control of DPC without AER, which is illegal per spec. Also, we could enable CONFIG_PCIE_DPC without CONFIG_PCIE_EDR, which is also illegal. This series addresses both. Bjorn Helgaas (3): PCI/DPC: Request DPC only if also requesting AER PCI/DPC: Remove CONFIG_PCIE_EDR PCI/DPC: Encapsulate pci_acpi_add_edr_notifier() drivers/acpi/pci_root.c | 22 ++++++++++++---------- drivers/pci/pci.h | 4 ++++ drivers/pci/pcie/Kconfig | 14 ++++---------- drivers/pci/pcie/Makefile | 5 ++++- drivers/pci/pcie/dpc.c | 10 ---------- include/linux/pci-acpi.h | 8 -------- 6 files changed, 24 insertions(+), 39 deletions(-)