1. 21 Sep, 2018 3 commits
  2. 20 Sep, 2018 3 commits
  3. 18 Sep, 2018 8 commits
    • Lukas Wunner's avatar
      PCI: hotplug: Document TODOs · a0d58937
      Lukas Wunner authored
      While refactoring the PCI hotplug core's API, I noticed a significant
      amount of technical debt in some of the hotplug drivers.  Document the
      issues that caught my eye for starters.
      
      I do not have hardware at my disposal that utilizes the listed drivers
      and I think that's a prerequisite to work on them to ensure that no
      regressions sneak in.  But some of this hardware is so old that it may be
      hard to come by.  Obviously, it is fine to support old hardware, but the
      drivers need to be maintained.
      
      If noone steps up, perhaps we should consider sunsetting a few drivers
      by moving them to staging.  Based on my findings, ibmphp would be the
      first candidate.  I've found it fairly difficult to apply my API
      refactorings to it and have listed some obvious bugs in the driver.
      cpqphp is also in need of a modernization and would be a second
      candidate for relegation to staging.
      
      shpchp was introduced in the same commit as pciehp but hasn't benefited
      from the same amount of refactoring due to the decline of conventional
      PCI's relevance.  Yet hardware supporting it may be more prevalent than
      for the proprietary hotplug methods.
      
      Per Documentation/process/2.Process.rst, "a TODO file should be present"
      for drivers in staging.  The file introduced by the present commit may
      serve as a basis for this.
      Signed-off-by: default avatarLukas Wunner <lukas@wunner.de>
      Signed-off-by: default avatarBjorn Helgaas <bhelgaas@google.com>
      Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
      Cc: Scott Murray <scott@spiteful.org>
      Cc: Dan Zink <dan.zink@hpe.com>
      Cc: Prarit Bhargava <prarit@redhat.com>
      a0d58937
    • Lukas Wunner's avatar
      PCI: hotplug: Embed hotplug_slot · 125450f8
      Lukas Wunner authored
      When the PCI hotplug core and its first user, cpqphp, were introduced in
      February 2002 with historic commit a8a2069f, cpqphp allocated a slot
      struct for its internal use plus a hotplug_slot struct to be registered
      with the hotplug core and linked the two with pointers:
      https://git.kernel.org/tglx/history/c/a8a2069f432c
      
      Nowadays, the predominant pattern in the tree is to embed ("subclass")
      such structures in one another and cast to the containing struct with
      container_of().  But it wasn't until July 2002 that container_of() was
      introduced with historic commit ec4f2142:
      https://git.kernel.org/tglx/history/c/ec4f214232cf
      
      pnv_php, introduced in 2016, did the right thing and embedded struct
      hotplug_slot in its internal struct pnv_php_slot, but all other drivers
      cargo-culted cpqphp's design and linked separate structs with pointers.
      
      Embedding structs is preferrable to linking them with pointers because
      it requires fewer allocations, thereby reducing overhead and simplifying
      error paths.  Casting an embedded struct to the containing struct
      becomes a cheap subtraction rather than a dereference.  And having fewer
      pointers reduces the risk of them pointing nowhere either accidentally
      or due to an attack.
      
      Convert all drivers to embed struct hotplug_slot in their internal slot
      struct.  The "private" pointer in struct hotplug_slot thereby becomes
      unused, so drop it.
      Signed-off-by: default avatarLukas Wunner <lukas@wunner.de>
      Signed-off-by: default avatarBjorn Helgaas <bhelgaas@google.com>
      Reviewed-by: default avatarRafael J. Wysocki <rafael.j.wysocki@intel.com>
      Acked-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>  # drivers/pci/hotplug/rpa*
      Acked-by: Sebastian Ott <sebott@linux.ibm.com>        # drivers/pci/hotplug/s390*
      Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> # drivers/platform/x86
      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: Oliver OHalloran <oliveroh@au1.ibm.com>
      Cc: Gavin Shan <gwshan@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>
      125450f8
    • Lukas Wunner's avatar
      PCI: hotplug: Drop hotplug_slot_info · a7da2161
      Lukas Wunner authored
      Ever since the PCI hotplug core was introduced in 2002, drivers had to
      allocate and register a struct hotplug_slot_info for every slot:
      https://git.kernel.org/tglx/history/c/a8a2069f432c
      
      Apparently the idea was that drivers furnish the hotplug core with an
      up-to-date card presence status, power status, latch status and
      attention indicator status as well as notify the hotplug core of changes
      thereof.  However only 4 out of 12 hotplug drivers bother to notify the
      hotplug core with pci_hp_change_slot_info() and the hotplug core never
      made any use of the information:  There is just a single macro in
      pci_hotplug_core.c, GET_STATUS(), which uses the hotplug_slot_info if
      the driver lacks the corresponding callback in hotplug_slot_ops.  The
      macro is called when the user reads the attribute via sysfs.
      
      Now, if the callback isn't defined, the attribute isn't exposed in sysfs
      in the first place (see e.g. has_power_file()).  There are only two
      situations when the hotplug_slot_info would actually be accessed:
      
      * If the driver defines ->enable_slot or ->disable_slot but not
        ->get_power_status.
      
      * If the driver defines ->set_attention_status but not
        ->get_attention_status.
      
      There is no driver doing the former and just a single driver doing the
      latter, namely pnv_php.c.  Amend it with a ->get_attention_status
      callback.  With that, the hotplug_slot_info becomes completely unused by
      the PCI hotplug core.  But a few drivers use it internally as a cache:
      
      cpcihp uses it to cache the latch_status and adapter_status.
      cpqhp uses it to cache the adapter_status.
      pnv_php and rpaphp use it to cache the attention_status.
      shpchp uses it to cache all four values.
      
      Amend these drivers to cache the information in their private slot
      struct.  shpchp's slot struct already contains members to cache the
      power_status and adapter_status, so additional members are only needed
      for the other two values.  In the case of cpqphp, the cached value is
      only accessed in a single place, so instead of caching it, read the
      current value from the hardware.
      
      Caution:  acpiphp, cpci, cpqhp, shpchp, asus-wmi and eeepc-laptop
      populate the hotplug_slot_info with initial values on probe.  That code
      is herewith removed.  There is a theoretical chance that the code has
      side effects without which the driver fails to function, e.g. if the
      ACPI method to read the adapter status needs to be executed at least
      once on probe.  That seems unlikely to me, still maintainers should
      review the changes carefully for this possibility.
      
      Rafael adds: "I'm not aware of any case in which it will break anything,
      [...] but if that happens, it may be necessary to add the execution of
      the control methods in question directly to the initialization part."
      Signed-off-by: default avatarLukas Wunner <lukas@wunner.de>
      Signed-off-by: default avatarBjorn Helgaas <bhelgaas@google.com>
      Reviewed-by: default avatarRafael J. Wysocki <rafael.j.wysocki@intel.com>
      Acked-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>  # drivers/pci/hotplug/rpa*
      Acked-by: Sebastian Ott <sebott@linux.ibm.com>        # drivers/pci/hotplug/s390*
      Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> # drivers/platform/x86
      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: Oliver OHalloran <oliveroh@au1.ibm.com>
      Cc: Gavin Shan <gwshan@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>
      a7da2161
    • Lukas Wunner's avatar
      PCI: hotplug: Constify hotplug_slot_ops · 81c4b5bf
      Lukas Wunner authored
      Hotplug drivers cannot declare their hotplug_slot_ops const, making them
      attractive targets for attackers, because upon registration of a hotplug
      slot, __pci_hp_initialize() writes to the "owner" and "mod_name" members
      in that struct.
      
      Fix by moving these members to struct hotplug_slot and constify every
      driver's hotplug_slot_ops except for pciehp.
      
      pciehp constructs its hotplug_slot_ops at runtime based on the PCIe
      port's capabilities, hence cannot declare them const.  It can be
      converted to __write_rarely once that's mainlined:
      http://www.openwall.com/lists/kernel-hardening/2016/11/16/3Signed-off-by: default avatarLukas Wunner <lukas@wunner.de>
      Signed-off-by: default avatarBjorn Helgaas <bhelgaas@google.com>
      Reviewed-by: default avatarRafael J. Wysocki <rafael.j.wysocki@intel.com>
      Acked-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>  # drivers/pci/hotplug/rpa*
      Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> # drivers/platform/x86
      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: Oliver OHalloran <oliveroh@au1.ibm.com>
      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>
      81c4b5bf
    • Lukas Wunner's avatar
      PCI: pciehp: Reshuffle controller struct for clarity · d7587142
      Lukas Wunner authored
      The members in pciehp's controller struct are arranged in a seemingly
      arbitrary order and have grown to an amount that I no longer consider
      easily graspable by contributors.
      
      Sort the members into 5 rubrics:
      * Slot Capabilities register and quirks
      * Slot Control register access
      * Slot Status register event handling
      * state machine
      * hotplug core interface
      
      Obviously, this is just my personal bikeshed color and if anyone has a
      better idea, please come forward.  Any ordering will do as long as the
      information is presented in a manageable manner.
      
      No functional change intended.
      Signed-off-by: default avatarLukas Wunner <lukas@wunner.de>
      Signed-off-by: default avatarBjorn Helgaas <bhelgaas@google.com>
      d7587142
    • Lukas Wunner's avatar
      PCI: pciehp: Rename controller struct members for clarity · 4ff3126e
      Lukas Wunner authored
      Of the members which were just moved from pciehp's slot struct to the
      controller struct, rename "lock" to "state_lock" and rename "work" to
      "button_work" for clarity.  Perform the rename separately to the
      unification of the two structs per Sinan's request.
      
      No functional change intended.
      Signed-off-by: default avatarLukas Wunner <lukas@wunner.de>
      Signed-off-by: default avatarBjorn Helgaas <bhelgaas@google.com>
      Cc: Sinan Kaya <okaya@kernel.org>
      4ff3126e
    • Lukas Wunner's avatar
      PCI: pciehp: Unify controller and slot structs · 5790a9c7
      Lukas Wunner authored
      pciehp was originally introduced together with shpchp in a single
      commit, c16b4b14 ("PCI Hotplug: Add SHPC and PCI Express hot-plug
      drivers"):
      https://git.kernel.org/tglx/history/c/c16b4b14d980
      
      shpchp supports up to 31 slots per controller, hence uses separate slot
      and controller structs.  pciehp has a 1:1 relationship between slot and
      controller and therefore never required this separation.  Nevertheless,
      because much of the code had been copy-pasted between the two drivers,
      pciehp likewise uses separate structs to this very day.
      
      The artificial separation of data structures adds unnecessary complexity
      and bloat to pciehp and requires constantly chasing pointers at runtime.
      
      Simplify the driver by merging struct slot into struct controller.
      Merge the slot constructor pcie_init_slot() and the destructor
      pcie_cleanup_slot() into the controller counterparts.
      
      No functional change intended.
      Signed-off-by: default avatarLukas Wunner <lukas@wunner.de>
      Signed-off-by: default avatarBjorn Helgaas <bhelgaas@google.com>
      5790a9c7
    • Lukas Wunner's avatar
      PCI: pciehp: Tolerate Presence Detect hardwired to zero · 80696f99
      Lukas Wunner authored
      The WiGig Bus Extension (WBE) specification allows tunneling PCIe over
      IEEE 802.11.  A product implementing this spec is the wil6210 from
      Wilocity (now part of Qualcomm Atheros).  It integrates a PCIe switch
      with a wireless network adapter:
      
        00.0-+              [1ae9:0101]  Upstream Port
             +-00.0-+       [1ae9:0200]  Downstream Port
             |      +-00.0  [168c:0034]  Atheros AR9462 Wireless Network Adapter
             +-02.0         [1ae9:0201]  Downstream Port
             +-03.0         [1ae9:0201]  Downstream Port
      
      Wirelessly attached devices presumably appear below the hotplug ports
      with device ID [1ae9:0201].  Oddly, the Downstream Port [1ae9:0200]
      leading to the wireless network adapter is likewise Hotplug Capable,
      but has its Presence Detect State bit hardwired to zero.  Even if the
      Link Active bit is set, Presence Detect is zero, so this cannot be
      caused by in-band presence detection but only by broken hardware.
      
      pciehp assumes an empty slot if Presence Detect State is zero,
      regardless of Link Active being one.  Consequently, up until v4.18 it
      removes the wireless network adapter in pciehp_resume().  From v4.19 it
      already does so in pciehp_probe().
      
      Be lenient towards broken hardware and assume the slot is occupied if
      Link Active is set:  Introduce pciehp_card_present_or_link_active()
      and use it in lieu of pciehp_get_adapter_status() everywhere, except
      in pciehp_handle_presence_or_link_change() whose log messages depend
      on which of Presence Detect State or Link Active is set.
      
      Remove the Presence Detect State check from __pciehp_enable_slot()
      because it is only called if either of Presence Detect State or Link
      Active is set.
      
      Caution: There is a possibility that broken hardware exists which has
      working Presence Detect but hardwires Link Active to one.  On such
      hardware the slot will now incorrectly be considered always occupied.
      If such hardware is discovered, this commit can be rolled back and a
      quirk can be added which sets is_hotplug_bridge = 0 for [1ae9:0200].
      
      Link: https://bugzilla.kernel.org/show_bug.cgi?id=200839Reported-and-tested-by: default avatarDavid Yang <mmyangfl@gmail.com>
      Signed-off-by: default avatarLukas Wunner <lukas@wunner.de>
      Signed-off-by: default avatarBjorn Helgaas <bhelgaas@google.com>
      Cc: Rajat Jain <rajatja@google.com>
      Cc: Ashok Raj <ashok.raj@intel.com>
      80696f99
  4. 17 Sep, 2018 4 commits
    • Lukas Wunner's avatar
      PCI: pciehp: Drop hotplug_slot_ops wrappers · eee6e273
      Lukas Wunner authored
      pciehp's ->enable_slot, ->disable_slot, ->get_attention_status and
      ->reset_slot callbacks are currently implemented by wrapper functions
      that do nothing else but call down to a backend function.  The backends
      are not called from anywhere else, so drop the wrappers and use the
      backends directly as callbacks, thereby shaving off a few lines of
      unnecessary code.
      
      No functional change intended.
      Signed-off-by: default avatarLukas Wunner <lukas@wunner.de>
      Signed-off-by: default avatarBjorn Helgaas <bhelgaas@google.com>
      eee6e273
    • Lukas Wunner's avatar
      PCI: pciehp: Drop unnecessary includes · 7d4ba523
      Lukas Wunner authored
      Drop the following includes from pciehp source files which no longer use
      any of the included symbols:
      
      * <linux/sched/signal.h> in pciehp.h
        <linux/signal.h> in pciehp_hpc.c
        Added by commit de25968c ("fix more missing includes") to
        accommodate for a call to signal_pending().
        The call was removed by commit 262303fe ("pciehp: fix wait command
        completion").
      
      * <linux/interrupt.h> in pciehp_core.c
        Added by historic commit f308a2df ("PCI: add PCI Express Port Bus
        Driver subsystem") to accommodate for a call to free_irq():
        https://git.kernel.org/tglx/history/c/f308a2dfbe63
        The call was removed by commit 407f452b ("pciehp: remove
        unnecessary free_irq").
      
      * <linux/time.h> in pciehp_core.c and pciehp_hpc.c
        Added by commit 34d03419 ("PCIEHP: Add Electro Mechanical
        Interlock (EMI) support to the PCIE hotplug driver."),
        which was reverted by commit bd3d99c1 ("PCI: Remove untested
        Electromechanical Interlock (EMI) support in pciehp.").
      
      * <linux/module.h> in pciehp_ctrl.c, pciehp_hpc.c and pciehp_pci.c
        Added by historic commit c16b4b14 ("PCI Hotplug: Add SHPC and PCI
        Express hot-plug drivers"):
        https://git.kernel.org/tglx/history/c/c16b4b14d980
        Module-related symbols were neither used back then in those files,
        nor are they used today.
      
      * <linux/slab.h> in pciehp_ctrl.c
        Added by commit 5a0e3ad6 ("include cleanup: Update gfp.h and
        slab.h includes to prepare for breaking implicit slab.h inclusion from
        percpu.h") to accommodate for calls to kmalloc().
        The calls were removed by commit 0e94916e ("PCI: pciehp: Handle
        events synchronously").
      
      * "../pci.h" in pciehp_ctrl.c
        Added by historic commit 67f4660b ("PCI: ASPM patch for") to
        accommodate for usage of the global variable pcie_mch_quirk:
        https://git.kernel.org/tglx/history/c/67f4660b72f2
        The global variable was removed by commit 0ba379ec ("PCI: Simplify
        hotplug mch quirk").
      Signed-off-by: default avatarLukas Wunner <lukas@wunner.de>
      Signed-off-by: default avatarBjorn Helgaas <bhelgaas@google.com>
      7d4ba523
    • Lukas Wunner's avatar
      PCI: pciehp: Differentiate between surprise and safe removal · 11e87702
      Lukas Wunner authored
      When removing PCI devices below a hotplug bridge, pciehp marks them as
      disconnected if the card is no longer present in the slot or it quiesces
      them if the card is still present (by disabling INTx interrupts, bus
      mastering and SERR# reporting).
      
      To detect whether the card is still present, pciehp checks the Presence
      Detect State bit in the Slot Status register.  The problem with this
      approach is that even if the card is present, the link to it may be
      down, and it that case it would be better to mark the devices as
      disconnected instead of trying to quiesce them.  Moreover, if the card
      in the slot was quickly replaced by another one, the Presence Detect
      State bit would be set, yet trying to quiesce the new card's devices
      would be wrong and the correct thing to do is to mark the previous
      card's devices as disconnected.
      
      Instead of looking at the Presence Detect State bit, it is better to
      differentiate whether the card was surprise removed versus safely
      removed (via sysfs or an Attention Button press).  On surprise removal,
      the devices should be marked as disconnected, whereas on safe removal it
      is correct to quiesce the devices.
      
      The knowledge whether a surprise removal or a safe removal is at hand
      does exist further up in the call stack:  A surprise removal is
      initiated by pciehp_handle_presence_or_link_change(), a safe removal by
      pciehp_handle_disable_request().
      
      Pass that information down to pciehp_unconfigure_device() and use it in
      lieu of the Presence Detect State bit.  While there, add kernel-doc to
      pciehp_unconfigure_device() and pciehp_configure_device().
      Tested-by: default avatarAlexandru Gagniuc <mr.nuke.me@gmail.com>
      Signed-off-by: default avatarLukas Wunner <lukas@wunner.de>
      Signed-off-by: default avatarBjorn Helgaas <bhelgaas@google.com>
      Cc: Keith Busch <keith.busch@intel.com>
      11e87702
    • Lukas Wunner's avatar
      PCI: Simplify disconnected marking · a50ac6bf
      Lukas Wunner authored
      Commit 89ee9f76 ("PCI: Add device disconnected state") iterates over
      the devices on a parent bus, marks each as disconnected, then marks
      each device's children as disconnected using pci_walk_bus().
      
      The same can be achieved more succinctly by calling pci_walk_bus() on
      the parent bus.  Moreover, this does not need to wait until acquiring
      pci_lock_rescan_remove(), so move it out of that critical section.
      
      The critical section in err.c contains a pci_dev_get() / pci_dev_put()
      pair which was apparently copy-pasted from pciehp_pci.c.  In the latter
      it serves the purpose of holding the struct pci_dev in place until the
      Command register is updated.  err.c doesn't do anything like that, hence
      the pair is unnecessary.  Remove it.
      Signed-off-by: default avatarLukas Wunner <lukas@wunner.de>
      Signed-off-by: default avatarBjorn Helgaas <bhelgaas@google.com>
      Cc: Keith Busch <keith.busch@intel.com>
      Cc: Oza Pawandeep <poza@codeaurora.org>
      Cc: Sinan Kaya <okaya@kernel.org>
      Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
      a50ac6bf
  5. 16 Sep, 2018 2 commits
  6. 15 Sep, 2018 8 commits
  7. 14 Sep, 2018 12 commits