From patchwork Thu Mar 20 06:47:12 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yinghai Lu X-Patchwork-Id: 3863681 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: X-Original-To: patchwork-linux-pci@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 6397CBF540 for ; Thu, 20 Mar 2014 06:47:17 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 5D6E9201FD for ; Thu, 20 Mar 2014 06:47:16 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 5F647201EF for ; Thu, 20 Mar 2014 06:47:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751463AbaCTGrO (ORCPT ); Thu, 20 Mar 2014 02:47:14 -0400 Received: from mail-ie0-f173.google.com ([209.85.223.173]:49320 "EHLO mail-ie0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751200AbaCTGrN (ORCPT ); Thu, 20 Mar 2014 02:47:13 -0400 Received: by mail-ie0-f173.google.com with SMTP id rl12so426677iec.4 for ; Wed, 19 Mar 2014 23:47:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=IsRdipEozDNdPtqhjg8MFFTXppOQyXCOnh5QicE7UrE=; b=zuu8ABvU0rf+ggFjHOgNQMo90EgJLluTj+sNXCes/Z5qSoWikbk9QSF7r2r1GYJckW 41oZl4LuuM1qbXaA8+6Xq+6w9yPC8h/l43O4nVlufQs8P7JsixwCX/zNzHLHJDv5q5kw xq+f8Pb0DAVVltpiBZFkIjsHinAvwRnbZ1Z3Z7LgcdUN5O73pdjxckK0V9N3DLj2lMAp K33nPKh21jsqiYU6v/yyJevIu5QQS2Zvu/2cTIQyOszQTQS+SrOpn520Z1uyJ5wzeOZ+ GTPaR9Bdq9p7J7TJzEqOZhUB/ooWTdqMoKSpgvkQiHKToXa19/FcguihvrwlPKH5ywIb DP3A== MIME-Version: 1.0 X-Received: by 10.50.154.66 with SMTP id vm2mr29525564igb.4.1395298033079; Wed, 19 Mar 2014 23:47:13 -0700 (PDT) Received: by 10.64.168.72 with HTTP; Wed, 19 Mar 2014 23:47:12 -0700 (PDT) In-Reply-To: References: <1393286367-16482-1-git-send-email-yinghai@kernel.org> Date: Wed, 19 Mar 2014 23:47:12 -0700 X-Google-Sender-Auth: TBGtIcdytpERZ1m1uqCZkivfTDE Message-ID: Subject: Re: [PATCH] pciehp: only wait command complete for really hotplug control From: Yinghai Lu To: Bjorn Helgaas Cc: "linux-pci@vger.kernel.org" , Rajat Jain Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID,T_RP_MATCHES_RCVD,T_TVD_MIME_EPI, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, Mar 19, 2014 at 8:16 PM, Yinghai Lu wrote: > On Wed, Mar 19, 2014 at 3:01 PM, Bjorn Helgaas wrote: >> On Wed, Mar 19, 2014 at 12:18 PM, Yinghai Lu wrote: >> >> I think the current pciehp design is a bit broken. Today we perform >> commands very synchronously: >> >> pcie_write_cmd() { >> write Slot Control >> if (Command Complete supported) { >> wait for Command Complete >> } >> } >> >> The typical driver pattern would be more asynchronous, like this: >> >> pcie_write_cmd() { >> read Slot Status >> if (!Command Completed) { >> wait for Command Complete >> } >> write Slot Control >> } >> >> With the asynchronous pattern, we would probably never wait at all >> unless we performed two commands in immediate succession. I wouldn't >> be surprised if doing this asynchronously would effectively cover up >> these hardware differences. > > I like this idea. Will give it a try. It works. Please check the patch that check CC before wait/write command. Subject: [PATCH v2] pciehp: only wait command complete for real hotplug control On system with 16 PCI express hotplug slots, customer complain every slot will report "Command not completed in 1000 msec" during initialization. Intel says that we should only wait command complete only for Electromechanical Interlock Control Power Controller Control Power Indicator Control Attention Indicator Control System with AMD/Nvidia chipset have same problem. Two ways to address the problem: 1. change to only wait when (EIC, PCC, PIC, AIC) bits get changed. 2. or check CMD_COMPLETE and wait before write command. Only wait when CC is set and cmd_busy is true. For chipset from intel will only have CC set for real hotplug control, so we could skip the wait for others. This patch is using second way. With that we could save 16 seconds during booting, later with 32 sockets system with 64 pcie hotplug slots we could save 64 seconds. -v2: use second way that is suggested by Bjorn. Signed-off-by: Yinghai Lu --- drivers/pci/hotplug/pciehp_hpc.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) Subject: [PATCH v2] pciehp: only wait command complete for real hotplug control On system with 16 PCI express hotplug slots, customer complain every slot will report "Command not completed in 1000 msec" during initialization. Intel says that we should only wait command complete only for Electromechanical Interlock Control Power Controller Control Power Indicator Control Attention Indicator Control System with AMD/Nvidia chipset have same problem. Two ways to address the problem: 1. change to only wait when (EIC, PCC, PIC, AIC) bits get changed. 2. or check CMD_COMPLETE and wait before write command. Only wait when CC is set and cmd_busy is true. For chipset from intel will only have CC set for real hotplug control, so we could skip the wait for others. This patch is using second way. With that we could save 16 seconds during booting, later with 32 sockets system with 64 pcie hotplug slots we could save 64 seconds. -v2: use second way that is suggested by Bjorn. Signed-off-by: Yinghai Lu --- drivers/pci/hotplug/pciehp_hpc.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) Index: linux-2.6/drivers/pci/hotplug/pciehp_hpc.c =================================================================== --- linux-2.6.orig/drivers/pci/hotplug/pciehp_hpc.c +++ linux-2.6/drivers/pci/hotplug/pciehp_hpc.c @@ -158,15 +158,8 @@ static void pcie_write_cmd(struct contro mutex_lock(&ctrl->ctrl_lock); pcie_capability_read_word(pdev, PCI_EXP_SLTSTA, &slot_status); - if (slot_status & PCI_EXP_SLTSTA_CC) { - if (!ctrl->no_cmd_complete) { - /* - * After 1 sec and CMD_COMPLETED still not set, just - * proceed forward to issue the next command according - * to spec. Just print out the error message. - */ - ctrl_dbg(ctrl, "CMD_COMPLETED not clear after 1 sec\n"); - } else if (!NO_CMD_CMPL(ctrl)) { + if (ctrl->no_cmd_complete && slot_status & PCI_EXP_SLTSTA_CC) { + if (!NO_CMD_CMPL(ctrl)) { /* * This controller seems to notify of command completed * event even though it supports none of power @@ -182,16 +175,11 @@ static void pcie_write_cmd(struct contro } pcie_capability_read_word(pdev, PCI_EXP_SLTCTL, &slot_ctrl); - slot_ctrl &= ~mask; - slot_ctrl |= (cmd & mask); - ctrl->cmd_busy = 1; - smp_mb(); - pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, slot_ctrl); - /* * Wait for command completion. */ - if (!ctrl->no_cmd_complete) { + if (!ctrl->no_cmd_complete && (slot_status & PCI_EXP_SLTSTA_CC) && + ctrl->cmd_busy) { int poll = 0; /* * if hotplug interrupt is not enabled or command @@ -203,6 +191,12 @@ static void pcie_write_cmd(struct contro poll = 1; pcie_wait_cmd(ctrl, poll); } + + slot_ctrl &= ~mask; + slot_ctrl |= (cmd & mask); + ctrl->cmd_busy = 1; + smp_mb(); + pcie_capability_write_word(pdev, PCI_EXP_SLTCTL, slot_ctrl); mutex_unlock(&ctrl->ctrl_lock); }