Bjorn Helgaas [Wed, 15 Aug 2018 19:59:01 +0000 (14:59 -0500)]
Merge branch 'pci/resource'
- Clean up devm_of_pci_get_host_bridge_resources() resource allocation
(Jan Kiszka)
- Fixup resizable BARs after suspend/resume (Christian König)
- Make "pci=earlydump" generic (Sinan Kaya)
- Fix ROM BAR access routines to stay in bounds and check for signature
correctly (Rex Zhu)
* pci/resource:
PCI: Make pci_get_rom_size() static
PCI: Add check code for last image indicator not set
PCI: Avoid accessing memory outside the ROM BAR
PCI: Make early dump functionality generic
PCI: Cleanup PCI_REBAR_CTRL_BAR_SHIFT handling
PCI: Restore resized BAR state on resume
PCI: Clean up resource allocation in devm_of_pci_get_host_bridge_resources()
Bjorn Helgaas [Wed, 15 Aug 2018 19:58:58 +0000 (14:58 -0500)]
Merge branch 'pci/peer-to-peer'
- Add "pci=disable_acs_redir=" parameter to disable ACS redirection for
peer-to-peer DMA support (we don't have the peer-to-peer support yet;
this is just one piece) (Logan Gunthorpe)
* pci/peer-to-peer:
PCI: Add ACS Redirect disable quirk for Intel Sunrise Point
PCI: Add device-specific ACS Redirect disable infrastructure
PCI: Convert device-specific ACS quirks from NULL termination to ARRAY_SIZE
PCI: Add "pci=disable_acs_redir=" parameter for peer-to-peer support
PCI: Allow specifying devices using a base bus and path of devfns
PCI: Make specifying PCI devices in kernel parameters reusable
PCI: Hide ACS quirk declarations inside PCI core
Bjorn Helgaas [Wed, 15 Aug 2018 19:58:54 +0000 (14:58 -0500)]
Merge branch 'pci/misc'
- Mark fall-through switch cases before enabling -Wimplicit-fallthrough
(Gustavo A. R. Silva)
- Move DMA-debug PCI init from arch code to PCI core (Christoph Hellwig)
- Fix pci_request_irq() usage of IRQF_ONESHOT when no handler is supplied
(Heiner Kallweit)
- Unify PCI and DMA direction #defines (Shunyong Yang)
- Add PCI_DEVICE_DATA() macro (Andy Shevchenko)
- Check for VPD completion before checking for timeout (Bert Kenward)
- Limit Netronome NFP5000 config space size to work around erratum (Jakub
Kicinski)
* pci/misc:
PCI: Limit config space size for Netronome NFP5000
PCI/VPD: Check for VPD access completion before checking for timeout
PCI: Add PCI_DEVICE_DATA() macro to fully describe device ID entry
PCI: Unify PCI and normal DMA direction definitions
PCI: Use IRQF_ONESHOT if pci_request_irq() called with no handler
PCI: Call dma_debug_add_bus() for pci_bus_type from PCI core
PCI: Mark fall-through switch cases before enabling -Wimplicit-fallthrough
- Convert pciehp to threaded IRQ handling (Lukas Wunner)
- Improve pciehp tolerance of missed events and initially unstable links
(Lukas Wunner)
- Clear spurious pciehp events on resume (Lukas Wunner)
- Add pciehp runtime PM support, including for Thunderbolt controllers
(Lukas Wunner)
- Support interrupts from pciehp bridges in D3hot (Lukas Wunner)
* pci/hotplug:
PCI: pciehp: Deduplicate presence check on probe & resume
PCI: pciehp: Avoid implicit fallthroughs in switch statements
PCI: Whitelist Thunderbolt ports for runtime D3
PCI: Whitelist native hotplug ports for runtime D3
PCI: sysfs: Resume to D0 on function reset
PCI: pciehp: Resume parent to D0 on config space access
PCI: pciehp: Resume to D0 on enable/disable
PCI: pciehp: Support interrupts sent from D3hot
PCI: pciehp: Obey compulsory command delay after resume
PCI: pciehp: Clear spurious events earlier on resume
PCI: portdrv: Deduplicate PM callback iterator
PCI: pciehp: Avoid slot access during reset
PCI: pciehp: Always enable occupied slot on probe
PCI: pciehp: Become resilient to missed events
PCI: pciehp: Tolerate initially unstable link
PCI: pciehp: Declare pciehp_enable/disable_slot() static
PCI: pciehp: Drop enable/disable lock
PCI: pciehp: Enable/disable exclusively from IRQ thread
PCI: pciehp: Track enable/disable status
PCI: pciehp: Publish to user space last on probe
PCI: hotplug: Demidlayer registration with the core
PCI: pciehp: Drop slot workqueue
PCI: pciehp: Handle events synchronously
PCI: pciehp: Stop blinking on slot enable failure
PCI: pciehp: Convert to threaded polling
PCI: pciehp: Convert to threaded IRQ
PCI: pciehp: Document struct slot and struct controller
PCI: pciehp: Declare pciehp_unconfigure_device() void
PCI: pciehp: Drop unnecessary NULL pointer check
PCI: pciehp: Fix unprotected list iteration in IRQ handler
PCI: pciehp: Fix use-after-free on unplug
PCI: hotplug: Don't leak pci_slot on registration failure
PCI: hotplug: Delete skeleton driver
PCI: shpchp: Separate existence of SHPC and permission to use it
Bjorn Helgaas [Wed, 15 Aug 2018 19:58:51 +0000 (14:58 -0500)]
Merge branch 'pci/dpc'
- Defer DPC event handling to work queue (Keith Busch)
- Use threaded IRQ for DPC bottom half (Keith Busch)
- Print AER status while handling DPC events (Keith Busch)
* pci/dpc:
PCI/DPC: Remove indirection waiting for inactive link
PCI/DPC: Use threaded IRQ for bottom half handling
PCI/DPC: Print AER status in DPC event handling
PCI/DPC: Remove rp_pio_status from dpc struct
PCI/DPC: Defer event handling to work queue
PCI/DPC: Leave interrupts enabled while handling event
Bjorn Helgaas [Wed, 15 Aug 2018 19:58:46 +0000 (14:58 -0500)]
Merge branch 'pci/aspm'
- Use sysfs_match_string() to simplify ASPM sysfs parsing (Andy
Shevchenko)
- Remove unnecessary includes of <linux/pci-aspm.h> (Bjorn Helgaas)
* pci/aspm:
PCI: Remove unnecessary include of <linux/pci-aspm.h>
iwlwifi: Remove unnecessary include of <linux/pci-aspm.h>
ath9k: Remove unnecessary include of <linux/pci-aspm.h>
igb: Remove unnecessary include of <linux/pci-aspm.h>
PCI/ASPM: Convert to use sysfs_match_string() helper
Bjorn Helgaas [Wed, 15 Aug 2018 19:58:45 +0000 (14:58 -0500)]
Merge branch 'pci/aer'
- Decode AER errors with names similar to "lspci" (Tyler Baicar)
- Expose AER statistics in sysfs (Rajat Jain)
- Clear AER status bits selectively based on the type of recovery (Oza
Pawandeep)
- Honor "pcie_ports=native" even if HEST sets FIRMWARE_FIRST (Alexandru
Gagniuc)
- Don't clear AER status bits if we're using the "Firmware-First"
strategy where firmware owns the registers (Alexandru Gagniuc)
* pci/aer:
PCI/AER: Don't clear AER bits if error handling is Firmware-First
PCI/AER: Remove duplicate PCI_EXP_AER_FLAGS definition
PCI/portdrv: Remove pcie_portdrv_err_handler.slot_reset
PCI/AER: Clear device status bits during ERR_COR handling
PCI/AER: Clear device status bits during ERR_FATAL and ERR_NONFATAL
PCI/AER: Remove ERR_FATAL code from ERR_NONFATAL path
PCI/AER: Factor out ERR_NONFATAL status bit clearing
PCI/AER: Clear only ERR_NONFATAL bits during non-fatal recovery
PCI/AER: Clear only ERR_FATAL status bits during fatal recovery
PCI/AER: Honor "pcie_ports=native" even if HEST sets FIRMWARE_FIRST
PCI/AER: Add sysfs attributes for rootport cumulative stats
PCI/AER: Add sysfs attributes to provide AER stats and breakdown
PCI/AER: Define aer_stats structure for AER capable devices
PCI/AER: Move internal declarations to drivers/pci/pci.h
PCI/AER: Adopt lspci names for AER error decoding
PCI/AER: Expose internal API for obtaining AER information
PCI/AER: Don't clear AER bits if error handling is Firmware-First
If the platform requests Firmware-First error handling, firmware is
responsible for reading and clearing AER status bits. If OSPM also clears
them, we may miss errors. See ACPI v6.2, sec 18.3.2.5 and 18.4.
This race is mostly of theoretical significance, as it is not easy to
reasonably demonstrate it in testing.
Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
[bhelgaas: add similar guards to pci_cleanup_aer_uncorrect_error_status()
and pci_aer_clear_fatal_status()] Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Jakub Kicinski [Tue, 14 Aug 2018 23:48:50 +0000 (16:48 -0700)]
PCI: Limit config space size for Netronome NFP5000
Like the NFP4000 and NFP6000, the NFP5000 as an erratum where reading/
writing to PCI config space addresses above 0x600 can cause the NFP to
generate PCIe completion timeouts.
Limit the NFP5000's PF's config space size to 0x600 bytes as is already
done for the NFP4000 and NFP6000.
The NFP5000's VF is 0x6003 (PCI_DEVICE_ID_NETRONOME_NFP6000_VF), the same
device ID as the NFP6000's VF. Thus, its config space is already limited
by the existing use of quirk_nfp6000().
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Tony Egan <tony.egan@netronome.com>
Heiner Kallweit [Sun, 5 Aug 2018 20:31:03 +0000 (22:31 +0200)]
PCI/MSI: Set IRQCHIP_ONESHOT_SAFE for PCI-MSI irqchips
If flag IRQCHIP_ONESHOT_SAFE isn't set for an irqchip and we have a
threaded interrupt with no primary handler, flag IRQF_ONESHOT needs to be
set for the interrupt, causing some overhead in the threaded interrupt
handler. For more detailed explanation also check following comment in
__setup_irq():
The interrupt was requested with handler = NULL, so we use the default
primary handler for it. But it does not have the oneshot flag set. In
combination with level interrupts this is deadly, because the default
primary handler just wakes the thread, then the irq lines is reenabled,
but the device still has the level irq asserted. Rinse and repeat....
While this works for edge type interrupts, we play it safe and reject
unconditionally because we can't say for sure which type this interrupt
really has. The type flags are unreliable as the underlying chip
implementation can override them.
Another comment in __setup_irq() gives a hint already that this
overhead can be avoided for PCI-MSI:
Some irq chips like MSI based interrupts are per se one shot safe. Check
the chip flags, so we can avoid the unmask dance at the end of the
threaded handler for those.
Following this let's mark all PCI-MSI irqchips as oneshot-safe.
See also discussion here:
https://lkml.kernel.org/r/alpine.DEB.2.21.1808032136490.1658@nanos.tec.linutronix.de
PCI/VPD: Check for VPD access completion before checking for timeout
Previously we checked the timeout before checking the VPD access completion
bit. On a very heavily loaded system this can cause VPD access to timeout.
Check the completion bit before checking the timeout.
Drivers can be converted later on in independent way.
While here, remove the unused macro with the same name from Ralink wireless
driver.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Acked-by: Kalle Valo <kvalo@codeaurora.org> # for rt2x00
Myron Stowe [Mon, 13 Aug 2018 18:19:46 +0000 (12:19 -0600)]
PCI: Match Root Port's MPS to endpoint's MPSS as necessary
In commit e772deb74917 ("PCI: Set MPS to match upstream bridge"), we made
sure every device's MPS setting matches its upstream bridge, making it more
likely that a hot-added device will work in a system with an optimized MPS
configuration.
Recently I've started encountering systems where the endpoint device's MPSS
capability is less than its Root Port's current MPS value, thus the
endpoint is not capable of matching its upstream bridge's MPS setting (see:
bugzilla via "Link:" below). This leaves the system vulnerable - the
upstream Root Port could respond with larger TLPs than the device can
handle, and the device will consider them to be 'Malformed'.
One could use the "pci=pcie_bus_safe" kernel parameter to work around the
issue, but that forces a user to supply a kernel parameter to get the
system to function reliably and may end up limiting MPS settings of other
unrelated, sub-topologies which could benefit from maintaining their larger
values.
Augment Keith's approach to include tuning down a Root Port's MPS setting
when its hot-added endpoint device is not capable of matching it.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=200527 Signed-off-by: Myron Stowe <myron.stowe@redhat.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Acked-by: Jon Mason <jdmason@kudzu.us> Cc: Keith Busch <keith.busch@intel.com> Cc: Sinan Kaya <okaya@kernel.org> Cc: Dongdong Liu <liudongdong3@huawei.com>
Myron Stowe [Mon, 13 Aug 2018 18:19:39 +0000 (12:19 -0600)]
PCI: Skip MPS logic for Virtual Functions (VFs)
PCIe r4.0, sec 9.3.5.4, "Device Control Register", shows both
Max_Payload_Size (MPS) and Max_Read_request_Size (MRRS) to be 'RsvdP' for
VFs. Just prior to the table it states:
"PF and VF functionality is defined in Section 7.5.3.4 except where
noted in Table 9-16. For VF fields marked 'RsvdP', the PF setting
applies to the VF."
All of which implies that with respect to Max_Payload_Size Supported
(MPSS), MPS, and MRRS values, we should not be paying any attention to the
VF's fields, but rather only to the PF's. Only looking at the PF's fields
also logically makes sense as it's the sole physical interface to the PCIe
bus.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=200527 Fixes: e772deb74917 ("PCI: Set MPS to match upstream bridge") Signed-off-by: Myron Stowe <myron.stowe@redhat.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Cc: stable@vger.kernel.org # 4.3+ Cc: Keith Busch <keith.busch@intel.com> Cc: Sinan Kaya <okaya@kernel.org> Cc: Dongdong Liu <liudongdong3@huawei.com> Cc: Jon Mason <jdmason@kudzu.us>
When both ends of a PCIe Link are capable of a higher bandwidth than is
currently in use, the Link is said to be "downtrained". A downtrained Link
may indicate hardware or configuration problems in the system, but it's
hard to identify such Links from userspace.
Refactor pcie_print_link_status() so it continues to always print PCIe
bandwidth information, as several NIC drivers desire.
Add a new internal __pcie_print_link_status() to emit a message only when a
device's bandwidth is constrained by the fabric and call it from the PCI
core for all devices, which identifies all downtrained Links. It also
emits messages for a few cases that are technically not downtrained, such
as a x4 device in an open-ended x1 slot.
Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
[bhelgaas: changelog, move __pcie_print_link_status() declaration to
drivers/pci/, rename pcie_check_upstream_link() to
pcie_report_downtraining()] Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Logan Gunthorpe [Thu, 9 Aug 2018 22:09:17 +0000 (17:09 -0500)]
PCI: Add ACS Redirect disable quirk for Intel Sunrise Point
Intel Sunrise Point PCH hardware has an implementation of the ACS bits that
does not comply with the PCIe standard. Add a device-specific quirk,
pci_quirk_disable_intel_spt_pch_acs_redir() to disable ACS Redirection on
this system.
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
[bhelgaas: changelog, split to separate patch] Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
Intel Sunrise Point (SPT) PCH hardware has an implementation of the ACS
bits that does not comply with the PCIe standard. To deal with this we
need device-specific quirks to disable ACS redirection.
Add a new pci_dev_specific_disable_acs_redir() quirk and a new
.disable_acs_redir() function pointer for use by non-compliant devices. No
functional change intended.
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
[bhelgaas: split to separate patch, move
pci_dev_specific_disable_acs_redir() declarations to drivers/pci/pci.h] Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
Logan Gunthorpe [Thu, 9 Aug 2018 21:45:47 +0000 (16:45 -0500)]
PCI: Convert device-specific ACS quirks from NULL termination to ARRAY_SIZE
Convert the search for device-specific ACS enable quirks from searching a
NULL-terminated array to iterating through the array, which is always
fixed-size anyway. No functional change intended.
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
[bhelgaas: changelog, split to separate patch for reviewability] Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
PCI: Add "pci=disable_acs_redir=" parameter for peer-to-peer support
To support peer-to-peer traffic on a segment of the PCI hierarchy, we must
disable the ACS redirect bits for select PCI bridges. The bridges must be
selected before the devices are discovered by the kernel and the IOMMU
groups created. Therefore, add a kernel command line parameter to specify
devices which must have their ACS bits disabled.
The new parameter takes a list of devices separated by a semicolon. Each
device specified will have its ACS redirect bits disabled. This is
similar to the existing 'resource_alignment' parameter.
The ACS Request P2P Request Redirect, P2P Completion Redirect and P2P
Egress Control bits are disabled, which is sufficient to always allow
passing P2P traffic uninterrupted. The bits are set after the kernel
(optionally) enables the ACS bits itself. It is also done regardless of
whether the kernel or platform firmware sets the bits.
If the user tries to disable the ACS redirect for a device without the ACS
capability, print a warning to dmesg.
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
[bhelgaas: reorder to add the generic code first and move the
device-specific quirk to subsequent patches] Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Stephen Bates <sbates@raithlin.com> Reviewed-by: Alex Williamson <alex.williamson@redhat.com> Acked-by: Christian König <christian.koenig@amd.com>
PCI: Allow specifying devices using a base bus and path of devfns
When specifying PCI devices on the kernel command line using a
bus/device/function address, bus numbers can change when adding or
replacing a device, changing motherboard firmware, or applying kernel
parameters like "pci=assign-buses". When bus numbers change, it's likely
the command line tweak will be applied to the wrong device.
Therefore, it is useful to be able to specify devices with a base bus
number and the path of devfns needed to get to it, similar to the "device
scope" structure in the Intel VT-d spec, Section 8.3.1.
Thus, we add an option to specify devices in the following format:
The path can be any segment within the PCI hierarchy of any length and
determined through the use of 'lspci -t'. When specified this way, it is
less likely that a renumbered bus will result in a valid device
specification and the tweak won't be applied to the wrong device.
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
[bhelgaas: use "device" instead of "slot" in documentation since that's the
usual language in the PCI specs] Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Stephen Bates <sbates@raithlin.com> Reviewed-by: Alex Williamson <alex.williamson@redhat.com> Acked-by: Christian König <christian.koenig@amd.com>
PCI: Make specifying PCI devices in kernel parameters reusable
Separate out the code to match a PCI device with a string (typically
originating from a kernel parameter) from the
pci_specified_resource_alignment() function into its own helper function.
While we are at it, this change fixes the kernel style of the function
(fixing a number of long lines and extra parentheses).
Additionally, make the analogous change to the kernel parameter
documentation: Separate the description of how to specify a PCI device
into its own section at the head of the "pci=" parameter.
This patch should have no functional alterations.
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
[bhelgaas: use "device" instead of "slot" in documentation since that's the
usual language in the PCI specs] Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Stephen Bates <sbates@raithlin.com> Reviewed-by: Alex Williamson <alex.williamson@redhat.com> Acked-by: Christian König <christian.koenig@amd.com>
igb: Remove unnecessary include of <linux/pci-aspm.h>
The igb driver doesn't need anything provided by pci-aspm.h, so remove
the unnecessary include of it.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Sinan Kaya <okaya@kernel.org> Acked-by: Alexander Duyck <alexander.h.duyck@intel.com> Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Shunyong Yang [Wed, 18 Jul 2018 01:40:35 +0000 (09:40 +0800)]
PCI: Unify PCI and normal DMA direction definitions
Current DMA direction definitions in pci-dma-compat.h and dma-direction.h
are mirrored in value. Unify them to enhance readability and avoid
possible inconsistency.
PCI_EXP_AER_FLAGS was defined twice (with identical definitions), once
under #ifdef CONFIG_ACPI_APEI, and again at the top level. This looks like
my merge error from these commits:
c384c46ac8b3 ("PCI/AER: Squash aerdrv_core.c into aerdrv.c") 01ba2fd871c7 ("PCI/AER: Squash ecrc.c into aerdrv.c")
Remove the duplicate PCI_EXP_AER_FLAGS definition.
PCI: pciehp: Deduplicate presence check on probe & resume
On driver probe and on resume from system sleep, pciehp checks the
Presence Detect State bit in the Slot Status register to bring up an
occupied slot or bring down an unoccupied slot. Both code paths are
identical, so deduplicate them per Mika's request.
On probe, an additional check is performed to disable power of an
unoccupied slot. This can e.g. happen if power was enabled by BIOS.
It cannot happen once pciehp has taken control, hence is not necessary
on resume: The Slot Control register is set to the same value that it
had on suspend by pci_restore_state(), so if the slot was occupied,
power is enabled and if it wasn't, power is disabled. Should occupancy
have changed during the system sleep transition, power is adjusted by
bringing up or down the slot per the paragraph above.
To allow for deduplication of the presence check, move the power check
to pcie_init(). This seems safer anyway, because right now it is
performed while interrupts are already enabled, and although I can't
think of a scenario where pciehp_power_off_slot() and the IRQ thread
collide, it does feel brittle.
However this means that pcie_init() may now write to the Slot Control
register before the IRQ is requested. If both the CCIE and HPIE bits
happen to be set, pcie_wait_cmd() will wait for an interrupt (instead
of polling the Command Completed bit) and eventually emit a timeout
message. Additionally, if a level-triggered INTx interrupt is used,
the user may see a spurious interrupt splat. Avoid by disabling
interrupts before disabling power. (Normally the HPIE and CCIE bits
should be clear on probe, but conceivably they may already have been
set e.g. by BIOS.)
Hari Vyas [Tue, 3 Jul 2018 09:05:41 +0000 (14:35 +0530)]
PCI: Fix is_added/is_busmaster race condition
When a PCI device is detected, pdev->is_added is set to 1 and proc and
sysfs entries are created.
When the device is removed, pdev->is_added is checked for one and then
device is detached with clearing of proc and sys entries and at end,
pdev->is_added is set to 0.
is_added and is_busmaster are bit fields in pci_dev structure sharing same
memory location.
A strange issue was observed with multiple removal and rescan of a PCIe
NVMe device using sysfs commands where is_added flag was observed as zero
instead of one while removing device and proc,sys entries are not cleared.
This causes issue in later device addition with warning message
"proc_dir_entry" already registered.
Debugging revealed a race condition between the PCI core setting the
is_added bit in pci_bus_add_device() and the NVMe driver reset work-queue
setting the is_busmaster bit in pci_set_master(). As these fields are not
handled atomically, that clears the is_added bit.
Move the is_added bit to a separate private flag variable and use atomic
functions to set and retrieve the device addition state. This avoids the
race because is_added no longer shares a memory location with is_busmaster.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=200283 Signed-off-by: Hari Vyas <hari.vyas@broadcom.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Lukas Wunner <lukas@wunner.de> Acked-by: Michael Ellerman <mpe@ellerman.id.au>
Thunderbolt controllers can be runtime suspended to D3cold to save ~1.5W.
This requires that runtime D3 is allowed on its PCIe ports, so whitelist
them.
The 2015 BIOS cutoff that we've instituted for runtime D3 on PCIe ports
is unnecessary on Thunderbolt because we know that even the oldest
controller, Light Ridge (2010), is able to suspend its ports to D3 just
fine -- specifically including its hotplug ports. And the power saving
should be afforded to machines even if their BIOS predates 2015.
Signed-off-by: Lukas Wunner <lukas@wunner.de> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Cc: Andreas Noever <andreas.noever@gmail.com>
PCI: Whitelist native hotplug ports for runtime D3
Previously we blacklisted PCIe hotplug ports for runtime D3 because:
(a) Ports handled by the firmware must not be transitioned to D3 by the
OS behind the firmware's back:
https://bugzilla.kernel.org/show_bug.cgi?id=53811
(b) Ports handled natively by the OS lacked runtime D3 support in the
pciehp driver.
We've just rectified the latter, so allow users to manually enable and
test it by passing pcie_port_pm=force on the command line. Vendors are
thus put in a position to validate hotplug ports for runtime D3 and
perhaps we can someday enable it by default, but with a BIOS cutoff date.
Ashok Raj tested runtime D3 on hotplug ports of a SkyLake Xeon-SP in
2017 and encountered Hardware Error NMIs, so this feature clearly cannot
be enabled for everyone yet:
https://lkml.kernel.org/r/20170503180426.GA4058@otc-nc-03
While at it, remove an erroneous code comment I added with 4f45ce2b139e
("PCI: Consolidate conditions to allow runtime PM on PCIe ports") which
claims that parents of a hotplug port must stay awake lest interrupts
cannot be delivered. That has turned out to be wrong at least for
Thunderbolt hotplug ports.
Signed-off-by: Lukas Wunner <lukas@wunner.de> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> Cc: Ashok Raj <ashok.raj@intel.com> Cc: Keith Busch <keith.busch@intel.com> Cc: Yinghai Lu <yinghai@kernel.org>
When performing a function reset via sysfs, the device's config space is
accessed in places such as pcie_flr() and its MMIO space is accessed e.g.
in reset_ivb_igd(), so ensure accessibility by resuming the device to D0.
Signed-off-by: Lukas Wunner <lukas@wunner.de> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> Cc: Ashok Raj <ashok.raj@intel.com> Cc: Keith Busch <keith.busch@intel.com> Cc: Yinghai Lu <yinghai@kernel.org>
pciehp's IRQ thread ensures accessibility of the port by runtime resuming
its parent to D0. However when the slot is enabled/disabled, the port
itself needs to be in D0 because its secondary bus is accessed in:
pciehp_check_link_status(),
pciehp_configure_device() (both called from board_added())
and
pciehp_unconfigure_device() (called from remove_board()).
Thus, acquire a runtime PM ref on enable/disablement of the slot.
Yinghai Lu additionally discovered that some SkyLake servers feature a
Power Controller for their PCIe hotplug ports (PCIe r3.1, sec 6.7.1.8)
which requires the port to be in D0 when invoking
pciehp_power_on_slot() (likewise called from board_added()).
If slot power is turned on while in D3hot, link training later fails:
https://lkml.kernel.org/r/20170205073454.GA253@wunner.de
The spec is silent about such a requirement, but it seems prudent to
assume that any hotplug port with a Power Controller may need this.
The present commit holds a runtime PM ref whenever slot power is turned
on and off, but it doesn't keep the port in D0 as long as slot power is
on. If vendors determine that's necessary, they need to amend pciehp to
acquire a runtime PM ref in pciehp_power_on_slot() and release one in
pciehp_power_off_slot().
Signed-off-by: Lukas Wunner <lukas@wunner.de> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> Cc: Ashok Raj <ashok.raj@intel.com> Cc: Keith Busch <keith.busch@intel.com> Cc: Yinghai Lu <yinghai@kernel.org>
If a hotplug port is able to send an interrupt, one would naively assume
that it is accessible at that moment. After all, if it wouldn't be
accessible, i.e. if its parent is in D3hot and the link to the hotplug
port is thus down, how should an interrupt come through?
It turns out that assumption is wrong at least for Thunderbolt: Even
though its parents are in D3hot, a Thunderbolt hotplug port is able to
signal interrupts. Because the port's config space is inaccessible and
resuming the parents may sleep, the hard IRQ handler has to defer
runtime resuming the parents and reading the Slot Status register to the
IRQ thread.
If the hotplug port uses a level-triggered INTx interrupt, it needs to
be masked until the IRQ thread has cleared the signaled events. For
simplicity, this commit also masks edge-triggered MSI/MSI-X interrupts.
Note that if the interrupt is shared (which can only happen for INTx),
other devices are starved from receiving interrupts until the IRQ thread
is scheduled, has runtime resumed the hotplug port's parents and has
read and cleared the Slot Status register.
That delay is dominated by the 10 ms D3hot->D0 transition time of each
parent port. The worst case is a Thunderbolt downstream port at the
end of a daisy chain: There may be up to six Thunderbolt controllers
in-between it and the root port, each comprising an upstream and
downstream port, plus its own upstream port. That's 13 x 10 = 130 ms.
Possible mitigations are polling the interrupt while it's disabled or
reducing the d3_delay of Thunderbolt ports if possible.
Open code masking of the interrupt instead of requesting it with the
IRQF_ONESHOT flag to minimize the period during which it is masked.
(IRQF_ONESHOT unmasks the IRQ only after the IRQ thread has finished.)
PCIe r4.0 sec 6.7.3.4 states that "If wake generation is required by the
associated form factor specification, a hotplug capable Downstream Port
must support generation of a wakeup event (using the PME mechanism) on
hotplug events that occur when the system is in a sleep state or the
Port is in device state D1, D2, or D3Hot."
This would seem to imply that PME needs to be enabled on the hotplug
port when it is runtime suspended. pci_enable_wake() currently doesn't
enable PME on bridges, it may be necessary to add an exemption for
hotplug bridges there. On "Light Ridge" Thunderbolt controllers, the
PME_Status bit is not set when an interrupt occurs while the hotplug
port is in D3hot, even if PME is enabled. (I've tested this on a Mac
and we hardcode the OSC_PCI_EXPRESS_PME_CONTROL bit to 0 on Macs in
negotiate_os_control(), modifying it to 1 didn't change the behavior.)
(Side note: Section 6.7.3.4 also states that "PME and Hot-Plug Event
interrupts (when both are implemented) always share the same MSI or
MSI-X vector". That would only seem to apply to Root Ports, however
the section never mentions Root Ports, only Downstream Ports. This is
explained in the definition of "Downstream Port" in the "Terms and
Acronyms" section of the PCIe Base Spec: "The Ports on a Switch that
are not the Upstream Port are Downstream Ports. All Ports on a Root
Complex are Downstream Ports.")
Signed-off-by: Lukas Wunner <lukas@wunner.de> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> Cc: Ashok Raj <ashok.raj@intel.com> Cc: Keith Busch <keith.busch@intel.com> Cc: Yinghai Lu <yinghai@kernel.org>
PCIe r4.0, sec 6.7.3.2 says that after "issuing a write transaction that
targets any portion of the Port's Slot Control register, [...] software
must wait for [the] command to complete before issuing the next command".
pciehp currently fails to enforce that rule after the above-mentioned
write. Fix it.
(Moving restoration of the Slot Control register to pciehp doesn't seem
to make sense because the other PCIe hotplug drivers may need it as
well.)
PCI: pciehp: Clear spurious events earlier on resume
Thunderbolt hotplug ports that were occupied before system sleep resume
with their downstream link in "off" state. Only after the Thunderbolt
controller has reestablished the PCIe tunnels does the link go up.
As a result, a spurious Presence Detect Changed and/or Data Link Layer
State Changed event occurs.
The events are not immediately acted upon because tunnel reestablishment
happens in the ->resume_noirq phase, when interrupts are still disabled.
Also, notification of events may initially be disabled in the Slot
Control register when coming out of system sleep and is reenabled in the
->resume_noirq phase through:
It is not guaranteed that the events are acted upon at all: PCIe r4.0,
sec 6.7.3.4 says that "a port may optionally send an MSI when there are
hot-plug events that occur while interrupt generation is disabled, and
interrupt generation is subsequently enabled." Note the "optionally".
If an MSI is sent, pciehp will gratuitously turn the slot off and back
on once the ->resume_early phase has commenced.
If an MSI is not sent, the extant, unacknowledged events in the Slot
Status register will prevent future notification of presence or link
changes.
Commit 696e50351237 ("PCI: pciehp: Clear Presence Detect and Data Link
Layer Status Changed on resume") fixed the latter by clearing the events
in the ->resume phase. Move this to the ->resume_noirq phase to also
fix the gratuitous disable/enablement of the slot.
The commit further restored the Slot Control register in the ->resume
phase, but that's dispensable because as shown above it's already been
done in the ->resume_noirq phase.
Replace suspend_iter() and resume_iter() with a single function pm_iter()
to allow addition of port service callbacks for further power management
phases without having to add another iterator each time.
5971141c786f ("PCI: pciehp: Add reset_slot() method") and 563f2293f470 ("PCI: pciehp: Disable link notification across slot reset")
disables notification of Presence Detect Changed and Data Link Layer
State Changed events for the duration of a secondary bus reset.
However a bus reset not only triggers these events, but may also clear
the Presence Detect State bit in the Slot Status register and the Data
Link Layer Link Active bit in the Link Status register momentarily.
According to Sinan Kaya:
"I know for a fact that bus reset clears the Data Link Layer Active bit
as soon as link goes down. It gets set again following link up.
Presence detect depends on the HW implementation. QDT root ports
don't change presence detect for instance since nobody actually
removed the card. If an implementation supports in-band presence
detect, the answer is yes. As soon as the link goes down, presence
detect bit will get cleared until recovery."
https://lkml.kernel.org/r/42e72f83-3b24-f7ef-e5bc-290fae99259a@codeaurora.org
In-band presence detect is also covered in Table 4-15 in PCIe r4.0,
sec 4.2.6.
pciehp should therefore ensure that any parts of the driver that access
those bits do not run concurrently to a bus reset. The only precaution
the commits took to that effect was to halt interrupt polling. They
made no effort to drain the slot workqueue, cancel an outstanding
Attention Button work, or block slot enable/disable requests via sysfs
and in the ->probe hook.
Now that pciehp is converted to enable/disable the slot exclusively from
the IRQ thread, the only places accessing the two above-mentioned bits
are the IRQ thread and the ->probe hook. Add locking to serialize them
with a bus reset. This obviates the need to halt interrupt polling.
Do not add locking to the ->get_adapter_status sysfs callback to afford
users unfettered access to that bit. Use an rw_semaphore in lieu of a
regular mutex to allow parallel execution of the non-reset code paths
accessing the critical bits, i.e. the IRQ thread and the ->probe hook.
Signed-off-by: Lukas Wunner <lukas@wunner.de> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Cc: Rajat Jain <rajatja@google.com> Cc: Alex Williamson <alex.williamson@redhat.com> Cc: Sinan Kaya <okaya@kernel.org>
PCI: Use IRQF_ONESHOT if pci_request_irq() called with no handler
If we have a threaded interrupt with the handler being NULL, then
request_threaded_irq() -> __setup_irq() will complain and bail out if the
IRQF_ONESHOT flag isn't set. Therefore check for the handler being NULL
and set IRQF_ONESHOT in this case.
This change is needed to migrate the mei_me driver to
pci_alloc_irq_vectors() and pci_request_irq().
Bjorn Helgaas [Tue, 26 Jun 2018 22:45:34 +0000 (17:45 -0500)]
PCI: Document ACPI description of PCI host bridges
Add a writeup about how PCI host bridges should be described in ACPI
using PNP0A03/PNP0A08 devices, PNP0C02 devices, and the MCFG table.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Sinan Kaya <okaya@kernel.org> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Thomas Tai [Thu, 26 Jul 2018 17:13:04 +0000 (12:13 -0500)]
PCI/AER: Work around use-after-free in pcie_do_fatal_recovery()
When an fatal error is received by a non-bridge device, the device is
removed, and pci_stop_and_remove_bus_device() deallocates the device
structure. The freed device structure is used by subsequent code to send
uevents and print messages.
Hold a reference on the device until we're finished using it. This is not
an ideal fix because pcie_do_fatal_recovery() should not use the device at
all after removing it, but that's too big a project for right now.
Fixes: bfcbf1909dc0 ("PCI/AER: Handle ERR_FATAL with removal and re-enumeration of devices") Signed-off-by: Thomas Tai <thomas.tai@oracle.com>
[bhelgaas: changelog, reduce get/put coverage] Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Per PCIe r4.0, sec 6.7.3.4, a "port may optionally send an MSI when
there are hot-plug events that occur while interrupt generation is
disabled, and interrupt generation is subsequently enabled."
On probe, we currently clear all event bits in the Slot Status register
with the notable exception of the Presence Detect Changed bit. Thereby
we seek to receive an interrupt for an already occupied slot once event
notification is enabled.
But because the interrupt is optional, users may have to specify the
pciehp_force parameter on the command line, which is inconvenient.
Moreover, now that pciehp's event handling has become resilient to
missed events, a Presence Detect Changed interrupt for a slot which is
powered on is interpreted as removal of the card. If the slot has
already been brought up by the BIOS, receiving such an interrupt on
probe causes the slot to be powered off and immediately back on, which
is likewise undesirable.
Avoid both issues by making the behavior of pciehp_force the default and
clearing the Presence Detect Changed bit on probe.
Note that the stated purpose of pciehp_force per the MODULE_PARM_DESC
("Force pciehp, even if OSHP is missing") seems nonsensical because the
OSHP control method is only relevant for SHCP slots according to the
PCI Firmware specification r3.0, sec 4.8.
Signed-off-by: Lukas Wunner <lukas@wunner.de> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
A hotplug port's Slot Status register does not count how often each type
of event occurred, it only records the fact *that* an event has occurred.
Previously pciehp queued a work item for each event. But if it missed
an event, e.g. removal of a card in-between two back-to-back insertions,
it queued up the wrong work item or no work item at all. Commit ef863f8256e1 ("PCI: pciehp: Process all hotplug events before looking
for new ones") sought to improve the situation by shrinking the window
during which events may be missed.
But Stefan Roese reports unbalanced Card present and Link Up events,
suggesting that we're still missing events if they occur very rapidly.
Bjorn Helgaas responds that he considers pciehp's event handling
"baroque" and calls for its simplification and rationalization:
https://lkml.kernel.org/r/20180202192045.GA53759@bhelgaas-glaptop.roam.corp.google.com
It gets worse once a hotplug port is runtime suspended: The port can
signal an interrupt while it and its parents are in D3hot, i.e. while
it is inaccessible. By the time we've runtime resumed all parents to D0
and read the port's Slot Status register, we may have missed an arbitrary
number of events. Event handling therefore needs to be reworked to
become resilient to missed events.
Assume that a Presence Detect Changed event has occurred.
Consider the following truth table:
- Slot is in OFF_STATE and is currently empty. => Do nothing.
(The event is trailing a Link Down or we've
missed an insertion and subsequent removal.)
- Slot is in OFF_STATE and is currently occupied. => Turn the slot on.
- Slot is in ON_STATE and is currently empty. => Turn the slot off.
- Slot is in ON_STATE and is currently occupied. => Turn the slot off,
(Be cautious and assume the card in then back on.
the slot isn't the same as before.)
This leads to the following simple algorithm:
1 If the slot is in ON_STATE, turn it off unconditionally.
2 If the slot is currently occupied, turn it on.
Because those actions are now carried out synchronously, rather than by
scheduled work items, pciehp reacts to the *current* situation and
missed events no longer matter.
Data Link Layer State Changed events can be handled identically to
Presence Detect Changed events. Note that in the above truth table,
a Link Up trailing a Card present event didn't have to be accounted for:
It is filtered out by pciehp_check_link_status().
As for Attention Button Pressed events, PCIe r4.0, sec 6.7.1.5 says:
"Once the Power Indicator begins blinking, a 5-second abort interval
exists during which a second depression of the Attention Button cancels
the operation." In other words, the user can only expect the system to
react to a button press after it starts blinking. Missed button presses
that occur in-between are irrelevant.
When a device is hotplugged, Presence Detect and Link Up events often do
not occur simultaneously, but with a lag of a few milliseconds. Only
the first event received is relevant, the other one can be disregarded.
Moreover, Stefan Roese reports that on certain platforms, Link State and
Presence Detect may flap for up to 100 ms before stabilizing, suggesting
that such events should be disregarded for at least this long:
https://lkml.kernel.org/r/20180130084121.18653-1-sr@denx.de
On slot enablement, pciehp_check_link_status() waits for 100 ms per
PCIe r4.0, sec 6.7.3.3, then probes the hotplugged device's vendor
register for up to 1 second.
If this succeeds, the link is definitely up, so ignore any Presence
Detect or Link State events that occurred up to this point.
pciehp_check_link_status() then checks the Link Training bit in the
Link Status register. This is the final opportunity to detect
inaccessibility of the device and abort slot enablement. Any link
or presence change that occurs afterwards will cause the slot to be
disabled again immediately after attempting to enable it.
The astute reviewer may appreciate that achieving this behavior would be
more complicated had pciehp not just been converted to enable/disable
the slot exclusively from the IRQ thread: When the slot is enabled via
sysfs, each link or presence flap would otherwise cause the IRQ thread
to run and it would have to sense that those events are belonging to a
concurrent slot enablement operation and disregard them. It would be
much more difficult than this mere 3 line change.
No callers of pciehp_enable/disable_slot() outside of pciehp_ctrl.c
remain, so declare the functions static. For now this requires forward
declarations. Those can be eliminated by reshuffling functions once the
ongoing effort to refactor the driver has settled.
Previously slot enablement and disablement could happen concurrently.
But now it's under the exclusive control of the IRQ thread, rendering
the locking obsolete. Drop it.
PCI: pciehp: Enable/disable exclusively from IRQ thread
Besides the IRQ thread, there are several other places in the driver
which enable or disable the slot:
- pciehp_probe() enables the slot if it's occupied and the pciehp_force
module parameter is used.
- pciehp_resume() enables or disables the slot after system sleep.
- pciehp_queue_pushbutton_work() enables or disables the slot after the
5 second delay following an Attention Button press.
- pciehp_sysfs_enable_slot() and pciehp_sysfs_disable_slot() enable or
disable the slot on sysfs write.
This requires locking and complicates pciehp's state machine.
A simplification can be achieved by enabling and disabling the slot
exclusively from the IRQ thread.
Amend the functions listed above to request slot enable/disablement from
the IRQ thread by either synthesizing a Presence Detect Changed event or,
in the case of a disable user request (via sysfs or an Attention Button
press), submitting a newly introduced force disable request. The latter
is needed because the slot shall be forced off despite being occupied.
For this force disable request, avoid colliding with Slot Status register
bits by using a bit number greater than 16.
For synchronous execution of requests (on sysfs write), wait for the
request to finish and retrieve the result. There can only ever be one
sysfs write in flight due to the locking in kernfs_fop_write(), hence
there is no risk of returning the result of a different sysfs request to
user space.
The POWERON_STATE and POWEROFF_STATE is now no longer entered by the
above-listed functions, but solely by the IRQ thread when it begins a
power transition. Afterwards, it moves to STATIC_STATE. The same
applies to canceling the Attention Button work, it likewise becomes an
IRQ thread only operation.
An immediate consequence is that the POWERON_STATE and POWEROFF_STATE is
never observed by the IRQ thread itself, only by functions called in a
different context, such as pciehp_sysfs_enable_slot(). So remove
handling of these states from pciehp_handle_button_press() and
pciehp_handle_link_change() which are exclusively called from the IRQ
thread.
handle_button_press_event() currently determines whether the slot has
been turned on or off by looking at the Power Controller Control bit in
the Slot Control register. This assumes that an attention button
implies presence of a power controller even though that's not mandated
by the spec. Moreover the Power Controller Control bit is unreliable
when a power fault occurs (PCIe r4.0, sec 6.7.1.8). This issue has
existed since the driver was introduced in 2004.
Fix by replacing STATIC_STATE with ON_STATE and OFF_STATE and tracking
whether the slot has been turned on or off. This is also a required
ingredient to make pciehp resilient to missed events, which is the
object of an upcoming commit.
The PCI hotplug core has just been refactored to separate slot
initialization for in-kernel use from publication to user space.
Take advantage of it in pciehp by publishing to user space last on
probe. This will allow enable/disablement of the slot exclusively from
the IRQ thread because the IRQ is requested after initialization for
in-kernel use (thereby getting its unique name needed by the IRQ thread)
but before user space is able to submit enable/disable requests.
On teardown, the order is the same in reverse: The user space interface
is removed prior to freeing the IRQ and destroying the slot.
PCI: hotplug: Demidlayer registration with the core
When a hotplug driver calls pci_hp_register(), all steps necessary for
registration are carried out in one go, including creation of a kobject
and addition to sysfs. That's a problem for pciehp once it's converted
to enable/disable the slot exclusively from the IRQ thread: The thread
needs to be spawned after creation of the kobject (because it uses the
kobject's name), but before addition to sysfs (because it will handle
enable/disable requests submitted via sysfs).
pci_hp_deregister() does offer a ->release callback that's invoked
after deletion from sysfs and before destruction of the kobject. But
because pci_hp_register() doesn't offer a counterpart, hotplug drivers'
->probe and ->remove code becomes asymmetric, which is error prone
as recently discovered use-after-free bugs in pciehp's ->remove hook
have shown.
In a sense, this appears to be a case of the midlayer antipattern:
"The core thesis of the "midlayer mistake" is that midlayers are
bad and should not exist. That common functionality which it is
so tempting to put in a midlayer should instead be provided as
library routines which can [be] used, augmented, or ignored by
each bottom level driver independently. Thus every subsystem
that supports multiple implementations (or drivers) should
provide a very thin top layer which calls directly into the
bottom layer drivers, and a rich library of support code that
eases the implementation of those drivers. This library is
available to, but not forced upon, those drivers."
-- Neil Brown (2009), https://lwn.net/Articles/336262/
The presence of midlayer traits in the PCI hotplug core might be ascribed
to its age: When it was introduced in February 2002, the blessings of a
library approach might not have been well known:
https://git.kernel.org/tglx/history/c/a8a2069f432c
For comparison, the driver core does offer split functions for creating
a kobject (device_initialize()) and addition to sysfs (device_add()) as
an alternative to carrying out everything at once (device_register()).
This was introduced in October 2002:
https://git.kernel.org/tglx/history/c/8b290eb19962
The odd ->release callback in the PCI hotplug core was added in 2003:
https://git.kernel.org/tglx/history/c/69f8d663b595
Clearly, a library approach would not force every hotplug driver to
implement a ->release callback, but rather allow the driver to remove
the sysfs files, release its data structures and finally destroy the
kobject. Alternatively, a driver may choose to remove everything with
pci_hp_deregister(), then release its data structures.
To this end, offer drivers pci_hp_initialize() and pci_hp_add() as a
split-up version of pci_hp_register(). Likewise, offer pci_hp_del()
and pci_hp_destroy() as a split-up version of pci_hp_deregister().
Eliminate the ->release callback and move its code into each driver's
teardown routine.
Declare pci_hp_deregister() void, in keeping with the usual kernel
pattern that enablement can fail, but disablement cannot. It only
returned an error if the caller passed in a NULL pointer or a slot which
has never or is no longer registered or is sharing its name with another
slot. Those would be bugs, so WARN about them. Few hotplug drivers
actually checked the return value and those that did only printed a
useless error message to dmesg. Remove that.
For most drivers the conversion was straightforward since it doesn't
matter whether the code in the ->release callback is executed before or
after destruction of the kobject. But in the case of ibmphp, it was
unclear to me whether setting slot_cur->ctrl and slot_cur->bus_on to
NULL needs to happen before the kobject is destroyed, so I erred on
the side of caution and ensured that the order stays the same. Another
nontrivial case is pnv_php, I've found the list and kref logic difficult
to understand, however my impression was that it is safe to delete the
list element and drop the references until after the kobject is
destroyed.
Signed-off-by: Lukas Wunner <lukas@wunner.de> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> # drivers/platform/x86 Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Len Brown <lenb@kernel.org> Cc: Scott Murray <scott@spiteful.org> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Cc: Paul Mackerras <paulus@samba.org> Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Gavin Shan <gwshan@linux.vnet.ibm.com> Cc: Sebastian Ott <sebott@linux.vnet.ibm.com> Cc: Gerald Schaefer <gerald.schaefer@de.ibm.com> Cc: Corentin Chary <corentin.chary@gmail.com> Cc: Darren Hart <dvhart@infradead.org> Cc: Andy Shevchenko <andy@infradead.org>
Previously the slot workqueue was used to handle events and enable or
disable the slot. That's no longer the case as those tasks are done
synchronously in the IRQ thread. The slot workqueue is thus merely used
to handle a button press after the 5 second delay and only one such work
item may be in flight at any given time. A separate workqueue isn't
necessary for this simple task, so use the system workqueue instead.
Up until now, pciehp's IRQ handler schedules a work item for each event,
which in turn schedules a work item to enable or disable the slot. This
double indirection was necessary because sleeping wasn't allowed in the
IRQ handler.
However it is now that pciehp has been converted to threaded IRQ handling
and polling, so handle events synchronously in pciehp_ist() and remove
the work item infrastructure (with the exception of work items to handle
a button press after the 5 second delay).
For link or presence change events, move the register read to determine
the current link or presence state behind acquisition of the slot lock
to prevent it from becoming stale while the lock is contended.
If the attention button is pressed to power on the slot AND the user
powers on the slot via sysfs before 5 seconds have elapsed AND powering
on the slot fails because either the slot is unoccupied OR the latch is
open, we neglect turning off the green LED so it keeps on blinking.
That's because the error path of pciehp_sysfs_enable_slot() doesn't call
pciehp_green_led_off(), unlike pciehp_power_thread() which does.
The bug has been present since 2004 when the driver was introduced.
Fix by deduplicating common code in pciehp_sysfs_enable_slot() and
pciehp_power_thread() into a wrapper function pciehp_enable_slot() and
renaming the existing function to __pciehp_enable_slot(). Same for
pciehp_disable_slot(). This will also simplify the upcoming rework of
pciehp's event handling.
We've just converted pciehp to threaded IRQ handling, but still cannot
sleep in pciehp_ist() because the function is also called in poll mode,
which runs in softirq context (from a timer).
Convert poll mode to a kthread so that pciehp_ist() always runs in task
context.
pciehp's IRQ handler queues up a work item for each event signaled by
the hardware. A more modern alternative is to let a long running
kthread service the events. The IRQ handler's sole job is then to check
whether the IRQ originated from the device in question, acknowledge its
receipt to the hardware to quiesce the interrupt and wake up the kthread.
One benefit is reduced latency to handle the IRQ, which is a necessity
for realtime environments. Another benefit is that we can make pciehp
simpler and more robust by handling events synchronously in process
context, rather than asynchronously by queueing up work items. pciehp's
usage of work items is a historic artifact, it predates the introduction
of threaded IRQ handlers by two years. (The former was introduced in
2007 with commit 20c5dc5daca6 ("pciehp: Event handling rework"), the
latter in 2009 with commit 68a607cfe977 ("genirq: add threaded interrupt
handler support").)
Convert pciehp to threaded IRQ handling by retrieving the pending events
in pciehp_isr(), saving them for later consumption by the thread handler
pciehp_ist() and clearing them in the Slot Status register.
By clearing the Slot Status (and thereby acknowledging the events) in
pciehp_isr(), we can avoid requesting the IRQ with IRQF_ONESHOT, which
would have the unpleasant side effect of starving devices sharing the
IRQ until pciehp_ist() has finished.
pciehp_isr() does not count how many times each event occurred, but
merely records the fact *that* an event occurred. If the same event
occurs a second time before pciehp_ist() is woken, that second event
will not be recorded separately, which is problematic according to
commit ef863f8256e1 ("PCI: pciehp: Process all hotplug events before
looking for new ones") because we may miss removal of a card in-between
two back-to-back insertions. We're about to make pciehp_ist() resilient
to missed events. The present commit regresses the driver's behavior
temporarily in order to separate the changes into reviewable chunks.
This doesn't affect regular slow-motion hotplug, only plug-unplug-plug
operations that happen in a timespan shorter than wakeup of the IRQ
thread.
Since commit 6f888193d976 ("PCI: hotplug: Drop checking of PCI_BRIDGE_
CONTROL in *_unconfigure_device()"), pciehp_unconfigure_device() can no
longer fail, so declare it and its sole caller remove_board() void, in
keeping with the usual kernel pattern that enablement can fail, but
disablement cannot. No functional change intended.
pciehp_disable_slot() checks if the ctrl attribute of the slot is NULL
and bails out if so. However the function is not called prior to the
attribute being set in pcie_init_slot(), and pcie_init_slot() is not
called if ctrl is NULL. So the check is unnecessary. Drop it.
It has been present ever since the driver was introduced in 2004, but it
was already unnecessary back then:
https://git.kernel.org/tglx/history/c/c16b4b14d980
PCI: pciehp: Fix unprotected list iteration in IRQ handler
Commit 0d42a440e1b4 ("PCI: Add pci_ignore_hotplug() to ignore hotplug
events for a device") iterates over the devices on a hotplug port's
subordinate bus in pciehp's IRQ handler without acquiring pci_bus_sem.
It is thus possible for a user to cause a crash by concurrently
manipulating the device list, e.g. by disabling slot power via sysfs
on a different CPU or by initiating a remove/rescan via sysfs.
This can't be fixed by acquiring pci_bus_sem because it may sleep.
The simplest fix is to avoid the list iteration altogether and just
check the ignore_hotplug flag on the port itself. This works because
pci_ignore_hotplug() sets the flag both on the device as well as on its
parent bridge.
We do lose the ability to print the name of the device blocking hotplug
in the debug message, but that's probably bearable.
Fixes: 0d42a440e1b4 ("PCI: Add pci_ignore_hotplug() to ignore hotplug events for a device") Signed-off-by: Lukas Wunner <lukas@wunner.de> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Cc: stable@vger.kernel.org
When pciehp is unbound (e.g. on unplug of a Thunderbolt device), the
hotplug_slot struct is deregistered and thus freed before freeing the
IRQ. The IRQ handler and the work items it schedules print the slot
name referenced from the freed structure in various informational and
debug log messages, each time resulting in a quadruple dereference of
freed pointers (hotplug_slot -> pci_slot -> kobject -> name).
At best the slot name is logged as "(null)", at worst kernel memory is
exposed in logs or the driver crashes:
pciehp 0000:10:00.0:pcie204: Slot((null)): Card not present
An attacker may provoke the bug by unplugging multiple devices on a
Thunderbolt daisy chain at once. Unplugging can also be simulated by
powering down slots via sysfs. The bug is particularly easy to trigger
in poll mode.
It has been present since the driver's introduction in 2004:
https://git.kernel.org/tglx/history/c/c16b4b14d980
Fix by rearranging teardown such that the IRQ is freed first. Run the
work items queued by the IRQ handler to completion before freeing the
hotplug_slot struct by draining the work queue from the ->release_slot
callback which is invoked by pci_hp_deregister().
PCI: hotplug: Don't leak pci_slot on registration failure
If addition of sysfs files fails on registration of a hotplug slot, the
struct pci_slot as well as the entry in the slot_list is leaked. The
issue has been present since the hotplug core was introduced in 2002:
https://git.kernel.org/tglx/history/c/a8a2069f432c
Perhaps the idea was that even though sysfs addition fails, the slot
should still be usable. But that's not how drivers use the interface,
they abort probe if a non-zero value is returned.
Lukas Wunner [Tue, 19 Jun 2018 14:26:50 +0000 (16:26 +0200)]
PCI: hotplug: Delete skeleton driver
Ten years ago, commit 9b35690095bb ("PCI: Hotplug core: remove 'name'")
dropped the name element from struct hotplug_slot but neglected to update
the skeleton driver.
That same year, commit b58916f81c31 ("PCI: introduce pci_slot") raised the
number of arguments to pci_hp_register() from one to four.
Fourteen years ago, historic commit 7ab60fc1b8e7 ("PCI Hotplug skeleton:
final cleanups") removed all usages of the retval variable from
pcihp_skel_init() but not the variable itself, provoking a compiler
warning: https://git.kernel.org/tglx/history/c/7ab60fc1b8e7
It seems fair to assume the driver hasn't been used as a template for a new
driver in a while. Per Bjorn's and Christoph's preference, delete it.
The pci_error_handlers.slot_reset() callback is only used for non-bridge
devices (see broadcast_error_message()). Since portdrv only binds to
bridges, we don't need pcie_portdrv_slot_reset(), so remove it.
PCI/AER: Factor out ERR_NONFATAL status bit clearing
aer_error_resume() clears all ERR_NONFATAL error status bits. This is
exactly what pci_cleanup_aer_uncorrect_error_status(), so use that instead
of duplicating the code.
Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
[bhelgaas: split to separate patch] Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
PCI/AER: Clear only ERR_NONFATAL bits during non-fatal recovery
pci_cleanup_aer_uncorrect_error_status() is called by driver .slot_reset()
methods when handling ERR_NONFATAL errors. Previously this cleared *all*
the bits, including ERR_FATAL bits.
Since we're only handling ERR_NONFATAL errors, clear only the ERR_NONFATAL
error status bits.
Signed-off-by: Oza Pawandeep <poza@codeaurora.org>
[bhelgaas: split to separate patch] Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
PCI/AER: Clear only ERR_FATAL status bits during fatal recovery
During recovery from fatal errors, we previously called
pci_cleanup_aer_uncorrect_error_status(), which cleared *all* uncorrectable
error status bits (both ERR_FATAL and ERR_NONFATAL).
Instead, call a new pci_aer_clear_fatal_status() that clears only the
ERR_FATAL bits (as indicated by the PCI_ERR_UNCOR_SEVER register).
Keith Busch [Mon, 16 Jul 2018 22:05:05 +0000 (17:05 -0500)]
PCI/DPC: Print AER status in DPC event handling
A DPC enabled device suppresses ERR_(NON)FATAL messages, preventing the AER
handler from reporting error details. If the DPC trigger reason says the
downstream port detected the error, collect the AER uncorrectable status
for logging, then clear the status.
Signed-off-by: Keith Busch <keith.busch@intel.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Sinan Kaya <okaya@kernel.org> Reviewed-by: Oza Pawandeep <poza@codeaurora.org>
Keith Busch [Wed, 20 Jun 2018 21:38:27 +0000 (15:38 -0600)]
PCI/DPC: Leave interrupts enabled while handling event
Now that the DPC driver clears the interrupt status before exiting the
IRQ handler, we don't need to abuse the DPC control register to know if
a shared interrupt is for a new DPC event: a DPC port can not trigger
a second interrupt until the host clears the trigger status later in the
work queue handler.
Signed-off-by: Keith Busch <keith.busch@intel.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Sinan Kaya <okaya@kernel.org> Reviewed-by: Oza Pawandeep <poza@codeaurora.org>
PCI/AER: Honor "pcie_ports=native" even if HEST sets FIRMWARE_FIRST
According to the documentation, "pcie_ports=native", linux should use
native AER and DPC services. While that is true for the _OSC method
parsing, this is not the only place that is checked. Should the HEST
list PCIe ports as firmware-first, linux will not use native services.
This happens because aer_acpi_firmware_first() doesn't take 'pcie_ports'
into account. This is wrong. DPC uses the same logic when it decides
whether to load or not, so fixing this also fixes DPC not loading.
Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
[bhelgaas: return "false" from bool function (from kbuild robot)] Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Rajat Jain [Sat, 30 Jun 2018 20:07:17 +0000 (15:07 -0500)]
PCI/AER: Define aer_stats structure for AER capable devices
Define a structure to hold the AER statistics. There are 2 groups of
statistics: dev_* counters that are to be collected for all AER capable
devices and rootport_* counters that are collected for all (AER capable)
rootports only. Allocate and free this structure when device is added or
released (thus counters survive the lifetime of the device).
Signed-off-by: Rajat Jain <rajatja@google.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Rajat Jain [Thu, 21 Jun 2018 23:48:26 +0000 (16:48 -0700)]
PCI/AER: Move internal declarations to drivers/pci/pci.h
Since pci_aer_init() and pci_no_aer() are used only internally, move their
declarations to the PCI internal header file. Also, no one cares about
return value of pci_aer_init(), so make it void.
Signed-off-by: Rajat Jain <rajatja@google.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Keith Busch [Thu, 19 Jul 2018 21:16:55 +0000 (16:16 -0500)]
PCI/AER: Expose internal API for obtaining AER information
Export some common AER functions and structures for other PCI core drivers
to use. Since this is making the function externally visible inside the
PCI core, prepend "aer_" to the function name.
Signed-off-by: Keith Busch <keith.busch@intel.com>
[bhelgaas: move AER declarations from linux/aer.h to drivers/pci/pci.h] Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Sinan Kaya <okaya@kernel.org> Reviewed-by: Oza Pawandeep <poza@codeaurora.org>
It turned out that pci_remap_iospace() wasn't undone when the driver's
probe failed, and since devm_phy_optional_get() returned -EPROBE_DEFER,
the probe was retried, finally causing the BUG due to trying to remap
already remapped pages.
The V3 Semiconductor PCI driver has the same issue.
Replace devm_pci_remap_iospace() with its devm_ managed version to fix
the bug.
It turned out that pci_remap_iospace() wasn't undone when the driver's
probe failed, and since devm_phy_optional_get() returned -EPROBE_DEFER,
the probe was retried, finally causing the BUG due to trying to remap
already remapped pages.
The MediaTek PCIe driver has the same issue.
Replace devm_pci_remap_iospace() with its devm_ managed counterpart
to fix the bug.
It turned out that pci_remap_iospace() wasn't undone when the driver's
probe failed, and since devm_phy_optional_get() returned -EPROBE_DEFER,
the probe was retried, finally causing the BUG due to trying to remap
already remapped pages.
The Faraday PCI driver has the same issue. Replace pci_remap_iospace()
with its devm_ managed version to fix the bug.
It turned out that pci_remap_iospace() wasn't undone when the driver's
probe failed, and since devm_phy_optional_get() returned -EPROBE_DEFER,
the probe was retried, finally causing the BUG due to trying to remap
already remapped pages.
The Aardvark PCI controller driver has the same issue.
Replace pci_remap_iospace() with its devm_ managed version to fix the bug.
Fixes: 08fd7d0c64af ("PCI: aardvark: Add Aardvark PCI host controller driver") Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
[lorenzo.pieralisi@arm.com: updated the commit log] Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
It turned out that pci_remap_iospace() wasn't undone when the driver's
probe failed, and since devm_phy_optional_get() returned -EPROBE_DEFER,
the probe was retried, finally causing the BUG due to trying to remap
already remapped pages.
The DesignWare PCIe controller driver has the same issue.
Replace devm_pci_remap_iospace() with a devm_ managed version to fix the
bug.
Fixes: 26c171bc9975 ("PCI: designware: Make driver arch-agnostic") Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
[lorenzo.pieralisi@arm.com: updated the commit log] Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Acked-by: Jingoo Han <jingoohan1@gmail.com>
It turned out that pci_remap_iospace() wasn't undone when the driver's
probe failed, and since devm_phy_optional_get() returned -EPROBE_DEFER,
the probe was retried, finally causing the BUG due to trying to remap
already remapped pages.
The Versatile PCI controller driver has the same issue.
Replace pci_remap_iospace() with the devm_ managed version to fix the bug.
Fixes: ed2ec5b1c240 ("PCI: versatile: Add DT-based ARM Versatile PB PCIe host driver") Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
[lorenzo.pieralisi@arm.com: updated the commit log] Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
It turned out that pci_remap_iospace() wasn't undone when the driver's
probe failed, and since devm_phy_optional_get() returned -EPROBE_DEFER,
the probe was retried, finally causing the BUG due to trying to remap
already remapped pages.
The X-Gene PCI controller driver has the same issue.
Replace pci_remap_iospace() with the devm_ managed version so that the
pages get unmapped automagically on any probe failure.
Fixes: a61a0209a848 ("PCI: xgene: Add APM X-Gene PCIe driver") Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
[lorenzo.pieralisi@arm.com: updated the commit log] Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
It turned out that pci_remap_iospace() wasn't undone when the driver's
probe failed, and since devm_phy_optional_get() returned -EPROBE_DEFER,
the probe was retried, finally causing the BUG due to trying to remap
already remapped pages.
Introduce the devm_pci_remap_iospace() managed API and replace the
pci_remap_iospace() call with it to fix the bug.
Fixes: 6bc42d832aa1 ("PCI: generic: Convert to DT resource parsing API") Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
[lorenzo.pieralisi@arm.com: split commit/updated the commit log] Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
projects / kernel.git / log