* [PATCH 1/2] i2c: i801: Fix runtime PM @ 2018-06-26 14:39 Jarkko Nikula 2018-06-26 14:55 ` Mika Westerberg 2018-06-27 20:15 ` Jean Delvare 0 siblings, 2 replies; 7+ messages in thread From: Jarkko Nikula @ 2018-06-26 14:39 UTC (permalink / raw) To: linux-i2c Cc: Jean Delvare, Wolfram Sang, Mika Westerberg, Jarkko Nikula, stable Commit 9c8088c7988 ("i2c: i801: Don't restore config registers on runtime PM") nullified the runtime PM suspend/resume callback pointers while keeping the runtime PM enabled. This causes that device stays in D0 power state and sysfs /sys/bus/pci/devices/.../power/runtime_status shows "error" when runtime PM framework attempts to autosuspend the device. This is due PCI bus runtime PM which checks for driver runtime PM callbacks and returns with -ENOSYS if they are not set. Fix this by having a shared dummy runtime PM callback that returns with success. Fixes: a9c8088c7988 ("i2c: i801: Don't restore config registers on runtime PM") Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com> Cc: <stable@vger.kernel.org> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> --- drivers/i2c/busses/i2c-i801.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index aa726607645e..3747484c2669 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1731,7 +1731,20 @@ static int i801_resume(struct device *dev) } #endif -static SIMPLE_DEV_PM_OPS(i801_pm_ops, i801_suspend, i801_resume); +static int __maybe_unused i801_runtime_nop(struct device *dev) +{ + /* + * PCI core expects runtime PM suspend/resume callbacks return + * successfully before really suspending/resuming the device. + * Have a shared dummy callback that returns with success. + */ + return 0; +} + +static const struct dev_pm_ops i801_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(i801_suspend, i801_resume) + SET_RUNTIME_PM_OPS(i801_runtime_nop, i801_runtime_nop, NULL) +}; static struct pci_driver i801_driver = { .name = "i801_smbus", -- 2.18.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] i2c: i801: Fix runtime PM 2018-06-26 14:39 [PATCH 1/2] i2c: i801: Fix runtime PM Jarkko Nikula @ 2018-06-26 14:55 ` Mika Westerberg 2018-06-27 20:15 ` Jean Delvare 1 sibling, 0 replies; 7+ messages in thread From: Mika Westerberg @ 2018-06-26 14:55 UTC (permalink / raw) To: Jarkko Nikula; +Cc: linux-i2c, Jean Delvare, Wolfram Sang, stable On Tue, Jun 26, 2018 at 05:39:12PM +0300, Jarkko Nikula wrote: > Commit 9c8088c7988 ("i2c: i801: Don't restore config registers on > runtime PM") nullified the runtime PM suspend/resume callback pointers > while keeping the runtime PM enabled. This causes that device stays in > D0 power state and sysfs /sys/bus/pci/devices/.../power/runtime_status > shows "error" when runtime PM framework attempts to autosuspend the > device. > > This is due PCI bus runtime PM which checks for driver runtime PM > callbacks and returns with -ENOSYS if they are not set. Fix this by > having a shared dummy runtime PM callback that returns with success. > > Fixes: a9c8088c7988 ("i2c: i801: Don't restore config registers on runtime PM") > Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com> This fixes the issue for me, thanks Jarkko! Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] i2c: i801: Fix runtime PM 2018-06-26 14:39 [PATCH 1/2] i2c: i801: Fix runtime PM Jarkko Nikula 2018-06-26 14:55 ` Mika Westerberg @ 2018-06-27 20:15 ` Jean Delvare 2018-06-27 21:23 ` Bjorn Helgaas 1 sibling, 1 reply; 7+ messages in thread From: Jean Delvare @ 2018-06-27 20:15 UTC (permalink / raw) To: Jarkko Nikula Cc: linux-i2c, Wolfram Sang, Mika Westerberg, stable, Bjorn Helgaas, linux-pci Hi Jarkko, On Tue, 26 Jun 2018 17:39:12 +0300, Jarkko Nikula wrote: > Commit 9c8088c7988 ("i2c: i801: Don't restore config registers on > runtime PM") nullified the runtime PM suspend/resume callback pointers > while keeping the runtime PM enabled. This causes that device stays in > D0 power state and sysfs /sys/bus/pci/devices/.../power/runtime_status > shows "error" when runtime PM framework attempts to autosuspend the > device. > > This is due PCI bus runtime PM which checks for driver runtime PM > callbacks and returns with -ENOSYS if they are not set. Fix this by > having a shared dummy runtime PM callback that returns with success. > > Fixes: a9c8088c7988 ("i2c: i801: Don't restore config registers on runtime PM") I don't want to sound like I'm trying to decline all responsibility for a regression I caused, but frankly, if just using SIMPLE_DEV_PM_OPS() breaks runtime PM, then it's the PM model which is broken, not the i2c-i801 driver. I will boldly claim that the PCI bus runtime code is simply wrong in returning -ENOSYS in the absence of runtime PM callbacks, and it should be changed to return 0 instead. Or whoever receives that -ENOSYS should not treat it as an error - whatever makes more sense. Having to add dummy functions in every PCI driver that doesn't need to do anything special for runtime PM sounds plain stupid. It should be pretty obvious that a whole lot of drivers are going to use SIMPLE_DEV_PM_OPS() because it exists and seems to do what they want, and all of them will be bugged because the PCI core is doing something silly and unexpected. So please let's fix it at the PCI subsystem core level. Adding Bjorn and the linux-pci list to Cc. Thanks, -- Jean Delvare SUSE L3 Support ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] i2c: i801: Fix runtime PM 2018-06-27 20:15 ` Jean Delvare @ 2018-06-27 21:23 ` Bjorn Helgaas 2020-08-28 16:26 ` Bjorn Helgaas 0 siblings, 1 reply; 7+ messages in thread From: Bjorn Helgaas @ 2018-06-27 21:23 UTC (permalink / raw) To: Jean Delvare Cc: Jarkko Nikula, linux-i2c, Wolfram Sang, Mika Westerberg, stable, Bjorn Helgaas, linux-pci, Rafael J. Wysocki, linux-pm, linux-kernel [+cc Rafael, linux-pm, linux-kernel] On Wed, Jun 27, 2018 at 10:15:50PM +0200, Jean Delvare wrote: > Hi Jarkko, > > On Tue, 26 Jun 2018 17:39:12 +0300, Jarkko Nikula wrote: > > Commit 9c8088c7988 ("i2c: i801: Don't restore config registers on > > runtime PM") nullified the runtime PM suspend/resume callback pointers > > while keeping the runtime PM enabled. This causes that device stays in > > D0 power state and sysfs /sys/bus/pci/devices/.../power/runtime_status > > shows "error" when runtime PM framework attempts to autosuspend the > > device. > > > > This is due PCI bus runtime PM which checks for driver runtime PM > > callbacks and returns with -ENOSYS if they are not set. Fix this by > > having a shared dummy runtime PM callback that returns with success. > > > > Fixes: a9c8088c7988 ("i2c: i801: Don't restore config registers on runtime PM") > > I don't want to sound like I'm trying to decline all responsibility for > a regression I caused, but frankly, if just using SIMPLE_DEV_PM_OPS() > breaks runtime PM, then it's the PM model which is broken, not the > i2c-i801 driver. > > I will boldly claim that the PCI bus runtime code is simply wrong in > returning -ENOSYS in the absence of runtime PM callbacks, and it should > be changed to return 0 instead. Or whoever receives that -ENOSYS should > not treat it as an error - whatever makes more sense. > > Having to add dummy functions in every PCI driver that doesn't need to > do anything special for runtime PM sounds plain stupid. It should be > pretty obvious that a whole lot of drivers are going to use > SIMPLE_DEV_PM_OPS() because it exists and seems to do what they want, > and all of them will be bugged because the PCI core is doing something > silly and unexpected. > > So please let's fix it at the PCI subsystem core level. Adding Bjorn > and the linux-pci list to Cc. Thanks Jean. What you describe does sound broken. I think the PM guys (cc'd) will have a better idea of how to deal with this. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] i2c: i801: Fix runtime PM 2018-06-27 21:23 ` Bjorn Helgaas @ 2020-08-28 16:26 ` Bjorn Helgaas 2020-08-31 15:15 ` Vaibhav Gupta 0 siblings, 1 reply; 7+ messages in thread From: Bjorn Helgaas @ 2020-08-28 16:26 UTC (permalink / raw) To: Jean Delvare Cc: Jarkko Nikula, linux-i2c, Wolfram Sang, Mika Westerberg, stable, Bjorn Helgaas, linux-pci, Rafael J. Wysocki, linux-pm, linux-kernel, Vaibhav Gupta [+cc Vaibhav] On Wed, Jun 27, 2018 at 04:23:40PM -0500, Bjorn Helgaas wrote: > [+cc Rafael, linux-pm, linux-kernel] > > On Wed, Jun 27, 2018 at 10:15:50PM +0200, Jean Delvare wrote: > > Hi Jarkko, > > > > On Tue, 26 Jun 2018 17:39:12 +0300, Jarkko Nikula wrote: > > > Commit 9c8088c7988 ("i2c: i801: Don't restore config registers on > > > runtime PM") nullified the runtime PM suspend/resume callback pointers > > > while keeping the runtime PM enabled. This causes that device stays in > > > D0 power state and sysfs /sys/bus/pci/devices/.../power/runtime_status > > > shows "error" when runtime PM framework attempts to autosuspend the > > > device. > > > > > > This is due PCI bus runtime PM which checks for driver runtime PM > > > callbacks and returns with -ENOSYS if they are not set. Fix this by > > > having a shared dummy runtime PM callback that returns with success. > > > > > > Fixes: a9c8088c7988 ("i2c: i801: Don't restore config registers on runtime PM") > > > > I don't want to sound like I'm trying to decline all responsibility for > > a regression I caused, but frankly, if just using SIMPLE_DEV_PM_OPS() > > breaks runtime PM, then it's the PM model which is broken, not the > > i2c-i801 driver. > > > > I will boldly claim that the PCI bus runtime code is simply wrong in > > returning -ENOSYS in the absence of runtime PM callbacks, and it should > > be changed to return 0 instead. Or whoever receives that -ENOSYS should > > not treat it as an error - whatever makes more sense. > > > > Having to add dummy functions in every PCI driver that doesn't need to > > do anything special for runtime PM sounds plain stupid. It should be > > pretty obvious that a whole lot of drivers are going to use > > SIMPLE_DEV_PM_OPS() because it exists and seems to do what they want, > > and all of them will be bugged because the PCI core is doing something > > silly and unexpected. > > > > So please let's fix it at the PCI subsystem core level. Adding Bjorn > > and the linux-pci list to Cc. > > Thanks Jean. What you describe does sound broken. I think the PM > guys (cc'd) will have a better idea of how to deal with this. Did we ever get anywhere with this? It seems like the thread petered out. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] i2c: i801: Fix runtime PM 2020-08-28 16:26 ` Bjorn Helgaas @ 2020-08-31 15:15 ` Vaibhav Gupta 2020-09-01 8:19 ` Jarkko Nikula 0 siblings, 1 reply; 7+ messages in thread From: Vaibhav Gupta @ 2020-08-31 15:15 UTC (permalink / raw) To: Bjorn Helgaas Cc: Jean Delvare, Jarkko Nikula, linux-i2c, Wolfram Sang, Mika Westerberg, stable, Bjorn Helgaas, linux-pci, Rafael J. Wysocki, linux-pm, linux-kernel On Fri, Aug 28, 2020 at 11:26:40AM -0500, Bjorn Helgaas wrote: > [+cc Vaibhav] > > On Wed, Jun 27, 2018 at 04:23:40PM -0500, Bjorn Helgaas wrote: > > [+cc Rafael, linux-pm, linux-kernel] > > > > On Wed, Jun 27, 2018 at 10:15:50PM +0200, Jean Delvare wrote: > > > Hi Jarkko, > > > > > > On Tue, 26 Jun 2018 17:39:12 +0300, Jarkko Nikula wrote: > > > > Commit 9c8088c7988 ("i2c: i801: Don't restore config registers on > > > > runtime PM") nullified the runtime PM suspend/resume callback pointers > > > > while keeping the runtime PM enabled. This causes that device stays in > > > > D0 power state and sysfs /sys/bus/pci/devices/.../power/runtime_status > > > > shows "error" when runtime PM framework attempts to autosuspend the > > > > device. > > > > > > > > This is due PCI bus runtime PM which checks for driver runtime PM > > > > callbacks and returns with -ENOSYS if they are not set. Fix this by > > > > having a shared dummy runtime PM callback that returns with success. > > > > > > > > Fixes: a9c8088c7988 ("i2c: i801: Don't restore config registers on runtime PM") > > > > > > I don't want to sound like I'm trying to decline all responsibility for > > > a regression I caused, but frankly, if just using SIMPLE_DEV_PM_OPS() > > > breaks runtime PM, then it's the PM model which is broken, not the > > > i2c-i801 driver. > > > > > > I will boldly claim that the PCI bus runtime code is simply wrong in > > > returning -ENOSYS in the absence of runtime PM callbacks, and it should > > > be changed to return 0 instead. Or whoever receives that -ENOSYS should > > > not treat it as an error - whatever makes more sense. > > > > > > Having to add dummy functions in every PCI driver that doesn't need to > > > do anything special for runtime PM sounds plain stupid. It should be > > > pretty obvious that a whole lot of drivers are going to use > > > SIMPLE_DEV_PM_OPS() because it exists and seems to do what they want, > > > and all of them will be bugged because the PCI core is doing something > > > silly and unexpected. > > > > > > So please let's fix it at the PCI subsystem core level. Adding Bjorn > > > and the linux-pci list to Cc. > > > > Thanks Jean. What you describe does sound broken. I think the PM > > guys (cc'd) will have a better idea of how to deal with this. > > Did we ever get anywhere with this? It seems like the thread petered > out. This does seems worrying. I remember, few days earlier you pointed out a driver i2c-nvidia-gpuc.c. In the code, gpu_i2c_suspend() is an empty-body function. And comment mentioned that empty stub is necessary for runtime_pm to work. And this driver also uses UNIVERSAL_DEV_PM_OPS. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] i2c: i801: Fix runtime PM 2020-08-31 15:15 ` Vaibhav Gupta @ 2020-09-01 8:19 ` Jarkko Nikula 0 siblings, 0 replies; 7+ messages in thread From: Jarkko Nikula @ 2020-09-01 8:19 UTC (permalink / raw) To: Vaibhav Gupta, Bjorn Helgaas Cc: Jean Delvare, linux-i2c, Wolfram Sang, Mika Westerberg, stable, Bjorn Helgaas, linux-pci, Rafael J. Wysocki, linux-pm, linux-kernel On 8/31/20 6:15 PM, Vaibhav Gupta wrote: > On Fri, Aug 28, 2020 at 11:26:40AM -0500, Bjorn Helgaas wrote: >> [+cc Vaibhav] >> >> On Wed, Jun 27, 2018 at 04:23:40PM -0500, Bjorn Helgaas wrote: >>> [+cc Rafael, linux-pm, linux-kernel] >>> >>> On Wed, Jun 27, 2018 at 10:15:50PM +0200, Jean Delvare wrote: >>>> Hi Jarkko, >>>> >>>> On Tue, 26 Jun 2018 17:39:12 +0300, Jarkko Nikula wrote: >>>>> Commit 9c8088c7988 ("i2c: i801: Don't restore config registers on >>>>> runtime PM") nullified the runtime PM suspend/resume callback pointers >>>>> while keeping the runtime PM enabled. This causes that device stays in >>>>> D0 power state and sysfs /sys/bus/pci/devices/.../power/runtime_status >>>>> shows "error" when runtime PM framework attempts to autosuspend the >>>>> device. >>>>> >>>>> This is due PCI bus runtime PM which checks for driver runtime PM >>>>> callbacks and returns with -ENOSYS if they are not set. Fix this by >>>>> having a shared dummy runtime PM callback that returns with success. >>>>> >>>>> Fixes: a9c8088c7988 ("i2c: i801: Don't restore config registers on runtime PM") >>>> >>>> I don't want to sound like I'm trying to decline all responsibility for >>>> a regression I caused, but frankly, if just using SIMPLE_DEV_PM_OPS() >>>> breaks runtime PM, then it's the PM model which is broken, not the >>>> i2c-i801 driver. >>>> >>>> I will boldly claim that the PCI bus runtime code is simply wrong in >>>> returning -ENOSYS in the absence of runtime PM callbacks, and it should >>>> be changed to return 0 instead. Or whoever receives that -ENOSYS should >>>> not treat it as an error - whatever makes more sense. >>>> >>>> Having to add dummy functions in every PCI driver that doesn't need to >>>> do anything special for runtime PM sounds plain stupid. It should be >>>> pretty obvious that a whole lot of drivers are going to use >>>> SIMPLE_DEV_PM_OPS() because it exists and seems to do what they want, >>>> and all of them will be bugged because the PCI core is doing something >>>> silly and unexpected. >>>> >>>> So please let's fix it at the PCI subsystem core level. Adding Bjorn >>>> and the linux-pci list to Cc. >>> >>> Thanks Jean. What you describe does sound broken. I think the PM >>> guys (cc'd) will have a better idea of how to deal with this. >> >> Did we ever get anywhere with this? It seems like the thread petered >> out. > This does seems worrying. I remember, few days earlier you pointed out a driver > i2c-nvidia-gpuc.c. In the code, gpu_i2c_suspend() is an empty-body function. And > comment mentioned that empty stub is necessary for runtime_pm to work. > > And this driver also uses UNIVERSAL_DEV_PM_OPS. > This was fixed by c5eb1190074c ("PCI / PM: Allow runtime PM without callback functions"). So no need for empty runtime PM callbacks anymore. -- Jarkko ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-09-01 8:19 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-26 14:39 [PATCH 1/2] i2c: i801: Fix runtime PM Jarkko Nikula 2018-06-26 14:55 ` Mika Westerberg 2018-06-27 20:15 ` Jean Delvare 2018-06-27 21:23 ` Bjorn Helgaas 2020-08-28 16:26 ` Bjorn Helgaas 2020-08-31 15:15 ` Vaibhav Gupta 2020-09-01 8:19 ` Jarkko Nikula
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.