[PATCH 0/4] PCI/hotplug: Fix interrupt / event handling problems

[PATCH 0/4] PCI/hotplug: Fix interrupt / event handling problems

[Ilpo Järvinen]

Hi all,

It was reported introducing bwctrl broke attaching a PCI device into
VM. I tracked this down to problems in hotplug interrupt and event
handling where hotplug code assumed all events are for it without
proper checking. As a result, the extra interrupts that occurred due to
bwctrl caused hotplug pick events during slot reset due to shared irq
and eventually hotplug unconfigured the card spuriously.

This series fixes the hotplug slot reset so that no hotplug events can
be picked up during slot reset which was the original intention of the
reset code but it failed to synchronize its intention with the
interrupt and event handling.

I've intentionally split the three patches because to be careful and
allow bisect to detect if the two follow up changes make assumptions
that do not hold water, but logically they belong to the same single
change altering the synchronization between the reset slot and hotplug
event handling. It should be technically possible to fold them into the
same change, but I feel there are benefits of keeping them as separate
so bisect can see them as separate changes.

The fourth patch fixes an oversight I found while reading the HPIE
related code and is unrelated to the other three patches.

As there were small changes into the first patch since Joel's test
to address Lukas' comments in the bugzilla thread. I'd prefer him
to test it again, just in case, so I dropped the tested-by tag until
that happens.

Ilpo Järvinen (4):
  PCI/hotplug: Disable HPIE over reset
  PCI/hotplug: Clearing HPIE for the duration of reset is enough
  PCI/hotplug: reset_lock is not required synchronizing with irq thread
  PCI/hotplug: Don't enabled HPIE in poll mode

 drivers/pci/hotplug/pciehp_hpc.c | 39 +++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 11 deletions(-)


base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
-- 
2.39.5

[PATCH 1/4] PCI/hotplug: Disable HPIE over reset

[Ilpo Järvinen]

pciehp_reset_slot() disables PDCE (Presence Detect Changed Enable) and
DLLSCE (Data Link Layer State Changed Enable) for the duration of reset
and clears the related status bits PDC and DLLSC from the Slot Status
register after the reset to avoid hotplug incorrectly assuming the card
was removed.

However, hotplug shares interrupt with PME and BW notifications both of
which can make pciehp_isr() to run despite PDCE and DLLSCE bits being
off. pciehp_isr() then picks PDC or DLLSC bits from the Slot Status
register due to the events that occur during reset and caches them into
->pending_events. Later, the IRQ thread in pciehp_ist() will process
the ->pending_events and will assume the Link went Down due to a card
change (in pciehp_handle_presence_or_link_change()).

Change pciehp_reset_slot() to also clear HPIE (Hot-Plug Interrupt
Enable) as pciehp_isr() will first check HPIE to see if the interrupt
is not for it. Then synchronize with the IRQ handling to ensure no
events are pending, before invoking the reset.

Similarly, if the poll mode is in use, park the poll thread over the
duration of the reset to stop handling events.

In order to not race irq_syncronize()/kthread_{,un}park() with the irq
/ poll_thread freeing from pciehp_remove(), take reset_lock in
pciehp_free_irq() and check the irq / poll_thread variable validity in
pciehp_reset_slot().

Fixes: 06a8d89af551 ("PCI: pciehp: Disable link notification across slot reset")
Fixes: 720d6a671a6e ("PCI: pciehp: Do not handle events if interrupts are masked")
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219765
Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/pci/hotplug/pciehp_hpc.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index bb5a8d9f03ad..c487e274b282 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -77,10 +77,15 @@ static inline int pciehp_request_irq(struct controller *ctrl)
 
 static inline void pciehp_free_irq(struct controller *ctrl)
 {
-	if (pciehp_poll_mode)
+	down_read_nested(&ctrl->reset_lock, ctrl->depth);
+	if (pciehp_poll_mode) {
 		kthread_stop(ctrl->poll_thread);
-	else
+		ctrl->poll_thread = NULL;
+	} else {
 		free_irq(ctrl->pcie->irq, ctrl);
+		ctrl->pcie->irq = IRQ_NOTCONNECTED;
+	}
+	up_read(&ctrl->reset_lock);
 }
 
 static int pcie_poll_cmd(struct controller *ctrl, int timeout)
