Re: [PATCH RESEND] PCI: disable runtime PM for PLX switches

From: Bjorn Helgaas <helgaas@kernel.org>
To: Alexander Fomichev <fomichev.ru@gmail.com>
Cc: linux-pci@vger.kernel.org, linux@yadro.com,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	linux-pm@vger.kernel.org,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	Logan Gunthorpe <logang@deltatee.com>
Subject: Re: [PATCH RESEND] PCI: disable runtime PM for PLX switches
Date: Wed, 24 Apr 2019 09:11:48 -0500	[thread overview]
Message-ID: <20190424141148.GA244134@google.com> (raw)
In-Reply-To: <20190424100102.iyxogbsa4l7dyusb@yadro.com>

[+cc Mika for runtime PM of bridges, Logan for switchtec question]

On Wed, Apr 24, 2019 at 01:01:02PM +0300, Alexander Fomichev wrote:
> On Tue, Apr 23, 2019 at 04:53:40PM -0500, Bjorn Helgaas wrote:
> > On Mon, Apr 15, 2019 at 09:15:54AM -0500, Bjorn Helgaas wrote:
> > > On Mon, Apr 15, 2019 at 04:59:03PM +0300, Alexander Fomichev wrote:
> > > > PLX switches have an issue that their internal registers
> > > > become inaccessible when runtime PM is enabled. Therefore PLX
> > > > service tools can't communicate with the hardware. A kernel
> > > > option "pcie_port_pm=off" can be used as a workaround. But it
> > > > affects all the devices.
> > > >
> > > > So this solution is to add PLX switch devices to the quirk
> > > > list for disabling runtime PM only for them.
> > >
> > > I assume the problem is actually that the config space registers
> > > are inaccessible when the device is in D3hot?
> >
> > Reading this again, I realize you said "internal registers".  I
> > don't know whether that actually means config space registers
> > (which *should* work even when the device is in D3hot (see the
> > PCIe reference below and PCI Power Management Spec r1.2, sec
> > 5.4.1)), or MMIO registers (which are not expected to work while
> > in D3hot).
> > 
> > If the service tools read MMIO registers, presumably that goes
> > through some driver that should be able to manage runtime PM.  Or,
> > if there's no driver, I think your service tool could prevent
> > runtime power management by changing
> > /sys/devices/.../power/control to "on" (see
> > Documentation/power/runtime_pm.txt).
>
> You're right. Config space registers are accessible. The driver
> can't read/write MMIO registers (Device-Specific Registers as
> they're called by Broadcom).

Ah, perfect.  That's exactly what's supposed to happen from a PCI
hardware point of view.  Unfortunately I don't know much about how
Linux power management works, but Rafael and Mika do.

How do your service tools access these MMIO registers?

  - via a PLX driver that provides read/write/ioctl on a char dev?
  - read/write on /sys/devices/pci*/.../resource<x> ?
  - mmap on /sys/devices/pci*/.../resource<x> ?
  - read/write on /dev/mem?
  - mmap on /dev/mem?
  - something else?

I think there are several ways we might be able to fix this:

  - Write a driver along the lines of drivers/pci/switch/switchtec.c.
    I don't see any runtime PM stuff in that driver, so maybe it's
    magically taken care of by the PM/PCI core?  There might also be
    an issue if both portdrv and your driver want to claim the same
    device.  I don't know how switchtec deals with that.

  - Maybe the PCI sysfs accessors (pci_mmap_resource(), etc) should
    turn off runtime PM?  If we allow mmap of a BAR and then put the
    device in D3hot, that seems like a bug that could affect lots of
    things.  But maybe that's already done magically elsewhere?

Bjorn