Re: [PATCH v1 2/2] PCI: pciehp: Rework hotplug interrupt routine

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Aug 23, 2016 at 08:59:49AM +0000, Patel, Mayurkumar wrote:
> First scenario, on any slot events, pcie_isr() does as following,
> pcie_isr() -> do {...} while(detected) loop in which it
> reads PCI_EXP_SLTSTA, stores it in the intr_loc, then
> clears respective interrupts by writing to PCI_EXP_SLTSTA.
> Again, due to loop, it reads PCI_EXP_SLTSTA, which might
> have been changed already for the same type of interrupts
> because in the previous iteration they already got cleared.
> In this case, it will execute pciehp_queue_interrupt_event() only once
> based on the last event happened. This can be problematic
> for PCI_EXP_SLTSTA_PDC and PCI_EXP_SLTSTA_DLLSC types of
> interrupts as if they miss to process previous events then PCIe device
> enumeration can get effected.
> 
> Second scenario, pcie_isr() after clearing interrupts, it calls
> pciehp_get_adapter_status() before processing PCI_EXP_SLTSTA_PDS
> and pciehp_check_link_active() before processing PCI_EXP_SLTSTA_DLLSC
> and takes decisions based on that to do pciehp_queue_interrupt_event()
> which might also have already got changed due to the same
> fact that the respective interrupts got cleared earlier.
> 
> The patch removes re-inspection to avoid first scenario happening
> and just reads the events once and clears them as soon as possible.
> To successfully execute right Slot events for PDC and DLLSC types which
> triggered pcie_isr() it reads the PCI_EXP_SLTSTA_PDS and
> PCI_EXP_LNKSTA_DLLLA earlier before clearing the respective interrupts
> and executes pciehp_queue_interrupt_event() based on the stored
> status of these two Slot events.

I think this is great.  I split it into two patches, one to deal with
the loop and a second to stop re-reading PCI_EXP_SLTSTA.

I propose that we keep the loop, but restructure it a little.  If we
remove the loop completely, I worry that we may be able to miss an
interrupt.  I'm not confident that there is a hole, so maybe we
*could* remove the loop competely; I just couldn't convince myself
that it was safe both for INTx and MSI/MSI-X signaling.

I also added a few trivial patches for cleanup in the area.  I'll post
the whole set as a v2 for your comments.

Bjorn