Re: [PATCH RFC 3/3] pciehp_hpc: workaround to not wait in pciehp_isr on surprise removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Concurrent calls to `pci_hp_spurious_link_change()` corrupt state flags, breaking hot-plug recovery logic.
- [High] Lockless read of `pending_events` races with `atomic_xchg`, bypassing spurious link checks and causing premature disconnections.
- [Medium] Incorrect truthiness evaluation of `pciehp_card_present()` breaks the disconnect workaround during severe surprise removals.
--

--- Patch [3]: [PATCH RFC 3/3] pciehp_hpc: workaround to not wait in pciehp_isr on surprise removal ---
Note: The format of this review has been altered to provide a free-form
summary of the findings due to automated recitation restrictions. The original
patch diff is not quoted inline.

commit df7f3f7b5143de6a32373ccd864f5ca514636a79
Author: Abhin Parekadan Jose <abhinjoses@gmail.com>
pciehp_hpc: workaround to not wait in pciehp_isr on surprise removal

This patch adds a workqueue to handle PCIe hotplug device disconnects without
waiting in the interrupt service routine. It checks the presence detect state
bit to differentiate between safe and surprise removals, scheduling the
disconnect work if the card is no longer present.

[Severity: High]
Could the concurrent evaluation of pci_hp_spurious_link_change() in the new
disconnect workqueue corrupt state flags? The function
pci_hp_spurious_link_change() is stateful and clears the PCI_LINK_CHANGED bit
using test_and_clear_bit(). If the asynchronous workqueue destructively
consumes this flag before the primary hot-plug IRQ thread evaluates it, won't
the core handler erroneously tear down the device tree instead of safely
ignoring a transient link flap?

[Severity: High]
Does the lockless read of pending_events in pciehp_disconnect_work() race
with the IRQ thread? The workqueue reads ctrl->pending_events locklessly.
However, the threaded IRQ handler pciehp_ist() unconditionally clears these
events using atomic_xchg(&ctrl->pending_events, 0). If the workqueue executes
just after pciehp_ist() clears the events, it will observe a value of 0.
Wouldn't this cause the workqueue to completely bypass the spurious link down
safety checks and prematurely disconnect all devices on the bus?

[Severity: Medium]
Will the truthiness checks for pciehp_card_present() fail during an actual
hardware surprise removal? When a device is abruptly removed, configuration
space reads typically return 0xffff, causing pciehp_card_present() to return
-ENODEV. Because -ENODEV is a truthy value in C, the condition (!present) in
the interrupt handler will evaluate to false, meaning the disconnect work is
never scheduled. Furthermore, the check inside the workqueue evaluates
pciehp_card_present(ctrl) != 0, which is true for -ENODEV, causing an early
return. Does this logic inadvertently disable the workaround precisely when a
surprise removal happens?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260823183458.982699-1-abhinjoses@gmail.com?part=3