@@ -766,8 +771,9 @@ static int pciehp_poll(void *data)
 
 	while (!kthread_should_stop()) {
 		/* poll for interrupt events or user requests */
-		while (pciehp_isr(IRQ_NOTCONNECTED, ctrl) == IRQ_WAKE_THREAD ||
-		       atomic_read(&ctrl->pending_events))
+		while (!kthread_should_park() &&
+		       (pciehp_isr(IRQ_NOTCONNECTED, ctrl) == IRQ_WAKE_THREAD ||
+			atomic_read(&ctrl->pending_events)))
 			pciehp_ist(IRQ_NOTCONNECTED, ctrl);
 
 		if (pciehp_poll_time <= 0 || pciehp_poll_time > 60)
@@ -907,6 +913,8 @@ int pciehp_reset_slot(struct hotplug_slot *hotplug_slot, bool probe)
 
 	down_write_nested(&ctrl->reset_lock, ctrl->depth);
 
+	if (!pciehp_poll_mode)
+		ctrl_mask |= PCI_EXP_SLTCTL_HPIE;
 	if (!ATTN_BUTTN(ctrl)) {
 		ctrl_mask |= PCI_EXP_SLTCTL_PDCE;
 		stat_mask |= PCI_EXP_SLTSTA_PDC;
@@ -918,9 +926,21 @@ int pciehp_reset_slot(struct hotplug_slot *hotplug_slot, bool probe)
 	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
 		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, 0);
 
+	/* Make sure HPIE is no longer seen by the interrupt handler. */
+	if (pciehp_poll_mode) {
+		if (ctrl->poll_thread)
+			kthread_park(ctrl->poll_thread);
+	} else {
+		if (ctrl->pcie->irq != IRQ_NOTCONNECTED)
+			synchronize_irq(ctrl->pcie->irq);
+	}
+
 	rc = pci_bridge_secondary_bus_reset(ctrl->pcie->port);
 
 	pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, stat_mask);
+	if (pciehp_poll_mode && ctrl->poll_thread)
+		kthread_unpark(ctrl->poll_thread);
+
 	pcie_write_cmd_nowait(ctrl, ctrl_mask, ctrl_mask);
 	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
 		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, ctrl_mask);
-- 
2.39.5

[PATCH 2/4] PCI/hotplug: Clearing HPIE for the duration of reset is enough

[Ilpo Järvinen]

