From patchwork Sat Feb 11 02:39:16 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yinghai Lu X-Patchwork-Id: 726805 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3vKwzw0QRxz9s78 for ; Sat, 11 Feb 2017 13:41:16 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="rGzqNBlQ"; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753195AbdBKClO (ORCPT ); Fri, 10 Feb 2017 21:41:14 -0500 Received: from mail-ua0-f196.google.com ([209.85.217.196]:33839 "EHLO mail-ua0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753149AbdBKClN (ORCPT ); Fri, 10 Feb 2017 21:41:13 -0500 Received: by mail-ua0-f196.google.com with SMTP id i68so4357531uad.1; Fri, 10 Feb 2017 18:39:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=S7z5lPvhJiXR23B4sjea3emCW3t4aWyMXXUGh2J1bj4=; b=rGzqNBlQMw1Kaja1AkG32VfigednQ8/o4T868++UXTpclDjaCJK2wPdG27WcGrTu79 W55PC50+xExlFyPjk1C9mg7nszzc7mJzqel2HSnHs1OYec1XvCb7aPj2lKS0NUz2FS+1 GqIu1pEIjddW+723KI1T5+VaIAIKESrVpGoAolYVzj+AsomSBPmobz/xIs8HHsIaFdPa 0wwmmjMZceM2b+Mu1oeFNlUDygozaV/4ZH8sXGlWmEQj59PCYOIhrCC9x8aR0P0uxvjU o5GFPVfR4D1u3CmoSpT1hitwS7ox7e3kh7eALJE22DgTBcL8Ab/1I0R1eorQCgOgyBzF 5T+g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=S7z5lPvhJiXR23B4sjea3emCW3t4aWyMXXUGh2J1bj4=; b=qVqfB1kNB0VSlerKgZgy3Xco7X5h5qybTzYzlWxwzyqhPL4ManJf+py+zVdg2jFQWU evImRaZqpJtgGxRKGcxwTXwFVAHFkdOSo2MzAj6t9FBb0MxVMylZw47fy1M7/TZN8VOD RYoC85oSEBeRlpWqSZFfqoDoEHBuFhgCcJlbNt36sC4G+Oj/rz6vR+e3N/nisAxLpu9M kGAeYfYfdup7nla3pZDsCtpinmS7fJinaV1fvmH/ilvb4v7VG/OMuEhekJeV1l7DyVUq XrVHZ4FzME8CGadBzFrdJ3/HZr2dBqZQxpxYL6jVWaJvPKysdQCAGaC0JyZjcdL1gw7W WPDA== X-Gm-Message-State: AMke39k1On08Xk8wE/GHOsF1NJsemn6viVXTgCQekVa3V/mHuVOkZju8Fr9AadJHvI6TGF9zPhBgO0hcCp6E9Q== X-Received: by 10.159.35.137 with SMTP id 9mr6533268uao.110.1486780756775; Fri, 10 Feb 2017 18:39:16 -0800 (PST) MIME-Version: 1.0 Received: by 10.103.37.199 with HTTP; Fri, 10 Feb 2017 18:39:16 -0800 (PST) In-Reply-To: <20170209201154.GA22458@bhelgaas-glaptop.roam.corp.google.com> References: <20170208192054.GA31395@bhelgaas-glaptop.roam.corp.google.com> <20170208192256.GB31395@bhelgaas-glaptop.roam.corp.google.com> <20170209040648.GA1304@wunner.de> <20170209150950.GA11905@bhelgaas-glaptop.roam.corp.google.com> <20170209201154.GA22458@bhelgaas-glaptop.roam.corp.google.com> From: Yinghai Lu Date: Fri, 10 Feb 2017 18:39:16 -0800 X-Google-Sender-Auth: ALwwCFSfq4WaA236yGZf5a4AFmk Message-ID: Subject: Re: [GIT PULL] PCI fixes for v4.10 To: Bjorn Helgaas Cc: Lukas Wunner , Linus Torvalds , "linux-pci@vger.kernel.org" , Linux Kernel Mailing List , Bart Van Assche , Christoph Hellwig , "Rafael J. Wysocki" , Mika Westerberg , Ashok Raj , Keith Busch Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org On Thu, Feb 9, 2017 at 12:11 PM, Bjorn Helgaas wrote: > On Thu, Feb 09, 2017 at 09:09:50AM -0600, Bjorn Helgaas wrote: >> [+cc Ashok, Keith] >> >> On Thu, Feb 09, 2017 at 05:06:48AM +0100, Lukas Wunner wrote: >> > On Wed, Feb 08, 2017 at 01:22:56PM -0600, Bjorn Helgaas wrote: >> > > Bjorn Helgaas (1): >> > > Revert "PCI: pciehp: Add runtime PM support for PCIe hotplug ports" >> > >> > What's the rationale for reverting this? >> > >> > You've received patches to fix the issue on both affected machines, >> > so a revert seems unnecessary: >> > >> > https://patchwork.kernel.org/patch/9557113/ >> > https://patchwork.kernel.org/patch/9562007/ >> >> I don't think we've gotten to the root cause of the problem yet, >> and I don't want to throw in fixes at the last minute without a better >> understanding of it. >> >> PCIe hotplug hardware is not very complicated, it hasn't changed in >> many years, and at least for the Intel hardware in question, is >> generally pretty well-tested with Windows. So I want to be careful >> about asserting that this new piece of hardware is broken. > > I apologize: I had quirks on the brain, but neither of the patches > above is device-specific. So neither is claiming broken hardware. > > However, 9557113 claims we get unwanted PME interrupts if the slot is > occupied when we suspend to D3hot. This is what I want to explore > further, because that hardware behavior doesn't really make sense to > me. > > 9562007 apparently fixes something, but at this point it's a debugging > patch (no changelog or signed-off-by) so not a candidate for tossing > into v4.10 at this late date. Agreed. It should need more test coverage. Found more problems. Actually we don't need 9557113. as even with that, we still saw link up when power off slots with some cards. please check updated version of 9562007, that fix power on/off link up problem. Ashok, Can ask your QA guys check only attached patch and commit 68db9bc ? Thanks Yinghai Subject:[PATCH v2] PCI, pciechp: Only power on/off slots when it is D0 Found power on via /sys has problem. sca05-0a81fd7f:~ # echo 1 > /sys/bus/pci/slots/7/power [ 300.949937] pci_hotplug: power_write_file: power = 1 [ 300.955502] pciehp 0000:73:00.0:pcie004: pciehp_get_power_status: SLOTCTRL a8 value read 17f1 [ 300.982557] pciehp 0000:73:00.0:pcie004: pending interrupts 0x0010 from Slot Status [ 300.991171] pciehp 0000:73:00.0:pcie004: pciehp_power_on_slot: SLOTCTRL a8 write cmd 0 [ 301.000033] pciehp 0000:73:00.0:pcie004: pciehp_green_led_blink: SLOTCTRL a8 write cmd 200 [ 301.009274] pciehp 0000:73:00.0:pcie004: pending interrupts 0x0010 from Slot Status [ 301.662172] pciehp 0000:73:00.0:pcie004: pciehp_check_link_active: lnk_status = f083 [ 301.670827] pciehp 0000:73:00.0:pcie004: pending interrupts 0x0108 from Slot Status [ 301.679376] pciehp 0000:73:00.0:pcie004: Slot(7): Link Up [ 301.685463] pciehp 0000:73:00.0:pcie004: Slot(7): Link Up event ignored; already powering on [ 301.685508] pciehp 0000:73:00.0:pcie004: pciehp_check_link_active: lnk_status = f083 [ 302.005967] pciehp 0000:73:00.0:pcie004: pciehp_check_link_status: lnk_status = f083 [ 302.014859] pci 0000:74:00.0: [15b3:1003] type 00 class 0x0c0600 also find other slot with other card still have extra link up problem on power off even has can_wake patch. sca05-0a81fd7f:~ # echo 0 > /sys/bus/pci/slots/1/power [ 6116.873632] pci_hotplug: power_write_file: power = 0 [ 6116.879198] pciehp 0000:16:00.0:pcie004: pciehp_get_power_status: SLOTCTRL a8 value read 11f1 [ 6116.888730] pciehp 0000:16:00.0:pcie004: pciehp_unconfigure_device: domain:bus:dev = 0000:17:00 [ 6116.898464] pci 0000:17:00.0: PME# disabled [ 6116.903541] pci 0000:17:00.0: freeing pci_dev info [ 6116.909662] pciehp 0000:16:00.0:pcie004: pending interrupts 0x0010 from Slot Status [ 6116.918277] pciehp 0000:16:00.0:pcie004: pciehp_power_off_slot: SLOTCTRL a8 write cmd 400 [ 6116.982048] pciehp 0000:16:00.0:pcie004: pending interrupts 0x0108 from Slot Status [ 6116.990608] pciehp 0000:16:00.0:pcie004: Slot(1): Link Down [ 6116.996876] pciehp 0000:16:00.0:pcie004: Slot(1): Link Down event ignored; already powering off [ 6117.961521] pciehp 0000:16:00.0:pcie004: pciehp_green_led_off: SLOTCTRL a8 write cmd 300 [ 6117.970575] pciehp 0000:16:00.0:pcie004: pending interrupts 0x0018 from Slot Status [ 6117.970581] pciehp 0000:16:00.0:pcie004: Slot(1): Card present [ 6117.985660] pciehp 0000:16:00.0:pcie004: pciehp_get_power_status: SLOTCTRL a8 value read 17f1 [ 6117.995825] pciehp 0000:16:00.0:pcie004: pending interrupts 0x0010 from Slot Status [ 6118.005489] pciehp 0000:16:00.0:pcie004: pciehp_power_on_slot: SLOTCTRL a8 write cmd 0 [ 6118.014628] pciehp 0000:16:00.0:pcie004: pciehp_green_led_blink: SLOTCTRL a8 write cmd 200 [ 6118.023880] pciehp 0000:16:00.0:pcie004: pending interrupts 0x0010 from Slot Status [ 6118.602855] pciehp 0000:16:00.0:pcie004: pciehp_check_link_active: lnk_status = f103 [ 6118.611507] pciehp 0000:16:00.0:pcie004: pending interrupts 0x0108 from Slot Status [ 6118.620057] pciehp 0000:16:00.0:pcie004: Slot(1): Link Up [ 6118.626151] pciehp 0000:16:00.0:pcie004: pciehp_check_link_active: lnk_status = f103 [ 6118.634828] pciehp 0000:16:00.0:pcie004: Slot(1): Link Up event ignored; already powering on [ 6118.741520] pciehp 0000:16:00.0:pcie004: pciehp_check_link_status: lnk_status = f103 [ 6118.750201] pci 0000:17:00.0: [108e:2088] type 00 class 0x020700 ... That mean commit 68db9bc assumpation about power on/off on D3 is not right. - The configuration space of the port remains accessible in D3hot, so all the functions to read or modify the Slot Status and Slot Control registers need not be modified. Even turning on slot power doesn't seem to require the port to be in D0, at least the PCIe spec doesn't say so and I confirmed that by testing with a Thunderbolt controller. This patch put back D0 when trying to power on/off the slots. Signed-off-by: Yinghai Lu --- drivers/pci/hotplug/pciehp_ctrl.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) Index: linux-2.6/drivers/pci/hotplug/pciehp_ctrl.c =================================================================== --- linux-2.6.orig/drivers/pci/hotplug/pciehp_ctrl.c +++ linux-2.6/drivers/pci/hotplug/pciehp_ctrl.c @@ -89,17 +89,17 @@ static int board_added(struct slot *p_sl struct controller *ctrl = p_slot->ctrl; struct pci_bus *parent = ctrl->pcie->port->subordinate; + pm_runtime_get_sync(&ctrl->pcie->port->dev); if (POWER_CTRL(ctrl)) { /* Power on slot */ retval = pciehp_power_on_slot(p_slot); if (retval) - return retval; + goto err_exit; } pciehp_green_led_blink(p_slot); /* Check link training status */ - pm_runtime_get_sync(&ctrl->pcie->port->dev); retval = pciehp_check_link_status(ctrl); if (retval) { ctrl_err(ctrl, "Failed to check link status\n"); @@ -143,9 +143,10 @@ static int remove_board(struct slot *p_s pm_runtime_get_sync(&ctrl->pcie->port->dev); retval = pciehp_unconfigure_device(p_slot); - pm_runtime_put(&ctrl->pcie->port->dev); - if (retval) + if (retval) { + pm_runtime_put(&ctrl->pcie->port->dev); return retval; + } if (POWER_CTRL(ctrl)) { pciehp_power_off_slot(p_slot); @@ -157,6 +158,7 @@ static int remove_board(struct slot *p_s */ msleep(1000); } + pm_runtime_put(&ctrl->pcie->port->dev); /* turn off Green LED */ pciehp_green_led_off(p_slot);