Hello,

There seem to be some differences between the generic ops and the i2c
and platform busses' implementations of the interaction between runtime
PM and system sleep:

  (1) The platform bus does not implement the
      don't-call-pm->suspend()-if pm_runtime_suspended()-returns-true
      functionality implemented by the generic ops and i2c.

  (2) Both I2C and platform do not set the device as active when a
      pm->resume callback exists and it succeeds.

      This seems to have been done in i2c until recently, but has been
      removed by 753419f59e ("i2c: Fix for suspend/resume issue").  It
      seems to me that this removal is incorrect, and instead the real
      problem with the implementation was that it set the device as
      active even if no resume callback existed, whereas it should only
      do so when it exists and returns zero, like the generic ops.

Are these divergences from the generic ops to be considered as bugs?
Atleast (2) will cause devices which use UNIVERSAL_DEV_PM_OPS to have
incorrect runtime pm state after a resume from system sleep.

If so, before I send patches to fix them: can it be assumed that only
drivers using dev_pm_ops (and not the legacy ops of these busses) will
need the interactions between runtime PM and system sleep as done in the
generic ops?  This would mean that simple busses could simply use the
generic ops like below instead of duplicating their behaviour:

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 6b4cc56..46117e0 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -212,7 +212,7 @@ static int i2c_device_pm_resume(struct device *dev)
 	int ret;

 	if (pm)
-		ret = pm->resume ? pm->resume(dev) : 0;
+		ret = pm_generic_resume(dev);
 	else
 		ret = i2c_legacy_resume(dev);

 thanks,
 Rabin
--

[ message continues ]

I think so.  I'm not sure about (1), because someone may already depend on

Yes, you can make this assumption safely.  The drivers that don't use

Thanks,
Rafael
--

[ message continues ]

This is platform lagging behind I2C in implementation - both originally 
did what platform does and then I2C was updated and platform wasn't.

It'd be really good if this could all be factored out into the PM core,
we're going to have to do the same thing for at least SPI as well and
possibly some other buses :/
--

[ message continues ]

So how exactly the PM core is supposed to include those things?

There certainly are other buses that don't want to do them.

Thanks,
Rafael
--

[ message continues ]

By, for example, providing default implementations which the buses can
use if they choose to.
--

[ message continues ]

OK, so we have generic_subsys_pm_ops.  Do we need anything beyond that?

Rafael
--

[ message continues ]

Hrm.  Possibly just some fiddling with those or alternative versions.
For example, looking at the I2C bus suspend it's this:

static int i2c_device_pm_suspend(struct device *dev)
{
	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;

	if (pm) {
		if (pm_runtime_suspended(dev))
			return 0;
		else
			return pm->suspend ? pm->suspend(dev) : 0;
	}

	return i2c_legacy_suspend(dev, PMSG_SUSPEND);
}

Ideally the if (pm) block could just be factored out into the pm core as
there's nothing I2C-specific about that at all.  Possibly even the whole
logic surrounding fall back to legacy, though that smells a bit.  The
generic suspend operation doesn't fit here:

int pm_generic_suspend(struct device *dev)
{
	return __pm_generic_call(dev, PM_EVENT_SUSPEND);
}
EXPORT_SYMBOL_GPL(pm_generic_suspend);
--

[ message continues ]

Well, looking at __pm_generic_call(), I think it does.

It appears to do exactly what the pm block above does, so it should be possible
to have something like this:

static int i2c_device_pm_suspend(struct device *dev)
{
	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
	return pm ? pm_generic_suspend(dev) : i2c_legacy_suspend(dev, PMSG_SUSPEND);
}

if I'm not missing anything.

Thanks,
Rafael
--

[ message continues ]

SPI and platform (the first two buses I looked at) both seem to have
legacy suspend operations too?  Clearly the bus would need to provide an
op to invoke the legacy call but the logic which prioritises the pm_ops


Yes, I think that should work - thanks.  A similar thing would work for
the default platform bus implementation, too, and bring it into line
with I2C here.  I'll have a play.  Similarly for SPI.
--

[ message continues ]

Well, the problem with that is the driver would need to tell the generic call
what the legacy routine is and there's no, er, generic way to do that.

In the i2c case, for example, there is struct i2c_driver that contains the
->suspend() and ->resume() pointers, so the bus type driver _knows_ how to
get there, but the PM core doesn't have this information.

Thanks,
Rafael
--

[ message continues ]

Sure, but this could be readily accomplished by providing bus
legacy_suspend() and legacy_resume() operations that the generic code
could use to do the actual call.  This would save them all implmeneting
essentially the same decision making code for all the various different 
PM operations - the only bit that differs between buses is going to be
the actual process for calling the legacy API.

Like I say, I'm not sure if it's actually worth it.
--

[ message continues ]

First, there already are ->suspend() and ->resume() callbacks in
struct bus_type which are regarded as "legacy".  The PM core uses those as
appropriate in drivers/base/power/main.c .

Second, the situation at hand is that the bus type implements dev_pm_ops,
but the driver doesn't.  Now, pm_generic_suspend() is called with a struct
device pointer, so it would have to go back to dev->bus, find the
->legacy_suspend() callback (as opposed to ->suspend(), which also is legacy,

Well, I don't really think so.

Thanks,
Rafael
--

[ message continues ]

Well, the trouble is that the whole situation is already pretty
confusing for what should be very simple buses, each one needs to write
a bunch of not really bus specific code in order to get basic behaviour
which allows the drivers to make use of runtime PM, requiring more
thought and care per bus than I'd expect given that they've nothing
really to contribute.  This leads to the sort of random variations
between buses that Rabin is reporting, and means that updates keep
having to get done in multiple different places.

The overall effect is that from the point of view of trying to use
runtime PM in drivers which work with these simple buses everything
feels like it's much harder work than it should be.  Moving all the
decision making out of the buses and into the PM core seems like a win
here.
--

[ message continues ]

Well, the _solution_ is to get rid of the legacy stuff from those buses in the
first place.  Then, they'll just need to use the generic ops without any
trouble.

What you're proposing is a workaround that people will use as an excuse for
not doing the right thing forever.

Thanks,
Rafael
--

[ message continues ]

I hadn't actually realised that there was any urgency about removing the
old APIs - I'd not really seen much work in that direction.  If the
intention is to actively push people over to dev_pm_ops that does make a
bit more sense.
--

[ message continues ]

There's no urgency strictly speaking, but there's a preference. :-)

I think it probably is the time to actually start pushing people into that,
because the legacy stuff has been causing too much pain recently.

Thanks,
Rafael
--

[ message continues ]