Re: [PATCH RFC 2/3] PCI: pciehp: Report surprise removal from pciehp_isr()

["Michael S. Tsirkin" <mst@redhat.com>]

On Sat, Sep 05, 2026 at 06:38:59PM +0000, Abhin Parekadan Jose wrote:
> A surprise removal during a safe removal cannot be reported: the
> removal blocks waiting on a device interrupt or status read, and the
> single-threaded IRQ thread is itself executing that removal, so it
> cannot report that the device is gone. The removal hangs.
> 
> The hardirq handler pciehp_isr() still runs while the IRQ thread is
> blocked, so it can report the disconnect. However, pciehp_ist()
> deliberately ignores link and presence changes caused by a Secondary
> Bus Reset or Downstream Port Containment, where the device is only
> temporarily inaccessible. Distinguishing those normally requires
> waiting for the SBR or DPC to conclude, which takes seconds and is not
> possible in hardirq context.
> 
> Schedule a work item from pciehp_isr() when a PDC or DLLSC event
> arrives and PDS indicates if the device is connected/disconnected.
> This provides us a pathway to wait/block/sleep as we will not be
> in pciehp_isr().
> 
> Move the scheduling of the driver's disconnect notification out of
> pci_dev_set_disconnected() into schedule_notification_work() so it can
> be invoked from the new work item. Factor the spurious link change
> test out of pciehp_ist() into pciehp_is_spurious_link_change() so both
> pciehp_ist() and pciehp_disconnect_work() can use it.
> 
> Link: https://lore.kernel.org/all/aHlZE18kPuHuDtTT@wunner.de/
> Signed-off-by: Abhin Parekadan Jose <abhinjoses@gmail.com>
> ---
>  drivers/pci/hotplug/pciehp.h     |  1 +
>  drivers/pci/hotplug/pciehp_hpc.c | 56 +++++++++++++++++++++++++++-----
>  drivers/pci/pci.h                |  6 ++++
>  3 files changed, 55 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> index debc79b0adfb..c8ceb9320e2e 100644
> --- a/drivers/pci/hotplug/pciehp.h
> +++ b/drivers/pci/hotplug/pciehp.h
> @@ -116,6 +116,7 @@ struct controller {
>  	unsigned int ist_running;
>  	int request_result;
>  	wait_queue_head_t requester;
> +	struct work_struct disconnect_work;
>  };
>  
>  /**
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 4c62140a3cb4..235ca8a176f1 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -620,6 +620,45 @@ static void pciehp_ignore_link_change(struct controller *ctrl,
>  	up_read(&ctrl->reset_lock);
>  }
>  
> +/*
> + * Link Down/Up events caused by Downstream Port Containment if recovery
> + * succeeded, or caused by Secondary Bus Reset, suspend to D3cold, firmware
> + * update, FPGA reconfiguration, etc. are spurious and should be ignored.
> + */
> +static bool pciehp_is_spurious_link_change(struct controller *ctrl,
> +						  struct pci_dev *pdev,
> +						  u32 events)
> +{
> +	return (events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC)) &&
> +		(pci_dpc_recovered(pdev) || pci_hp_spurious_link_change(pdev)) &&
> +		ctrl->state == ON_STATE;
> +}
> +
> +/*
> + * Workaround to not wait in the isr.
> + */
> +static void pciehp_disconnect_work(struct work_struct *work)
> +{
> +	struct pci_bus *bus;
> +	struct controller *ctrl = container_of(work, struct controller,
> +					       disconnect_work);
> +	struct pci_dev *pdev = ctrl_dev(ctrl);
> +	u32 events;
> +
> +	events = atomic_read(&ctrl->pending_events);
> +
> +	if (pciehp_is_spurious_link_change(ctrl, pdev, events))
> +		return;
> +
> +	bus = ctrl->pcie->port->subordinate;
> +
> +	/* The card may have returned */
> +	if (!bus || pciehp_card_present(ctrl) != 0)
> +		return;
> +
> +	pci_walk_bus(bus, schedule_notification_work, NULL);
> +}
> +
>  static irqreturn_t pciehp_isr(int irq, void *dev_id)
>  {
>  	struct controller *ctrl = (struct controller *)dev_id;
> @@ -722,6 +761,12 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
>  
>  	/* Save pending events for consumption by IRQ thread. */
>  	atomic_or(events, &ctrl->pending_events);
> +
> +	/* presence change events */
> +	if ((events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC)) &&
> +	    !pciehp_card_present(ctrl))
> +		schedule_work(&ctrl->disconnect_work);
> +
>  	return IRQ_WAKE_THREAD;
>  }
>  
> @@ -761,14 +806,7 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
>  				      PCI_EXP_SLTCTL_ATTN_IND_ON);
>  	}
>  
> -	/*
> -	 * Ignore Link Down/Up events caused by Downstream Port Containment
> -	 * if recovery succeeded, or caused by Secondary Bus Reset,
> -	 * suspend to D3cold, firmware update, FPGA reconfiguration, etc.
> -	 */
> -	if ((events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC)) &&
> -	    (pci_dpc_recovered(pdev) || pci_hp_spurious_link_change(pdev)) &&
> -	    ctrl->state == ON_STATE) {
> +	if (pciehp_is_spurious_link_change(ctrl, pdev, events)) {
>  		u16 ignored_events = PCI_EXP_SLTSTA_DLLSC;
>  
>  		if (!ctrl->inband_presence_disabled)
> @@ -1036,6 +1074,7 @@ struct controller *pcie_init(struct pcie_device *dev)
>  	init_waitqueue_head(&ctrl->requester);
>  	init_waitqueue_head(&ctrl->queue);
>  	INIT_DELAYED_WORK(&ctrl->button_work, pciehp_queue_pushbutton_work);
> +	INIT_WORK(&ctrl->disconnect_work, pciehp_disconnect_work);
>  	dbg_ctrl(ctrl);
>  
>  	down_read(&pci_bus_sem);
> @@ -1096,6 +1135,7 @@ struct controller *pcie_init(struct pcie_device *dev)
>  void pciehp_release_ctrl(struct controller *ctrl)
>  {
>  	cancel_delayed_work_sync(&ctrl->button_work);
> +	cancel_work_sync(&ctrl->disconnect_work);
>  	kfree(ctrl);
>  }
>  
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 23b1605e783a..4e17878edeab 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -805,6 +805,12 @@ static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
>  	pci_dev_set_io_state(dev, pci_channel_io_perm_failure);
>  	pci_doe_disconnected(dev);
>  
> +	return 0;
> +}
> +
> +static inline int schedule_notification_work(struct pci_dev *dev, void *unused)
> +{
> +	pci_dev_set_disconnected(dev, NULL);


Does this not break what patch 1 was trying to do,
for everyone who does not call schedule_notification_work,
that is, everyone except pciehp?

I'd say do the reverse: make schedule_notification_work
schedule the work, and have both pcieh and 
pci_dev_set_disconnected call that.


>  	if (READ_ONCE(dev->disconnect_work_enable)) {
>  		/* Make sure work is up to date. */
>  		smp_rmb();
> -- 
> 2.51.1
>