On Tue, Jul 31, 2018 at 07:50:37AM +0200, Lukas Wunner wrote:
> 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: Alexandru Gagniuc <mr.nuke.me@xxxxxxxxx>
> Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
> Cc: Keith Busch <keith.busch@xxxxxxxxx>
> ---
> drivers/pci/hotplug/pciehp.h | 2 +-
> drivers/pci/hotplug/pciehp_ctrl.c | 22 +++++++++++++---------
> drivers/pci/hotplug/pciehp_pci.c | 23 ++++++++++++++++++++---
> 3 files changed, 34 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index 811cf83f956d..39c9c8815a35 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -181,7 +181,7 @@ void pciehp_handle_button_press(struct slot *slot);
> void pciehp_handle_disable_request(struct slot *slot);
> void pciehp_handle_presence_or_link_change(struct slot *slot, u32 events);
> int pciehp_configure_device(struct slot *p_slot);
> -void pciehp_unconfigure_device(struct slot *p_slot);
> +void pciehp_unconfigure_device(struct slot *p_slot, bool presence);
> void pciehp_queue_pushbutton_work(struct work_struct *work);
> struct controller *pcie_init(struct pcie_device *dev);
> int pcie_init_notification(struct controller *ctrl);
> diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
> index 6855933ab372..7932e70e9f29 100644
> --- a/drivers/pci/hotplug/pciehp_ctrl.c
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> @@ -26,6 +26,9 @@
> hotplug controller logic
> */
>
> +#define SAFE_REMOVAL true
> +#define SURPRISE_REMOVAL false
> +
> static void set_slot_off(struct controller *ctrl, struct slot *pslot)
> {
> /* turn off slot, turn on Amber LED, turn off Green LED if supported*/
> @@ -101,12 +104,13 @@ static int board_added(struct slot *p_slot)
> /**
> * remove_board - Turns off slot and LEDs
> * @p_slot: slot where board is being removed
> + * @safe_removal: whether the board is safely removed (versus surprise removed)
> */
> -static void remove_board(struct slot *p_slot)
> +static void remove_board(struct slot *p_slot, bool safe_removal)
> {
> struct controller *ctrl = p_slot->ctrl;
>
> - pciehp_unconfigure_device(p_slot);
> + pciehp_unconfigure_device(p_slot, safe_removal);
Below we turn off power to the slot if it has power controller. Even if
we disable slot from sysfs, I think it ends up being inaccessible after
power is turned off. I wonder if we should mark the devices disconnected
in that case as well?
>
> if (POWER_CTRL(ctrl)) {
> pciehp_power_off_slot(p_slot);