The previous change cleared HPIE (Hot-Plug Interrupt Enable) in
pciehp_reset_slot(). Clearing HPIE should be enough to synchronize with
the interrupt and event handling so that clearing PDCE (Presence Detect
Changed Enable) and DLLSCE (Data Link Layer State Changed Enable) is
not necessary. However, the commit be54ea5330d ("PCI: pciehp: Disable
Data Link Layer State Changed event on suspend") found out that under
some circumstances, clearing also DLLSCE is necessary.

While this is logically part of the previous change, remove PDCE and
DLLSCE clearing in now separately to allow bisect pinpoint it better if
removing their clearing causes some issues.

Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/hotplug/pciehp_hpc.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index c487e274b282..634cf5004f76 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -915,11 +915,8 @@ int pciehp_reset_slot(struct hotplug_slot *hotplug_slot, bool probe)
 
 	if (!pciehp_poll_mode)
 		ctrl_mask |= PCI_EXP_SLTCTL_HPIE;
-	if (!ATTN_BUTTN(ctrl)) {
-		ctrl_mask |= PCI_EXP_SLTCTL_PDCE;
+	if (!ATTN_BUTTN(ctrl))
 		stat_mask |= PCI_EXP_SLTSTA_PDC;
-	}
-	ctrl_mask |= PCI_EXP_SLTCTL_DLLSCE;
 	stat_mask |= PCI_EXP_SLTSTA_DLLSC;
 
 	pcie_write_cmd(ctrl, 0, ctrl_mask);
-- 
2.39.5

[PATCH 3/4] PCI/hotplug: reset_lock is not required synchronizing with irq thread

[Ilpo Järvinen]

Disabling HPIE (Hot-Plug Interrupt Enable) and synchronizing with irq
handling in pciehp_reset_slot() is enough to ensure no pending events
are processed during the slot reset. Thus, there is no need to take
reset_lock in the IRQ thread.

Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/hotplug/pciehp_hpc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 634cf5004f76..26150a6b48f4 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -748,12 +748,10 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
 	 * Disable requests have higher priority than Presence Detect Changed
 	 * or Data Link Layer State Changed events.
 	 */
-	down_read_nested(&ctrl->reset_lock, ctrl->depth);
 	if (events & DISABLE_SLOT)
 		pciehp_handle_disable_request(ctrl);
 	else if (events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC))
 		pciehp_handle_presence_or_link_change(ctrl, events);
-	up_read(&ctrl->reset_lock);
 
 	ret = IRQ_HANDLED;
 out:
-- 
2.39.5

[PATCH 4/4] PCI/hotplug: Don't enabled HPIE in poll mode

[Ilpo Järvinen]

PCIe hotplug can operate in poll mode without interrupt handlers using
a polling kthread only. The commit eb34da60edee ("PCI: pciehp: Disable
hotplug interrupt during suspend") failed to consider that and enables
HPIE (Hot-Plug Interrupt Enable) unconditionally when resuming the
Port.

Only set HPIE if non-poll mode is in use. This makes
pcie_enable_interrupt() match how pcie_enable_notification() already
handles HPIE.

Fixes: eb34da60edee ("PCI: pciehp: Disable hotplug interrupt during suspend")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/hotplug/pciehp_hpc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 26150a6b48f4..7e1ed179c7f3 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -846,7 +846,9 @@ void pcie_enable_interrupt(struct controller *ctrl)
 {
 	u16 mask;
 
-	mask = PCI_EXP_SLTCTL_HPIE | PCI_EXP_SLTCTL_DLLSCE;
+	mask = PCI_EXP_SLTCTL_DLLSCE;
+	if (!pciehp_poll_mode)
+		mask |= PCI_EXP_SLTCTL_HPIE;
 	pcie_write_cmd(ctrl, mask, mask);
 }
 
-- 
2.39.5

Re: [PATCH 3/4] PCI/hotplug: reset_lock is not required synchronizing with irq thread

[Lukas Wunner]

On Thu, Mar 13, 2025 at 04:23:32PM +0200, Ilpo Järvinen wrote:
> Disabling HPIE (Hot-Plug Interrupt Enable) and synchronizing with irq
> handling in pciehp_reset_slot() is enough to ensure no pending events
> are processed during the slot reset. Thus, there is no need to take
> reset_lock in the IRQ thread.
[...]
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -748,12 +748,10 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
>  	 * Disable requests have higher priority than Presence Detect Changed
>  	 * or Data Link Layer State Changed events.
>  	 */
> -	down_read_nested(&ctrl->reset_lock, ctrl->depth);
>  	if (events & DISABLE_SLOT)
>  		pciehp_handle_disable_request(ctrl);
>  	else if (events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC))
>  		pciehp_handle_presence_or_link_change(ctrl, events);
> -	up_read(&ctrl->reset_lock);
>  
>  	ret = IRQ_HANDLED;
>  out:

The release and re-acquisition of reset_lock in
pciehp_configure_device() and pciehp_unconfigure_device()
needs to be removed as well if the above hunk is applied.

