| Message ID | 20240212063233.5599-1-raag.jadav@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] PCI / PM: Really allow runtime PM without callback functions | expand |
On Mon, Feb 12, 2024 at 7:32 AM Raag Jadav <raag.jadav@intel.com> 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 */ > > Debugged-by: Mika Westerberg <mika.westerberg@linux.intel.com> > 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> Acked-by: Rafael J. Wysocki <rafael@kernel.org> > --- > > 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. > > 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 > >
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 >
Hi On 2/13/24 22:06, Bjorn Helgaas wrote: >> 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? > I don't think there's known regression but my above commit wasn't complete. Autosuspending works without runtime PM callback as long as the driver has the PM callbacks structure set. For example the drivers/i2c/busses/i2c-i801.c has system suspend/resume callbacks. I tested this patch by hack-removing them and yes, autosuspend doesn't work without this patch. Raag and Mika noticed the issue when cleaning up empty runtime PM callbacks from an another driver which doesn't have any other PM callbacks.
On Tue, Feb 13, 2024 at 02:06:48PM -0600, Bjorn Helgaas wrote: > 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? It's a raw ftrace dump. > > 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? As pointed out by Jarkko, it's not a regression. The implementation in original commit is incomplete. We discovered it while cleaning up another PCI based driver. > > 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). This is why I've added Greg and Sasha here. Raag
On Wed, Feb 14, 2024 at 12:43:35PM +0200, Raag Jadav wrote: > On Tue, Feb 13, 2024 at 02:06:48PM -0600, Bjorn Helgaas wrote: > > On Mon, Feb 12, 2024 at 12:02:33PM +0530, Raag Jadav wrote: ... > > > 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? > > It's a raw ftrace dump. (Told ya that people would be surprised with this without seeing how you get this and what fields mean)
On Wed, Feb 14, 2024 at 03:01:29PM +0200, Andy Shevchenko wrote: > On Wed, Feb 14, 2024 at 12:43:35PM +0200, Raag Jadav wrote: > > On Tue, Feb 13, 2024 at 02:06:48PM -0600, Bjorn Helgaas wrote: > > > On Mon, Feb 12, 2024 at 12:02:33PM +0530, Raag Jadav wrote: > > ... > > > > > 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? > > > > It's a raw ftrace dump. > > (Told ya that people would be surprised with this without seeing how you get > this and what fields mean) I can add stat headers in v2 which I think will be more helpful. Raag
On Wed, Feb 14, 2024 at 03:20:35PM +0200, Raag Jadav wrote: > On Wed, Feb 14, 2024 at 03:01:29PM +0200, Andy Shevchenko wrote: > > On Wed, Feb 14, 2024 at 12:43:35PM +0200, Raag Jadav wrote: > > > On Tue, Feb 13, 2024 at 02:06:48PM -0600, Bjorn Helgaas wrote: > > > > On Mon, Feb 12, 2024 at 12:02:33PM +0530, Raag Jadav wrote: > > > > ... > > > > > > > 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? > > > > > > It's a raw ftrace dump. > > > > (Told ya that people would be surprised with this without seeing how you get > > this and what fields mean) > > I can add stat headers in v2 which I think will be more helpful. That's not what I was asking. *Why* is the ftrace dump here? Is the point that we're calling a function we shouldn't? That this patch improves performance? Without some interpretation of what the dump shows, it's just noise. Bjorn
On Wed, Feb 14, 2024 at 08:58:48AM +0200, Jarkko Nikula wrote: > On 2/13/24 22:06, Bjorn Helgaas wrote: > > > 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? > > > I don't think there's known regression but my above commit wasn't complete. > Autosuspending works without runtime PM callback as long as the driver has > the PM callbacks structure set. I didn't suggest there was a regression, but if we mention that Mika debugged something, I want to know what the something was. I'm guessing runtime PM doesn't work for some subset of drivers, and this patch fixes that. So let's say exactly how to find that subset of drivers, e.g., "drivers that implement X but not Y" or whatever. > For example the drivers/i2c/busses/i2c-i801.c has system suspend/resume > callbacks. I tested this patch by hack-removing them and yes, autosuspend > doesn't work without this patch. > > Raag and Mika noticed the issue when cleaning up empty runtime PM callbacks > from an another driver which doesn't have any other PM callbacks. Bjorn
On Wed, Feb 14, 2024 at 10:58:00AM -0600, Bjorn Helgaas wrote: > On Wed, Feb 14, 2024 at 08:58:48AM +0200, Jarkko Nikula wrote: > > On 2/13/24 22:06, Bjorn Helgaas wrote: > > > > 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? > > > > > I don't think there's known regression but my above commit wasn't complete. > > Autosuspending works without runtime PM callback as long as the driver has > > the PM callbacks structure set. > > I didn't suggest there was a regression, but if we mention that Mika > debugged something, I want to know what the something was. Considering it's not a bug to begin with, perhaps we can change it to Suggested-by or Co-developed-by? Raag
On Wed, Feb 14, 2024 at 10:15:29PM +0200, Raag Jadav wrote: > On Wed, Feb 14, 2024 at 10:58:00AM -0600, Bjorn Helgaas wrote: > > On Wed, Feb 14, 2024 at 08:58:48AM +0200, Jarkko Nikula wrote: > > > On 2/13/24 22:06, Bjorn Helgaas wrote: > > > > > 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? > > > > > > > I don't think there's known regression but my above commit wasn't complete. > > > Autosuspending works without runtime PM callback as long as the driver has > > > the PM callbacks structure set. > > > > I didn't suggest there was a regression, but if we mention that Mika > > debugged something, I want to know what the something was. > > Considering it's not a bug to begin with, perhaps we can change it to > Suggested-by or Co-developed-by? Hi Mika, If you are okay with this, please let me know and perhaps suggest a better fit for the scenario. Raag
On Mon, Feb 26, 2024 at 09:35:37AM +0200, Raag Jadav wrote: > On Wed, Feb 14, 2024 at 10:15:29PM +0200, Raag Jadav wrote: > > On Wed, Feb 14, 2024 at 10:58:00AM -0600, Bjorn Helgaas wrote: > > > On Wed, Feb 14, 2024 at 08:58:48AM +0200, Jarkko Nikula wrote: > > > > On 2/13/24 22:06, Bjorn Helgaas wrote: > > > > > > 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? > > > > > > > > > I don't think there's known regression but my above commit wasn't complete. > > > > Autosuspending works without runtime PM callback as long as the driver has > > > > the PM callbacks structure set. > > > > > > I didn't suggest there was a regression, but if we mention that Mika > > > debugged something, I want to know what the something was. > > > > Considering it's not a bug to begin with, perhaps we can change it to > > Suggested-by or Co-developed-by? > > Hi Mika, > > If you are okay with this, please let me know and perhaps suggest a better > fit for the scenario. You can just drop my name from it completely.
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;
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 */ Debugged-by: Mika Westerberg <mika.westerberg@linux.intel.com> 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. drivers/pci/pci-driver.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)