Re: [PATCH] PCI/PM: Set D3 power state only if the end device supports it.

[Alex Williamson]

On Fri, 2 Jun 2017 11:14:27 +0530
Geetha Akula <geethasowjanya.akula@xxxxxxxxx> wrote:

> On Wed, May 31, 2017 at 6:13 PM, Alex Williamson
> <alex.williamson@xxxxxxxxxx> wrote:
> > On Wed, 31 May 2017 17:03:32 +0530
> > Geetha sowjanya <gakula@xxxxxxxxxxxxxxxxxx> 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 : [<ffff0000085a5a60>] lr : [<ffff0000085a5a08>]
> 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] [<ffff0000085a5a60>] pci_generic_config_read+0xa0/0xf0
> [74259.866434] [<ffff0000085a583c>] pci_bus_read_config_word+0xb4/0xd8
> [74259.872688] [<ffff0000085ad048>] pci_raw_set_power_state+0x108/0x250
> [74259.879028] [<ffff0000085b02ec>] pci_set_power_state+0xc4/0x160
> [74259.884939] [<ffff000000dc0e88>] vfio_pci_probe+0xf0/0x1c8 [vfio_pci]
> [74259.891367] [<ffff0000085b42f4>] local_pci_probe+0x44/0xb0
> [74259.896839] [<ffff0000085b50c0>] pci_device_probe+0x140/0x170
> [74259.902574] [<ffff0000086f0dac>] driver_probe_device+0x2bc/0x450
> [74259.908568] [<ffff0000086f1064>] __driver_attach+0x124/0x128
> [74259.914214] [<ffff0000086ee610>] bus_for_each_dev+0x88/0xe8
> [74259.919773] [<ffff0000086f0500>] driver_attach+0x30/0x40
> [74259.925072] [<ffff0000085b3af0>] pci_add_dynid+0xa8/0xd0
> [74259.930370] [<ffff0000085b472c>] store_new_id+0x144/0x198
> [74259.935756] [<ffff0000086ee0a8>] drv_attr_store+0x40/0x58
> [74259.941143] [<ffff000008310ea4>] sysfs_kf_write+0x5c/0x78
> [74259.946528] [<ffff000008310094>] kernfs_fop_write+0xbc/0x1e8
> [74259.952176] [<ffff0000082877d8>] __vfs_write+0x60/0x168
> [74259.957387] [<ffff000008288b20>] vfs_write+0xa8/0x1b8
> [74259.962426] [<ffff000008289cd4>] SyS_write+0x6c/0xd8
> [74259.967378] [<ffff0000080833b0>] 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 <gakula@xxxxxxxxxxxxxxxxxx>
> >> ---
> >>  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);  
> >