But please wait a little while before respinning so that I can
think through the whole series.

Thanks,

Lukas

Re: [PATCH 3/4] PCI/hotplug: reset_lock is not required synchronizing with irq thread

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1861 bytes --]

On Fri, 14 Mar 2025, Lukas Wunner wrote:

> On Thu, Mar 13, 2025 at 04:23:32PM +0200, Ilpo Järvinen wrote:
> > Disabling HPIE (Hot-Plug Interrupt Enable) and synchronizing with irq
> > handling in pciehp_reset_slot() is enough to ensure no pending events
> > are processed during the slot reset. Thus, there is no need to take
> > reset_lock in the IRQ thread.
> [...]
> > --- a/drivers/pci/hotplug/pciehp_hpc.c
> > +++ b/drivers/pci/hotplug/pciehp_hpc.c
> > @@ -748,12 +748,10 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
> >  	 * Disable requests have higher priority than Presence Detect Changed
> >  	 * or Data Link Layer State Changed events.
> >  	 */
> > -	down_read_nested(&ctrl->reset_lock, ctrl->depth);
> >  	if (events & DISABLE_SLOT)
> >  		pciehp_handle_disable_request(ctrl);
> >  	else if (events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC))
> >  		pciehp_handle_presence_or_link_change(ctrl, events);
> > -	up_read(&ctrl->reset_lock);
> >  
> >  	ret = IRQ_HANDLED;
> >  out:
> 
> The release and re-acquisition of reset_lock in
> pciehp_configure_device() and pciehp_unconfigure_device()
> needs to be removed as well if the above hunk is applied.

Ah, right. I also now removed reset_lock from 
pciehp_ignore_dpc_link_change() that is directly called from pciehp_ist().

This leaves reset_lock only into pciehp_free_irq(), pciehp_reset_slot(), 
and pciehp_check_presence(). It seems to me pciehp_check_presence() 
requires keeping reset_lock as is. The former two could have synchronized 
with a mutex and shorter critical sections in pciehp_reset_slot() but it 
doesn't look worth the effort (IMO) to divide reset_lock into two to 
realize that.

> But please wait a little while before respinning so that I can
> think through the whole series.

Sure, take your time. :-)

-- 
 i.

Re: [PATCH 4/4] PCI/hotplug: Don't enabled HPIE in poll mode

[Lukas Wunner]

