From patchwork Tue Aug 23 08:59:49 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Patel, Mayurkumar" X-Patchwork-Id: 9295279 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 2CAD560574 for ; Tue, 23 Aug 2016 09:03:36 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 1A4B0281A7 for ; Tue, 23 Aug 2016 09:03:36 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 0E2D1288B3; Tue, 23 Aug 2016 09:03:36 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3AB83281A7 for ; Tue, 23 Aug 2016 09:03:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753741AbcHWJDb convert rfc822-to-8bit (ORCPT ); Tue, 23 Aug 2016 05:03:31 -0400 Received: from mga11.intel.com ([192.55.52.93]:25286 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933337AbcHWJDR (ORCPT ); Tue, 23 Aug 2016 05:03:17 -0400 Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga102.fm.intel.com with ESMTP; 23 Aug 2016 01:59:51 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,565,1464678000"; d="scan'208";a="159718171" Received: from irsmsx152.ger.corp.intel.com ([163.33.192.66]) by fmsmga004.fm.intel.com with ESMTP; 23 Aug 2016 01:59:50 -0700 Received: from irsmsx101.ger.corp.intel.com ([169.254.1.183]) by IRSMSX152.ger.corp.intel.com ([169.254.6.43]) with mapi id 14.03.0248.002; Tue, 23 Aug 2016 09:59:49 +0100 From: "Patel, Mayurkumar" To: 'Bjorn Helgaas' CC: 'Rajat Jain' , "'bhelgaas@google.com'" , "'linux-pci@vger.kernel.org'" , "Wysocki, Rafael J" , "'mika.westerberg@linux.intel.com'" , "Busch, Keith" , "Tarazona-Duarte, Luis Antonio" , 'Rajat Jain' , 'Andy Shevchenko' Subject: [PATCH v1 2/2] PCI: pciehp: Rework hotplug interrupt routine Thread-Topic: [PATCH v1 2/2] PCI: pciehp: Rework hotplug interrupt routine Thread-Index: AdH9HLMdyFR09bFMS0SQ8NEg7WBnMQ== Date: Tue, 23 Aug 2016 08:59:49 +0000 Message-ID: <92EBB4272BF81E4089A7126EC1E7B28466598F35@IRSMSX101.ger.corp.intel.com> Accept-Language: de-DE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.182] MIME-Version: 1.0 Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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. Signed-off-by: Mayurkumar Patel --- Resending the patch. The patch is just one proposal. It is just prototype tested on HW on which I had PATCH 1 issue and currently I don't know it would work any HW. drivers/pci/hotplug/pciehp_hpc.c | 45 ++++++++++++++++---------------------- 1 file changed, 19 insertions(+), 26 deletions(-) -- 1.7.9.5 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index 5c24e93..2d01b7d 100644 --- a/drivers/pci/hotplug/pciehp_hpc.c +++ b/drivers/pci/hotplug/pciehp_hpc.c @@ -542,36 +542,30 @@ 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 slot_status, intr_loc = 0; u8 present; bool link; + 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; + } + intr_loc = (slot_status & (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD | + PCI_EXP_SLTSTA_PDC | + PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC)); + if (!intr_loc) + return IRQ_NONE; + /* - * In order to guarantee that all interrupt events are - * serviced, we need to re-inspect Slot Status register after - * clearing what is presumed to be the last pending interrupt. + * update link status before clearing interrupts to process + * it later */ - intr_loc = 0; - do { - pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &detected); - if (detected == (u16) ~0) { - ctrl_info(ctrl, "%s: no response from device\n", - __func__); - return IRQ_HANDLED; - } - - detected &= (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); + if (intr_loc & PCI_EXP_SLTSTA_DLLSC) + link = pciehp_check_link_active(ctrl); + pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, intr_loc); ctrl_dbg(ctrl, "pending interrupts %#06x from Slot Status\n", intr_loc); /* Check Command Complete Interrupt Pending */ @@ -603,7 +597,7 @@ 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); + present = !!(slot_status & PCI_EXP_SLTSTA_PDS); 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 +612,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 :