Re: [PATCH v5] PCI: Enable runtime pm of the host bridge

[Mayank Rana]

Hi Bjorn

On 10/10/2024 12:25 PM, Bjorn Helgaas wrote:
> On Wed, Oct 09, 2024 at 08:10:32PM +0200, Marek Szyprowski wrote:
> > On 03.10.2024 08:02, Krishna chaitanya chundru wrote:
> > > The Controller driver is the parent device of the PCIe host bridge,
> > > PCI-PCI bridge and PCIe endpoint as shown below.
> > >
> > >           PCIe controller(Top level parent & parent of host bridge)
> > >                           |
> > >                           v
> > >           PCIe Host bridge(Parent of PCI-PCI bridge)
> > >                           |
> > >                           v
> > >           PCI-PCI bridge(Parent of endpoint driver)
> > >                           |
> > >                           v
> > >                   PCIe endpoint driver
> > >
> > > Now, when the controller device goes to runtime suspend, PM framework
> > > will check the runtime PM state of the child device (host bridge) and
> > > will find it to be disabled. So it will allow the parent (controller
> > > device) to go to runtime suspend. Only if the child device's state was
> > > 'active' it will prevent the parent to get suspended.
> > >
> > > It is a property of the runtime PM framework that it can only
> > > follow continuous dependency chains.  That is, if there is a device
> > > with runtime PM disabled in a dependency chain, runtime PM cannot be
> > > enabled for devices below it and above it in that chain both at the
> > > same time.
> > >
> > > Since runtime PM is disabled for host bridge, the state of the child
> > > devices under the host bridge is not taken into account by PM framework
> > > for the top level parent, PCIe controller. So PM framework, allows
> > > the controller driver to enter runtime PM irrespective of the state
> > > of the devices under the host bridge. And this causes the topology
> > > breakage and also possible PM issues like controller driver goes to
> > > runtime suspend while endpoint driver is doing some transfers.
> > >
> > > Because of the above, in order to enable runtime PM for a PCIe
> > > controller device, one needs to ensure that runtime PM is enabled for
> > > all devices in every dependency chain between it and any PCIe endpoint
> > > (as runtime PM is enabled for PCIe endpoints).
> > >
> > > This means that runtime PM needs to be enabled for the host bridge
> > > device, which is present in all of these dependency chains.
> > >
> > > After this change, the host bridge device will be runtime-suspended
> > > by the runtime PM framework automatically after suspending its last
> > > child and it will be runtime-resumed automatically before resuming its
> > > first child which will allow the runtime PM framework to track
> > > dependencies between the host bridge device and all of its
> > > descendants.
> > >
> > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@xxxxxxxxxxx>
> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> >
> > This patch landed in today's linux-next as commit 02787a3b4d10 ("PCI/PM:
> > Enable runtime power management for host bridges"). In my tests I found
> > that it triggers a warning on StarFive VisionFive2 RISC-V board. It
> > looks that some more changes are needed in the dwc-pci driver or so.
> > There is a message from runtime pm subsystem about inactive device with
> > active children and suspicious locking pattern. Here is the log I
> > observed on that board:
> > ...
>
> Thanks very much for the testing and report, Marek!
>
> I dropped this patch from the PCI -next for now.  We can add it back
> with the fix squashed into it after the complete patch is posted and
> tested.

I just sent fix for above issue as
https://patchwork.kernel.org/project/linux-pci/patch/20241010202950.3263899-1-quic_mrana@xxxxxxxxxxx/

Can you please consider to include both changes if proposed fix looks 
good and if feasible ?

Regards,
Mayank

> Bjorn