[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <whinfqhjgqbbvlnuzgbukklqi6gocjji4gakqyycovrse4qq6s@6xq5imqbzvux>
Date: Wed, 3 Sep 2025 18:51:14 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Siddharth Vadapalli <s-vadapalli@...com>
Cc: lpieralisi@...nel.org, kwilczynski@...nel.org, robh@...nel.org,
bhelgaas@...gle.com, helgaas@...nel.org, kishon@...nel.org, vigneshr@...com,
stable@...r.kernel.org, linux-pci@...r.kernel.org, linux-omap@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, srk@...com
Subject: Re: [PATCH v3] PCI: j721e: Fix programming sequence of "strap"
settings
On Tue, Sep 02, 2025 at 10:34:51AM GMT, Siddharth Vadapalli wrote:
> On Mon, Sep 01, 2025 at 08:15:24PM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Sep 01, 2025 at 04:50:02PM GMT, Siddharth Vadapalli wrote:
> > > On Mon, Sep 01, 2025 at 12:14:51PM +0530, Manivannan Sadhasivam wrote:
> > > > On Mon, Sep 01, 2025 at 11:51:33AM GMT, Siddharth Vadapalli wrote:
>
> [...]
>
> > > > >
> > > > > If I understand correctly, are you suggesting the following?
> > > > >
> > > > > j721e_pcie_probe()
> > > > > pm_runtime_set_active()
> > > > > pm_runtime_enable()
> > > > > ret = j721e_pcie_ctrl_init(pcie);
> > > > > /*
> > > > > * PCIe Controller should be powered off here, but is there
> > > > > * a way to ensure that it has been powered off?
> > > > > */
> > > > > => Program the strap settings and return to
> > > > > j721e_pcie_probe()
> > > > > /* Power on the PCIe Controller now */
> > > > > ret = pm_runtime_get_sync(dev);
> > > >
> > > > This pm_runtime_get_sync() should be part of j721e_pcie_ctrl_init() where you
> > > > do power off, program strap and power on.
> > > >
> > > > This should not be part of probe() as by that time, controller is already
> > > > powered on. So pm_runtime_set_active() and pm_runtime_enable() should be enough
> > > > to convey the state of the device to PM core.
> > >
> > > I have tried out the suggestion in the following manner:
> > >
> > > j721e_pcie_probe()
> > > ...
> > > pm_runtime_set_active(dev);
> > > pm_runtime_enable(dev);
> > > ret = j721e_pcie_ctrl_init(pcie);
> > > ... within j721e_pcie_ctrl_init()
> > > ret = pm_runtime_put_sync(dev);
> > > /* Program Strap Settings */
> > > ret = pm_runtime_get_sync(dev);
> > > ...
> > > ...
> > > exit probe
> > >
> > > Since a 'pm_runtime_get_sync()' hasn't yet been invoked prior to the
> > > section where 'pm_runtime_put_sync()' is invoked, it leads to a ref-count
> > > underflow error at runtime. Please let me know if I am missing
> > > something.
> > >
> >
> > Doh... At the start of probe(), device PM usage_count will be 0. So we cannot
> > decrement it without incrementing it.
> >
> > Could you try below sequence?
> >
> > probe()
> > ...
> > pm_runtime_set_active()
> > pm_runtime_enable()
> > j721e_pcie_ctrl_init()
> > ...
> > pm_runtime_get() /* Just increment usage_count */
> > pm_runtime_put_sync() /* ask PM core to turn off */
> > /* program strap setting */
> > pm_runtime_get_sync() /* ask PM core to turn on */
> > pm_runtime_put_noidle() /* balance the usage_count without
> > suspending the device */
> > ...
>
> The above sequence powers off the controller at the point in time that
> the strap settings are programmed. 'pm_runtime_get_sync()' is powering
> on the controller afterwards. However, the 'pm_runtime_put_noidle()'
> at the end is causing the controller to be powered off again leading to
> a crash. Removing 'pm_runtime_put_noidle()' results in a functional
> sequence.
>
> Please consider the existing sequence prior to this patch:
>
> probe()
> ...
> pm_runtime_enable()
> pm_runtime_get_sync() => usage_count is 1
> ...
> exit probe
>
> With the suggested sequence above, we have:
>
> probe()
> ...
> pm_runtime_set_active()
> pm_runtime_enable()
> j721e_pcie_ctrl_init()
> ...
> pm_runtime_get() => usage_count is 1
> pm_runtime_put_sync() => usage_count is 0
> /* Controller is powered off now */
> /* Strap settings are programmed */
> pm_runtime_get_sync() => usage_count is 1
> /* Controller is powered on now */
> pm_runtime_put_noidle() => usage_count is 0
> /* Controller is powered off in a while */
> ...
> /* Crash is observed aroung this point before probe finishes */
>
> Please let me know if the fix is to drop 'pm_runtime_put_noidle()'
> from the above sequence.
>
I thought put_noidle() will just reduce the refcount and not invoke the
idle/suspend callbacks, but I seem to be wrong here.
Anyway, I guess we have no option here other than to drop the
pm_runtime_put_noidle() call. This will keep refcount as 1 and will prevent the
parent (genpd) to not enter runtime suspend, but we have to live with it (this
was also the previous beahvior as well).
Btw, pm_runtime_set_active/enable change belongs to a separate patch.
- Mani
--
மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists