Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751359AbdFEWAv (ORCPT ); Mon, 5 Jun 2017 18:00:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56802 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751203AbdFEWAt (ORCPT ); Mon, 5 Jun 2017 18:00:49 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com E1BA63D956 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=alex.williamson@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com E1BA63D956 Date: Mon, 5 Jun 2017 16:00:47 -0600 From: Alex Williamson To: Geetha Akula Cc: Geetha sowjanya , bhelgaas@google.com, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, jcm@redhat.com, Sunil Goutham , Robert Richter , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] PCI/PM: Set D3 power state only if the end device supports it. Message-ID: <20170605160047.2d000cce@t450s.home> In-Reply-To: References: <1496230412-19028-1-git-send-email-gakula@caviumnetworks.com> <20170531064340.0dcec477@w520.home> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Mon, 05 Jun 2017 22:00:49 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5723 Lines: 124 On Fri, 2 Jun 2017 11:14:27 +0530 Geetha Akula wrote: > On Wed, May 31, 2017 at 6:13 PM, Alex Williamson > wrote: > > On Wed, 31 May 2017 17:03:32 +0530 > > Geetha sowjanya wrote: > > > >> Pci driver doesn't check if the device supports D3hot/D3cold power states > >> while setting these power states. The device that doesn't support these > >> states will fail when a driver like vfio try to do D0->D3 power transition. > > > > > > That's because support for D0 and D3 device states is REQUIRED by the > > PCIe spec (Rev 3.1a, 7.6). Is this yet more non-spec compliance? > > Thanks, > > Hi Alex, > > The issue is seen with the LSI sata cards SAS3008 SAS-3 and SAS-2 > (vendor id: 1000 device id: 0087/0097). Both cards only support D1, D2 > power state. This device fails when vfio driver tries to set the power > state to D3hot. The issue is seen across platforms. > > Do you recommend to implement quirk for these devices? As I understand the original proposal, the test for whether a device supports D3hot is whether the device can issue a PME# from the D3hot state. Even on the laptop I'm using right now I see a number of devices that report that they cannot issue a PME# from D3hot. That's really an orthogonal capability to D3hot support. Disabling D3hot for those devices is likely to have negative side effects with power consumption. We already have quirks to avoid D3 by setting PCI_DEV_FLAGS_NO_D3, I'd suggest we use it. Thanks, Alex > Kernel crash when LSI card attached to vfio driver. > > [74259.180968] Unhandled fault: synchronous external abort > (0x96000610) at 0x0000000033a80008 > [74259.189223] Internal error: : 96000610 [#1] SMP > [74259.193740] Modules linked in: vfio_pci irqbypass vfio_virqfd > vfio_iommu_type1 vfio > [74259.201389] CPU: 104 PID: 2714 Comm: bash Not tainted 4.11.0+ #1 > [74259.207382] Hardware name: www.cavium.com Cavium CN99XX/CN99XX, > BIOS 00:03:23 May 30 2017 > [74259.215545] task: ffff800ec6680000 task.stack: ffff800ec0e00000 > [74259.221454] PC is at pci_generic_config_read+0xa0/0xf0 > [74259.226579] LR is at pci_generic_config_read+0x48/0xf0 > [74259.231704] pc : [] lr : [] > pstate: 604001c9 > [74259.239084] sp : ffff800ec0e03900 > [74259.242386] x29: ffff800ec0e03900 x28: ffff800ec6680000 > [74259.247685] x27: 0000000000001000 x26: 0000000000000097 > [74259.252984] x25: 0000000000000140 x24: ffff000009466000 > [74259.258283] x23: 0000000000000054 x22: 0000000000000000 > [74259.263581] x21: ffff800ec0e03994 x20: 0000000000000002 > [74259.268880] x19: ffff800f4c913800 x18: ffff0000091d3c10 > [74259.274179] x17: 0000fffee43373d0 x16: ffff000008289c68 > [74259.279477] x15: ffff800ec56b991c x14: 0000000000000000 > [74259.284776] x13: 0000000000000040 x12: 0000000000000000 > [74259.290074] x11: 0000000000000220 x10: 00000000000009b0 > [74259.295373] x9 : ffff800ec0e03800 x8 : ffff800ec6680a10 > [74259.300671] x7 : 0000000000000002 x6 : 0000000000000000 > [74259.305970] x5 : 0000000000000013 x4 : ffff800f4c920500 > [74259.311269] x3 : 0000000001300054 x2 : 0000000000000014 > [74259.316567] x1 : ffff000010000000 x0 : ffff000011300054 > [74259.321866] [.......] > > [74259.860266] [] pci_generic_config_read+0xa0/0xf0 > [74259.866434] [] pci_bus_read_config_word+0xb4/0xd8 > [74259.872688] [] pci_raw_set_power_state+0x108/0x250 > [74259.879028] [] pci_set_power_state+0xc4/0x160 > [74259.884939] [] vfio_pci_probe+0xf0/0x1c8 [vfio_pci] > [74259.891367] [] local_pci_probe+0x44/0xb0 > [74259.896839] [] pci_device_probe+0x140/0x170 > [74259.902574] [] driver_probe_device+0x2bc/0x450 > [74259.908568] [] __driver_attach+0x124/0x128 > [74259.914214] [] bus_for_each_dev+0x88/0xe8 > [74259.919773] [] driver_attach+0x30/0x40 > [74259.925072] [] pci_add_dynid+0xa8/0xd0 > [74259.930370] [] store_new_id+0x144/0x198 > [74259.935756] [] drv_attr_store+0x40/0x58 > [74259.941143] [] sysfs_kf_write+0x5c/0x78 > [74259.946528] [] kernfs_fop_write+0xbc/0x1e8 > [74259.952176] [] __vfs_write+0x60/0x168 > [74259.957387] [] vfs_write+0xa8/0x1b8 > [74259.962426] [] SyS_write+0x6c/0xd8 > [74259.967378] [] el0_svc_naked+0x24/0x28 > [74259.972677] Code: a9425bf5 f9401bf7 a8c47bfd d65f03c0 (79400000) > [74259.978794] ---[ end trace e0cfdf3c1d94e7aa ]--- > > > Thank you, > Geetha. > > > > > Alex > > > > > >> This patch adds a check that allows to set D3 power state only > >> for the supported devices. > >> > >> Signed-off-by: Geetha sowjanya > >> --- > >> drivers/pci/pci.c | 3 ++- > >> 1 files changed, 2 insertions(+), 1 deletions(-) > >> > >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > >> index 563901c..cadd046 100644 > >> --- a/drivers/pci/pci.c > >> +++ b/drivers/pci/pci.c > >> @@ -661,7 +661,8 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state) > >> > >> /* check if this device supports the desired state */ > >> if ((state == PCI_D1 && !dev->d1_support) > >> - || (state == PCI_D2 && !dev->d2_support)) > >> + || (state == PCI_D2 && !dev->d2_support) > >> + || (state == PCI_D3hot && !pci_pme_capable(dev, state))) > >> return -EIO; > >> > >> pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr); > >