Received-SPF: pass (google.com: domain of linux-kernel+bounces-64208-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 13 Feb 2024 14:06:48 -0600
From: Bjorn Helgaas <helgaas@kernel.org>
To: Raag Jadav <raag.jadav@intel.com>
Cc: bhelgaas@google.com, jarkko.nikula@linux.intel.com,
	mika.westerberg@linux.intel.com, andriy.shevchenko@linux.intel.com,
	stanislaw.gruszka@linux.intel.com, lukas@wunner.de,
	rafael@kernel.org, ilpo.jarvinen@linux.intel.com,
	linux-pci@vger.kernel.org, linux-pm@vger.kernel.org,
	linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org,
	sashal@kernel.org
Subject: Re: [PATCH v1] PCI / PM: Really allow runtime PM without callback
 functions
Message-ID: <20240213200648.GA1219964@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240212063233.5599-1-raag.jadav@intel.com>

On Mon, Feb 12, 2024 at 12:02:33PM +0530, Raag Jadav wrote:
> Commit c5eb1190074c ("PCI / PM: Allow runtime PM without callback
> functions") tried to eliminate the need for runtime PM callbacks
> by modifying pci_pm_runtime_suspend() and pci_pm_runtime_resume(),
> but didn't modify pci_pm_runtime_idle() with relevant changes, which
> still returns -ENOSYS if the driver supplies no runtime PM callbacks.
> 
> Fix this by modifying pci_pm_runtime_idle() such that it allows PCI
> device power state transitions without runtime PM callbacks.
> 
>  0)               |  pm_runtime_work() {
>  0)               |    rpm_idle() {
>  0)               |      rpm_check_suspend_allowed() {
>  0)   1.500 us    |        __dev_pm_qos_resume_latency(); /* = 0x7fffffff */
>  0)   4.840 us    |      } /* rpm_check_suspend_allowed = 0x0 */
>  0)   1.550 us    |      __rpm_get_callback(); /* = 0xffffffffb4bc84f0 */
>  0)   1.800 us    |      pci_pm_runtime_idle(); /* = -38 */
>  0) + 17.070 us   |    } /* rpm_idle = -38 */
>  0) + 22.450 us   |  } /* pm_runtime_work = -38 */

What is this timing information telling me?

> Debugged-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Sounds like this resolves a problem report?  Is there a URL we can
cite?  If not, at least a mention of what the user-visible problem is?

From the c5eb1190074c commit log, it sounds like maybe this allows
devices to be autosuspended when they previously could not be?

Possibly this should have "Fixes: c5eb1190074c ("PCI / PM: Allow
runtime PM without callback functions")" since it sounds like it goes
with it?

> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> ---
> 
> This is not marked for linux-stable for the need of extensive testing
> and can be backported after a few releases if no issues are reported.

If you think this should not get backported to stable, you'll have to
watch the backports to prevent it.  Lots of stuff gets auto-backported
even though not explicitly marked for stable.  This comment won't
prevent it (and won't even appear in the commit log).

>  drivers/pci/pci-driver.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 51ec9e7e784f..bb7f6775b350 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1382,10 +1382,7 @@ static int pci_pm_runtime_idle(struct device *dev)
>  	if (!pci_dev->driver)
>  		return 0;
>  
> -	if (!pm)
> -		return -ENOSYS;
> -
> -	if (pm->runtime_idle)
> +	if (pm && pm->runtime_idle)
>  		return pm->runtime_idle(dev);
>  
>  	return 0;
> -- 
> 2.35.3
>