On Fri, Mar 11, 2016 at 06:20:39PM -0600, Bjorn Helgaas wrote:
> On Mon, Feb 29, 2016 at 02:56:03PM +0200, Mika Westerberg wrote:
> > Currently the PCI core does not do this automatically as it avoids changing
> > power state for bridges and PCIe ports. With recent hardware PCIe ports can
> > be moved to D3hot given that we take into account few restrictions:
> >
> > - Devices connected to the ports are effectively in D3cold once the root
> > port is moved to D3hot (the config space is not accessible anymore and
> > the link may be powered down).
> >
> > - The device needs to be able to transition to D3cold.
>
> I think this is talking about "devices below the PCIe port", right?
That's right. I'll fix it in the next version.
> > - If the device is capable of waking the system it needs to be able to
> > do so from D3cold (PME from D3cold).
>
> Again, "a device below the PCIe port"?
Right (will fix in the next version).
> > We assume all recent hardware (starting from 2015) is capable of doing this
> > but make it possible to add exceptions via entries in pcie_port_configs[].
>
> Since you have no exceptions yet, I'm not sure it's worth having the
> exception infrastructure. That infrastructure kind of obscures the
> meat of the patch, so if we really do need it right away, maybe it
> could be added in a new separate patch.
I don't think we need it right now. I'll drop it from the patch.
> > Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > ---
> > drivers/pci/pcie/portdrv_pci.c | 103 +++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 100 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c
> > index 6c6bb03392ea..fe3349685141 100644
> > --- a/drivers/pci/pcie/portdrv_pci.c
> > +++ b/drivers/pci/pcie/portdrv_pci.c
> > @@ -20,6 +20,7 @@
> >
> > #include "portdrv.h"
> > #include "aer/aerdrv.h"
> > +#include "../pci.h"
> >
> > /*
> > * Version Information
> > @@ -78,11 +79,105 @@ static int pcie_portdrv_restore_config(struct pci_dev *dev)
> > return 0;
> > }
> >
> > +enum pcie_port_type {
> > + PCIE_PORT_DEFAULT,
> > +};
> > +
> > +struct pcie_port_config {
> > + bool suspend_allowed;
> > +};
> > +
> > +static const struct pcie_port_config pcie_port_configs[] = {
> > + [PCIE_PORT_DEFAULT] = {
> > + .suspend_allowed = true,
> > + },
> > +};
> > +
> > #ifdef CONFIG_PM
> > +static const struct pcie_port_config *pcie_port_get_config(struct pci_dev *pdev)
> > +{
> > + const struct pci_device_id *id = pci_match_id(pdev->driver->id_table,
> > + pdev);
> > + return &pcie_port_configs[id->driver_data];
> > +}
> > +
> > +static int pcie_port_check_d3cold(struct pci_dev *pdev, void *data)
> > +{
> > + bool *d3cold_ok = data;
> > +
> > + if (pdev->no_d3cold || !pdev->d3cold_allowed)
> > + *d3cold_ok = false;
> > + if (device_may_wakeup(&pdev->dev) && !pci_pme_capable(pdev, PCI_D3cold))
> > + *d3cold_ok = false;
> > +
> > + return !*d3cold_ok;
> > +}
> > +
> > +static bool pcie_port_can_suspend(struct pci_dev *pdev)
> > +{
> > + bool d3cold_ok = true;
> > +
> > + /*
> > + * When the port is put to D3hot the devices behind the port are
> > + * effectively in D3cold as their config space cannot be accessed
> > + * anymore and the link may be powered down.
> > + *
> > + * We only allow the port to go to D3hot the devices:
>
> s/the devices/if the subordinate devices/
OK
> > + * - Are allowed to go to D3cold
> > + * - Can wake up from D3cold if they are wake capable
> > + */
> > + pci_walk_bus(pdev->subordinate, pcie_port_check_d3cold, &d3cold_ok);
> > + return d3cold_ok;
>
> What happens if the PCIe port initially can be put in D3hot, but we we
> later hot-add a device that would disqualify it?
>
> Oh, I think I see -- we walk the subordinate devices every time we
> would like to put the port in D3hot:
>
> pcie_port_suspend_noirq
> pcie_port_suspend_allowed
> dmi_get_date
> pcie_port_can_suspend
> pci_walk_bus
>
> I guess that should work, but it seems clunky to do all that work,
> even though it's not really a performance path. What if we did this:
>
> - add a d3hot_allowed bit in struct pci_dev
>
> - when enumerating a port, set d3hot_allowed if BIOS is new enough
> (it makes me a bit nervous that we apparently default to enabling
> D3hot on arches without DMI)
>
> - when enumerating devices, clear d3hot_allowed in upstream bridge
> if necessary
>
> - when removing last device on a bus, re-do port config (set
> d3hot_allowed if appropriate)
Sounds reasonable. I'll give it a try.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
