Re: [PATCH] i2c: s3c2410: remove superfluous runtime PM calls
On Sat, 19 Dec 2015, Wolfram Sang wrote: > Asking linux-pm for help here: If we want to support RuntimePM for I2C > clients, do we need to enable RuntimePM on the logical I2C adapter > device (the bus master) which is already marked using > pm_runtime_no_callbacks? In theory you don't need to. But there are some advantages if you do: You get automatic runtime PM time accounting for the adapter device (how much time active and how much suspended), and suspend events will propagate from the I2C clients all the way up to the adapter's parent. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] i2c: Prevent runtime suspend of parent during adapter registration
On Tue, 15 Dec 2015, Jarkko Nikula wrote:
> If runtime PM of parent device is enabled before it registers the I2C
> adapter there can be unnecessary runtime suspend during adapter device
> registration. This can happen when adapter is registered from parent's
> probe and if parent device is initially active platform device.
>
> In that case power.usage_count of parent device is zero and
> pm_runtime_get()/pm_runtime_put() cycle during probe could put the
> parent into runtime suspend. This happens when the i2c_register_adapter()
> calls the device_register():
>
> i2c_register_adapter
> device_register
> device_add
> bus_probe_device
> device_initial_probe
> __device_attach
> if (dev->parent) pm_runtime_get_sync(dev->parent)
> ...
> if (dev->parent) pm_runtime_put(dev->parent)
>
> After that i2c_register_adapter() continues registering I2C slave
> devices. In case slave device probe does I2C transfers the parent will
> resume again and thus get a needless runtime suspend/resume cycle during
> adapter registration.
>
> Prevent this while retaining the runtime PM status of parent by only
> incrementing/decrementing parent device power usage count during I2C
> adapter registration. That makes sure there won't be spurious runtime PM
> status changes for parent's probe and lets the driver core to idle the
> device after probe finishes.
>
> Signed-off-by: Jarkko Nikula <jarkko.nik...@linux.intel.com>
> ---
> I noticed this with i2c-designware-platdrv.c which started to do so
> after commit 36d48fb5766a ("i2c: designware-platdrv: enable RuntimePM
> before registering to the core"). I guess the same could happen also
> with a few other I2C drivers that enable runtime PM similar way. For
> instance i2c-at91.c, i2c-cadence.c, i2c-hix5hd2.c and i2c-qup.c.
> That made me thinking if issue might be best to handle in i2c-core.c.
>
> Device core drivers/base/dd.c: driver_probe_device() or
> drivers/base/platform.c: platform_drv_probe() could be other
> alternatives but that would cause a regression to a driver that
> purposively tries to suspend the device in its probe().
This isn't how the problem is handled in other places. In all the
cases I know about, the parent's subsystem or driver makes sure that
the parent will not go into runtime suspend while it is being probed.
At the very least, it should insure that the parent will not go into
runtime suspend while it is registering a child device (which
typically happens within a probe routine).
In general, it's bad form to do pm_runtime_get/put calls on your parent
-- you should make changes only to the device you're responsible for.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] On-demand device probing
On Tue, 20 Oct 2015, Rob Herring wrote: > > The probe ordering is not the entire picture, though. > > > > Even if you get the probe ordering right, the problem is going to show up in > > multiple other places: system suspend/resume, runtime PM, system shutdown, > > unbinding of drivers. In all of those cases it is necessary to handle > > things > > in a specific order if there is a dependency. > > My understanding was with deferred probe that it also solves suspend > ordering problems because things are suspended in reverse order of > probing. Devices are suspended in reverse order of _registration_. Not of probing. Furthermore, that applies only to devices that use synchronous suspend. Async suspend is becoming common, and there the only restrictions are parent-child relations plus whatever explicit requirements that drivers impose by calling device_pm_wait_for_dev(). Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] On-demand device probing
On Tue, 20 Oct 2015, Mark Brown wrote: > On Tue, Oct 20, 2015 at 10:40:03AM -0400, Alan Stern wrote: > > > Furthermore, that applies only to devices that use synchronous suspend. > > Async suspend is becoming common, and there the only restrictions are > > parent-child relations plus whatever explicit requirements that drivers > > impose by calling device_pm_wait_for_dev(). > > Hrm, this is the first I'd noticed that feature though I see the initial > commit dates from January. Async suspend and device_pm_wait_for_dev() were added in January 2010, not 2015! > It looks like most of the users are PCs at > the minute but we should be using it more widely for embedded things, > there's definitely some cases I'm aware of where it will allow us to > remove some open coding. > > It does seem like we want to be feeding dependency information we > discover for probing way into the suspend dependencies... Rafael has been thinking about a way to do this systematically. Nothing concrete has emerged yet. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] On-demand device probing
On Tue, 20 Oct 2015, Tomeu Vizoso wrote: > On 20 October 2015 at 18:04, Alan Stern <st...@rowland.harvard.edu> wrote: > > On Tue, 20 Oct 2015, Mark Brown wrote: > > > >> On Tue, Oct 20, 2015 at 10:40:03AM -0400, Alan Stern wrote: > >> > >> > Furthermore, that applies only to devices that use synchronous suspend. > >> > Async suspend is becoming common, and there the only restrictions are > >> > parent-child relations plus whatever explicit requirements that drivers > >> > impose by calling device_pm_wait_for_dev(). > >> > >> Hrm, this is the first I'd noticed that feature though I see the initial > >> commit dates from January. > > > > Async suspend and device_pm_wait_for_dev() were added in January 2010, > > not 2015! > > > >> It looks like most of the users are PCs at > >> the minute but we should be using it more widely for embedded things, > >> there's definitely some cases I'm aware of where it will allow us to > >> remove some open coding. > >> > >> It does seem like we want to be feeding dependency information we > >> discover for probing way into the suspend dependencies... > > > > Rafael has been thinking about a way to do this systematically. > > Nothing concrete has emerged yet. > > This iteration of the series would make this quite easy, as > dependencies are calculated before probes are attempted: > > https://lkml.org/lkml/2015/6/17/311 But what Rafael is proposing is quite general; it would apply to _all_ dependencies as opposed to just those present in DT drivers or those affecting platform_devices. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] i2c: bcm-kona: register with subsys_initcall
On Wed, 5 Feb 2014, Matt Porter wrote: On Wed, Feb 05, 2014 at 10:08:18AM +0100, Wolfram Sang wrote: On Tue, Feb 04, 2014 at 07:19:07AM -0500, Matt Porter wrote: Voltage regulators are needed very early due to deferred probe being incompatible with built-in USB gadget drivers. What does it need to fix those instead? [added Alan/Felipe for more insight] Discussion on that topic came about from this submission: http://www.spinics.net/lists/linux-usb/msg94217.html End of it is: http://www.spinics.net/lists/linux-usb/msg94731.html We can either add to the many drivers that already do subsys_initcall() for similar reasons...or I can drop this from the series and add gadget probe ordering to my TODO list. In short, it can't be a late_initcall() hack like the original post and really could be solved by converting to a real bus (and letting deferred probe do its job)..but Alan voiced concerns about that. Don't worry too much about what I said. If adding a gadget bus will solve the problem in an appropriate way, and if nobody else objects (particularly Felipe, who is on vacation now), then go for it. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: suppress lockdep warning on delete_device
On Fri, 17 May 2013, Alexander Sverdlin wrote: i2c: suppress lockdep warning on delete_device Since commit 846f99749ab68bbc7f75c74fec305de675b1a1bf the following lockdep warning is thrown in case i2c device is removed (via delete_device sysfs attribute) which contains subdevices (e.g. i2c multiplexer): Can you explain this in a little more detail? Exactly what does the delete_device attribute do, when called for device D? That is, what does a write to /sys/bus/i2c/devices/D/delete_device do? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: suppress lockdep warning on delete_device
On Fri, 17 May 2013, Alexander Sverdlin wrote: Hi! On 05/17/2013 04:42 PM, ext Alan Stern wrote: i2c: suppress lockdep warning on delete_device Since commit 846f99749ab68bbc7f75c74fec305de675b1a1bf the following lockdep warning is thrown in case i2c device is removed (via delete_device sysfs attribute) which contains subdevices (e.g. i2c multiplexer): Can you explain this in a little more detail? Exactly what does the delete_device attribute do, when called for device D? That is, what does a write to /sys/bus/i2c/devices/D/delete_device do? Like USB with its hubs, I2C structure could be tree-like, with I2C multiplexers. delete_device attribute removes I2C device from the subsystem, which is usually not a problem, except the case when device is a multiplexer and sub-devices should be removed first. Here we have the problem: holding a s_active lock on delete_device attribute of a parent (multiplexer) we need to delete children, but they were created with the same static attribute delete_device. The safety of this operation is exactly the same as in USB case... Any more clever lockdep annotation is exactly not so easy... If I understand you correctly, if D is an I2C multiplexer then writing to /sys/bus/i2c/devices/D/delete_device will get rid of all of D's children (and their descendants) and will also get rid of D itself -- right? That's _not_ what the USB attributes do. For example, if D is a USB hub then writing 0 to /sys/bus/usb/devices/D/bConfigurationValue will get rid of all D's descendants but will not get rid of D. Instead of doing it this way, the attribute method should call device_schedule_callback(). See commit d9a9cdfb078d. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: suppress lockdep warning on delete_device
On Fri, 17 May 2013, Alexander Sverdlin wrote: Hi! On 05/17/2013 05:31 PM, ext Alan Stern wrote: i2c: suppress lockdep warning on delete_device Since commit 846f99749ab68bbc7f75c74fec305de675b1a1bf the following lockdep warning is thrown in case i2c device is removed (via delete_device sysfs attribute) which contains subdevices (e.g. i2c multiplexer): Can you explain this in a little more detail? Exactly what does the delete_device attribute do, when called for device D? That is, what does a write to /sys/bus/i2c/devices/D/delete_device do? Like USB with its hubs, I2C structure could be tree-like, with I2C multiplexers. delete_device attribute removes I2C device from the subsystem, which is usually not a problem, except the case when device is a multiplexer and sub-devices should be removed first. Here we have the problem: holding a s_active lock on delete_device attribute of a parent (multiplexer) we need to delete children, but they were created with the same static attribute delete_device. The safety of this operation is exactly the same as in USB case... Any more clever lockdep annotation is exactly not so easy... If I understand you correctly, if D is an I2C multiplexer then writing to /sys/bus/i2c/devices/D/delete_device will get rid of all of D's children (and their descendants) and will also get rid of D itself -- right? That's _not_ what the USB attributes do. For example, if D is a USB hub then writing 0 to /sys/bus/usb/devices/D/bConfigurationValue will get rid of all D's descendants but will not get rid of D. Well, seems that what I've described above wasn't precise enough... That's why I asked what exactly the attribute does. :-) Actually only bus controllers have delete_device attribute. Simple devices on the bus do not have it. User write an address of the slave device to this attribute to delete it. This works fine for simple devices. In case of multiplexer (which is in turn also a bus controller) there are nested delete_device attributes. So it's only possible to delete child nodes and not the controller itself, writing to its delete_device. So from my POV, it fits to USB concept... Okay. If writing to the attribute doesn't delete the object which the attribute is attached to, then your patch is the correct approach. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC][PATCH] Enable async suspend/resume of i2c devices
On Thu, 7 Apr 2011, Jean Delvare wrote: On Thu, 7 Apr 2011 09:00:43 +0100, Mark Brown wrote: On Thu, Apr 07, 2011 at 09:55:13AM +0200, Jean Delvare wrote: On Thu, 7 Apr 2011 07:31:24 +0900, Mark Brown wrote: You definitely don't know *anything* about the relationships for I2C, especially in embedded systems. Can you please elaborate? The i2c subsystem uses a standard parent-children relationship. It seems fairly similar to USB for example. The only special case I can think of is with bus multiplexing, but it would be easy enough to switch off async suspend/resume in this case. For I2C itself that's the case but for power and GPIO (which are very common in conjunction with I2C in the embedded context) we loose the tree. This is much less of an issue with buses like PCI where everything is bundled into the bus. Out of curiosity, how is this handled in the synchronous suspend/resume case? With synchronous resume, devices are resumed in the order they were originally registered. Since the power and GPIO controllers must have been registered before the I2C controller (otherwise the I2C wouldn't have been accessible when it was registered!), they will be resumed first. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] [RFC][PATCH] Enable async suspend/resume of i2c devices
On Wed, 6 Apr 2011, Jean Delvare wrote: On Wed, 6 Apr 2011 06:23:35 +0100, Mark Brown wrote: On Mon, Apr 04, 2011 at 08:47:01PM -0700, Sonny Rao wrote: This improves our resume time when we have devices on an i2c bus that are slow to resume. In particular we have a light sensor that adds about 50ms of resume time on one device. We have to enable it both on the i2c master and i2c client side and then we get fully async suspend/resume. I suspect we'll see nice gains on systems with more i2c devices and will test that out soon. Signed-off-by: Sonny Rao sonny...@chromium.org It'd probably help if the patch explained why this is safe - my immediate question is why if it's safe to just unconditionally enable async suspend for all I2C clients and adaptors it's not safe to do so for all devices of all types? I have exactly the same concern. From Documentation/ABI/testing/sysfs-devices-power: It generally is unsafe to permit the asynchronous suspend/resume of a device unless it is certain that all of the PM dependencies of the device are known to the PM core. However, for some devices this attribute is set to enabled by bus type code or device drivers and in that cases it should be safe to leave the default value. As I don't see any code being added to guarantee that all of the PM dependencies of the device are known to the PM core, I am skeptical about the general correctness of the proposed change. On the other hand, a quick grep on the kernel tree shows that the scsi, usb and pci subsystem enable async suspend unconditionally for all devices. This seems quite contradictory with the quoted statement above. Rafael, can you please clarify? Is the attribute description too alarmist, or are the subsystems too optimistic? ;) Neither is the case. For these subsystems, the PM dependencies _are_ known. The statement in the Documentation file refers to dependencies other than the usual parent-child relationships. PCI and SCSI have no such dependencies. In USB some do exist, but the subsystem code knows about them and takes them into account. Grep for device_pm_wait_for_dev in the appropriate subtrees and you'll see what I mean. Now, I have no idea what the situation is with regard to I2C... Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: platform/i2c busses: pm runtime and system sleep
On Fri, 18 Feb 2011, Rabin Vincent wrote:
How about something like the below? If we have something like this, we
can just switch platform to GENERIC_PM_OPS and add the
pm_runtime_want_interaction() (or something better named)
Yes, please find a better name! Maybe something starting with
generic_ to indicate that this applies only to devices using the
GENERIC_PM_OPS.
call to the
i2c and spi drivers using runtime PM.
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 42615b4..2b8a099 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1069,6 +1069,30 @@ void pm_runtime_allow(struct device *dev)
EXPORT_SYMBOL_GPL(pm_runtime_allow);
/**
+ * pm_runtime_want_interaction - Enable interaction between system sleep
+ *and runtime PM callbacks at the bus/subsystem
+ *level.
+ * @dev: Device to handle
+ *
+ * Set the power.want_interaction flage, which tells the generic PM subsystem
+ * ops that the following actions should be done during system
suspend/resume:
+ *
+ * - If the device has been runtime suspended, the driver's
+ * suspend() handler will not be invoked.
+ *
+ * - If the device has a resume() pm callback, and the resume()
+ * callback returns success on system resume, the device's
+ * runtime PM status will be set to active.
+ */
This last part is normally true for all devices. If you don't want it
to hold when want_interaction isn't set, you should add a good
explanation to sections 6 and 7 in Documentation/power/runtime.txt.
+void pm_runtime_want_interaction(struct device *dev)
+{
+ spin_lock_irq(dev-power.lock);
+ dev-power.want_interaction = 1;
+ spin_unlock_irq(dev-power.lock);
+}
+EXPORT_SYMBOL_GPL(pm_runtime_want_interaction);
+
+/**
* pm_runtime_no_callbacks - Ignore run-time PM callbacks for a device.
* @dev: Device to handle.
*
Don't forget that you also need to describe these things in
Documentation/power/runtime.txt.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] PM: Allow pm_runtime_suspend() to succeed during system suspend
On Fri, 11 Feb 2011, Rafael J. Wysocki wrote: The reason why runtime suspend is not allowed during system power transitions if the following race: - A device has been suspended via a system suspend callback. - The runtime PM framework executes a (scheduled) suspend on that device, not knowing that it's already been suspended, which potentially results in accessing the device's registers in a low-power state. Now, it can be avoided if every driver does the right thing and checks whether the device is already suspended in its runtime suspend callback, but that would kind of defeat the purpose of the runtime PM framework, at least partially. In fact, I've just realized that the above race cannot really occur, because pm_wq is freezable, so I'm proposing the following change. Yes, I had reached essentially the same conclusion. Of course, there may still be other kernel threads running or interrupt handlers that can interfere. It's probably okay to assume that drivers will handle these things. Of course, it still doesn't prevent user space from disabling the runtime PM framework's helpers via /sys/devices/.../power/control. True. So in the end this won't make much difference, but we might as well do it. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] [PATCH] i2c: OMAP: fix static suspend vs. runtime suspend
On Mon, 31 Jan 2011, Rajendra Nayak wrote: Can you elaborate a bit more on how/why runtime PM transitions are disabled during system suspend, and how is it taken care of that a runtime resume of a device works however a subsequent runtime (re)suspend does not? I'll answer for Kevin. This is done by the PM core, in order to prevent runtime power transitions from interfering with a system power transition. The PM core increments the device's usage_count; this prevents the device from being runtime-suspended but it allows runtime-resume calls to go through. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] [PATCH] i2c: OMAP: fix static suspend vs. runtime suspend
On Mon, 31 Jan 2011, Kevin Hilman wrote: I understand how this works, but frankly I'm still a bit fuzzy on why. I guess I'm still missing a good understanding of what interfering with a system power transition means, and why a runtime suspend qualifies as interfering but not a runtime resume. These are good questions. Rafael implemented this design originally; my contribution was only to warn him of the potential for problems. Therefore he should explain the rationale for the design. More specifically, the reason for $SUBJECT patch is precisely because a runtime resume is allowed, a runtime suspend is not, and thus a system power transititon is prevented. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] pm_runtime_suspended() and non-pm_runtime-using (i2c) drivers
On Wed, 15 Dec 2010, Rafael J. Wysocki wrote:
From: Rafael J. Wysocki r...@sisk.pl
Subject: PM / Runtime: Fix pm_runtime_suspended()
pm_runtime_suspended() shouldn't return true if the runtime PM of the
given device is disabled.
Signed-off-by: Rafael J. Wysocki r...@sisk.pl
---
include/linux/pm_runtime.h |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Index: linux-2.6/include/linux/pm_runtime.h
===
--- linux-2.6.orig/include/linux/pm_runtime.h
+++ linux-2.6/include/linux/pm_runtime.h
@@ -78,7 +78,8 @@ static inline void device_set_run_wake(s
static inline bool pm_runtime_suspended(struct device *dev)
{
- return dev-power.runtime_status == RPM_SUSPENDED;
+ return dev-power.runtime_status == RPM_SUSPENDED
+ !dev-power.disable_depth;
}
You need to update the documentation entry for pm_runtime_suspended as
well.
I think this is okay. If a driver or subsystem uses
pm_runtime_suspended() then it must be runtime-PM-aware, so it wouldn't
leave a device disabled for runtime PM.
So in theory the only place this would matter is if the function is
called in a generic setting, and AFAICT the only place that happens is
in generic_ops.c, where the change is correct.
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] pm_runtime_suspended() and non-pm_runtime-using (i2c) drivers
On Tue, 14 Dec 2010, Rabin Vincent wrote: Hello, If an i2c driver uses dev_pm_ops and pm_runtime_suspended() returns true for the device, the i2c core will not call the driver's pm-suspend() routine. Similar behaviour (except for the if dev_pm_ops check) is present in the generic PM ops provided in drivers/base/power/generic_ops.c. Since pm_runtime_suspended() returns true if the relevant driver did not call any pm_runtime functions, this means that any driver which does not use pm_runtime APIs will not get its pm-suspend() callback called during system sleep, if CONFIG_PM_RUNTIME is enabled. For the i2c case, there are several such drivers (in drivers/input/*, etc) lacking these calls. How is this to be handled? Do all of these drivers need to be patched to use the pm_runtime API if they are to be used on a kernel with PM_RUNTIME enabled? I'm not familiar with the details of how the i2c subsystem works. But in general, the subsystem code should call pm_runtime_set_active() for every device before registering it. Then if a driver doesn't use any runtime-PM functions, pm_runtime_suspended() will return false. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] pm_runtime_suspended() and non-pm_runtime-using (i2c) drivers
On Tue, 14 Dec 2010, Mark Brown wrote: On Tue, Dec 14, 2010 at 12:44:24PM -0500, Alan Stern wrote: On Tue, 14 Dec 2010, Mark Brown wrote: On Tue, Dec 14, 2010 at 11:16:45AM -0500, Alan Stern wrote: I'm not familiar with the details of how the i2c subsystem works. But in general, the subsystem code should call pm_runtime_set_active() for every device before registering it. Then if a driver doesn't use any runtime-PM functions, pm_runtime_suspended() will return false. Hrm, if that's the case then we also need to update at least the platform and SPI buses. Though looking at the documentation this is going to get a bit interesting as there's a requirement that the parent has runtime PM enabled on it... The parent can either be set to the active state or set to ignore its children. The parent does not have to be enabled for runtime PM. Both of those require that the parent is set up to know something about runtime PM to some extent - in the case of buses like I2C the parent is a largely unrelated thing on a different bus which may or may not have runtime PM implemented. It's certainly not terribly apparent from the documentation. What part isn't clear from the documentation? I think the description of pm_runtime_set_active() in Documentation/power/runtime_pm.txt is pretty straightforward. It's not clear to me that one needs to do this in order to avoid breaking the suspend and resume calls of drivers that aren't doing anything with runtime PM. It's clear what it does, but it's less clear that the bus should do it or that not doing it will have an impact on stuff that isn't using runtime PM. It'd be really helpful if it were clearer what noop buses like this were supposed to do to get runtime PM working. I'm a little confused. When you say this is a noop bus, do you mean that it can't do any power management? If so, why does it need to get runtime PM working? The bus as a whole may not be able to do anything useful with runtime PM but individual devices on it may be able to do so - for example, a multi function device provides a parent device and a bunch of children for that device. Runtime PM provides a nice way for the children to individually suspend themselves and let the parent implement extra power savings if all the children suspend. It also provides a userspace API for controlling runtime suspend behaviour which drivers may find useful, and stuff like the autosuspend delays might be useful to some. I see. It sounds like you're really saying that new devices default to the wrong runtime state. If pm_runtime_init() would set new devices to RPM_ACTIVE instead of RPM_SUSPENDED then this problem wouldn't arise. Children could do whatever they want, and even if the parent's driver was totally ignorant of runtime PM, it would work out. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] pm_runtime_suspended() and non-pm_runtime-using (i2c) drivers
On Tue, 14 Dec 2010, Mark Brown wrote: On Tue, Dec 14, 2010 at 02:10:58PM -0500, Alan Stern wrote: I see. It sounds like you're really saying that new devices default to the wrong runtime state. If pm_runtime_init() would set new devices to RPM_ACTIVE instead of RPM_SUSPENDED then this problem wouldn't arise. Children could do whatever they want, and even if the parent's driver was totally ignorant of runtime PM, it would work out. In this case that's the desired behaviour but I worry that it'd break some other use case (eg, keeping a parent awake because there was an unclaimed device sitting around). When I've been working with the runtime PM subsystem before I've thought that it might be nice to have a specific RPM_UNINITIAZLIZED state which would generally get out of the way. It might be a bit clearer than the current situation conceptually but I've never felt I had a firm enough grasp on what was going on to really say, everything always feels more complicated than I feel it should if I understood it properly. That's an interesting suggestion. In general the PM core can't tell what power state a device is really in when it is first discovered and registered. I don't know if it would really solve your problem, though. What we really need is a better way to tell when a device shouldn't prevent its parent from suspending. Something like: If a device has no driver and no children, it should automatically be put in the RPM_SUSPENDED state. Thinking about it in the case of I2C we probably also want to mark the device as ignoring its children when it registers as an I2C controller, while an I2C controller that does runtime PM will probably figure this out for itself it would save a bit of effort. That might work, at least for your purposes. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] [PATCH] i2c: Use generic subsystem-level power management callbacks
On Mon, 15 Mar 2010, Rafael J. Wysocki wrote: In fact, I'd like the legacy callbacks to go, at least at the subsystem level, so I'd prefer not to make it easier to keep them around. How many bus types are there and how many of them actually implement power management callbacks? I don't really think the issue is so big. I have no idea. But I'm in the middle of implementing runtime PM for SCSI. SCSI already has bus-level legacy callbacks, so to add the runtime methods without changing the existing system sleep behavior requires something like this. The alternative is to write a bunch of tiny shim functions, sort of like what I ended up doing in the USB stack (see usb_dev_prepare() and the following routines in drivers/usb/core/usb.c). Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] [PATCH] i2c: Use generic subsystem-level power management callbacks
On Sun, 14 Mar 2010, Rafael J. Wysocki wrote: You know, maybe we should allow bus types to use both the old and new interfaces. It would make life easier for other subsystems in addition to i2c. This doesn't mean that the core would end up calling two sets of suspend routines. If the bus type uses legacy routines then all the non-runtime entries in the pm_ops structure would be empty. The changes to the PM core necessary to do this are quite small. Not really. The detection that the particular callback is not present happens in pm_op(), while the decision which framework to use is made at the device_[suspend|resume]() level. All you have to do is change the else if lines in device_[suspend|resume]() to if. Does it seem like a reasonable thing to do? Well, if someone spends time on implementing new callbacks for a bus type, writing them in such a way that they will call the legacy callbacks from drivers if necessary is not really a big deal IMO. Sure. But suppose you _don't_ want to spend the time implementing new callbacks to replace the existing legacy suspend and resume methods, whereas you _do_ want to implement runtime PM. Runtime PM forces the bus type to have a pm_ops member, which as you point out, will prevent the legacy methods from being called. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] [PATCH] i2c: Use generic subsystem-level power management callbacks
On Sun, 14 Mar 2010, Rafael J. Wysocki wrote: On Sunday 14 March 2010, Alan Stern wrote: On Sun, 14 Mar 2010, Rafael J. Wysocki wrote: You know, maybe we should allow bus types to use both the old and new interfaces. It would make life easier for other subsystems in addition to i2c. This doesn't mean that the core would end up calling two sets of suspend routines. If the bus type uses legacy routines then all the non-runtime entries in the pm_ops structure would be empty. The changes to the PM core necessary to do this are quite small. Not really. The detection that the particular callback is not present happens in pm_op(), while the decision which framework to use is made at the device_[suspend|resume]() level. All you have to do is change the else if lines in device_[suspend|resume]() to if. Then, if a bus type implements both new and old callbacks, we'll end up calling both. Not nice. Why would a bus type implement both? Or looked at another way, if it did implement both then wouldn't it want both sets of callbacks to be invoked? If a bus type does define both types of callback by mistake and only wants one of them to be used, that's a bug -- pure and simple. It's not the PM core's fault if something goes wrong under those circumstances. Sure. But suppose you _don't_ want to spend the time implementing new callbacks to replace the existing legacy suspend and resume methods, whereas you _do_ want to implement runtime PM. Runtime PM forces the bus type to have a pm_ops member, Which is very much on purpose, because the legacy suspend and resume have no idea about the runtime stuff. which as you point out, will prevent the legacy methods from being called. Yes, that's intentional. That said, I think we might modify the generic callbacks in generic_ops.c to call the drivers' legacy callbacks if the new ones are not defined. That's no good -- the routines in generic_ops.c invoke the _driver's_ callbacks, not the _bus type's_ callbacks. I suppose you could write a new set of generic legacy callbacks that would work just as though the new ops weren't defined. Removing those elses would be a lot simpler, though. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] [PATCH] i2c: Use generic subsystem-level power management callbacks
On Sat, 13 Mar 2010, Rafael J. Wysocki wrote: Please note that the legacy callbacks removed by this patch are already effectively dead, because they are not going to be called by the PM core anyway. I'm not sure if this is what we want, though, so please let me know in case it's not. You know, maybe we should allow bus types to use both the old and new interfaces. It would make life easier for other subsystems in addition to i2c. This doesn't mean that the core would end up calling two sets of suspend routines. If the bus type uses legacy routines then all the non-runtime entries in the pm_ops structure would be empty. The changes to the PM core necessary to do this are quite small. Does it seem like a reasonable thing to do? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