On Thu, Mar 13, 2025 at 04:23:33PM +0200, Ilpo Järvinen wrote:
> PCIe hotplug can operate in poll mode without interrupt handlers using
> a polling kthread only. The commit eb34da60edee ("PCI: pciehp: Disable
> hotplug interrupt during suspend") failed to consider that and enables
> HPIE (Hot-Plug Interrupt Enable) unconditionally when resuming the
> Port.
> 
> Only set HPIE if non-poll mode is in use. This makes
> pcie_enable_interrupt() match how pcie_enable_notification() already
> handles HPIE.
> 
> Fixes: eb34da60edee ("PCI: pciehp: Disable hotplug interrupt during suspend")
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Reviewed-by: Lukas Wunner <lukas@wunner.de>

There's a typo in the subject line (s/enabled/enable/).

This patch does not depend on the preceding patches in the series
and can be applied by itself.

Thanks,

Lukas

Re: [PATCH 1/4] PCI/hotplug: Disable HPIE over reset

[Lukas Wunner]

On Thu, Mar 13, 2025 at 04:23:30PM +0200, Ilpo Järvinen wrote:
> pciehp_reset_slot() disables PDCE (Presence Detect Changed Enable) and
> DLLSCE (Data Link Layer State Changed Enable) for the duration of reset
> and clears the related status bits PDC and DLLSC from the Slot Status
> register after the reset to avoid hotplug incorrectly assuming the card
> was removed.
> 
> However, hotplug shares interrupt with PME and BW notifications both of
> which can make pciehp_isr() to run despite PDCE and DLLSCE bits being
> off. pciehp_isr() then picks PDC or DLLSC bits from the Slot Status
> register due to the events that occur during reset and caches them into
> ->pending_events. Later, the IRQ thread in pciehp_ist() will process
> the ->pending_events and will assume the Link went Down due to a card
> change (in pciehp_handle_presence_or_link_change()).
> 
> Change pciehp_reset_slot() to also clear HPIE (Hot-Plug Interrupt
> Enable) as pciehp_isr() will first check HPIE to see if the interrupt
> is not for it. Then synchronize with the IRQ handling to ensure no
> events are pending, before invoking the reset.

After dwelling on this for a while, I'm thinking that it may re-introduce
the issue fixed by commit f5eff5591b8f ("PCI: pciehp: Fix AB-BA deadlock
between reset_lock and device_lock"):

Looking at the second and third stack trace in its commit message,
down_write(reset_lock) in pciehp_reset_slot() is basically equivalent
to synchronize_irq() and we're holding device_lock() at that point,
hindering progress of pciehp_ist().

So I think I have guided you in the wrong direction and I apologize
for that.

However it seems to me that this should be solvable with the small
patch below.  Am I missing something?

@Joel Mathew Thomas, could you give the below patch a spin and see
if it helps?

Thanks!

-- >8 --

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index bb5a8d9f03ad..99a2ac13a3d1 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -688,6 +688,11 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
 		return IRQ_HANDLED;
 	}
 
+	/* Ignore events masked by pciehp_reset_slot(). */
+	events &= ctrl->slot_ctrl;
+	if (!events)
+		return IRQ_HANDLED;
+
 	/* Save pending events for consumption by IRQ thread. */
 	atomic_or(events, &ctrl->pending_events);
 	return IRQ_WAKE_THREAD;

Re: [PATCH 1/4] PCI/hotplug: Disable HPIE over reset

[Lukas Wunner]

On Sat, Mar 15, 2025 at 07:57:55PM +0100, proxy0@tutamail.com wrote:
> Mar 15, 2025, 9:28 PM by lukas@wunner.de:
> > After dwelling on this for a while, I'm thinking that it may re-introduce
> > the issue fixed by commit f5eff5591b8f ("PCI: pciehp: Fix AB-BA deadlock
> > between reset_lock and device_lock"):
> >
> > Looking at the second and third stack trace in its commit message,
> > down_write(reset_lock) in pciehp_reset_slot() is basically equivalent
> > to synchronize_irq() and we're holding device_lock() at that point,
> > hindering progress of pciehp_ist().
> >
> > So I think I have guided you in the wrong direction and I apologize
> > for that.
> >
> > However it seems to me that this should be solvable with the small
> > patch below.  Am I missing something?
> >
> > @Joel Mathew Thomas, could you give the below patch a spin and see
> > if it helps?
> 
> I've tested the patch series along with the additional patch provided.
> 
> Kernel: 6.14.0-rc6-00043-g3571e8b091f4-dirty-pci-hotplug-reset-fixes-eventmask-fix
> 
> Patches applied:
> - [PATCH 1/4] PCI/hotplug: Disable HPIE over reset
> - [PATCH 2/4] PCI/hotplug: Clearing HPIE for the duration of reset is enough
> - [PATCH 3/4] PCI/hotplug: reset_lock is not required synchronizing with irq thread
> - [PATCH 4/4] PCI/hotplug: Don't enable HPIE in poll mode
> - The latest patch from you:
> +	/* Ignore events masked by pciehp_reset_slot(). */
> +	events &= ctrl->slot_ctrl;
> +	if (!events)
> +		return IRQ_HANDLED;

Could you test *only* the quoted diff, i.e. without patches [1/4] - [4/4],
on top of a recent kernel?

Sorry for not having been clear about this.

I believe that patch [1/4] will re-introduce a deadlock we've
already fixed two years ago, so the small diff above seeks to
replace it with a simpler approach that will hopefully avoid
the issue as well.

Thanks,

Lukas

Re: [PATCH 1/4] PCI/hotplug: Disable HPIE over reset

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3671 bytes --]

On Sat, 15 Mar 2025, Lukas Wunner wrote:

> On Thu, Mar 13, 2025 at 04:23:30PM +0200, Ilpo Järvinen wrote:
> > pciehp_reset_slot() disables PDCE (Presence Detect Changed Enable) and
> > DLLSCE (Data Link Layer State Changed Enable) for the duration of reset
> > and clears the related status bits PDC and DLLSC from the Slot Status
> > register after the reset to avoid hotplug incorrectly assuming the card
> > was removed.
> > 
> > However, hotplug shares interrupt with PME and BW notifications both of
> > which can make pciehp_isr() to run despite PDCE and DLLSCE bits being
> > off. pciehp_isr() then picks PDC or DLLSC bits from the Slot Status
> > register due to the events that occur during reset and caches them into
> > ->pending_events. Later, the IRQ thread in pciehp_ist() will process
> > the ->pending_events and will assume the Link went Down due to a card
> > change (in pciehp_handle_presence_or_link_change()).
> > 
> > Change pciehp_reset_slot() to also clear HPIE (Hot-Plug Interrupt
> > Enable) as pciehp_isr() will first check HPIE to see if the interrupt
> > is not for it. Then synchronize with the IRQ handling to ensure no
> > events are pending, before invoking the reset.
> 
> After dwelling on this for a while, I'm thinking that it may re-introduce
> the issue fixed by commit f5eff5591b8f ("PCI: pciehp: Fix AB-BA deadlock
> between reset_lock and device_lock"):
> 
> Looking at the second and third stack trace in its commit message,
> down_write(reset_lock) in pciehp_reset_slot() is basically equivalent
> to synchronize_irq() and we're holding device_lock() at that point,
> hindering progress of pciehp_ist().

This description was somewhat confusing but what I can see, now that you 
mentioned this, is that if pciehp_reset_slot() calls synchronize_irq(), it 
can result in trying to acquire device_lock() again while trying to drain 
the pending events. ->reset_lock seems irrelevant to that problem.

Thus, pciehp_reset_slot() cannot ever rely on completing the processing of 
all pending events before it invokes the reset as long as any of its 
callers is holding device_lock().

It's a bit sad, because removing most of the reset_lock complexity would 
have been nice simplification in locking, effectively it would have 
reverted f5eff5591b8f too.

> So I think I have guided you in the wrong direction and I apologize
> for that.
> 
> However it seems to me that this should be solvable with the small
> patch below.  Am I missing something?
> 
> @Joel Mathew Thomas, could you give the below patch a spin and see
> if it helps?
> 
> Thanks!
> 
> -- >8 --
> 
> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index bb5a8d9f03ad..99a2ac13a3d1 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -688,6 +688,11 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
>  		return IRQ_HANDLED;
>  	}
>  
> +	/* Ignore events masked by pciehp_reset_slot(). */
> +	events &= ctrl->slot_ctrl;
> +	if (!events)
> +		return IRQ_HANDLED;
> +
>  	/* Save pending events for consumption by IRQ thread. */
>  	atomic_or(events, &ctrl->pending_events);
>  	return IRQ_WAKE_THREAD;

Yes, this should work, I think.

I'm not entirely sure though how reading ->slot_ctrl here synchronizes 
wrt. pciehp_reset_slot() invoking reset. What guarantees pciehp_isr() sees 
the updated ->slot_ctrl when pciehp_reset_slot() has proceeded to invoke 
the reset? (I'm in general very hesitant about lockless and barrierless 
reader being race free, I might be just paranoid about it.)

-- 
 i.