| Message ID | 20250313142333.5792-4-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI/hotplug: Fix interrupt / event handling problems | expand |
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
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. :-)
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:
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(-)