* Re: [PATCH 1/2] i2c: i801: Fix runtime PM [not found] ` <20180627221550.0fa5f2df@endymion> @ 2018-06-27 21:23 ` Bjorn Helgaas 2020-08-28 16:26 ` Bjorn Helgaas 0 siblings, 1 reply; 4+ 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] 4+ messages in thread
* Re: [PATCH 1/2] i2c: i801: Fix runtime PM 2018-06-27 21:23 ` [PATCH 1/2] i2c: i801: Fix runtime PM Bjorn Helgaas @ 2020-08-28 16:26 ` Bjorn Helgaas 2020-08-31 15:15 ` Vaibhav Gupta 0 siblings, 1 reply; 4+ 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] 4+ 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; 4+ 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] 4+ 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; 4+ 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] 4+ messages in thread
end of thread, other threads:[~2020-09-01 8:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20180626143913.7361-1-jarkko.nikula@linux.intel.com>
[not found] ` <20180627221550.0fa5f2df@endymion>
2018-06-27 21:23 ` [PATCH 1/2] i2c: i801: Fix runtime PM 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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).