> >
> > > On Wed, Aug 17, 2016 at 10:37:13PM +0000, Patel, Mayurkumar wrote:
> > > > > On Wed, Aug 17, 2016 at 10:54:12AM -0700, Rajat Jain wrote:
> > >
> > > > > > We need to do something about that *in addition * to the
> > > > > > above patch to cover the
> > > > > > whole story. However I think there still will be a room for some
> > > > > > interrupt misses because we are
> > > > > > collecting the interrupts in intr_loc, and theoretically we could be
> > > > > > in a situation where in the pcie_isr, the
> > > > > >
> > > > > > do {
> > > > > > ...
> > > > > > } while(detected)
> > > > > >
> > > > > > loop gets a removal->insertion->removal all while in the same
> > > > > > invocation of pcie_isr().
> > > > > > If this happens, the intr_loc will have recorded a single insertion
> > > > > > and a single removal, and
> > > > > > the final result will depend on the order in which we decide to
> > > > > > process the events in intr_loc.
> > > > >
> > > > > I don't quite understand how that "do { .. } while (detected)" loop
> > > > > works or why it's done that way. Collecting interrupt status bits in
> > > > > an ISR is obviously a very common task; it seems like there should be
> > > > > a standard, idiomatic way of doing it, but I don't know it.
> > > > >
> > > > > > Or, may be we can make the calls to pciehp_queue_interrupt_event()
> > > > > > before clearing the
> > > > > > RW1C in the slot status register (in the loop)?
> > > > >
> > > > > Yeah, it seems like we should read PCI_EXP_SLTSTA once, queue up any
> > > > > events related to it, then clear the relevant SLTSTA bits.
> > > > >
> > > >
> > > > Do you mean to remove the do {...} while loop and just
> > > > read PCI_EXP_SLTSTA once in ISR , queue the work and clear interrupts?
> > >
> > > I don't know if removing the loop is the right thing or not. We need
> > > to understand why the loop is there in the first place and make sure
> > > removing it wouldn't break things.
> > >
> >
> >
> > The pcie base spec. 3.0 says from chapter 6.7.3.1. Slot Events:
> >
> > A Downstream Port with hot-plug capabilities monitors the slot it controls for the slot events listed
> > above. When one of these slot events is detected, the Port indicates that the event has occurred by
> > setting the status field associated with the event. At that point, the event is pending until software
> > clears the status field.
> > Once a slot event is pending on a particular slot, all subsequent events of that type are ignored on
> > that slot until the event is cleared. The Port must continue to monitor the slot for all other slot
> > event types and report them as they occur.
> >
> >
> > So does it mean that the port would continue providing MSI if there has been
> > any other events occurred apart from the event which is not cleared? If that
> > is the case then it's not sure why the loop is still needed.
>
> I'd think that the loop is needed because we don't want to handle just
> one event on one interrupt. We want to handle as many events as we can
> (to keep the interrupt latency overhead low), that have happened
> during the ISR invocation. And hence the loop.
>
Hmm, Right. I created a proposal patch
(=>[v1,2/2] PCI: pciehp: Rework hotplug interrupt routine)
to remove the do {...} while loop which
I guess then should be rejected, right? As we decide to keep this loop?
> >
> > Also with having a loop, pcie_isr() is reading the PCI_EXP_SLTSTA again and taking action just based on
> > latest event happens. That may mean removal->insertion happens fast enough then
> > removal could be overwritten by insertion and pciehp_get_adapter_status ()
> > will return insertion only and we may miss the removal event to be processed for the loop.
>
> Current. This is the problem that Bjorn highlighted above. I think the
> current code works well when events of different kinds have happened
> (due to the loop).
>
> In case of similar kind of events (insertion / removal), I think there
> are 2 problems: (1) we use the transient values of slot status which
> may not be valid at that time. (2) because we ack the event before we
> queue it up, it leaves chance for error. I think we'd need more
> careful examination in order to fix these issues.
>
I also see 3rd problem too with this Loop.
For example, if we are in the loop,
If only slot removal comes -> detect = PCI_EXP_SLTSTA_PDC -> intr_lock |= detected
-> detected==true so clear PDC interrupt bit => So we have a chance to detect
another insertion event.
We are in the same loop & if insertion comes again -> so detected = PCI_EXP_SLTSTA_PDC
-> then detected &= ~intr_loc -> detected == false => This means
PCI_EXP_SLTSTA would not be cleared for insertion event happened
and there we could get a problem that No further PDC events can get triggered by HW.
So if we want to keep the loop, I think clearing PCI_EXP_SLTSTA should be shifted
outside of the loop to clear Slot events(but not based on detected) to ensure that loop
just looks for the events Which has not occurred, also before clearing SLTSTA outside
the loop, we can update link and slot_status to trigger the right events for DLLSC and PDC types.
What do you think about following proposal?
@@ -542,7 +542,7 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
struct pci_bus *subordinate = pdev->subordinate;
struct pci_dev *dev;
struct slot *slot = ctrl->slot;
- u16 detected, intr_loc;
+ u16 detected, intr_loc, slot_status;
u8 present;
bool link;
@@ -553,26 +553,30 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
*/
intr_loc = 0;
do {
- pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &detected);
- if (detected == (u16) ~0) {
+ pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status);
+ if (slot_status == (u16) ~0) {
ctrl_info(ctrl, "%s: no response from device\n",
__func__);
return IRQ_HANDLED;
}
- detected &= (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD |
+ detected = slot_status & (PCI_EXP_SLTSTA_ABP |
+ PCI_EXP_SLTSTA_PFD |
PCI_EXP_SLTSTA_PDC |
PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC);
detected &= ~intr_loc;
intr_loc |= detected;
if (!intr_loc)
return IRQ_NONE;
- if (detected)
- pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
- intr_loc);
} while (detected);
ctrl_dbg(ctrl, "pending interrupts %#06x from Slot Status\n", intr_loc);
+ if (intr_loc & PCI_EXP_SLTSTA_PDC)
+ present = !!(slot_status & PCI_EXP_SLTSTA_PDS);
+ if (intr_loc & PCI_EXP_SLTSTA_DLLSC)
+ link = pciehp_check_link_active(ctrl);
+
+ pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, intr_loc);
/* Check Command Complete Interrupt Pending */
if (intr_loc & PCI_EXP_SLTSTA_CC) {
@@ -603,7 +607,6 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
/* Check Presence Detect Changed */
if (intr_loc & PCI_EXP_SLTSTA_PDC) {
- pciehp_get_adapter_status(slot, &present);
ctrl_info(ctrl, "Card %spresent on Slot(%s)\n",
present ? "" : "not ", slot_name(slot));
pciehp_queue_interrupt_event(slot, present ? INT_PRESENCE_ON :
@@ -618,7 +621,6 @@ static irqreturn_t pcie_isr(int irq, void *dev_id)
}
if (intr_loc & PCI_EXP_SLTSTA_DLLSC) {
- link = pciehp_check_link_active(ctrl);
ctrl_info(ctrl, "slot(%s): Link %s event\n",
slot_name(slot), link ? "Up" : "Down");
pciehp_queue_interrupt_event(slot, link ? INT_LINK_UP :
> >
> > If we don't clear the Slot event quick enough then according to spec. statement above
> > that event could get ignored and SW may never get notified.
> >
> >
> >
> > > But I do think that in the resulting code, the connection between
> > >
> > > (1) the events we learn from the interrupt and
> > > (2) the queued work items
> > >
> > > needs to be crystal clear. Right now it's a bit muddy because of
> > > things like the case you fixed: a work item that goes back and looks
> > > at PCI_EXP_SLTSTA after it's been cleared and the hardware may have
> > > already set bits for new events.
> > >
> >
> > I have made a prototype patch in my follow up reply and tested ok on my existing setup
> > on which I caught the previous issue although I cant say it will work on any HW.
> > Clearing events after queuing events gave some problems and did not work properly on my
> > existing HW where I test very fast insertion and removal events, in which
> > case only "removal" event comes and "insertion" does not occur even if HW gets powered.
> >
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
��.n��������+%������w��{.n�����{���"�)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
